Skip to content

Commit

Permalink
Give temporaries in if let guards correct scopes
Browse files Browse the repository at this point in the history
- Make temporaries in if-let guards be the same variable in MIR when
  the guard is duplicated due to or-patterns.
- Change the "destruction scope" for match arms to be the arm scope rather
  than the arm body scope.
- Add tests.
  • Loading branch information
matthewjasper committed Dec 21, 2023
1 parent 68d684c commit d437a11
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 10 deletions.
6 changes: 3 additions & 3 deletions compiler/rustc_hir_analysis/src/check/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,10 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
fn resolve_arm<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) {
let prev_cx = visitor.cx;

visitor.enter_scope(Scope { id: arm.hir_id.local_id, data: ScopeData::Node });
visitor.cx.var_parent = visitor.cx.parent;
visitor.terminating_scopes.insert(arm.hir_id.local_id);

visitor.terminating_scopes.insert(arm.body.hir_id.local_id);
visitor.enter_node_scope_with_dtor(arm.hir_id.local_id);
visitor.cx.var_parent = visitor.cx.parent;

if let Some(hir::Guard::If(expr)) = arm.guard {
visitor.terminating_scopes.insert(expr.hir_id.local_id);
Expand Down
9 changes: 8 additions & 1 deletion compiler/rustc_mir_build/src/build/expr/as_temp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

let expr_ty = expr.ty;
let temp = {
let deduplicate_temps =
this.fixed_temps_scope.is_some() && this.fixed_temps_scope == temp_lifetime;
let temp = if deduplicate_temps && let Some(temp_index) = this.fixed_temps.get(&expr_id) {
*temp_index
} else {
let mut local_decl = LocalDecl::new(expr_ty, expr_span);
if mutability.is_not() {
local_decl = local_decl.immutable();
Expand Down Expand Up @@ -72,6 +76,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
**local_decl.local_info.as_mut().assert_crate_local() = local_info;
this.local_decls.push(local_decl)
};
if deduplicate_temps {
this.fixed_temps.insert(expr_id, temp);
}
let temp_place = Place::from(temp);

match expr.kind {
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,9 @@ 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
Expand Down Expand Up @@ -427,6 +430,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
false,
);

this.fixed_temps_scope = old_dedup_scope;

if let Some(source_scope) = scope {
this.source_scope = source_scope;
}
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_mir_build/src/build/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ struct Builder<'a, 'tcx> {
/// finish building it.
guard_context: Vec<GuardFrame>,

/// Temporaries with fixed indexes. Used so that if-let guards on arms
/// with an or-pattern are only created once.
fixed_temps: FxHashMap<ExprId, Local>,
/// Scope of temporaries that should be deduplicated using [Self::fixed_temps].
fixed_temps_scope: Option<region::Scope>,

/// Maps `HirId`s of variable bindings to the `Local`s created for them.
/// (A match binding can have two locals; the 2nd is for the arm's guard.)
var_indices: FxHashMap<LocalVarId, LocalsForNode>,
Expand Down Expand Up @@ -752,6 +758,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
source_scopes: IndexVec::new(),
source_scope: OUTERMOST_SOURCE_SCOPE,
guard_context: vec![],
fixed_temps: Default::default(),
fixed_temps_scope: None,
in_scope_unsafe: safety,
local_decls: IndexVec::from_elem_n(LocalDecl::new(return_ty, return_span), 1),
canonical_user_type_annotations: IndexVec::new(),
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/drop/dynamic-drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,17 @@ fn if_let_guard(a: &Allocator, c: bool, d: i32) {
}
}

fn if_let_guard_2(a: &Allocator, num: i32) {
let d = a.alloc();
match num {
#[allow(irrefutable_let_patterns)]
1 | 2 if let Ptr(ref _idx, _) = a.alloc() => {
a.alloc();
}
_ => {}
}
}

fn panic_after_return(a: &Allocator) -> Ptr<'_> {
// Panic in the drop of `p` or `q` can leak
let exceptions = vec![8, 9];
Expand Down Expand Up @@ -514,6 +525,9 @@ fn main() {
run_test(|a| if_let_guard(a, false, 0));
run_test(|a| if_let_guard(a, false, 1));
run_test(|a| if_let_guard(a, false, 2));
run_test(|a| if_let_guard_2(a, 0));
run_test(|a| if_let_guard_2(a, 1));
run_test(|a| if_let_guard_2(a, 2));

run_test(|a| {
panic_after_return(a);
Expand Down
59 changes: 59 additions & 0 deletions tests/ui/rfcs/rfc-2294-if-let-guard/drop-order.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// check drop order of temporaries create in match guards.
// For normal guards all temporaries are dropped before the body of the arm.
// For let guards temporaries live until the end of the arm.

// run-pass

#![feature(if_let_guard)]
#![allow(irrefutable_let_patterns)]

use std::sync::Mutex;

static A: Mutex<Vec<i32>> = Mutex::new(Vec::new());

struct D(i32);

fn make_d(x: i32) -> D {
A.lock().unwrap().push(x);
D(x)
}

impl Drop for D {
fn drop(&mut self) {
A.lock().unwrap().push(!self.0);
}
}

fn if_guard(num: i32) {
let _d = make_d(1);
match num {
1 | 2 if make_d(2).0 == 2 => {
make_d(3);
}
_ => {}
}
}

fn if_let_guard(num: i32) {
let _d = make_d(1);
match num {
1 | 2 if let D(ref _x) = make_d(2) => {
make_d(3);
}
_ => {}
}
}

fn main() {
if_guard(1);
if_guard(2);
if_let_guard(1);
if_let_guard(2);
let expected = [
1, 2, !2, 3, !3, !1,
1, 2, !2, 3, !3, !1,
1, 2, 3, !3, !2, !1,
1, 2, 3, !3, !2, !1,
];
assert_eq!(*A.lock().unwrap(), expected);
}
19 changes: 19 additions & 0 deletions tests/ui/rfcs/rfc-2294-if-let-guard/loop-mutability.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// check-pass

#![feature(if_let_guard)]

fn split_last(_: &()) -> Option<(&i32, &i32)> {
None
}

fn assign_twice() {
loop {
match () {
#[allow(irrefutable_let_patterns)]
() if let _ = split_last(&()) => {}
_ => {}
}
}
}

fn main() {}
32 changes: 32 additions & 0 deletions tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency-async.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Check that temporaries in if-let guards are correctly scoped.
// Regression test for #116079.

// build-pass
// edition:2018
// -Zvalidate-mir

#![feature(if_let_guard)]

static mut A: [i32; 5] = [1, 2, 3, 4, 5];

async fn fun() {
unsafe {
match A {
_ => (),
i if let Some(1) = async { Some(1) }.await => (),
_ => (),
}
}
}

async fn funner() {
unsafe {
match A {
_ => (),
_ | _ if let Some(1) = async { Some(1) }.await => (),
_ => (),
}
}
}

fn main() {}
24 changes: 24 additions & 0 deletions tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Check that temporaries in if-let guards are correctly scoped.

// build-pass
// -Zvalidate-mir

#![feature(if_let_guard)]

fn fun() {
match 0 {
_ => (),
_ if let Some(s) = std::convert::identity(&Some(String::new())) => {}
_ => (),
}
}

fn funner() {
match 0 {
_ => (),
_ | _ if let Some(s) = std::convert::identity(&Some(String::new())) => {}
_ => (),
}
}

fn main() {}
12 changes: 6 additions & 6 deletions tests/ui/thir-print/thir-tree-match.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ body:
body:
Expr {
ty: bool
temp_lifetime: Some(Node(13))
temp_lifetime: Some(Node(12))
span: $DIR/thir-tree-match.rs:17:36: 17:40 (#0)
kind:
Scope {
Expand All @@ -133,7 +133,7 @@ body:
value:
Expr {
ty: bool
temp_lifetime: Some(Node(13))
temp_lifetime: Some(Node(12))
span: $DIR/thir-tree-match.rs:17:36: 17:40 (#0)
kind:
Literal( lit: Spanned { node: Bool(true), span: $DIR/thir-tree-match.rs:17:36: 17:40 (#0) }, neg: false)
Expand Down Expand Up @@ -176,7 +176,7 @@ body:
body:
Expr {
ty: bool
temp_lifetime: Some(Node(19))
temp_lifetime: Some(Node(18))
span: $DIR/thir-tree-match.rs:18:27: 18:32 (#0)
kind:
Scope {
Expand All @@ -185,7 +185,7 @@ body:
value:
Expr {
ty: bool
temp_lifetime: Some(Node(19))
temp_lifetime: Some(Node(18))
span: $DIR/thir-tree-match.rs:18:27: 18:32 (#0)
kind:
Literal( lit: Spanned { node: Bool(false), span: $DIR/thir-tree-match.rs:18:27: 18:32 (#0) }, neg: false)
Expand Down Expand Up @@ -220,7 +220,7 @@ body:
body:
Expr {
ty: bool
temp_lifetime: Some(Node(24))
temp_lifetime: Some(Node(23))
span: $DIR/thir-tree-match.rs:19:24: 19:28 (#0)
kind:
Scope {
Expand All @@ -229,7 +229,7 @@ body:
value:
Expr {
ty: bool
temp_lifetime: Some(Node(24))
temp_lifetime: Some(Node(23))
span: $DIR/thir-tree-match.rs:19:24: 19:28 (#0)
kind:
Literal( lit: Spanned { node: Bool(true), span: $DIR/thir-tree-match.rs:19:24: 19:28 (#0) }, neg: false)
Expand Down

0 comments on commit d437a11

Please sign in to comment.