Auto merge of rust-lang#143376 - dianne:guard-scope, r=matthewjasper

add a scope for `if let` guard temporaries and bindings

This fixes my concern with `if let` guard drop order, namely that the guard's bindings and temporaries were being dropped after their arm's pattern's bindings, instead of before (rust-lang#141295 (comment)). The guard's bindings and temporaries now live in a new scope, which extends until (but not past) the end of the arm, guaranteeing they're dropped before the arm's pattern's bindings.

This only introduces a new scope for match arms with guards. Perf results (rust-lang#143376 (comment)) seemed to indicate there wasn't a significant hit to introduce a new scope on all match arms, but guard patterns (rust-lang#129967) will likely benefit from only adding new scopes when necessary (with some patterns requiring multiple nested scopes).

Tracking issue for `if_let_guard`: rust-lang#51114

Tests are adapted from examples by `@traviscross,` `@est31,` and myself on rust-lang#141295.

Author: bors

compiler/rustc_hir_analysis/src/check/region.rs

@@ -199,6 +199,11 @@ fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir:
 
     resolve_pat(visitor, arm.pat);
     if let Some(guard) = arm.guard {
+        // We introduce a new scope to contain bindings and temporaries from `if let` guards, to
+        // ensure they're dropped before the arm's pattern's bindings. This extends to the end of
+        // the arm body and is the scope of its locals as well.
+        visitor.enter_scope(Scope { local_id: arm.hir_id.local_id, data: ScopeData::MatchGuard });
+        visitor.cx.var_parent = visitor.cx.parent;
         resolve_cond(visitor, guard);
     }
     resolve_expr(visitor, arm.body, false);

compiler/rustc_middle/src/middle/region.rs

@@ -96,6 +96,7 @@ impl fmt::Debug for Scope {
             ScopeData::Destruction => write!(fmt, "Destruction({:?})", self.local_id),
             ScopeData::IfThen => write!(fmt, "IfThen({:?})", self.local_id),
             ScopeData::IfThenRescope => write!(fmt, "IfThen[edition2024]({:?})", self.local_id),
+            ScopeData::MatchGuard => write!(fmt, "MatchGuard({:?})", self.local_id),
             ScopeData::Remainder(fsi) => write!(
                 fmt,
                 "Remainder {{ block: {:?}, first_statement_index: {}}}",
@@ -131,6 +132,11 @@ pub enum ScopeData {
     /// whose lifetimes do not cross beyond this scope.
     IfThenRescope,
 
+    /// Scope of the condition and body of a match arm with a guard
+    /// Used for variables introduced in an if-let guard,
+    /// whose lifetimes do not cross beyond this scope.
+    MatchGuard,
+
     /// Scope following a `let id = expr;` binding in a block.
     Remainder(FirstStatementIndex),
 }

compiler/rustc_middle/src/ty/rvalue_scopes.rs

@@ -44,7 +44,7 @@ impl RvalueScopes {
                     debug!("temporary_scope({expr_id:?}) = {id:?} [enclosing]");
                     return (Some(id), backwards_incompatible);
                 }
-                ScopeData::IfThenRescope => {
+                ScopeData::IfThenRescope | ScopeData::MatchGuard => {
                     debug!("temporary_scope({expr_id:?}) = {p:?} [enclosing]");
                     return (Some(p), backwards_incompatible);
                 }

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

@@ -428,47 +428,53 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 let arm_source_info = self.source_info(arm.span);
                 let arm_scope = (arm.scope, arm_source_info);
                 let match_scope = self.local_scope();
+                let guard_scope = arm
+                    .guard
+                    .map(|_| region::Scope { data: region::ScopeData::MatchGuard, ..arm.scope });
                 self.in_scope(arm_scope, arm.lint_level, |this| {
-                    let old_dedup_scope =
-                        mem::replace(&mut this.fixed_temps_scope, Some(arm.scope));
-
-                    // `try_to_place` may fail if it is unable to resolve the given
-                    // `PlaceBuilder` inside a closure. In this case, we don't want to include
-                    // a scrutinee place. `scrutinee_place_builder` will fail to be resolved
-                    // if the only match arm is a wildcard (`_`).
-                    // Example:
-                    // ```
-                    // let foo = (0, 1);
-                    // let c = || {
-                    //    match foo { _ => () };
-                    // };
-                    // ```
-                    let scrutinee_place = scrutinee_place_builder.try_to_place(this);
-                    let opt_scrutinee_place =
-                        scrutinee_place.as_ref().map(|place| (Some(place), scrutinee_span));
-                    let scope = this.declare_bindings(
-                        None,
-                        arm.span,
-                        &arm.pattern,
-                        arm.guard,
-                        opt_scrutinee_place,
-                    );
+                    this.opt_in_scope(guard_scope.map(|scope| (scope, arm_source_info)), |this| {
+                        // `if let` guard temps needing deduplicating will be in the guard scope.
+                        let old_dedup_scope =
+                            mem::replace(&mut this.fixed_temps_scope, guard_scope);
+
+                        // `try_to_place` may fail if it is unable to resolve the given
+                        // `PlaceBuilder` inside a closure. In this case, we don't want to include
+                        // a scrutinee place. `scrutinee_place_builder` will fail to be resolved
+                        // if the only match arm is a wildcard (`_`).
+                        // Example:
+                        // ```
+                        // let foo = (0, 1);
+                        // let c = || {
+                        //    match foo { _ => () };
+                        // };
+                        // ```
+                        let scrutinee_place = scrutinee_place_builder.try_to_place(this);
+                        let opt_scrutinee_place =
+                            scrutinee_place.as_ref().map(|place| (Some(place), scrutinee_span));
+                        let scope = this.declare_bindings(
+                            None,
+                            arm.span,
+                            &arm.pattern,
+                            arm.guard,
+                            opt_scrutinee_place,
+                        );
 
-                    let arm_block = this.bind_pattern(
-                        outer_source_info,
-                        branch,
-                        &built_match_tree.fake_borrow_temps,
-                        scrutinee_span,
-                        Some((arm, match_scope)),
-                    );
+                        let arm_block = this.bind_pattern(
+                            outer_source_info,
+                            branch,
+                            &built_match_tree.fake_borrow_temps,
+                            scrutinee_span,
+                            Some((arm, match_scope)),
+                        );
 
-                    this.fixed_temps_scope = old_dedup_scope;
+                        this.fixed_temps_scope = old_dedup_scope;
 
-                    if let Some(source_scope) = scope {
-                        this.source_scope = source_scope;
-                    }
+                        if let Some(source_scope) = scope {
+                            this.source_scope = source_scope;
+                        }
 
-                    this.expr_into_dest(destination, arm_block, arm.body)
+                        this.expr_into_dest(destination, arm_block, arm.body)
+                    })
                 })
                 .into_block()
             })
@@ -2517,7 +2523,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             // bindings and temporaries created for and by the guard. As a result, the drop order
             // for the arm will correspond to the binding order of the final sub-branch lowered.
             if matches!(schedule_drops, ScheduleDrops::No) {
-                self.clear_top_scope(arm.scope);
+                self.clear_match_arm_and_guard_scopes(arm.scope);
             }
 
             let source_info = self.source_info(guard_span);

compiler/rustc_mir_build/src/builder/scope.rs

@@ -697,6 +697,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         block.and(rv)
     }
 
+    /// Convenience wrapper that executes `f` either within the current scope or a new scope.
+    /// Used for pattern matching, which introduces an additional scope for patterns with guards.
+    pub(crate) fn opt_in_scope<R>(
+        &mut self,
+        opt_region_scope: Option<(region::Scope, SourceInfo)>,
+        f: impl FnOnce(&mut Builder<'a, 'tcx>) -> BlockAnd<R>,
+    ) -> BlockAnd<R> {
+        if let Some(region_scope) = opt_region_scope {
+            self.in_scope(region_scope, LintLevel::Inherited, f)
+        } else {
+            f(self)
+        }
+    }
+
     /// Push a scope onto the stack. You can then build code in this
     /// scope and call `pop_scope` afterwards. Note that these two
     /// calls must be paired; using `in_scope` as a convenience
@@ -1750,17 +1764,24 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         success_block
     }
 
-    /// Unschedules any drops in the top scope.
+    /// Unschedules any drops in the top two scopes.
     ///
-    /// This is only needed for `match` arm scopes, because they have one
-    /// entrance per pattern, but only one exit.
-    pub(crate) fn clear_top_scope(&mut self, region_scope: region::Scope) {
-        let top_scope = self.scopes.scopes.last_mut().unwrap();
+    /// This is only needed for pattern-matches combining guards and or-patterns: or-patterns lead
+    /// to guards being lowered multiple times before lowering the arm body, so we unschedle drops
+    /// for guards' temporaries and bindings between lowering each instance of an match arm's guard.
+    pub(crate) fn clear_match_arm_and_guard_scopes(&mut self, region_scope: region::Scope) {
+        let [.., arm_scope, guard_scope] = &mut *self.scopes.scopes else {
+            bug!("matches with guards should introduce separate scopes for the pattern and guard");
+        };
 
-        assert_eq!(top_scope.region_scope, region_scope);
+        assert_eq!(arm_scope.region_scope, region_scope);
+        assert_eq!(guard_scope.region_scope.data, region::ScopeData::MatchGuard);
+        assert_eq!(guard_scope.region_scope.local_id, region_scope.local_id);
 
-        top_scope.drops.clear();
-        top_scope.invalidate_cache();
+        arm_scope.drops.clear();
+        arm_scope.invalidate_cache();
+        guard_scope.drops.clear();
+        guard_scope.invalidate_cache();
     }
 }
 

tests/ui/drop/if-let-guards.rs

@@ -0,0 +1,88 @@
+//! Tests drop order for `if let` guard bindings and temporaries. This is for behavior specific to
+//! `match` expressions, whereas `tests/ui/drop/drop-order-comparisons.rs` compares `let` chains in
+//! guards to `let` chains in `if` expressions. Drop order for `let` chains in guards shouldn't
+//! differ between Editions, so we test on both 2021 and 2024, expecting the same results.
+//@ revisions: e2021 e2024
+//@ [e2021] edition: 2021
+//@ [e2024] edition: 2024
+//@ run-pass
+
+#![feature(if_let_guard)]
+#![deny(rust_2024_compatibility)]
+
+use core::{cell::RefCell, ops::Drop};
+
+fn main() {
+    // Test that `let` guard bindings and temps are dropped before the arm's pattern's bindings.
+    assert_drop_order(1..=6, |o| {
+        // We move out of the scrutinee, so the drop order of the array's elements are based on
+        // binding declaration order, and they're dropped in the arm's scope.
+        match [o.log(6), o.log(5)] {
+            // Partially move from the guard temporary to test drops both for temps and the binding.
+            [_x, _y] if let [_, _z, _] = [o.log(4), o.log(2), o.log(3)]
+                && true => { let _a = o.log(1); }
+            _ => unreachable!(),
+        }
+    });
+
+    // Sanity check: we don't move out of the match scrutinee when the guard fails.
+    assert_drop_order(1..=4, |o| {
+        // The scrutinee uses the drop order for arrays since its elements aren't moved.
+        match [o.log(3), o.log(4)] {
+            [_x, _y] if let _z = o.log(1)
+                && false => unreachable!(),
+            _ => { let _a = o.log(2); }
+        }
+    });
+
+    // Test `let` guards' temporaries are dropped immediately when a guard fails, even if the guard
+    // is lowered and run multiple times on the same arm due to or-patterns.
+    assert_drop_order(1..=8, |o| {
+        let mut _x = 1;
+        // The match's scrutinee isn't bound by-move, so it outlives the match.
+        match o.log(8) {
+            // Failing a guard breaks out of the arm's scope, dropping the `let` guard's scrutinee.
+            _ | _ | _ if let _ = o.log(_x)
+                && { _x += 1; false } => unreachable!(),
+            // The temporaries from a failed guard are dropped before testing the next guard.
+            _ if let _ = o.log(5)
+                && { o.push(4); false } => unreachable!(),
+            // If the guard succeeds, we stay in the arm's scope to execute its body.
+            _ if let _ = o.log(7)
+                && true => { o.log(6); }
+            _ => unreachable!(),
+        }
+    });
+}
+
+// # Test scaffolding...
+
+struct DropOrder(RefCell<Vec<u64>>);
+struct LogDrop<'o>(&'o DropOrder, u64);
+
+impl DropOrder {
+    fn log(&self, n: u64) -> LogDrop<'_> {
+        LogDrop(self, n)
+    }
+    fn push(&self, n: u64) {
+        self.0.borrow_mut().push(n);
+    }
+}
+
+impl<'o> Drop for LogDrop<'o> {
+    fn drop(&mut self) {
+        self.0.push(self.1);
+    }
+}
+
+#[track_caller]
+fn assert_drop_order(
+    ex: impl IntoIterator<Item = u64>,
+    f: impl Fn(&DropOrder),
+) {
+    let order = DropOrder(RefCell::new(Vec::new()));
+    f(&order);
+    let order = order.0.into_inner();
+    let expected: Vec<u64> = ex.into_iter().collect();
+    assert_eq!(order, expected);
+}