Drop Elaboration debug-ICEs when `SimplifyCfg` is disabled

[clubby789]

#160524 (comment)
When compiling the following with debug assertions:

// rustc ice.rs -Zmir-enable-passes=-SimplifyCfg-initial,-SimplifyCfg-promote-consts,-SimplifyCfg-post-analysis
fn mir_drop<T>(_place: T) {
    panic!()
}

fn main() {
    mir_drop(());
}

We get an ICE in dataflow during drop elaboration:

thread 'rustc' panicked at compiler/rustc_mir_dataflow/src/framework/cursor.rs:120:9:
assertion failed: self.reachable_blocks.contains(block)

Full ICE

Unsure if this is an issue in practice, but there does seem to be some implicit dependencies between passes.

[clubby789]

cc @RalfJung

[saethlin]

The explanation I have heard in the past is that this assertion exists because forbidding unreachable blocks is helpful for dataflow analysis.

It really doesn't make sense to forbid unreachable blocks in the middle of an optimization pipeline. So I don't think the assertion is worth it, but I am not an expert on the dataflow code.

[RalfJung]

Yeah I tend to agree, unreachable blocks are a perfectly natural thing to have in valid well-formed MIR so we shouldn't panic on that.

Cc @rust-lang/wg-mir-opt

[oli-obk]

Let's remove the assert. Since this is an easy issue, to any new contributors who pick this up: do not use an LLM in any part of solving this issue, it's meant to teach you how to use our tooling, as the change itself is trivial.

[tmiasko]

FYI: The assertion prevents querying results from unreachable blocks by default because results are usually nonsensical. The result at the start of an unreachable block is a bottom value, but for locations within a block it still has a transfer function applied to it. To obtain bottom value consistently one can use MaybeReachable<T> as a lattice and call allow_unreachable().

[RalfJung]

I think it's quite fragile to expect the input MIR to be in a state of "no unreachable blocks".
Is there a better way to make MIR passes robust against such MIR?

[tmiasko]

I think it's quite fragile to expect the input MIR to be in a state of "no unreachable blocks".

No one is suggesting otherwise.

Is there a better way to make MIR passes robust against such MIR?

I was primarily interested in describing why those asserts are there in the first place. That being said, my comment did suggest two alternatives in addition to simply removing the asserts. To make it more explicit:

1. Make explicit decision how to handle unreachable blocks by querying results only for reachable blocks.
2. Make explicit decision how to handle unreachable blocks by using MaybeReachable lattice and allow querying unreachable blocks (this perhaps should be property of an Analysis).

One could expect that results for unreachable blocks shouldn't matter at all. Historically this was demonstrated to be false, which is why we use MaybeReachable.

[RalfJung]

I don't understand how those proposals avoid the footgun of MIR opts running into this panic. Sure, we can fix up the passes we have today to avoid querying unreachable blocks or to use MaybeReachable, but if that's the only thing we do we'll inevitably see this ICE again.

Or are you implying there is some way to do this systematically in the core analysis API without needing to audit each pass? You didn't say that so I don't expect this to be the case, but then I am left confused about your proposal.

OTOH I also don't understand the failures that can arise if a mir-opt queries the results for unreachable blocks without being prepared for that. You say "Historically this was demonstrated to be false" -- what are examples of things that go wrong? Is the argument that it's better to ICE the compiler in such cases rather than do whatever nonsense used to happen instead?

I am left guessing about what you actually want to say. Please be a bit more explicit. :)

[tmiasko]

As for fixing the issue, I think the best way forward is to change data flow results to consistently return bottom value for unreachable locations (e.g., by using MaybeReachable internally) and remove the asserts.