Skip to content

Commit b2241c7

Browse files
diannedianne
committed
add a scope for if let guard temporaries and bindings
This ensures `if let` guard temporaries and bindings are dropped before the match arm's pattern's bindings.
1 parent 4ec6881 commit b2241c7

File tree

8 files changed

+84
-63
lines changed

8 files changed

+84
-63
lines changed

‎compiler/rustc_hir_analysis/src/check/region.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,11 @@ fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir:
198198
visitor.cx.var_parent = visitor.cx.parent;
199199

200200
resolve_pat(visitor, arm.pat);
201+
// We introduce a new scope to contain bindings and temporaries from `if let` guards, to
202+
// ensure they're dropped before the arm's pattern's bindings. This extends to the end of
203+
// the arm body and is the scope of its locals as well.
204+
visitor.enter_scope(Scope { local_id: arm.hir_id.local_id, data: ScopeData::MatchGuard });
205+
visitor.cx.var_parent = visitor.cx.parent;
201206
if let Some(guard) = arm.guard {
202207
resolve_cond(visitor, guard);
203208
}

‎compiler/rustc_middle/src/middle/region.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ impl fmt::Debug for Scope {
9696
ScopeData::Destruction => write!(fmt, "Destruction({:?})", self.local_id),
9797
ScopeData::IfThen => write!(fmt, "IfThen({:?})", self.local_id),
9898
ScopeData::IfThenRescope => write!(fmt, "IfThen[edition2024]({:?})", self.local_id),
99+
ScopeData::MatchGuard => write!(fmt, "MatchGuard({:?})", self.local_id),
99100
ScopeData::Remainder(fsi) => write!(
100101
fmt,
101102
"Remainder {{ block: {:?}, first_statement_index: {}}}",
@@ -131,6 +132,11 @@ pub enum ScopeData {
131132
/// whose lifetimes do not cross beyond this scope.
132133
IfThenRescope,
133134

135+
/// Scope of the condition and body of a match arm with a guard
136+
/// Used for variables introduced in an if-let guard,
137+
/// whose lifetimes do not cross beyond this scope.
138+
MatchGuard,
139+
134140
/// Scope following a `let id = expr;` binding in a block.
135141
Remainder(FirstStatementIndex),
136142
}

‎compiler/rustc_middle/src/ty/rvalue_scopes.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ impl RvalueScopes {
4444
debug!("temporary_scope({expr_id:?}) = {id:?} [enclosing]");
4545
return (Some(id), backwards_incompatible);
4646
}
47-
ScopeData::IfThenRescope => {
47+
ScopeData::IfThenRescope | ScopeData::MatchGuard => {
4848
debug!("temporary_scope({expr_id:?}) = {p:?} [enclosing]");
4949
return (Some(p), backwards_incompatible);
5050
}

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

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -435,46 +435,50 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
435435
let arm_scope = (arm.scope, arm_source_info);
436436
let match_scope = self.local_scope();
437437
self.in_scope(arm_scope, arm.lint_level, |this| {
438-
let old_dedup_scope =
439-
mem::replace(&mut this.fixed_temps_scope, Some(arm.scope));
440-
441-
// `try_to_place` may fail if it is unable to resolve the given
442-
// `PlaceBuilder` inside a closure. In this case, we don't want to include
443-
// a scrutinee place. `scrutinee_place_builder` will fail to be resolved
444-
// if the only match arm is a wildcard (`_`).
445-
// Example:
446-
// ```
447-
// let foo = (0, 1);
448-
// let c = || {
449-
// match foo { _ => () };
450-
// };
451-
// ```
452-
let scrutinee_place = scrutinee_place_builder.try_to_place(this);
453-
let opt_scrutinee_place =
454-
scrutinee_place.as_ref().map(|place| (Some(place), scrutinee_span));
455-
let scope = this.declare_bindings(
456-
None,
457-
arm.span,
458-
&arm.pattern,
459-
arm.guard,
460-
opt_scrutinee_place,
461-
);
438+
let guard_scope =
439+
region::Scope { data: region::ScopeData::MatchGuard, ..arm.scope };
440+
this.in_scope((guard_scope, arm_source_info), LintLevel::Inherited, |this| {
441+
let old_dedup_scope =
442+
mem::replace(&mut this.fixed_temps_scope, Some(guard_scope));
443+
444+
// `try_to_place` may fail if it is unable to resolve the given
445+
// `PlaceBuilder` inside a closure. In this case, we don't want to include
446+
// a scrutinee place. `scrutinee_place_builder` will fail to be resolved
447+
// if the only match arm is a wildcard (`_`).
448+
// Example:
449+
// ```
450+
// let foo = (0, 1);
451+
// let c = || {
452+
// match foo { _ => () };
453+
// };
454+
// ```
455+
let scrutinee_place = scrutinee_place_builder.try_to_place(this);
456+
let opt_scrutinee_place =
457+
scrutinee_place.as_ref().map(|place| (Some(place), scrutinee_span));
458+
let scope = this.declare_bindings(
459+
None,
460+
arm.span,
461+
&arm.pattern,
462+
arm.guard,
463+
opt_scrutinee_place,
464+
);
462465

463-
let arm_block = this.bind_pattern(
464-
outer_source_info,
465-
branch,
466-
&built_match_tree.fake_borrow_temps,
467-
scrutinee_span,
468-
Some((arm, match_scope)),
469-
);
466+
let arm_block = this.bind_pattern(
467+
outer_source_info,
468+
branch,
469+
&built_match_tree.fake_borrow_temps,
470+
scrutinee_span,
471+
Some((arm, match_scope)),
472+
);
470473

471-
this.fixed_temps_scope = old_dedup_scope;
474+
this.fixed_temps_scope = old_dedup_scope;
472475

473-
if let Some(source_scope) = scope {
474-
this.source_scope = source_scope;
475-
}
476+
if let Some(source_scope) = scope {
477+
this.source_scope = source_scope;
478+
}
476479

477-
this.expr_into_dest(destination, arm_block, arm.body)
480+
this.expr_into_dest(destination, arm_block, arm.body)
481+
})
478482
})
479483
.into_block()
480484
})
@@ -2523,7 +2527,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
25232527
// bindings and temporaries created for and by the guard. As a result, the drop order
25242528
// for the arm will correspond to the binding order of the final sub-branch lowered.
25252529
if matches!(schedule_drops, ScheduleDrops::No) {
2526-
self.clear_top_scope(arm.scope);
2530+
self.clear_match_arm_and_guard_scopes(arm.scope);
25272531
}
25282532

25292533
let source_info = self.source_info(guard_span);

‎compiler/rustc_mir_build/src/builder/scope.rs

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1750,17 +1750,24 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
17501750
success_block
17511751
}
17521752

1753-
/// Unschedules any drops in the top scope.
1753+
/// Unschedules any drops in the top two scopes.
17541754
///
1755-
/// This is only needed for `match` arm scopes, because they have one
1756-
/// entrance per pattern, but only one exit.
1757-
pub(crate) fn clear_top_scope(&mut self, region_scope: region::Scope) {
1758-
let top_scope = self.scopes.scopes.last_mut().unwrap();
1755+
/// This is only needed for pattern-matches combining guards and or-patterns: or-patterns lead
1756+
/// to guards being lowered multiple times before lowering the arm body, so we unschedle drops
1757+
/// for guards' temporaries and bindings between lowering each instance of an match arm's guard.
1758+
pub(crate) fn clear_match_arm_and_guard_scopes(&mut self, region_scope: region::Scope) {
1759+
let [.., arm_scope, guard_scope] = &mut *self.scopes.scopes else {
1760+
bug!("matches with guards should introduce separate scopes for the pattern and guard");
1761+
};
17591762

1760-
assert_eq!(top_scope.region_scope, region_scope);
1763+
assert_eq!(arm_scope.region_scope, region_scope);
1764+
assert_eq!(guard_scope.region_scope.data, region::ScopeData::MatchGuard);
1765+
assert_eq!(guard_scope.region_scope.local_id, region_scope.local_id);
17611766

1762-
top_scope.drops.clear();
1763-
top_scope.invalidate_cache();
1767+
arm_scope.drops.clear();
1768+
arm_scope.invalidate_cache();
1769+
guard_scope.drops.clear();
1770+
guard_scope.invalidate_cache();
17641771
}
17651772
}
17661773

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,12 @@ use core::{cell::RefCell, ops::Drop};
1414

1515
fn main() {
1616
// Test that `let` guard bindings and temps are dropped before the arm's pattern's bindings.
17-
// TODO: this is currently the old behavior (`let` bindings dropped after arm bindings).
1817
assert_drop_order(1..=6, |o| {
1918
// We move out of the scrutinee, so the drop order of the array's elements are based on
2019
// binding declaration order, and they're dropped in the arm's scope.
21-
match [o.log(3), o.log(2)] {
20+
match [o.log(6), o.log(5)] {
2221
// Partially move from the guard temporary to test drops both for temps and the binding.
23-
[_x, _y] if let [_, _z, _] = [o.log(6), o.log(4), o.log(5)]
22+
[_x, _y] if let [_, _z, _] = [o.log(4), o.log(2), o.log(3)]
2423
&& true => { let _a = o.log(1); }
2524
_ => unreachable!(),
2625
}

‎tests/ui/thir-print/thir-tree-loop-match.stdout

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ body:
129129
body:
130130
Expr {
131131
ty: bool
132-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(16)), backwards_incompatible: None }
132+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(16)), backwards_incompatible: None }
133133
span: $DIR/thir-tree-loop-match.rs:12:25: 15:18 (#0)
134134
kind:
135135
Scope {
@@ -138,14 +138,14 @@ body:
138138
value:
139139
Expr {
140140
ty: bool
141-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(16)), backwards_incompatible: None }
141+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(16)), backwards_incompatible: None }
142142
span: $DIR/thir-tree-loop-match.rs:12:25: 15:18 (#0)
143143
kind:
144144
NeverToAny {
145145
source:
146146
Expr {
147147
ty: !
148-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(16)), backwards_incompatible: None }
148+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(16)), backwards_incompatible: None }
149149
span: $DIR/thir-tree-loop-match.rs:12:25: 15:18 (#0)
150150
kind:
151151
Block {
@@ -227,7 +227,7 @@ body:
227227
body:
228228
Expr {
229229
ty: bool
230-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(24)), backwards_incompatible: None }
230+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(24)), backwards_incompatible: None }
231231
span: $DIR/thir-tree-loop-match.rs:16:26: 16:38 (#0)
232232
kind:
233233
Scope {
@@ -236,21 +236,21 @@ body:
236236
value:
237237
Expr {
238238
ty: bool
239-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(24)), backwards_incompatible: None }
239+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(24)), backwards_incompatible: None }
240240
span: $DIR/thir-tree-loop-match.rs:16:26: 16:38 (#0)
241241
kind:
242242
NeverToAny {
243243
source:
244244
Expr {
245245
ty: !
246-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(24)), backwards_incompatible: None }
246+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(24)), backwards_incompatible: None }
247247
span: $DIR/thir-tree-loop-match.rs:16:26: 16:38 (#0)
248248
kind:
249249
Return {
250250
value:
251251
Expr {
252252
ty: bool
253-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(24)), backwards_incompatible: None }
253+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(24)), backwards_incompatible: None }
254254
span: $DIR/thir-tree-loop-match.rs:16:33: 16:38 (#0)
255255
kind:
256256
Scope {
@@ -259,7 +259,7 @@ body:
259259
value:
260260
Expr {
261261
ty: bool
262-
temp_lifetime: TempLifetime { temp_lifetime: Some(Node(24)), backwards_incompatible: None }
262+
temp_lifetime: TempLifetime { temp_lifetime: Some(MatchGuard(24)), backwards_incompatible: None }
263263
span: $DIR/thir-tree-loop-match.rs:16:33: 16:38 (#0)
264264
kind:
265265
VarRef {

0 commit comments

Comments
 (0)