Reorder nesting scopes and declare bindings without drop schedule

[dingxiangfei2009]

Fix #99228
Fix #99975

Storages are previously not declared before entering the else block of a let .. else statement. However, when breaking out of the pattern matching into the else block, those storages are recorded as scheduled for drops. This is not expected.

This MR fixes this issue by not scheduling the drops for those storages.

cc @est31

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[est31]

The test for #99228 is good, but it should also have a test for #99975. Can you add a test with the file name src/test/ui/let-else/issue-99975.rs with the content from my comment in addition to // compile-flags: -C opt-level=3 at the top of the file?

[dingxiangfei2009]

Right. A test is added for that issue now.

[est31]

@dingxiangfei2009 thanks! Looks good from the tests point of view now. For the implementation, I can't comment much on that.

[dingxiangfei2009]

@est31 I see. Which reviewer do you think has the capacity to review this work at the moment? Hopefully this is not blocking the stabilisation effort.

[est31]

Yes it is blocking stabilization. Depending on how quickly this PR gets merged it depends whether let else makes it into 1.65 or 1.66. Generally I like let else stable as quickly as possible, but one release give or take won't be the end of it, and rushing things is usually not worth it (little nudges are fine but only up till a limit, you shouldn't stress people or stuff). The train model was designed precisely to prevent such rushing/pressure.

For the reviewers, you might try @oli-obk who has approved your earlier let else PRs but according to their github activity, they don't seem to work on weekends, so a review will probably only happen on monday. The current reviewer jackh726 is actually available as well, they have approved another PR 17 hours ago, but maybe it's not their area of expertise, idk.

[jackh726]

Yeah, not really my area of expertise, but I'll try to review this over the weekend.

[jackh726]

It honestly would be helpful to either have a test with mir output that covers this, or an example with the expected mir output in comments here.

[dingxiangfei2009]

Let me try both. 😄

[dingxiangfei2009]

Speeking too soon. It seems not possible to dump MIR in ui tests yet. I have added explanatory comments here instead.

[est31]

There is a list of MIR based tests in src/test/mir-opt. Despite the name of the directory, not all are about optimization. You can check out src/test/mir-opt/storage_ranges.rs for an example that just checks the generated MIR. It is also explained here.

[jackh726]

This seems weird to me. I would expect that Rc::new(()) would get dropped at the end of the else block, not the end of the function?

[dingxiangfei2009]

Sounds right. Indeed this is not expected as Rc::new(()) should be a temporary value and its lifetime terminates at the end of the else block.

else block should be marked as enclosed in a terminating scope. This should fix this bug.

[dingxiangfei2009]

Okay, now the temporaries are properly contained within the else block now.

[jackh726]

Beautiful diagram :)

[jackh726]

@bors r+

[bors]

📌 Commit d2bde7d has been approved by jackh726

It is now in the queue for this repository.

[bors]

☔ The latest upstream changes (presumably #101805) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

⌛ Testing commit 4a5d2a5 with merge 35a0407...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 35a0407 to master...

[rust-highfive]

📣 Toolstate changed by #101410!

Tested on commit 35a0407.
Direct link to PR: #101410

💔 miri on windows: test-pass → test-fail (cc @RalfJung @oli-obk).
💔 miri on linux: test-pass → test-fail (cc @RalfJung @oli-obk).

[rust-timer]

Finished benchmarking commit (35a0407): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	4.0%	[2.7%, 5.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[3.1%, 3.1%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-6.1%	[-10.0%, -2.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.3%	[-10.0%, 2.3%]	3

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²

[jackh726]

It's No Tools Breakage Week and this seems to have broken miri. So likely unlikely we can get a fix for miri fast, we'll need to revert this for now.

[jackh726]

@Mark-Simulacrum can you confirm the above?

[est31]

@jackh726 miri is a nightly only tool, no? And according to your link they are exempt?

[jackh726]

I'm not sure, maybe!

[saethlin]

This definitely breaks Miri. But the way it breaks Miri is strange to me. Miri now reports this on any program:

error: Undefined Behavior: StorageLive on a local that was already live
    --> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:1058:30
     |
1058 |                     let Some(id) = last.checked_add(1) else {
     |                              ^^ StorageLive on a local that was already live
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE:
     = note: inside `std::thread::ThreadId::new` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:1058:30
     = note: inside `std::thread::Thread::new` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:1159:43
     = note: inside `std::rt::init` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:104:22
     = note: inside closure at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:147:42
     = note: inside `std::panicking::try::do_call::<[closure@std::rt::lang_start_internal::{closure#1}], ()>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:492:40
     = note: inside `std::panicking::try::<(), [closure@std::rt::lang_start_internal::{closure#1}]>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:456:19
     = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#1}], ()>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panic.rs:137:14
     = note: inside `std::rt::lang_start_internal` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:147:5
     = note: inside `std::rt::lang_start::<()>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:165:17

error: aborting due to previous error

[est31]

Hmm that's interesting. I hope that it's miri being wrong and it's not UB...

[saethlin]

I'm pretty sure this is UB. Consider this program:

#![feature(let_else)]
fn main() {
    let x: Option<u8> = Some(1);
    let Some(y) = x else {
        panic!();
    };
}

After this PR, it compiles to this MIR:

fn main() -> () {
    let mut _0: ();                      // return place in scope 0 at main.rs:2:11: 2:11
    let _1: std::option::Option<u8>;     // in scope 0 at main.rs:3:9: 3:10
    let mut _2: !;                       // in scope 0 at main.rs:4:26: 6:6
    let _3: ();                          // in scope 0 at /home/ben/rust-master/library/std/src/panic.rs:19:9: 19:50
    let mut _4: !;                       // in scope 0 at /home/ben/rust-master/library/std/src/panic.rs:19:9: 19:50
    let mut _6: isize;                   // in scope 0 at main.rs:4:9: 4:16
    scope 1 {
        debug x => _1;                   // in scope 1 at main.rs:3:9: 3:10
        let _5: u8;                      // in scope 1 at main.rs:4:14: 4:15
        scope 2 {
            debug y => _5;               // in scope 2 at main.rs:4:14: 4:15
        }
    }

    bb0: {
        StorageLive(_1);                 // scope 0 at main.rs:3:9: 3:10
        Deinit(_1);                      // scope 0 at main.rs:3:25: 3:32
        ((_1 as Some).0: u8) = const 1_u8; // scope 0 at main.rs:3:25: 3:32
        discriminant(_1) = 1;            // scope 0 at main.rs:3:25: 3:32
        StorageLive(_5);                 // scope 1 at main.rs:4:14: 4:15
        _6 = discriminant(_1);           // scope 1 at main.rs:4:19: 4:20
        switchInt(move _6) -> [1_isize: bb1, otherwise: bb2]; // scope 1 at main.rs:4:9: 4:16
    }

    bb1: {
        StorageLive(_5);                 // scope 1 at main.rs:4:14: 4:15
        _5 = ((_1 as Some).0: u8);       // scope 1 at main.rs:4:14: 4:15
        _0 = const ();                   // scope 0 at main.rs:2:11: 7:2
        StorageDead(_5);                 // scope 1 at main.rs:7:1: 7:2
        StorageDead(_1);                 // scope 0 at main.rs:7:1: 7:2
        return;                          // scope 0 at main.rs:7:2: 7:2
    }

    bb2: {
        StorageDead(_5);                 // scope 1 at main.rs:7:1: 7:2
        StorageLive(_3);                 // scope 1 at /home/ben/rust-master/library/std/src/panic.rs:18:12: 20:6
        StorageLive(_4);                 // scope 1 at /home/ben/rust-master/library/std/src/panic.rs:19:9: 19:50
        _4 = begin_panic::<&str>(const "explicit panic"); // scope 1 at /home/ben/rust-master/library/std/src/panic.rs:19:9: 19:50
                                         // mir::Constant
                                         // + span: /home/ben/rust-master/library/std/src/panic.rs:19:9: 19:32
                                         // + literal: Const { ty: fn(&str) -> ! {begin_panic::<&str>}, val: Value(<ZST>) }
                                         // mir::Constant
                                         // + span: /home/ben/rust-master/library/std/src/panic.rs:19:33: 19:49
                                         // + literal: Const { ty: &str, val: Value(Slice(..)) }
    }
}

If we branch from bb0 to bb1, we StorageLive(_5) twice, and that's UB.

[est31]

Mhh yeah that's convincing.

[thomcc]

Someone should probably cut a PR reverting this.

[est31]

I would revert only after there has been a more thorough look on the problem. Maybe the fix is simple. Such a simple fix can then be backported to beta. The revert, if needed, can also easily be backported.

[dingxiangfei2009]

@est31 Ah okay, I did not know that we cannot declare storages live twice. I have one idea, but it needs to touch the bind_matched_candidate_for_arm_body. I am taking a look.

[RalfJung]

I did not know that we cannot declare storages live twice.

See https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/enum.StatementKind.html#variant.StorageLive for more details, and #99160 for further discussion.