-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Split Type::KnownInstance into two type variants
#18350
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
Conversation
|
00855a0 to
3034a18
Compare
3034a18 to
c5c1ad5
Compare
dcreager
left a comment
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 very much like not having to thread the db through in as many places as before
| /// Ordering within variants is based on the wrapped data's salsa-assigned id and not on its values. | ||
| /// The id may change between runs, or when e.g. a `TypeVarInstance` was garbage-collected and recreated. | ||
| #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, salsa::Update, Ord, PartialOrd)] | ||
| pub enum KnownInstanceType<'db> { |
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.
Was there a rationale for moving this back into types.rs? All else equal, I quite liked how we were moving things into dedicated modules, to help reduce the overwhelming size of this file.
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.
Oh, I'm definitely in favour of continuing to reduce the size of types.rs! I mainly put this here because it didn't seem big enough anymore to deserve its own module, and I wasn't sure it really had enough in common with SpecialFormType to share a module with that enum. (What would we call the module?)
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.
Perhaps the same thing it was called before, known_instance.rs?
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.
put them both in known_instance.rs? Or put them in separate modules: SpecialFormType in special_form.rs and KnownInstanceType in known_instance.rs?
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.
(Landing this, but happy to address this in a followup!)
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.
(Happy with a followup) I like your suggestion of separate modules special_form and known_instance
carljm
left a comment
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.
It gives me slight pause how many of the cases need to be the same for SpecialForm and KnownInstance, but overall I do think the simplification and clarification of SpecialForm is worth it. Nice work!
| /// Ordering within variants is based on the wrapped data's salsa-assigned id and not on its values. | ||
| /// The id may change between runs, or when e.g. a `TypeVarInstance` was garbage-collected and recreated. | ||
| #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, salsa::Update, Ord, PartialOrd)] | ||
| pub enum KnownInstanceType<'db> { |
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.
Perhaps the same thing it was called before, known_instance.rs?
c03bdce to
0bea68b
Compare
0bea68b to
40e0e9f
Compare
Summary
This PR splits
Type::KnownInstanceinto twoTypevariants.Although the variants of
KnownInstanceTypeare more similar to each other than they are to any otherTypevariants, there's nonetheless a blurring of two somewhat distinct concepts in the enum as it currently stands:typing_extensions) known locations at runtime. For example:typing.Required,typing.ClassVar,typing.Final, etc.Typevariant. I don't think that would be worth the extra branches everywhere (and the associated maintenance burden), but the point stands that these each represent distinct concepts to the other variants inKnownInstanceType. These variants areKnownInstanceType::TypeAliasType()(used for all PEP-695typestatements),KnownInstanceType::TypeVar(),KnownInstanceType::Generic()andKnownInstanceType::Protocol().This PR therefore splits the
Type::KnownInstancevariant into two, and splits theKnownInstanceTypeenum into two.Type::KnownInstance()continues to wrap associated data of typeKnownInstanceType, butKnownInstanceTypenow only holds four variants (the four very special variants described above).Type::SpecialFormis added in this PR, and it wraps associated data of typeSpecialFormType. All variants except the four special ones are moved to the newSpecialFormTypeenum.The vast majority of variants previously on
KnownInstanceTypeare now onSpecialFormType, and the refactor here enables the implementation ofSpecialFormTypeto be significantly simpler than the previous version ofKnownInstanceTypewas:SpecialFormTypecan now implementDisplaydirectly rather than having to have adisplay()method that takes adb. This simplifies the construction of many diagnostics.strum_macros::EnumStringto autogenerate theFromStrdeserialization used inSpecialFormType::try_from_file_and_name(). Previously this had to be written out by hand due to some variants wrapping data, but we know from experience that it's hard to remember to keep methods like this up to date (and the compiler can't enforce exhaustiveness over amatchwhere the enum variants are on the right-hand side of thematch).SpecialFormTypeno longer needs anormalized()method: now that no variants wrap any data, they all just normalize to themselves.Test Plan