Refactored ast_map and friends, mainly to have Paths without storing them.

[eddyb]

No description provided.

[alexcrichton]

Could you also add rationale for why this was done and the direction it's going in? I'm mostly just curious, I'm not really sure what the impetus for this was.

[eddyb]

I wanted to remove all the cloning associated with ast_map::Path, and I've found a pretty good way to retrieve paths without storing them (apart from the base paths of inlined items), by having "parent" NodeIds on each node, referring to its inner-most enclosing item/method node.

In the process of adapting the codebase to that model, I've come across the inefficient and deprecated methods on Session, related to the token interner, and I've ripped them out. It might be possible to split those changes into a separate commit, but they've become quite entangled.

[huonw]

In the process of adapting the codebase to that model, I've come across the inefficient and deprecated methods on Session, related to the token interner, and I've ripped them out. It might be possible to split those changes into a separate commit, but they've become quite entangled.

FWIW, it's much easier to review when things are not just a single monolithic 4500 line commit, so it's generally good practice to avoid the temptation to do too many things at once (which I'm not very good at either...). In any case, a longer commit message describing what's changing and why (so that it's possible to deduce it from the git history/git blame) is something that should be possible now.

[alexcrichton]

by having "parent" NodeIds on each node, referring to its inner-most enclosing item/method node

This seems like a fairly large change to the code base, and I just wanna make sure that you've talked it over with others first (it's certainly fine if you have already).

[nikomatsakis]

cc me (I want to read over)

[nikomatsakis]

I've been skimming at a high-level. This mostly makes sense, I think, but I'm not a fan of the <PI:PathIter> that is littered everywhere -- it's verbose and will generate a lot of monomorphized instances. Though perhaps that is precisely the point of this patch, to avoid the overhead of collecting.

Anyway, my biggest concern is that I fear the interaction with #12158 -- in particular, lazy iterators often cause borrow checker conflicts, since their closures claim the data they touch for the entirety of the iteration.

This means that if you have foo.iter().filter(|| expr1).map(|| expr2) and so on, expr1 and expr2 cannot mutate the same state etc. Anyway, I'm not sure if there'll be a negative interaction or not -- we'll have to see. Quite possibly it'll be fine since path iteration is mostly touching the ast_map, which is (I think) stored in a a RefCell and hence sharable.

But i'd request that PR #12158 land first, since I think closing up soundness holes takes higher precedence and I have no desire to undo work this PR does.

[eddyb]

PathIter includes Clone, to reduce vebosity (I originally had Iterator<PathElem> + Clone in a few bounds), so iter::Map wouldn't even work.
We can remove monomorphization completely by using FullPathIter (a chained vector value iterator and a stack linked list iterator) everywhere, there's definitely less composition now in metadata and whatnot.

[nikomatsakis]

I was wondering whether we could remove the <PI> generics and replace them with a specific type. I'd prefer to do that, I think, though I might prefer a shorter name than FullPathIter (maybe PathIter or even Path?). Actually, following our new convention I guess the appropriate thing would be some sort of pluralized noun like Names or Ids.

[eddyb]

I've renamed FullPathIter to PathElems, and now only 3 (terminal) functions are generic over <PI: Iterator<PathElem>.

@cmr has been nice enough to send my branch to try, yielding this interesting test failure.
I'm surprised to see we output these ugly "pretty" impl paths in errors.
Those changed in this PR, foo$$UP$$VEC$uint::foo is now ~[uint].foo::foo (trans doesn't have any problems because it mangles paths itself).

[nikomatsakis]

Pre-existing, but this is ... goofy! We should be able to remove this by now.

[eddyb]

Thought so myself, then forgot about it.