HirId-ification initiative

[zackmdavis]

HirIds were introduced in bc259ee (March 2017) as a replacement for ast::NodeIds after HIR lowering; this is said to be good for incremental compilation. The commentary for pull request #43740 (August 2017) says:

In the future the HirId should completely replace the NodeId in HIR nodes. [...] Ideally we convert more and more code from NodeId to HirId in the future so that there are no more NodeIds after HIR lowering anywhere.

The HIR chapter of the rustc development guide similarly says "while [NodeIds] are still in common use, they are being slowly phased out" (emphasis in original).

However, a drawback of the "gradually migrate over to the new way of doing things in the course of addressing other issues" strategy is that it's all too easy for the codebase to remain in a transitional state indefinitely. Thus, perhaps we should have this dedicated issue to track progress towards not using NodeIds after HIR lowering.

[michaelwoerister]

Thanks, @zackmdavis!

However, a drawback of the "gradually migrate over to the new way of doing things in the course of addressing other issues" strategy is that it's all too easy for the codebase to remain in a transitional state indefinitely.

Yes, that's indeed what has been happening over the last year. We've done enough to make incremental compilation work and then pretty much stopped.

Btw, it might be a good idea to add a custom implementation of Hash for HirId, something like:

impl Hash for HirId {
    fn hash<H: Hasher>(&self, state: &mut H) {
        let data: u64 = unsafe { mem::transmute(*self) };
        state.write_u64(data);
    }
}

It would have to be tested if that actually speeds things up.

[mark-i-m]

Perhaps add tracking-issue label?

[ljedrz]

I'm currently working on this.

[marmeladema]

Once #72777 and #72781 have landed, rustdoc should be free of NodeId.

In order to avoid re-introducing NodeId by mistake, would it be valuable to remove all NodeId related methods in hir map? The only remaining user of NodeId seems to be save_analysis and it can technically use the underlying Definitions object to access NodeIds.

[marmeladema]

Small update:

• save_analysis has been converted to work fully on hir tree instead of mix of ast/hir in save_analysis: work on HIR tree instead of AST #72882
• as a consequence, all NodeId related APIs have been removed from the hir map in Remove unsused NodeId related APIs in hir map #72996
• post-lowering uses of NodeIds have been shrunk down further in Use LocalDefId directly in Resolver::export_map #73090

I believe the solely remaining use of NodeIds post hir lowering is the conversion of the trait map:

• rust/src/librustc_resolve/lib.rs

  Lines 1285 to 1296 in ccac43b

  	let trait_map = self
  	.trait_map
  	.into_iter()
  	.map(|(k, v)| {
  	(
  	definitions.node_id_to_hir_id(k),
  	v.into_iter()
  	.map(|tc| tc.map_import_ids(|id| definitions.node_id_to_hir_id(id)))
  	.collect(),
  	)
  	})
  	.collect();

• rust/src/librustc_resolve/lib.rs

  Lines 1323 to 1337 in ccac43b

  	trait_map: self
  	.trait_map
  	.iter()
  	.map(|(&k, v)| {
  	(
  	self.definitions.node_id_to_hir_id(k),
  	v.iter()
  	.cloned()
  	.map(|tc| {
  	tc.map_import_ids(|id| self.definitions.node_id_to_hir_id(id))
  	})
  	.collect(),
  	)
  	})
  	.collect(),

I see two approaches to solve this:

1. Either keep the trait_map in the Resolver but move the conversion as part of the lowering and store the resulting hir trait_map into the current crate. That is not great because the mapping NodeId/HirId would still be required.

2. Or build that hir trait_map directly as part of the lowering. I don't know if its easily doable or not yet.

What do you people think about all of this?

[marmeladema]

Now that #73291 has landed, the table/map to convert NodeId to/from HirId are gone. They have been replaced by direct LocalDefId to/from HirId table/map.

Next step (and possibly final step?) is to move the remaining API's involving NodeId to the Resolver which means that there will be no way of dealing with NodeId once the lowering is done.

[petrochenkov]

Closing as discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60HirId.60-ification.20initiative/near/201835747