HirIdification: kill off NodeId stragglers

[ljedrz]

The final stages of HirIdification (#57578).

This PR, along with #59042, should finalize the HirIdification process (at least the more straightforward bits).

• replace NodeId with HirId in trait_impls
• remove all NodeIds from borrowck
• remove all NodeIds from typeck
• remove all NodeIds from mir
• remove trait_auto_impl (unused)

I would be cool to also remove NodeId from hir::def::Def, middle::privacy::AccessLevel and hir::ItemId, but I don't know if this is feasible.

I'll be happy to do more if I've missed anything.

[rust-highfive]

r? @eddyb

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

[ljedrz]

r? @Zoxc

[Zoxc]

@bors r+

[bors]

📌 Commit 584d61a has been approved by Zoxc

[Zoxc]

I would be cool to also remove NodeId from hir::def::Def, middle::privacy::AccessLevel and hir::ItemId, but I don't know if this is feasible.

Where do you get stuck on these?

[ljedrz]

With Def::Local and Def::Upvar (first component) everything is fine until rustc_resolve where they can (as far as I can tell) only be used with a NodeId. I haven't studied all the related code that much yet, but it looks as if they might be candidates for lowering; they are used in tandem with hir::Freevar which doesn't contain a HirId.

privacy::AccessLevel - issues with save_analysis.

ItemId - HashStable debug assertion for hir::Mod fails (probably error with id lowering).

I can give them another shot and send a WIP PR for further investigation.

[Zoxc]

@ljedrz For Def, I'd make it generic over the id used and have a type Def = def::Def<HirId> type alias for it.

Feel free to open PRs for the other ones.

[ljedrz]

I've tried the generic Def approach, but it quickly becomes quite messy (especially before the stable hash derive macro goes live); is an enum containing either NodeId or HirId out of the question (though I haven't attempted that yet - it might get messy too)?

[Zoxc]

I've tried the generic Def approach, but it quickly becomes quite messy

What do you mean by messy?

(especially before the stable hash derive macro goes live)

That will land soon at least.

[ljedrz]

What do you mean by messy?

There are other objects containing Def and they also need to become generic and often additionally have some of the bounds applicable to NodeId and HirId like Debug and Display.

[eddyb]

There are other objects containing Def and they also need to become generic

How so? I suspect that you can partition most of them into "used during/produced by rustc_resolve" (Def<NodeId>) and "contained in the HIR/elsewhere" (Def<HirId>).

As an aside, I should reiterate that the Def type should be named Res or something (since it's the result of name resolution, which is not exactly "definition" in same sense as DefId)

Also, things could be slightly nicer by having the HIR represent uses of local variables (including upvars) as a separate ExprKind variant (instead of Path), but that might not be a trivial refactoring right now.

[ljedrz]

Hmm, I guess I can give it another shot as soon as #56864 lands 👍.

[mark-i-m]

#56864 has already landed?

EDIT: ah, it landed 3 hours after your post 😛

[bors]

⌛ Testing commit 584d61a with merge c3182db4e8af03d6af9504e6a5389d1d4f4e861c...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: Zoxc
Pushing c3182db4e8af03d6af9504e6a5389d1d4f4e861c to master...

[bors]

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

[Zoxc]

@bors retry

[bors]

⌛ Testing commit 584d61a with merge 0633c55...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: Zoxc
Pushing 0633c55 to master...

[Zoxc]

@ljedrz Did you try dealing with Def or should I take a shot at it?

[ljedrz]

@Zoxc I've started, but I'm traveling this week, so I might not be able to resume until the next one; if you'd prefer this to be done more quickly you can use my branch that should have most of the HIR-and-above-side changes; what's left is splitting Def (or Res) into the HIR and resolver objects.

[Zoxc]

@ljedrz I made some progress in https://github.com/Zoxc/rust/tree/hiridify_def_id, but I run into issues lowering Defs with node ids that have yet to be lowered.

[eddyb]

@Zoxc Can't you allocate the NodeId -> HirId at the first use, and have the latter lowering of the actual node in question reuse the same HirId?