emit StorageLive and schedule StorageDead for let-else's bindings after matching

[dianne]

This PR removes special handling of let-else, so that StorageLives are emitted and StorageDeads are scheduled only after pattern-matching has succeeded. This means StorageDeads will no longer appear for all of its bindings on the else branch (because they're not live yet) and its drops&StorageDeads will happen together like they do elsewhere, rather than having all drops first, then all StorageDeads.

This fixes #142056, and is therefore a breaking change. I believe it'll need a crater run and a T-lang nomination/fcp thereafter. Specifically, this makes drop-checking slightly more restrictive for let-else to match the behavior of other variable binding forms. An alternative approach could be to change the relative order of drops and StorageDeads for other binding forms to make drop-checking more permissive, but making that consistent would be a significantly more involved change.

r? mir
cc @dingxiangfei2009

@rustbot label +T-lang +needs-crater

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[rustbot]

Error: Label needs-crater can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[workingjubilee]

Building for crater:
@bors2 try

[rust-bors]

⌛ Trying commit c6a97d3 with merge 8a03786…

To cancel the try build, run the command @bors2 try cancel.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8a03786 (8a0378607d6b34c242e3bfa75554871be641864b, parent: d14d202262d13df896b0c624b0cec6e4bfde631a)

[workingjubilee]

@craterbot check

[craterbot]

👌 Experiment pr-143028 created and queued.
🤖 Automatically detected try build 8a03786
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-143028 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-143028 is completed!
📊 6 regressed and 3 fixed (653904 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Kivooeo]

Since crater finished it's experiment, how's overall results? I've opened a link with a full report but it seems more complicated

[dianne]

The regressed and fixed crates look wholly unrelated to this PR. I'm not too surprised, since this is a very niche breakage. It doesn't look like there will be any ecosystem impact if this is merged.

There's a few failures to download/access dependencies, what looks like an failure in a perl build script¹, and a couple docker errors. I'm not too knowledgeable about crater's infrastructure so I couldn't say why any of it is happening or whether those crates should be put on the denylist, but none of that sounds like the fault of let-else's MIR changing.

Footnotes

1. This was as part of building OpenSSL as a dependency. Nothing else depending on OpenSSL had this issue, and it's not written in Rust to begin with. ↩

[Kivooeo]

@est31, I believe you should be interested in this changes, also if you are familiar with the mir building part could you please review this?

[dianne]

To be clear, the potential breakage from this PR should only be to let-else's static semantics (in particular, the drop check) and to other analyses that take StorageLive/StorageDead statements into account. It shouldn't affect runtime-observable drop order.

I intend to open some PRs for drop order changes as well, including for let guards if we don't intend to stabilize their current behavior, but I haven't gotten around to those just yet.

[est31]

This is done in the name of consistency, right? Naively, I'd tend towards making things more permissible rather than more restrictive, and change the existing constructs instead. But probably this way, the rules are easier to describe, and it is less surprising that this code stops compiling the moment becomes non-Copy.

I suppose a crater run in the other run wouldn't bring up any breakages either because it'd strictly make more things compile, rather than less? Otherwise I'd have suggested doing it, but I suppose it's not going to bring up any results either.

if you are familiar with the mir building part could you please review this?

Actually not that familiar with it, but I can still take a look. I see @dingxiangfei2009 is already in the zulip thread, he is definitely quite familiar with it.

none of that sounds like the fault of let-else's MIR changing.

Yeah it's a bit noisy, or at least was a while ago when I was looking at results. It's entirely possible that it shows only noise. That's a good sign, meaning that it really has low impact.

[dianne]

This is done in the name of consistency, right? Naively, I'd tend towards making things more permissible rather than more restrictive, and change the existing constructs instead.

I'd prefer to make it more permissive too, but the inconsistencies are indeed tougher to iron out in that direction. Though maybe it's not an issue? The big inconsistency I can think of is that without sorting the StorageDeads for multiple patterns to be after all their drops, introducing bindings with one pattern would be more permissive than using multiple patterns. e.g.:

• let (mut needs_drop, dropless); would be more permissive than let mut needs_drop; let dropless;
• |(mut needs_drop, dropless)| ... would be more permissive than |mut needs_drop, dropless| ...

If that's not an issue, I could see about opening an alternative PR implementing that relaxed behavior instead. Having all the StorageDeads last would also be nice for an or-pattern drop order experiment I'm considering.

But probably this way, the rules are easier to describe, and it is less surprising that this code stops compiling the moment becomes non-Copy.

I don't think it'd be too much harder to describe if the drops and StorageDeads were fully de-tangled. Unless its lifetime is otherwise restricted, anything without a significant drop would live until after everything needing to be dropped is dropped (vs. the current model where everything is treated as dropped in-order even if it doesn't need to be). I guess if they're still partially tangled, locals with insignificant drops would outlive locals with significant drops introduced by the same pattern. Partial tangling would limit equational reasoning about pattern bindings, as in the examples above, but I don't know if that's a priority.

edit: actually, doing all drops before StorageDeads might be easier than I thought. I'll consider that option too.