HIR: introduce a HirId to DefId map in Definitions

[ljedrz]

Split out from #62975.

Introduce a HirId to DefIndex map in map::Definitions; this introduces a little bit of extra complexity, but could result in a performance win. I'd do a perf run to check if it's worth it.

[rust-highfive]

r? @cramertj

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

[ljedrz]

@bors try

[bors]

⌛ Trying commit f71703abf45b28712dc34890d85d67991f8400a4 with merge 8a50d3d384210ef535686e4c3804884fede9a4de...

[bors]

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

[cramertj]

r? @Zoxc

[bjorn3]

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

[ljedrz]

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

[Mark-Simulacrum]

@rust-timer build 8a50d3d384210ef535686e4c3804884fede9a4de

Feel free to ping me if you need a perf run

[rust-timer]

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

[rust-timer]

Finished benchmarking try commit 8a50d3d384210ef535686e4c3804884fede9a4de, comparison URL.

[ljedrz]

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

[joelpalmer]

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.

[Alexendoo]

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

[nikomatsakis]

r? @nikomatsakis

[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.

[ljedrz]

@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?

[nikomatsakis]

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

[nikomatsakis]

@ljedrz -- the code here looks good! But one thing I was missing is a bit of context. Maybe you can add a comment that helps to clarify why the HirId-DefId mapping for these cases is different?

Clearly, it has to do with defs that are "synthesized" during lowering, but if you could say a bit more I think it'd be helpful for future readers. :)

[ljedrz]

@nikomatsakis I rebased and expanded on the previous comments.

[eddyb]

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).

[nikomatsakis]

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

[eddyb]

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.

[michaelwoerister]

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.

[nikomatsakis]

@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.))

[ljedrz]

@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.

[eddyb]

@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):

rust/src/librustc/hir/def.rs

Lines 50 to 83 in 0221e26

	pub enum DefKind {
	// Type namespace
	Mod,
	/// Refers to the struct itself, `DefKind::Ctor` refers to its constructor if it exists.
	Struct,
	Union,
	Enum,
	/// Refers to the variant itself, `DefKind::Ctor` refers to its constructor if it exists.
	Variant,
	Trait,
	/// `type Foo = impl Bar;`
	OpaqueTy,
	/// `type Foo = Bar;`
	TyAlias,
	ForeignTy,
	TraitAlias,
	AssocTy,
	/// `type Foo = impl Bar;`
	AssocOpaqueTy,
	TyParam,
	// Value namespace
	Fn,
	Const,
	ConstParam,
	Static,
	/// Refers to the struct or enum variant's constructor.
	Ctor(CtorOf, CtorKind),
	Method,
	AssocConst,
	// Macro namespace
	Macro(MacroKind),
	}

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).

[JohnCSimon]

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

[wirelessringo]

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

[nikomatsakis]

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 =)

[eddyb]

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.

[bors]

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

[nikomatsakis]

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.

[ljedrz]

I'm ok with this 👍.