Fix StorageDead on unwind paths and add MIR transform to remove them

- Add StorageDead to unwind paths for all functions (not just coroutines)
- Modify CleanupPostBorrowck to remove StorageDead from cleanup blocks
- Add tests for the fix and StorageDead removal
- Fixes #147875

Author: sladyn98

compiler/rustc_mir_build/src/builder/scope.rs

@@ -257,7 +257,9 @@ struct DropNodeKey {
 impl Scope {
     /// Whether there's anything to do for the cleanup path, that is,
     /// when unwinding through this scope. This includes destructors
-    /// and StorageDead statements to maintain proper drop ordering.
+    /// (Value and ForLint drops). StorageDead drops are not included
+    /// here because they don't require cleanup blocks - they're only
+    /// needed for borrow-checking and are removed before codegen.
     fn needs_cleanup(&self) -> bool {
         self.drops.iter().any(|drop| match drop.kind {
             DropKind::Value | DropKind::ForLint => true,
@@ -1655,17 +1657,36 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
         let is_coroutine = self.coroutine.is_some();
         // Check if there's a cleanup path (i.e., Value or ForLint drops that require cleanup)
-        let has_cleanup_path = self.scopes.scopes[uncached_scope..=target]
-            .iter()
-            .any(|scope| scope.drops.iter().any(|d| d.kind == DropKind::Value || d.kind == DropKind::ForLint));
+        let has_cleanup_path = self.scopes.scopes[uncached_scope..=target].iter().any(|scope| {
+            scope.drops.iter().any(|d| d.kind == DropKind::Value || d.kind == DropKind::ForLint)
+        });
         for scope in &mut self.scopes.scopes[uncached_scope..=target] {
             for drop in &scope.drops {
-                // Add all drops to unwind_drops for all functions (not just coroutines)
-                // to maintain proper drop ordering for borrow-checking. This matches
-                // the behavior in build_exit_tree.
-                // For coroutines, we always add all drops. For other functions, we now
-                // also add StorageDead and ForLint drops, but only when there's a cleanup path.
-                if is_coroutine || drop.kind == DropKind::Value || drop.kind == DropKind::ForLint || (drop.kind == DropKind::Storage && has_cleanup_path) {
+                // We add drops to unwind_drops to ensure the borrow-checker treats locals as dead
+                // at the same point on all paths (normal and unwind). This is for static semantics
+                // (borrow-checking), not runtime drop order.
+                //
+                // StorageDead drops are only needed when there are Value or ForLint drops present,
+                // because:
+                // 1. StorageDead is only relevant for borrow-checking when there are destructors
+                //    that might reference the dead variable
+                // 2. If there are no drops, there's no unwind path to emit StorageDead on
+                // 3. StorageDead on unwind paths ensures the borrow-checker correctly tracks
+                //    when variables are dead for the purpose of checking whether they might be
+                //    referenced by destructors
+                //
+                // These StorageDead statements are removed by the `RemoveStorageMarkers` MIR
+                // transform pass before codegen, so they don't affect LLVM output. They only
+                // affect static analysis (borrow-checking).
+                //
+                // For coroutines, we always add all drops. For other functions, we add:
+                // - Value and ForLint drops (always, as they require cleanup)
+                // - StorageDead drops (only when there's a cleanup path, for borrow-checking)
+                if is_coroutine
+                    || drop.kind == DropKind::Value
+                    || drop.kind == DropKind::ForLint
+                    || (drop.kind == DropKind::Storage && has_cleanup_path)
+                {
                     cached_drop = self.scopes.unwind_drops.add_drop(*drop, cached_drop);
                 }
             }
@@ -1885,9 +1906,13 @@ where
     // another set of arrows).
     //
     // We unwind from a drop on a local to its StorageDead statement for all
-    // functions (not just coroutines) to maintain proper drop ordering for
-    // borrow-checking. The drops for the unwind path should have already been
-    // generated by `diverge_cleanup_gen`.
+    // functions (not just coroutines) to ensure the borrow-checker treats locals
+    // as dead at the same point on all paths. This is for static semantics
+    // (borrow-checking), not runtime drop order. The drops for the unwind path
+    // should have already been generated by `diverge_cleanup_gen`.
+    //
+    // Note: StorageDead statements are removed by the `RemoveStorageMarkers` MIR
+    // transform pass before codegen, so they don't affect LLVM output.
 
     // `unwind_to` indicates what needs to be dropped should unwinding occur.
     // This is a subset of what needs to be dropped when exiting the scope.
@@ -1917,7 +1942,10 @@ where
                 // We adjust this BEFORE we create the drop (e.g., `drops[n]`)
                 // because `drops[n]` should unwind to `drops[n-1]`.
                 if unwind_to != DropIdx::MAX {
-                    debug_assert_eq!(unwind_drops.drop_nodes[unwind_to].data.local, drop_data.local);
+                    debug_assert_eq!(
+                        unwind_drops.drop_nodes[unwind_to].data.local,
+                        drop_data.local
+                    );
                     debug_assert_eq!(unwind_drops.drop_nodes[unwind_to].data.kind, drop_data.kind);
                     unwind_to = unwind_drops.drop_nodes[unwind_to].next;
                 }
@@ -1993,10 +2021,20 @@ where
                 );
             }
             DropKind::Storage => {
-                // We emit storage-dead nodes on the unwind path for borrow-checking
-                // purposes. When `storage_dead_on_unwind` is true, we need to adjust
-                // the `unwind_to` pointer now that the storage-dead has completed, so
-                // that any future drops we emit will not register storage-dead.
+                // We emit StorageDead statements on the unwind path to ensure the borrow-checker
+                // treats locals as dead at the same point on all paths. This is for static semantics
+                // (borrow-checking), not runtime drop order. StorageDead is only needed when there
+                // are Value or ForLint drops present, because:
+                // 1. StorageDead is only relevant for borrow-checking when there are destructors
+                //    that might reference the dead variable
+                // 2. If there are no drops, there's no unwind path to emit StorageDead on
+                //
+                // These StorageDead statements are removed by the `RemoveStorageMarkers` MIR
+                // transform pass before codegen, so they don't affect LLVM output.
+                //
+                // When `storage_dead_on_unwind` is true, we need to adjust the `unwind_to` pointer
+                // now that the storage-dead has completed, so that any future drops we emit will
+                // not register storage-dead.
                 if storage_dead_on_unwind && unwind_to != DropIdx::MAX {
                     debug_assert_eq!(
                         unwind_drops.drop_nodes[unwind_to].data.local,

compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs

@@ -6,6 +6,8 @@
 //!   - [`FakeRead`]
 //!   - [`Assign`] statements with a [`Fake`] borrow
 //!   - [`Coverage`] statements of kind [`BlockMarker`] or [`SpanMarker`]
+//!   - [`StorageDead`] statements (these are only needed for borrow-checking and are removed
+//!     after borrowck completes to ensure they don't affect later optimization passes or codegen)
 //!
 //! [`AscribeUserType`]: rustc_middle::mir::StatementKind::AscribeUserType
 //! [`Assign`]: rustc_middle::mir::StatementKind::Assign
@@ -15,6 +17,7 @@
 //! [`Coverage`]: rustc_middle::mir::StatementKind::Coverage
 //! [`BlockMarker`]: rustc_middle::mir::coverage::CoverageKind::BlockMarker
 //! [`SpanMarker`]: rustc_middle::mir::coverage::CoverageKind::SpanMarker
+//! [`StorageDead`]: rustc_middle::mir::StatementKind::StorageDead
 
 use rustc_middle::mir::coverage::CoverageKind;
 use rustc_middle::mir::*;
@@ -28,6 +31,10 @@ impl<'tcx> crate::MirPass<'tcx> for CleanupPostBorrowck {
         // Manually invalidate CFG caches if we actually change a terminator's edges.
         let mut invalidate_cfg = false;
         for basic_block in body.basic_blocks.as_mut_preserves_cfg().iter_mut() {
+            // Only remove StorageDead from cleanup blocks (unwind paths).
+            // StorageDead on normal paths is still needed for MIR validation
+            // and will be removed later by RemoveStorageMarkers during optimization.
+            let is_cleanup = basic_block.is_cleanup;
             for statement in basic_block.statements.iter_mut() {
                 match statement.kind {
                     StatementKind::AscribeUserType(..)
@@ -41,6 +48,9 @@ impl<'tcx> crate::MirPass<'tcx> for CleanupPostBorrowck {
                     | StatementKind::BackwardIncompatibleDropHint { .. } => {
                         statement.make_nop(true)
                     }
+                    StatementKind::StorageDead(..) if is_cleanup => {
+                        statement.make_nop(true)
+                    }
                     StatementKind::Assign(box (
                         _,
                         Rvalue::Cast(

tests/mir-opt/async_drop_live_dead.a-{closure#0}.coroutine_drop_async.0.panic-unwind.mir

@@ -44,7 +44,6 @@ fn a::{closure#0}(_1: Pin<&mut {async fn body of a<T>()}>, _2: &mut Context<'_>)
     }
 
     bb3 (cleanup): {
-        nop;
         nop;
         goto -> bb5;
     }

tests/mir-opt/basic_assignment.main.SimplifyCfg-initial.after.mir

@@ -73,14 +73,19 @@ fn main() -> () {
     }
 
     bb6 (cleanup): {
+        StorageDead(_6);
         drop(_5) -> [return: bb7, unwind terminate(cleanup)];
     }
 
     bb7 (cleanup): {
+        StorageDead(_5);
         drop(_4) -> [return: bb8, unwind terminate(cleanup)];
     }
 
     bb8 (cleanup): {
+        StorageDead(_4);
+        StorageDead(_2);
+        StorageDead(_1);
         resume;
     }
 }

tests/mir-opt/building/logical_or_in_conditional.test_complex.built.after.mir

@@ -19,7 +19,7 @@ fn test_complex() -> () {
     bb0: {
         StorageLive(_1);
         StorageLive(_2);
-        _2 = E::f() -> [return: bb1, unwind: bb35];
+        _2 = E::f() -> [return: bb1, unwind: bb38];
     }
 
     bb1: {
@@ -42,7 +42,7 @@ fn test_complex() -> () {
 
     bb5: {
         StorageLive(_4);
-        _4 = always_true() -> [return: bb6, unwind: bb35];
+        _4 = always_true() -> [return: bb6, unwind: bb38];
     }
 
     bb6: {
@@ -64,7 +64,7 @@ fn test_complex() -> () {
     }
 
     bb9: {
-        drop(_7) -> [return: bb11, unwind: bb35];
+        drop(_7) -> [return: bb11, unwind: bb37];
     }
 
     bb10: {
@@ -78,7 +78,7 @@ fn test_complex() -> () {
     }
 
     bb12: {
-        drop(_7) -> [return: bb13, unwind: bb35];
+        drop(_7) -> [return: bb13, unwind: bb37];
     }
 
     bb13: {
@@ -98,7 +98,7 @@ fn test_complex() -> () {
     }
 
     bb15: {
-        drop(_10) -> [return: bb17, unwind: bb35];
+        drop(_10) -> [return: bb17, unwind: bb36];
     }
 
     bb16: {
@@ -139,7 +139,7 @@ fn test_complex() -> () {
         StorageDead(_4);
         StorageDead(_1);
         StorageLive(_11);
-        _11 = always_true() -> [return: bb23, unwind: bb35];
+        _11 = always_true() -> [return: bb23, unwind: bb38];
     }
 
     bb23: {
@@ -156,7 +156,7 @@ fn test_complex() -> () {
 
     bb26: {
         StorageLive(_12);
-        _12 = E::f() -> [return: bb27, unwind: bb35];
+        _12 = E::f() -> [return: bb27, unwind: bb38];
     }
 
     bb27: {
@@ -199,6 +199,25 @@ fn test_complex() -> () {
     }
 
     bb35 (cleanup): {
+        StorageDead(_10);
+        StorageDead(_9);
+        StorageDead(_2);
+        goto -> bb38;
+    }
+
+    bb36 (cleanup): {
+        StorageDead(_10);
+        StorageDead(_9);
+        goto -> bb38;
+    }
+
+    bb37 (cleanup): {
+        StorageDead(_7);
+        StorageDead(_6);
+        goto -> bb38;
+    }
+
+    bb38 (cleanup): {
         resume;
     }
 }

tests/mir-opt/building/logical_or_in_conditional.test_or.built.after.mir

@@ -20,7 +20,7 @@ fn test_or() -> () {
     }
 
     bb1: {
-        drop(_3) -> [return: bb3, unwind: bb13];
+        drop(_3) -> [return: bb3, unwind: bb14];
     }
 
     bb2: {
@@ -34,7 +34,7 @@ fn test_or() -> () {
     }
 
     bb4: {
-        drop(_3) -> [return: bb5, unwind: bb13];
+        drop(_3) -> [return: bb5, unwind: bb14];
     }
 
     bb5: {
@@ -86,6 +86,19 @@ fn test_or() -> () {
     }
 
     bb13 (cleanup): {
+        StorageDead(_6);
+        StorageDead(_5);
+        goto -> bb15;
+    }
+
+    bb14 (cleanup): {
+        StorageDead(_3);
+        StorageDead(_2);
+        goto -> bb15;
+    }
+
+    bb15 (cleanup): {
+        StorageDead(_1);
         resume;
     }
 }