rust-lang/rust

[incremental] don't use `NodeId` in the HIR, but something local to an item

nikomatsakis opened this issue · 6 comments

The current NodeId design is hostile to incremental compilation because a change anywhere in the HIR affects numbering everywhere (particularly when desugarings are taken into account). @michaelwoerister encountered problems with metadata hashing being very unstable as a result; it also makes loading/unloading more painful.

At the compiler design sprint, @michaelwoerister and @eddyb hatched a plan to make all NodeId usage in the HIR be replaced with "local ids", that are relative to an enclosing item (or something like that). I'll let them describe it here.

eddyb commented

I think @michaelwoerister started work on it, actually. If he can't get back to it, maybe there's a branch?
To avoid redoing potentially most of the work, I mean.

@eddyb I'm back to work now and I'm looking into it. There are lots of little issues to be solved.

The general plan is to have a hir::NodeId:

struct NodeId {
    owner: DefIndex,
    local_id: LocalNodeId,
}

struct LocalNodeId(u32);

This hir::NodeId still completely identifies a piece of HIR within the current crate but it has the advantage of being stable with respect to changes in unrelated items. When the owner is known from the context, we can also only store the LocalNodeId to save space and relocation effort. One example of this (I presume) would be the keys in ty::TypeckTables.

@nrc, @jonathandturner: @eddyb indicated that save analysis data operates on ast::NodeId. Do you have any concerns if we moved the HIR to the above scheme?

eddyb commented

I think the only thing you need is a mapping to be able to access HIR paths and the astconv cache.

nrc commented

indicated that save analysis data operates on ast::NodeId

we basically only use node ids for looking up type (and name resolution) info. As long as you update the save-analysis crate to use whatever replacement ids you implement to do this lookup (I assume we won't even compile if you don't) then we should be fine. Externally, save-analysis uses def ids for everything. I guess there might be some issues if we convert node ids to def ids for the local crate? I forget exactly how that works now.

I guess there might be some issues if we convert node ids to def ids for the local crate?

Not every NodeId has a corresponding DefId, but if that were a problem, you would already run into it now, I think.

We implemented HirId a while ago and it works pretty well for what we need it for.