r? @cramertj

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

@bors try

⌛ Trying commit f71703abf45b28712dc34890d85d67991f8400a4 with merge 8a50d3d384210ef535686e4c3804884fede9a4de...

☀️ Try build successful - checks-azure
Build commit: 8a50d3d384210ef535686e4c3804884fede9a4de

r? @Zoxc

This should get a perf run, shouldn't it?

Yeah, I'd suggest doing that; I don't have perf creds, though.

@rust-timer build 8a50d3d384210ef535686e4c3804884fede9a4de

Feel free to ping me if you need a perf run

Success: Queued 8a50d3d384210ef535686e4c3804884fede9a4de with parent 4784645, comparison URL.

Finished benchmarking try commit 8a50d3d384210ef535686e4c3804884fede9a4de, comparison URL.

Not too shabby; @Zoxc you said you'd recommend someone else to review this specific change - can you suggest someone?

Ping from Triage: Hi @Zoxc - can we get an update on the review or the hunt for a reviewer? Also will cc @varkor since they are being suggested as a reviewer. Possibly they can help.

Triage: Requesting a review from @rust-lang/compiler

r? @nikomatsakis

Hmm, my read of the performance results is that this is basically neutral. It looked like almost everything was 0% except for one 1%, and that was for a known unreliable case. Am I mis-reading?

If not, I'd be inclined to leave things as they are, as @ljedrz noted that this change increased the complexity of the code.

@nikomatsakis This is a necessary step for the deprecation of NodeIds after HIR lowering (that is nearly complete), though - otherwise we still need to rely on definitions::opt_def_index,opt_local_def_id. Do you have any suggestions on how to do it in a more elegant manner?

@ljedrz ah sorry, I misunderstood the motivation. I thought this was a performance optimization.

@nikomatsakis I rebased and expanded on the previous comments.

I don't know where I last suggested it but IMO it would probably be nicer to have every node with a DefId be a HirId root, making this map unnecessary (as you can get the DefId from the HirId).

@eddyb that does indeed sound pretty clean, but it seems like a larger scale refactoring -- how hard do you think it would be?

It shouldn't be that hard, I think there a "whitelist" of HirId owners somewhere that would need to be relaxed, but other than that most things don't rely on the current stratification at all.

The main exception is ty::TypeckTables which would need nested tables of itself for closures, since it's optimized by having its maps be keyed by intra-HirId-owner IDs.

What are the interactions with incremental compilation? That's why we introduced HirId in the first place and I expect that at least some of the code relies on HirId-owner == item-like. To be clear: I don't object, I just don't expect the change to be entirely straight forward.

@eddyb is there a canonical list of "things that have def-ids"? To be honest I might be a little out of date on the current status here.

(This actually feels like a case where I'd have appreciated a design meeting, or at least some up-front docs on what the plans are -- @ljedrz I'm kind of assuming those don't exist? (Don't feel bad, it's not standard practice.))

@nikomatsakis I don't mind at all ^^.

Nah, they don't exist - at least none that I know of. I'm just continuing the ancient NodeId bashing crusade (AKA the HirIdification initiative), so that we can ditch NodeIds in favor of HirIds after the AST > HIR lowering (for uniformity and incremental gains).

I don't mind how it is done; the concept outlined by @eddyb is a bit foreign to me, but I wouldn't mind doing it that way with a little bit of mentoring.

@nikomatsakis Mostly whatever happens to get a DefId assigned to it in rustc::hir::map::def_collector.

DefKind lists a subset of those cases (most notably missing impls):

IMO "item-likes" is an outdated concept, but my preferred solution (i.e. consolidating a notion of "def" beyond just DefId and DefKind) requires more design and planning, which I'm less sure lately I can allocate time for (compared to what I was hopping to get done over the past few months).

Ping from triage
@nikomatsakis this appears to still be waiting on review.
Thanks.

Ping from triage. @nikomatsakis any updates on this? Thanks.

Yeah, sorry to @ljedrz for not providing more feedback here! I've been wanting to block out some time that I can try to better understand what @eddyb is saying =)

All I'm saying is reduce the "item-like" distinction further and make all things with a DefId more similar, in terms of being HirId roots, nothing more.

It's not a complex plan, and the only complications would be things that assume too much about HirId's current situation, such as TypeckTables wrt closures.

☔ The latest upstream changes (presumably #65733) made this pull request unmergeable. Please resolve the merge conflicts.

OK, @ljedrz -- so I talked some to @eddyb about this a few days back. I asked them if they'd be willing to mentor you through their preferred approach. They said yes, but they'd have to do a bit of exploratory refactoring first. The idea was that they would create a PR that points the direction and hand if off to you to fill in the details. I think they had the impression that this would be relatively quick but then again I think they're still working on it. =)

I think my preferred outcome would be:

• close this PR
• you + @eddyb communicate a bit and discuss a revised plan
• through some combination of PRs, we execute that plan

As I think I wrote somewhere, I would also like to see that plan written out as documentation -- it seems like we should be discussing how HirId, DefId, and friends relate to one another! This is pretty fundamental stuff. But at this point my primary concern is getting you + @eddyb on same page.

I'm ok with this 👍.