Rollup merge of rust-lang#143028 - dianne:let-else-storage, r=oli-obk,traviscross

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

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

This fixes rust-lang#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 `StorageDead`s 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

Author: Zalathar

compiler/rustc_mir_build/src/builder/block.rs

@@ -6,7 +6,7 @@ use rustc_span::Span;
 use tracing::debug;
 
 use crate::builder::ForGuard::OutsideGuard;
-use crate::builder::matches::{DeclareLetBindings, EmitStorageLive, ScheduleDrops};
+use crate::builder::matches::{DeclareLetBindings, ScheduleDrops};
 use crate::builder::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
@@ -199,15 +199,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             None,
                             Some((Some(&destination), initializer_span)),
                         );
-                        this.visit_primary_bindings(pattern, &mut |this, node, span| {
-                            this.storage_live_binding(
-                                block,
-                                node,
-                                span,
-                                OutsideGuard,
-                                ScheduleDrops::Yes,
-                            );
-                        });
                         let else_block_span = this.thir[*else_block].span;
                         let (matching, failure) =
                             this.in_if_then_scope(last_remainder_scope, else_block_span, |this| {
@@ -218,7 +209,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                                     None,
                                     initializer_span,
                                     DeclareLetBindings::No,
-                                    EmitStorageLive::No,
                                 )
                             });
                         matching.and(failure)

compiler/rustc_mir_build/src/builder/matches/mod.rs

@@ -69,18 +69,6 @@ pub(crate) enum DeclareLetBindings {
     LetNotPermitted,
 }
 
-/// Used by [`Builder::bind_matched_candidate_for_arm_body`] to determine
-/// whether or not to call [`Builder::storage_live_binding`] to emit
-/// [`StatementKind::StorageLive`].
-#[derive(Clone, Copy)]
-pub(crate) enum EmitStorageLive {
-    /// Yes, emit `StorageLive` as normal.
-    Yes,
-    /// No, don't emit `StorageLive`. The caller has taken responsibility for
-    /// emitting `StorageLive` as appropriate.
-    No,
-}
-
 /// Used by [`Builder::storage_live_binding`] and [`Builder::bind_matched_candidate_for_arm_body`]
 /// to decide whether to schedule drops.
 #[derive(Clone, Copy, Debug)]
@@ -207,7 +195,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 Some(args.variable_source_info.scope),
                 args.variable_source_info.span,
                 args.declare_let_bindings,
-                EmitStorageLive::Yes,
             ),
             _ => {
                 let mut block = block;
@@ -479,7 +466,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         &built_match_tree.fake_borrow_temps,
                         scrutinee_span,
                         Some((arm, match_scope)),
-                        EmitStorageLive::Yes,
                     );
 
                     this.fixed_temps_scope = old_dedup_scope;
@@ -533,7 +519,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         fake_borrow_temps: &[(Place<'tcx>, Local, FakeBorrowKind)],
         scrutinee_span: Span,
         arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>,
-        emit_storage_live: EmitStorageLive,
     ) -> BasicBlock {
         if branch.sub_branches.len() == 1 {
             let [sub_branch] = branch.sub_branches.try_into().unwrap();
@@ -544,7 +529,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 scrutinee_span,
                 arm_match_scope,
                 ScheduleDrops::Yes,
-                emit_storage_live,
             )
         } else {
             // It's helpful to avoid scheduling drops multiple times to save
@@ -577,7 +561,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     scrutinee_span,
                     arm_match_scope,
                     schedule_drops,
-                    emit_storage_live,
                 );
                 if arm.is_none() {
                     schedule_drops = ScheduleDrops::No;
@@ -741,7 +724,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             &[],
             irrefutable_pat.span,
             None,
-            EmitStorageLive::Yes,
         )
         .unit()
     }
@@ -2364,7 +2346,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         source_scope: Option<SourceScope>,
         scope_span: Span,
         declare_let_bindings: DeclareLetBindings,
-        emit_storage_live: EmitStorageLive,
     ) -> BlockAnd<()> {
         let expr_span = self.thir[expr_id].span;
         let scrutinee = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span));
@@ -2398,14 +2379,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             }
         }
 
-        let success = self.bind_pattern(
-            self.source_info(pat.span),
-            branch,
-            &[],
-            expr_span,
-            None,
-            emit_storage_live,
-        );
+        let success = self.bind_pattern(self.source_info(pat.span), branch, &[], expr_span, None);
 
         // If branch coverage is enabled, record this branch.
         self.visit_coverage_conditional_let(pat, success, built_tree.otherwise_block);
@@ -2428,7 +2402,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         scrutinee_span: Span,
         arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>,
         schedule_drops: ScheduleDrops,
-        emit_storage_live: EmitStorageLive,
     ) -> BasicBlock {
         debug!("bind_and_guard_matched_candidate(subbranch={:?})", sub_branch);
 
@@ -2547,7 +2520,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 post_guard_block,
                 ScheduleDrops::Yes,
                 by_value_bindings,
-                emit_storage_live,
             );
 
             post_guard_block
@@ -2559,7 +2531,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block,
                 schedule_drops,
                 sub_branch.bindings.iter(),
-                emit_storage_live,
             );
             block
         }
@@ -2730,7 +2701,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         block: BasicBlock,
         schedule_drops: ScheduleDrops,
         bindings: impl IntoIterator<Item = &'b Binding<'tcx>>,
-        emit_storage_live: EmitStorageLive,
     ) where
         'tcx: 'b,
     {
@@ -2740,19 +2710,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // Assign each of the bindings. This may trigger moves out of the candidate.
         for binding in bindings {
             let source_info = self.source_info(binding.span);
-            let local = match emit_storage_live {
-                // Here storages are already alive, probably because this is a binding
-                // from let-else.
-                // We just need to schedule drop for the value.
-                EmitStorageLive::No => self.var_local_id(binding.var_id, OutsideGuard).into(),
-                EmitStorageLive::Yes => self.storage_live_binding(
-                    block,
-                    binding.var_id,
-                    binding.span,
-                    OutsideGuard,
-                    schedule_drops,
-                ),
-            };
+            let local = self.storage_live_binding(
+                block,
+                binding.var_id,
+                binding.span,
+                OutsideGuard,
+                schedule_drops,
+            );
             if matches!(schedule_drops, ScheduleDrops::Yes) {
                 self.schedule_drop_for_binding(binding.var_id, binding.span, OutsideGuard);
             }

tests/mir-opt/building/issue_101867.main.built.after.mir

@@ -24,7 +24,6 @@ fn main() -> () {
         _1 = Option::<u8>::Some(const 1_u8);
         FakeRead(ForLet(None), _1);
         AscribeUserType(_1, o, UserTypeProjection { base: UserType(1), projs: [] });
-        StorageLive(_5);
         PlaceMention(_1);
         _6 = discriminant(_1);
         switchInt(move _6) -> [1: bb4, otherwise: bb3];
@@ -55,6 +54,7 @@ fn main() -> () {
     }
 
     bb6: {
+        StorageLive(_5);
         _5 = copy ((_1 as Some).0: u8);
         _0 = const ();
         StorageDead(_5);
@@ -63,7 +63,6 @@ fn main() -> () {
     }
 
     bb7: {
-        StorageDead(_5);
         goto -> bb1;
     }
 

tests/mir-opt/building/user_type_annotations.let_else.built.after.mir

@@ -21,9 +21,6 @@ fn let_else() -> () {
     }
 
     bb0: {
-        StorageLive(_2);
-        StorageLive(_3);
-        StorageLive(_4);
         StorageLive(_5);
         StorageLive(_6);
         StorageLive(_7);
@@ -51,26 +48,26 @@ fn let_else() -> () {
 
     bb4: {
         AscribeUserType(_5, +, UserTypeProjection { base: UserType(1), projs: [] });
+        StorageLive(_2);
         _2 = copy (_5.0: u32);
+        StorageLive(_3);
         _3 = copy (_5.1: u64);
+        StorageLive(_4);
         _4 = copy (_5.2: &char);
         StorageDead(_7);
         StorageDead(_5);
         _0 = const ();
-        StorageDead(_8);
         StorageDead(_4);
         StorageDead(_3);
         StorageDead(_2);
+        StorageDead(_8);
         return;
     }
 
     bb5: {
         StorageDead(_7);
         StorageDead(_5);
         StorageDead(_8);
-        StorageDead(_4);
-        StorageDead(_3);
-        StorageDead(_2);
         goto -> bb1;
     }
 

tests/ui/dropck/let-else-more-permissive.rs

@@ -1,5 +1,5 @@
-//! The drop check is currently more permissive when `let` statements have an `else` block, due to
-//! scheduling drops for bindings' storage before pattern-matching (#142056).
+//! Regression test for #142056. The drop check used to be more permissive for `let` statements with
+//! `else` blocks, due to scheduling drops for bindings' storage before pattern-matching.
 
 struct Struct<T>(T);
 impl<T> Drop for Struct<T> {
@@ -14,10 +14,11 @@ fn main() {
         //~^ ERROR `short1` does not live long enough
     }
     {
-        // This is OK: `short2`'s storage is live until after `long2`'s drop runs.
+        // This was OK: `short2`'s storage was live until after `long2`'s drop ran.
         #[expect(irrefutable_let_patterns)]
         let (mut long2, short2) = (Struct(&0), 1) else { unreachable!() };
         long2.0 = &short2;
+        //~^ ERROR `short2` does not live long enough
     }
     {
         // Sanity check: `short3`'s drop is significant; it's dropped before `long3`:

tests/ui/dropck/let-else-more-permissive.stderr

@@ -14,8 +14,24 @@ LL |     }
    |
    = note: values in a scope are dropped in the opposite order they are defined
 
+error[E0597]: `short2` does not live long enough
+  --> $DIR/let-else-more-permissive.rs:20:19
+   |
+LL |         let (mut long2, short2) = (Struct(&0), 1) else { unreachable!() };
+   |                         ------ binding `short2` declared here
+LL |         long2.0 = &short2;
+   |                   ^^^^^^^ borrowed value does not live long enough
+LL |
+LL |     }
+   |     -
+   |     |
+   |     `short2` dropped here while still borrowed
+   |     borrow might be used here, when `long2` is dropped and runs the `Drop` code for type `Struct`
+   |
+   = note: values in a scope are dropped in the opposite order they are defined
+
 error[E0597]: `short3` does not live long enough
-  --> $DIR/let-else-more-permissive.rs:27:19
+  --> $DIR/let-else-more-permissive.rs:28:19
    |
 LL |         let (mut long3, short3) = (Struct(&tmp), Box::new(1)) else { unreachable!() };
    |                         ------ binding `short3` declared here
@@ -30,6 +46,6 @@ LL |     }
    |
    = note: values in a scope are dropped in the opposite order they are defined
 
-error: aborting due to 2 previous errors
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0597`.