-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[red-knot] Eagerly normalize type[]
types
#15272
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
999704a
[red-knot] Eagerly normalize `type[]` types
AlexWaygood 72f8cec
fix bug where class-literal types were no longer understood as subtyp…
AlexWaygood a83b8e8
Apply suggestions from code review
AlexWaygood 6baefda
better docs and more constructor methods
AlexWaygood 34438f8
fix docs
AlexWaygood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
use super::{ClassBase, ClassLiteralType, Db, KnownClass, Symbol, Type}; | ||
|
||
/// A type that represents `type[C]`, i.e. the class object `C` and class objects that are subclasses of `C`. | ||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, salsa::Update)] | ||
pub struct SubclassOfType<'db> { | ||
// Keep this field private, so that the only way of constructing the struct is through the `from` method. | ||
subclass_of: ClassBase<'db>, | ||
} | ||
|
||
impl<'db> SubclassOfType<'db> { | ||
/// Construct a new [`Type`] instance representing a given class object (or a given dynamic type) | ||
/// and all possible subclasses of that class object/dynamic type. | ||
/// | ||
/// This method does not always return a [`Type::SubclassOf`] variant. | ||
/// If the class object is known to be a final class, | ||
/// this method will return a [`Type::ClassLiteral`] variant; this is a more precise type. | ||
/// If the class object is `builtins.object`, `Type::Instance(<builtins.type>)` will be returned; | ||
/// this is no more precise, but it is exactly equivalent to `type[object]`. | ||
/// | ||
/// The eager normalization here means that we do not need to worry elsewhere about distinguishing | ||
/// between `@final` classes and other classes when dealing with [`Type::SubclassOf`] variants. | ||
pub(crate) fn from(db: &'db dyn Db, subclass_of: impl Into<ClassBase<'db>>) -> Type<'db> { | ||
let subclass_of = subclass_of.into(); | ||
match subclass_of { | ||
ClassBase::Any | ClassBase::Unknown | ClassBase::Todo(_) => { | ||
Type::SubclassOf(Self { subclass_of }) | ||
} | ||
ClassBase::Class(class) => { | ||
if class.is_final(db) { | ||
Type::ClassLiteral(ClassLiteralType { class }) | ||
} else if class.is_known(db, KnownClass::Object) { | ||
KnownClass::Type.to_instance(db) | ||
} else { | ||
Type::SubclassOf(Self { subclass_of }) | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// Return a [`Type`] instance representing the type `type[Unknown]`. | ||
pub(crate) const fn subclass_of_unknown() -> Type<'db> { | ||
Type::SubclassOf(SubclassOfType { | ||
subclass_of: ClassBase::Unknown, | ||
}) | ||
} | ||
|
||
/// Return a [`Type`] instance representing the type `type[Any]`. | ||
pub(crate) const fn subclass_of_any() -> Type<'db> { | ||
Type::SubclassOf(SubclassOfType { | ||
subclass_of: ClassBase::Any, | ||
}) | ||
} | ||
|
||
/// Return the inner [`ClassBase`] value wrapped by this `SubclassOfType`. | ||
pub(crate) const fn subclass_of(self) -> ClassBase<'db> { | ||
self.subclass_of | ||
} | ||
|
||
pub const fn is_dynamic(self) -> bool { | ||
// Unpack `self` so that we're forced to update this method if any more fields are added in the future. | ||
let Self { subclass_of } = self; | ||
subclass_of.is_dynamic() | ||
} | ||
|
||
pub const fn is_fully_static(self) -> bool { | ||
!self.is_dynamic() | ||
} | ||
|
||
pub(crate) fn member(self, db: &'db dyn Db, name: &str) -> Symbol<'db> { | ||
Type::from(self.subclass_of).member(db, name) | ||
} | ||
|
||
/// Return `true` if `self` is a subtype of `other`. | ||
/// | ||
/// This can only return `true` if `self.subclass_of` is a [`ClassBase::Class`] variant; | ||
/// only fully static types participate in subtyping. | ||
pub(crate) fn is_subtype_of(self, db: &'db dyn Db, other: SubclassOfType<'db>) -> bool { | ||
match (self.subclass_of, other.subclass_of) { | ||
// Non-fully-static types do not participate in subtyping | ||
(ClassBase::Any | ClassBase::Unknown | ClassBase::Todo(_), _) | ||
| (_, ClassBase::Any | ClassBase::Unknown | ClassBase::Todo(_)) => false, | ||
|
||
// For example, `type[bool]` describes all possible runtime subclasses of the class `bool`, | ||
// and `type[int]` describes all possible runtime subclasses of the class `int`. | ||
// The first set is a subset of the second set, because `bool` is itself a subclass of `int`. | ||
(ClassBase::Class(self_class), ClassBase::Class(other_class)) => { | ||
// N.B. The subclass relation is fully static | ||
self_class.is_subclass_of(db, other_class) | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably not acceptable? That is,
Literal[X]
for classesX
is already a notation we are inventing; I don't think we can round-trip explicit user annotations oftype[X]
asLiteral[X]
in our type display, even when they are equivalent.We could always display
Literal[X]
astype[X]
in general. The downside here is that then we are conflating two types that are meaningfully different in how we handle them internally, with identical text representations.I'm not sure what the alternative is, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few points.
Firstly, we're far from unique in inventing our own notation for type display in some cases and using it in the output of
reveal_type
. E.g. take a look at the mypy output here. There have been a couple of complaints about some of the stranger symbols in mypy'sreveal_type
output being confusing, but I don't think it's really been a major issue at all. I can only remember one issue about it, and it's not highly upvoted.Secondly, we're also far from unique in eagerly simplifying types that we see in user annotations and using the simplified types in our type display. We already do this elsewhere when we simplify unions (removing duplicate elements and elements that are subtypes of each other); so does pyright, to some extent.
Notwithstanding those two points, I have been thinking for a while that our current display for class-literal and function-literal types is a little confusing for users. It looks too close to something that would be valid in a type annotation in user code, and users will probably think that we're claiming that
Literal[SomeClass]
is valid in a user type annotation. It'll be confusing for them when we then reject them usingLiteral[SomeClass]
in their own code.Prior to this PR, I was wondering about
Literal[<class 'SomeClass'>]
andLiteral[<function 'some_function'>]
as an alternative display for class-literal and function-literal types. But your comments here make me wonder if we could also considertype[SomeClass(final=True)]
ortype[SomeClass(@final=True)]
for class-literal typesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that both displaying our own notation for some types, and simplifying user-provided types, are things we can do. It just feels a bit like crossing another line to combine those two things in such a way that we eagerly replace a spellable user-provided type with a non-spellable type display. But maybe this isn't actually a problem in practice. I'm OK with deferring that question from this PR, and at some point following up more holistically on type display. There are a lot of interesting questions here, including how we balance conciseness/readability vs clarity (to first time readers without needing to refer to docs) in the notations we choose. I won't get too deep into the details here, I'll just say that a) I kind of like the idea of representing
ClassLiteral
types in some way usingtype[]
notation, since they are similar, and b) I'm not sure we should use the word "final" in that display, because I think it's too confusing in cases where we have aClassLiteral
type but for a non-final class.