user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
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 (https://github.com/rust-lang/rust/pull/141295#issuecomment-2968975596). 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 (https://github.com/rust-lang/rust/pull/143376#issuecomment-3034922617) seemed to indicate there wasn't a significant hit to introduce a new scope on all match arms, but guard patterns (rust-lang/rust#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/rust#51114

Tests are adapted from examples by `@traviscross,` `@est31,` and myself on rust-lang/rust#141295.
This commit is contained in:
bors 2025-08-09 03:19:26 +00:00
commit 2de2456fb7
6 changed files with 171 additions and 45 deletions
Download patch file Download diff file Expand all files Collapse all files

5
compiler/rustc_hir_analysis/src/check/region.rs
View file

@ -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);

6
compiler/rustc_middle/src/middle/region.rs
View file

@ -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),
}

2
compiler/rustc_middle/src/ty/rvalue_scopes.rs
View file

@ -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);
}

78
compiler/rustc_mir_build/src/builder/matches/mod.rs
View file

@ -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));
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,
);
// `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);

37
compiler/rustc_mir_build/src/builder/scope.rs
View file

@ -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();
}
}

88
tests/ui/drop/if-let-guards.rs Normal file
View file

@ -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);
}