Add ast ident iterator

[Ryan1729]

While working on #6086 which is a PR meant to address #6039, I found that I needed a way to iterate through the identifiers in an expression. Implementing an Ident iterator has turned out to be sufficiently complicated that I thought it would be nicer for reviewers if those changes were in a separate PR. This is that PR.

changelog: none

[rust-highfive]

r? @phansch

(rust_highfive has picked a reviewer for you, use r? to override)

[phansch]

Thanks for the PR! Unfortunately, I'm not too familiar with this part of the code, so maybe r? @flip1995

[flip1995]

So before reviewing all of this: IIUC the goal of this is to get an iterator over all idents in an expression. Is there a reason, that you need lazy evaluation here? Otherwise we could just use a Visitor, which collects every Ident in a Vec and then just turns this Vec into an iterator.

Downside is that you have to collect all Idents and have a worse performance if you often want to stop iterating after a few steps.

The advantage is, that you don't have to implement all of the expr/item/ty/... handling yourself, since it is already implemented by the Visitor.

[flip1995]

Can you give an example. where this would be necessary. Using P<_> in a type signature should not be possible, since it implements Deref.

[Ryan1729]

Given exprs is a &Vec<P<Expr>>,

exprs.iter()
    .flat_map(ExprIdentIter::new_p)

compiles, while using ExprIdentIter::new does not:

exprs.iter()
    .flat_map(ExprIdentIter::new)

IIRC this compiles:

exprs.iter()
    .flat_map(|e| ExprIdentIter::new(e))

So ExprIdentIter::new_p allows skipping writing the closure.
I'm willing to believe there might be a better way to do this. Maybe making ExprIdentIter::new generic or something like that?

[Ryan1729]

So before reviewing all of this: IIUC the goal of this is to get an iterator over all idents in an expression.

That is correct. And the main reason we want an is to create suggestions containing an expression with some of the Idents swapped. Specifically this code in #6086, (currently a WIP) uses such an iterator:

Is there a reason, that you need lazy evaluation here? Otherwise we could just use a Visitor, which collects every Ident in a Vec and then just turns this Vec into an iterator.

Downside is that you have to collect all Idents and have a worse performance if you often want to stop iterating after a few steps.

The advantage is, that you don't have to implement all of the expr/item/ty/... handling yourself, since it is already implemented by the Visitor.

Hmm. Well, the part that actually creates the suggestion can stop early. But at least for the purposes of #6086, that iteration would be preceded by some form of full iteration over the expression in question anyway, so maybe that savings would be insignificant. O(n + n / 2) is still O(n).

Given how much work handling all the possibilities myself has been so far, using a Visitor sounds like it would be worth doing, given we aren't positive the perf difference. If we only expose an Iterator trait object then if it turns out that laziness is worth doing we can change it without affecting the user(s) of the iterator. So I think I'll given switching to using a Visitor and walk_expr a go.

[Ryan1729]

Okay, I feel silly now for not using Visitor from the start, given how simple the code has now become.

[flip1995]

Since the code is now that simple, I think you can squash it into one commit and include it in the other PR?

[Ryan1729]

Since the code is now that simple, I think you can squash it into one commit and include it in the other PR?

I'll do that. Thanks for all your help simplifying the code!