Auto merge of #143376 - dianne:guard-scope, r=<try>

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 (#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. So far, this is the only way I've thought of to achieve this without explicitly rescheduling guards' drops to move them after the arm's pattern's.

I'm not sure this should be merged as-is. It's a little hacky and it introduces a new scope for *all* match arms rather than just those with `if let` guards. However, since I'm looking for feedback on the approach, I figured this is a relatively simple way to present it. As I mention in a FIXME comment, something like this will be needed for guard patterns (#129967) too[^1], so I think the final version should maybe only add these scopes as needed. That'll be better for perf too.

Tracking issue for `if_let_guard`: #51114

Tests are adapted from examples by `@traviscross,` `@est31,` and myself on #141295. cc, as I'd like your input on this.

I'm not entirely sure who to request for scoping changes, but let's start with r? `@Nadrieril` since this relates to guard patterns, we talked about it recently, and rustbot's going to ping you anyway. Feel free to reassign!

[^1]: e.g., new scopes are needed to keep failed guards inside `let` chain patterns from dropping existing bindings/temporaries; something like this could give a way of doing that without needing to reschedule drops. Unfortunately it may not help keep failed guards in `let` statement patterns from dropping the `let` statement's initializer, so it isn't a complete solution. I'm still not sure how to do that without rescheduling drops, changing how `let` statements' scopes work, or restricting the functionality of guard patterns in `let` statements (including `let`-`else`).

Author: rust-bors[bot]

compiler/rustc_hir_analysis/src/check/region.rs

@@ -191,6 +191,14 @@ fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir:
     visitor.cx.var_parent = visitor.cx.parent;
 
     resolve_pat(visitor, arm.pat);
+    // In order to ensure bindings and temporaries from `if let` guards are dropped before the arm's
+    // pattern's bindings, we introduce a new scope nested within the arm's. This contains the
+    // guard's bindings as well as any temporaries and scopes introduced within the arm's body.
+    // `ScopeData::IfThenRescope` is used as it restricts temporaries' lifetimes.
+    // FIXME(guard_patterns): This will likewise be needed for guard patterns. Nesting will be more
+    // complex in that case, and multiple may be needed for `let` guards inside `let` chains.
+    visitor.enter_scope(Scope { local_id: arm.hir_id.local_id, data: ScopeData::IfThenRescope });
+    visitor.cx.var_parent = visitor.cx.parent;
     if let Some(guard) = arm.guard {
         resolve_expr(visitor, guard, !has_let_expr(guard));
     }

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

@@ -448,47 +448,51 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 let arm_scope = (arm.scope, arm_source_info);
                 let match_scope = self.local_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,
-                    );
+                    let guard_scope =
+                        region::Scope { data: region::ScopeData::IfThenRescope, ..arm.scope };
+                    this.in_scope((guard_scope, arm_source_info), LintLevel::Inherited, |this| {
+                        let old_dedup_scope =
+                            mem::replace(&mut this.fixed_temps_scope, Some(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)),
-                        EmitStorageLive::Yes,
-                    );
+                        let arm_block = this.bind_pattern(
+                            outer_source_info,
+                            branch,
+                            &built_match_tree.fake_borrow_temps,
+                            scrutinee_span,
+                            Some((arm, match_scope)),
+                            EmitStorageLive::Yes,
+                        );
 
-                    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()
             })
@@ -569,7 +573,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             // be bound for each candidate.
             for sub_branch in branch.sub_branches {
                 if let Some(arm) = arm {
-                    self.clear_top_scope(arm.scope);
+                    self.clear_match_arm_scope(arm.scope);
                 }
                 let binding_end = self.bind_and_guard_matched_candidate(
                     sub_branch,

compiler/rustc_mir_build/src/builder/scope.rs

@@ -1724,17 +1724,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();
+    // FIXME(@dianne): This can hopefully be removed if we only lower guards once.
+    pub(crate) fn clear_match_arm_scope(&mut self, region_scope: region::Scope) {
+        let [.., arm_scope, guard_scope] = &mut *self.scopes.scopes else {
+            bug!("match arms should introduce two scopes");
+        };
 
-        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::IfThenRescope);
+        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,87 @@
+//! 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.
+//@ 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's kept until the end of 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);
+}