Conversation
This is a visitor which checks that all bound vars refer to an existing binder of the right type. This has been quite useful for me: we somewhat regularly have errors in the binders which then cause a crash some time down the line when something tries to substitute them. At that point it's usually harder to find the actual place the error comes from; also you don't always hit the error. For the latest of these bugs, I added this validator, and immediately had failing tests that pointed me to the right location. The disadvantage is that I needed to add some rather specific methods to `Visitor`. I didn't find a better way to model this (and this is also the reason I couldn't just implement it in rust-analyzer).
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
No objection, if it's useful.
jackh726
left a comment
jackh726
left a comment
There was a problem hiding this comment.
The only other thought I have is how useful it is to validate all bound vars in a "type" (or whatever we're checking), versus just checking for bound vars at the current level?
| /// Invoked for each occurence of a `Binders`, before visiting them. | ||
| fn before_binders(&mut self, _kinds: &crate::VariableKinds<I>) {} | ||
| /// Invoked for each occurence of a `Canonical`, before visiting them. | ||
| fn before_canonical(&mut self, _kinds: &crate::CanonicalVarKinds<I>) {} | ||
| /// Invoked for each occurence of a `FnPointer`, before visiting them. | ||
| fn before_fn_pointer_substs(&mut self, _number: usize) {} | ||
| /// Invoked for each occurence of a type introducing binders, after visiting them. | ||
| fn after_any_binders(&mut self) {} | ||
|
|
There was a problem hiding this comment.
I wonder if it's worth just adding a visit_binders function like in rustc? (Probably) Is it all helpful to have these split out? I don't know if rustc calls visit_binder for fn pointer substs or canonical items, but we should.
Unfortunately, I think that our visit_binders would have to look like fn visit_binder<T: SuperVisit<I>>(&mut self, binders: crate::VariableKinds<I>, value: T) -> ControlFlow<Self::BreakTy>);, but that's probably fine.
There was a problem hiding this comment.
Alternatively, we could just change this to only have before_binders and do the "into VariableKinds conversion" happen before calling it.
There was a problem hiding this comment.
I tried something like that first, the problem is that it makes the Visitor trait not object safe.
We could do the before_binders, but it'd mean doing the conversion in every visitor instead of just the one where it's necessary.
There was a problem hiding this comment.
Well, I mean, you only need to do the conversion in the Visit impls, right?
|
I'd like to see the change/cleanup from my comment, but if that doesn't work out for whatever reason, I'm fine with the current state if things can't be cleaned up. So, r=me preferring the requested changes but either way I'm fine. |
Hmm. I guess it would actually be possible to validate bound vars in each |
|
☔ The latest upstream changes (presumably #735) made this pull request unmergeable. Please resolve the merge conflicts. |
4 participants
This is a visitor which checks that all bound vars refer to an existing binder of the right type. This has been quite useful for me: we somewhat regularly have bugs in the Chalk transformation code (or now with the chalk_ir move earlier) where we don't shift the bound variables correctly, which then causes a crash some time down the line when something tries to substitute them. At that point it's usually harder to find the actual place the error comes from; also you don't always hit the error. For the latest of these bugs, I added this validator, and immediately had failing tests that pointed me to the right location.
The disadvantage is that I needed to add some rather specific methods to
Visitor. I didn't find a better way to model this (and this is also the reason I couldn't just implement it in rust-analyzer).