Separate fn signature from fn body in HIR

[nikomatsakis]

Currently, getting access to a HIR item gives you access to both the function signature and its body. This makes the incremental compilation graph more coarse than it should be. To fix this, the best thing would be to separate things out so that the item is just the external signature, and you have to separately load (e.g., from the HIR map) the body for a fn item. This way we could record that you actually accessed the fn body and track that separately.

[eddyb]

The body is a block and those are in the HIR map already, so I think the only missing thing is keeping an ID instead of a reference, and looking up that ID every time.
Not being able to use references without having implicit dependencies does seem like a bit of a limitation, which will impact the future design of HIR.

[flodiebold]

I'm currently looking into this, and I think I've got the general idea, but I've got a few questions to make sure I'm on the right track:

• I've found three places where functions or methods are defined in the HIR: TraitItem_::MethodTraitItem, ImplItemKind::Method, and Item_::ItemFn. The plan would basically be to replace the P<Block> in each of these by a NodeId, right? (By the way, is there a reason ImplItemKind isn't named ImplItem_?)
• We'll need some other place to store the top-level blocks. Would the Crate be right for this, like for items?
• Or maybe removing the P<Block> from the item is the wrong approach. Could we instead just make it private, and make sure that accesses are recorded that way?
• Should I wrap the NodeId of the block in a BlockId, similar to the ItemId?
• How should visitors be handled? I could add a method visit_fn_block similar to visit_nested_item, though I'm not sure if it should by default visit the block or not.
• Anything else I'm completely missing?

[nikomatsakis]

@flodiebold great! Sorry I haven't had a chance to respond yet. Let me start by giving a few quick answers, I only have a few minutes just now.

The plan would basically be to replace the P in each of these by a NodeId, right?

I would expect it to be a different kind of id (e.g., MethodBodyId or DefId), but a NodeId might also be fine.

By the way, is there a reason ImplItemKind isn't named ImplItem_?

The _ is just an older naming convention. I prefer Kind.

We'll need some other place to store the top-level blocks. Would the Crate be right for this, like for items?

Yes, I think so.

Could we instead just make it private, and make sure that accesses are recorded that way?

Well, we could, but we don't use privacy much for LIR; I would be inclined to move it into separate array myself.

Should I wrap the NodeId of the block in a BlockId, similar to the ItemId?

See above. I think it probably makes sense, just so that BodyId (or whatver) can be a small integer that directly indexes a vector.

How should visitors be handled? I could add a method visit_fn_block similar to visit_nested_item, though I'm not sure if it should by default visit the block or not.

Yeah, that's a tricky one. I'd say add the visitor and -- to start -- visit the block by default. That should preserve the existing behavior, and you can test and make sure everything is working. Then we can debate changing the default; it'd require going through the visitors a bit to see what they do.

[eddyb]

If I may make a suggestion, there is a more useful abstraction here than function bodies, that I keep needing time and time again, that is, definitions that have associated MIR (just "bodies"?).
This includes all function definitions, closure expressions, array lengths in types and enum discriminants.
It's a bit annoying to visit them, and you end up with ugliness like this.

[nikomatsakis]

@eddyb interesting. It seems like all the same benefits for incr comp would apply to most of those cases too. I wonder if the right type for these "bodies" would be expressions and not blocks, so that they can be used everywhere.

[eddyb]

So I got stuck in a place where I really really needed an unified way to refer to those bodies, so I tried moving all of them to hir::Expr. Which went well (yay deduplication) except for middle::region.

For some reason, if I have |...| expr instead of |...| {expr} (which is what the parser always did), there's a missing Misc CodeExtent (for expr.id?) so rustc_mir::build::scope ICEs sometimes.

It's not clear to me as to why this would be the case, since expr.id is marked as a terminating scope, and then resolve_expr calls visitor.new_node_extent_with_dtor(expr.id) which for terminating scopes it creates a DestructionScope - ooooh. I think I got it? The MIR builder is looking for Misc instead.

I might be able to fix this now, although I'm still not sure what the correct behavior should be, really.
For the record: at the time of the writing, the commit I'm talking about is eddyb@cc18ef0.

[nikomatsakis]

@eddyb I think I expect there to be both a Misc and a DestructionScope, one nested inside the other

[eddyb]

Update: it was these few lines which are both no longer necessary and require a Block for each function (but closures now don't have one so it kept going up reaching the outer function).

[nikomatsakis]

@flodiebold (so, I want to clarify something. I think that we should consider factoring out statics, constants, closure bodies, etc as a second step -- it makes sense to just focus on fn bodies first, we can refactor as a follow-up if we choose.)

[eddyb]

This is more complex than I expected, but I hope to have one or two commits soon that can be cherry-picked.

[eddyb]

@flodiebold Here you go: eddyb@203ccaf - should apply relatively cleanly on master.

I wanted to add a visit_body method to intravisit::Visitor but I have to do other things first.

[flodiebold]

Great! I'm taking a look.

[flodiebold]

@eddyb Ok, I've cherry-picked the commit without big conflicts. But I'm now getting two spurious 'unreachable statement' warnings in libsyntax when compiling stage 2, failing the build :( The lines the compiler says are unreachable are here and here. check-stage1-rpass works, and I haven't managed to make a small reproduction so far. So this is rather hard to debug for me, and I hope you have an idea... It seems the problem also occurs in your branch, so it's not just a merge problem.