-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Rewrite Type::any_over_type using a new generalised TypeVisitor trait
#19094
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
ec4e693 to
6034f2a
Compare
6034f2a to
1aa2fe0
Compare
|
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.
The code here looks nice. My main hesitation here is that I'm not totally clear that any_over_type is even a clearly defined operation for all types (see inline comment), and it's only currently used in deciding whether to issue a redundant-cast diagnostic. This seems like a lot of machinery to introduce for that small edge case, in the absence of a clear understanding (which I at least don't currently have) of the semantics of the generalized operation, and the future use cases for it.
I'm tempted to say that we should instead try to remove the need for any_over_type (as we already did for is_fully_static), and aim to avoid the need for these generalized recursive type walk tests entirely. To me, the ecosystem report on #19099 suggests that this is feasible. Removing the use of any_over_type entirely does not fail any existing tests or introduce many new diagnostics, and it seems like a fairly limited effort to address some Todos would remove most of the diagnostics it does introduce. (Plus, I don't think the current existence of todos and the desire to silence false positives arising from them should drive significant design decisions.)
I do think this PR is useful regardless, because I think a lot of it could be reused for a similar TypeTransformer trait, and that's something I do think we will need.
|
|
||
| type FxOrderSet<V> = ordermap::set::OrderSet<V, BuildHasherDefault<FxHasher>>; | ||
| type FxIndexMap<K, V> = indexmap::IndexMap<K, V, BuildHasherDefault<FxHasher>>; | ||
| type FxIndexSet<V> = indexmap::IndexSet<V, BuildHasherDefault<FxHasher>>; |
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 wonder if some of our existing uses of FxOrderSet should actually be FxIndexSet, if we don't need the stronger equality and order-maintenance guarantees provided by the ordermap wrapper crate? Not something for this PR, 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.
I don't think an FxIndexSet is hashable, so I think we do need to use FxOrderSet for the fields on IntersectionType, for example. But yeah, it does look like a bunch of places are using an OrderSet right now when they probably only really need an IndexSet.
| fn walk_pep_695_type_alias<'db, V: visitor::TypeVisitor<'db> + ?Sized>( | ||
| _db: &'db dyn Db, | ||
| _type_alias: PEP695TypeAliasType<'db>, | ||
| _visitor: &mut V, | ||
| ) { | ||
| } | ||
|
|
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.
These are the sorts of cases that made me somewhat hesitant to implement a generic walk-types facility (or at least, unclear how it should be implemented). Is the RHS of a PEP695 type alias "part of" its type? It's certainly relevant to the meaning of the type. Is the answer the same for every possible type walk? I would probably expect any_over_type to descend into the RHS, but I'm not sure what a type transformer could be expected to do here, unless we implemented a "Synthesized" variant of PEP 695 type aliases. (Maybe we will need that in order to support recursive type aliases?)
It seems somewhat parallel to the case of class-defined (non-synthesized) protocols, where the semantics of the type includes details that require further queries and aren't stored directly in the type itself.
I think this is fine for this PR, just musing.
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.
Follow-up from in-person discussion: this should be fixed to walk the RHS
|
Thank you for putting this together! I think my current feeling is we should go with #19099 instead. |
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.
After discussing in person, I'm convinced that we will likely have other future uses for this. I don't love that we do this potentially-expensive walk on two different types every time we see a cast, but it doesn't seem in practice like that's causing noticeable perf issues on any ecosystem project; we can deal with it later if it comes up.
So this looks good to me, modulo the PEP 695 type aliases fix.
|
Oh, and I do think we should rename my existing |
9f3412b to
737afeb
Compare
Summary
Our
Type::any_over_type()method has two problems:This PR introduces a new
TypeVisitortrait that aims to solve both of these issues in a generalized way:walk_*_typefunctions that live close to the structs they walk. For example, walking all nested types in aNominalInstanceTypeis handled by awalk_nominal_instance_typefunction that lives next to theNominalInstanceTypestruct ininstance.rs. This makes it less likely that we'll forget to update the function if we add a new field toNominalInstanceTypein the future.Type::any_over_typeis reimplemented as a standaloneany_over_typefunction that uses a concrete implementation of the newTypeVisitortrait as its implementation.If we like the direction this PR is going in, we might want to rename the existing
TypeVisitorstruct that @carljm added in #19003. We may also want to reimplementType::normalizeandType::apply_type_mappingusing a similarTypeTransformertrait, but I'll defer to @carljm on that point, since he has ongoing work in this area.Test Plan
All existing tests pass.