Stream: t-compiler

Topic: Improved span handling #47389


cjgillot (May 03 2020 at 18:04, on Zulip):

I am trying to make progress on Span handling for incremental compilation.
The point of this thread is to gauge interest, and to find the least painful way.

In the current state, spans are copied and replicated in AST, HIR and MIR. This leads to invalidation when the spans change.
@mw outlined two paths forward:

These goals can be achieved by:

Alternatively, this could also be achieved by:

Is there still interest for this?
What would be the preferred solution?

nikomatsakis (May 05 2020 at 11:48, on Zulip):

I think this is probably an important thing to work on -- I'm not sure the best design though

nikomatsakis (May 05 2020 at 11:48, on Zulip):

it'd be good to bring @matklad into the conversation a bit, I think

matklad (May 05 2020 at 11:56, on Zulip):

Yup, we are solving the same problem in rust-analyzer. I belive Coenen Benjamin is familiar with our general infra.

I wonder if I should spend some time writing up a markdown document documenting our various solutions to the problem of bidirectional mapping between source code and various lowered representations

nikomatsakis (May 05 2020 at 12:18, on Zulip):

This seems like a case where it'd be nice to be trying to migrate the two codebases closer

matklad (May 05 2020 at 13:08, on Zulip):

And also the case where even the basic vocabulary is different. Like, we don't have anything resembling span in rust-analyzer :)

cjgillot (May 05 2020 at 17:24, on Zulip):

I have two exploratory branches.

The first one makes the error handling system take an abstract span SpanId. There are some diagnostics that just pass around a span without manipulating it. The idea is to make an enum SpanId { Span(Span), DefSpan(DefId) };. The calls to tcx.def_span(did) would just return SpanId::DefSpan(did). Diagnostic tracking would register emitted diagnostics using this abstract representation, and translate it to concrete spans.

The second one attempts at removing the Spans from the HIR. The spans are collected during lowering into a HirId -> Span mapping. HIR consumers would need to query this map in order to formulate diagnostics.

Both approaches are invasive, and touch a lot of code. All I have is very WIP. This maybe requires a review of uses of Spans in the compiler. Especially, when those spans end up not being used for diagnostic emission.

cjgillot (May 09 2020 at 12:14, on Zulip):

I posted #72015 which aims at removing the Spans from the HIR. The point is to reduce HIR invalidation by span modification as much as possible. For now, only nodes that have HirIds have been migrated, and its already a mess.

oli (May 09 2020 at 12:35, on Zulip):

I'm going through the commits now, what do you mean by "it's already a mess"?

cjgillot (May 09 2020 at 13:45, on Zulip):

Well, the diff is horribly large (+3000/-2700), and touches pretty much all HIR consumers. At some point, I did it mechanically, and stopped caring for redundant accesses to the HirId -> Span map.

oli (May 11 2020 at 10:04, on Zulip):

so you're worried about perf losses due to the redundant accesses?

oli (May 11 2020 at 10:06, on Zulip):

I'd expect the diff for the full McCoy to be large, but as you've shown, this can be done incrementally, so maybe we should run perf on your change and see if it's ok, and if so, merge a small version and then the rest of the changes can be merged step by step. I think we'll also need to figure out how to handle spans in MIR at some point.

cjgillot (May 11 2020 at 14:53, on Zulip):

The point is to get better perf in incremental, so we shouldn't be careless in the accesses. Also, there are still a lot of spans in HIR, for nodes without HirIds. As I am not fully aware of the semantics of HirIds, I did not try to introduce more. Is there a blocker to adding HirIds everywhere?

oli (May 11 2020 at 17:08, on Zulip):

I think not, there was the "great hirification": https://github.com/rust-lang/rust/issues?q=hiridification+is%3Aclosed+ so... Let's ask the @ljedrz (they aren't on zulip it seems :confused:)

oli (May 11 2020 at 17:09, on Zulip):

maybe open an issue about it and/or ping them on the PR?

ecstatic-morse (May 11 2020 at 17:33, on Zulip):

I think one blocker was #71104, but it's actively being worked on. Thanks @marmeladema!!!

marmeladema (May 11 2020 at 17:37, on Zulip):

Yes i am trying to make progress! But as I am a new contributor, i takes me a bit of time to understand this complex machinery :)

ecstatic-morse (May 11 2020 at 17:42, on Zulip):

You're doing a great job so far IMO. I really appreciate your work on #70853

Last update: May 29 2020 at 18:00UTC