add `Visitor` and friends #373

basil-cow · 2020-04-05T12:58:29Z

Closes #333
Is needs_shift documentation accurate? Implementation gave me the impression that it's the opposite: check if there is any free variables (instead of bound variables).

nikomatsakis

This is great! I think there are two questions to decide before we land:

Should we change the VisitorResult to not use and_then, so that we can have a more straight-forward handling of for-loops that doesn't require O(n) stack space? I think probably "yes"
Should we remove the "visit alias eq" callback in favor of a different level in the hierarchy? I think probably "yes" but might be happy to defer the question until we investigate.

nikomatsakis · 2020-04-06T19:38:45Z

chalk-ir/src/visit.rs

+        }
+    }
+
+    fn visit_alias_eq(


Hmm, do we really want this accessor? I think we don't have similar one for other kinds of specific goals

nikomatsakis · 2020-04-06T19:39:51Z

chalk-ir/src/visit/boring_impls.rs

+    where
+        I: 'i,
+    {
+        (**self).visit_with(visitor, outer_binder)


Nit: I'm not sure what we wrote elsewhere, but I've migrated to writing code like this as T::visit_with(self, visitor, outer_binder). This removes the need to have a bunch of **, which I always find kind of hard to mentally track, and makes it quite clear that we are invoking the Visitor defined for the referent type T

(I've had e.g. infinite loops because I wrote (*self). etc...there is of course a litn for that now, but the T:: form seems more clear and secure.)

nikomatsakis · 2020-04-06T19:46:02Z

chalk-ir/src/visit/boring_impls.rs

+    {
+        let mut result = R::new();
+        for e in self {
+            result = result.and_then(|| e.visit_with(visitor, outer_binder))


Hmm, so, the one problem here is that we are not giving the and_then the ability to stop iterating through the vector. I think we'd have to write this to "unfold" across the stack like so:

fn unroll<I: Interner, T: Visit<I>, R: VisitResult>( visitor: &mut dyn Visitor<'_, I, Result = R>, iter: impl Iterator<Item = T> ) -> R { let mut iterator = self.iter(); match iterator.next() { None => R::new(), Some(elem) => elem.visit_with(visitor, outer_binder).and_then(|| unroll(visitor, iter)) } })

But seeing how nasty this is makes me wonder if and_then is really the right primitive, versus something that returns e.g. a Result-like enum that means "keep going", or perhaps adding a boolean to the result like return_early, so that the loop would be like:

for e in self { result = result.combine(e.visit_with(visitor, outer_binder)); if result.return_early() { return result; } } result

I thought about this when implementing, and I chose to do it that way instead of unrolling because it matches the behavior of the regular struct Visit.

fn visit_with(...) -> R { R::new() .and_then(|| a.visit() .and_then(|| b.visit() .and_then(|| c.visit()))) }

would be the struct visit rewritten as unrolling, which is kind of a weird mix of right and left folding.

Replacing and_then with combine + boolean sounds very annoying, since you would need to call it after each visit of a struct member too. I would just add an return_early method as a perf hack for iterator things, which is a little inconvenient since it would require and_then and return_early behavior to match (but when implementing VisitResult you will probably use return_early in and_then anyway, so who cares). Alternatively, we can hack the existing implementation by doing

for e in self { let visited = false; result = result.and_then(|| { e.visit_with(visitor, outer_binder); visited = true; }) if !visited { break } }

If we propose a contract that and_then not calling the closure means early return, then it kinda feels less hacky.

I feel like I'd prefer to have one mechanism, let's call it return_early. If we want, we can build the and_then mechanism on top of that readily enough:

fn and_then(self, op: impl FnOnce() -> Self) -> Self { if self.return_early() { self } else { self.combine(op()) } }

That said, I'd probably change the deriving at least to use return_early instead of and_then because it'll be more efficient at runtime and requires less compiler tricks to compile.

nikomatsakis · 2020-04-06T19:46:24Z

chalk-ir/src/visit/boring_impls.rs

+        let interner = visitor.interner();
+        let mut result = R::new();
+
+        for p in self.iter(interner) {


same as the for loop above; we should probably make a helper for for loop iterator regardless

nikomatsakis · 2020-04-06T19:46:29Z

chalk-ir/src/visit/boring_impls.rs

+        let interner = visitor.interner();
+        let mut result = R::new();
+
+        for p in self.iter(interner) {


same as the for loop above; we should probably make a helper for for loop iterator regardless

nikomatsakis · 2020-04-06T19:46:54Z

chalk-ir/src/visit/boring_impls.rs

+    }
+}
+
+impl<I: Interner, T, L> Visit<I> for ParameterKind<T, L>


can this not be derived..?

Derive is only for types that have one type parameter that either has an interner or is the interner. I don't know why is it so strict, but I guess there was a reason for that?

Oh. Well, it's almost always sufficient I guess -- and the more general pattern isn't as obvious -- but I think we could extend it readily enough. e.g., to any number of type parameters that are all HasInterner, which would apply here, right? But if it's only the one impl, maybe not worth it (I imagine this is why we didn't bother the first time)

nikomatsakis · 2020-04-06T19:53:59Z

chalk-solve/src/wf.rs

-impl<I: Interner> FoldInputTypes for Substitution<I> {
-    fn fold(&self, interner: &I, accumulator: &mut Vec<Ty<I>>) {
-        self.parameters(interner).fold(interner, accumulator)
+    fn visit_alias_eq(&mut self, alias_eq: &AliasEq<I>, outer_binder: DebruijnIndex) {


I guess this is why you added the AliasEq callback. Ah, I was imagining before that we would intercept AliasEq when we are visiting goals, but in this code we are going to visit WhereClause values that are outside of any goal or program clause, right? Hmm, that's annoying!

Something about having visit_alias_eq doesn't feel right, it's kind of a "random level" in the type hierarchy, and we've kept the "callback points" relatively contained up until now. I guess that having a callback either for WhereClause (going "up a level") or for AliasTy (going "down a level") feels potentially a bit better to me. An AliasTy would also get invoked when walking types, I imagine, which in some sense fits well -- it just adds one more category of type that gets a special callback. (But weird in that it also gets invoked from other contexts...)

That said, I think I'd be ok landing this "as is" with a kind of FIXME to resolve the right callback here -- it's a bit tied into how we are handling implied bounds (that's what this code is doing), and I also think the right answer might become more clear over time.

I changed callbacks a little (and added visitor for EnvElaborator), take a look please

basil-cow · 2020-04-07T04:49:07Z

Oh, and also I forgot to ask about the truncator, should I just replace it with a visitor that checks term size?

nikomatsakis · 2020-04-07T14:25:28Z

@Areredify re: truncator, yes, that'd be the idea -- we just want to know if any term exceeds a certain size

nikomatsakis

Seems good! A few more suggestions.

chalk-solve/src/wf.rs

basil-cow · 2020-04-07T21:06:36Z

I'll file a separate pr with the truncator, let's merge this first

nikomatsakis

OK, a few more minor things, this is getting really close =)

nikomatsakis · 2020-04-07T21:07:02Z

chalk-ir/src/visit/visitors.rs

+    }
+}
+
+pub struct FindFreeVarsVisitor<'i, I: Interner> {


I feel like this should be a private impl detail of the has_free_vars function

chalk-ir/src/visit/visitors.rs

nikomatsakis · 2020-04-07T21:07:35Z

chalk-ir/src/visit/visitors.rs

+        _bound_var: BoundVar,
+        _outer_binder: DebruijnIndex,
+    ) -> Self::Result {
+        FindAny { found: true }


then this can be FindAny::FOUND

nikomatsakis · 2020-04-07T21:07:39Z

chalk-ir/src/visit/visitors.rs

+        _bound_var: BoundVar,
+        _outer_binder: DebruijnIndex,
+    ) -> Self::Result {
+        FindAny { found: true }


chalk-ir/src/visit.rs

nikomatsakis · 2020-04-07T21:12:30Z

chalk-ir/src/visit/boring_impls.rs

+use chalk_engine::{context::Context, ExClause, FlounderedSubgoal, Literal};
+use std::{marker::PhantomData, sync::Arc};
+
+pub fn visit_iter<'i, T, I, IT, R>(


Comment:

Convenience function to visit all the items in the iterator it.

nikomatsakis · 2020-04-07T21:12:44Z

chalk-ir/src/visit/boring_impls.rs

+use std::{marker::PhantomData, sync::Arc};
+
+pub fn visit_iter<'i, T, I, IT, R>(
+    it: IT,


Personally, I would use impl Iterator<Item = T>

nikomatsakis · 2020-04-07T21:14:17Z

I'll file a separate pr with the truncator, let's merge this first

Agreed -- shall we file an issue to track it?

basil-cow · 2020-04-07T21:52:49Z

I gotta up my comments game 😅

nikomatsakis

OK, last thing! Thanks for staying on top of this. ❤️

chalk-ir/src/visit.rs

basil-cow force-pushed the visitor branch 3 times, most recently from 7aab941 to 52ee038 Compare April 6, 2020 10:14

nikomatsakis approved these changes Apr 6, 2020

View reviewed changes

nikomatsakis requested changes Apr 7, 2020

View reviewed changes

basil-cow marked this pull request as ready for review April 7, 2020 15:46

basil-cow force-pushed the visitor branch from f7a9c50 to d13ea19 Compare April 7, 2020 16:05

nikomatsakis mentioned this pull request Apr 7, 2020

adopt synstructure for our custom derives #381

Closed

nikomatsakis requested changes Apr 7, 2020

View reviewed changes

nikomatsakis mentioned this pull request Apr 7, 2020

port truncator to be a visitor #382

Closed

basil-cow added 9 commits April 8, 2020 00:27

first implementation

22f04dd

change ucanonicalize from folder to visitor

8cf6537

change needs_shift to use visitor

b65c184

change wf's input type collector to use visit

556d668

refactor shared visitors

936df35

address changes part 1

5687f89

use Visitor in EnvElaborator and change some callbacks

88614f6

rework VisitResult design, add helper method in wf

114716d

impl visit on ProgramClause(s)

f35edae

basil-cow force-pushed the visitor branch from d13ea19 to adb7401 Compare April 7, 2020 21:52

nikomatsakis requested changes Apr 9, 2020

View reviewed changes

chalk-ir/src/visit.rs Outdated Show resolved Hide resolved

basil-cow force-pushed the visitor branch from adb7401 to f2de0c0 Compare April 9, 2020 16:03

more nits

88a6bbb

basil-cow force-pushed the visitor branch from f2de0c0 to 88a6bbb Compare April 9, 2020 16:13

nikomatsakis approved these changes Apr 9, 2020

View reviewed changes

nikomatsakis merged commit 2c72044 into rust-lang:master Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `Visitor` and friends #373

add `Visitor` and friends #373

basil-cow commented Apr 5, 2020 •

edited

Loading

nikomatsakis left a comment

nikomatsakis Apr 6, 2020

nikomatsakis Apr 6, 2020

nikomatsakis Apr 6, 2020

nikomatsakis Apr 6, 2020

basil-cow Apr 7, 2020

nikomatsakis Apr 7, 2020

nikomatsakis Apr 6, 2020

nikomatsakis Apr 6, 2020

nikomatsakis Apr 6, 2020

basil-cow Apr 7, 2020

nikomatsakis Apr 7, 2020

nikomatsakis Apr 6, 2020

basil-cow Apr 7, 2020

nikomatsakis Apr 7, 2020

basil-cow commented Apr 7, 2020

nikomatsakis commented Apr 7, 2020

nikomatsakis left a comment

basil-cow commented Apr 7, 2020

nikomatsakis left a comment

nikomatsakis Apr 7, 2020

nikomatsakis Apr 7, 2020

nikomatsakis Apr 7, 2020

nikomatsakis Apr 7, 2020

nikomatsakis Apr 7, 2020

nikomatsakis commented Apr 7, 2020

basil-cow commented Apr 7, 2020

nikomatsakis left a comment

add Visitor and friends #373

add Visitor and friends #373

Conversation

basil-cow commented Apr 5, 2020 • edited Loading

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil-cow commented Apr 7, 2020

nikomatsakis commented Apr 7, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

basil-cow commented Apr 7, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Apr 7, 2020

basil-cow commented Apr 7, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

add `Visitor` and friends #373

add `Visitor` and friends #373

basil-cow commented Apr 5, 2020 •

edited

Loading