Deferred ICEs (i.e. delay_span_bug) are not preserved by incremental.

[eddyb]

When compiling a delay_span_bug testcase (e.g. the last snippet in #63154 (comment) on the current nightly, or any of the other examples in that issue on older compilers) with -Cincremental (or Cargo which does that by default), only the initial run will ICE.

Subsequent runs will succeed without an ICE, in the absence of any regular errors. However, changing the relevant code (e.g. adding a space) can cause the query that calls delay_span_bug to be recomputed and bring the ICE back.

IIUC, incremental compilation stores diagnostics and replays them on subsequent runs, but that appears to not apply to delay_span_bug ICEs.

cc @michaelwoerister

[eddyb]

It looks like without incremental we output artifacts (like rlib).
Do we do this in case of errors too? It seems bad to produce artifacts after an ICE.

[Centril]

Nominating and proposing P-high (mostly because of potential annoyance factor).

This should possibly also be labeled I-unsound under the assumption that under the general case of this issue, we might produce artifacts under subsequent runs that may produce UB traces. (However, it is probable that the user will notice the ICE at some other point in time and therefore discard the artifact.)

[michaelwoerister]

The current expected behavior of the compiler is to not save any incremental state if an error occurs. delay_span_bug may slip through the cracks here.

[Centril]

@michaelwoerister In that case, it seems to me like it's a matter of recording the fact that we did a delay_span_bug (which we already do, https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.HandlerInner.html#structfield.delayed_span_bugs) and then have this mean "there was an error" in terms of "should we save incremental state?".

[michaelwoerister]

Yep, the following check should also take delay_span_bug into account:

rust/src/librustc_incremental/persist/fs.rs

Line 310 in cf048cc

if sess.has_errors() {

While we are at it, we could also add a similar check to rustc_incremental::save_dep_graph() and rustc_incremental::save_work_product_index(), as not to waste time producing values that will then be discarded anyway:

rust/src/librustc_incremental/persist/save.rs

Line 18 in cf048cc

pub fn save_dep_graph(tcx: TyCtxt<'_>) {

[traxys]

I'd like to start contributing to rustc and I wanted to ask if this is a good issue to start on (I have read the rustc book)

[Centril]

@traxys Sure, this is a decently good issue to start on and should be pretty interesting in terms of learning the infrastructure. If you get stuck, need help, and want to talk synchronously you can hop into https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.2365401.20delay_span_bug.20not.20preserved.20by.20incremental and we'll try to help out.

[michaelwoerister]

#65470 fixes this issue. It would still be good to have a test case for it.

[traxys]

I have started to write the code making this test possible, I'll open a PR shortly