From d437a111f5c08cd85fb5eebecd9d2112a84610a4 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Fri, 15 Dec 2023 15:19:28 +0000 Subject: [PATCH] Give temporaries in if let guards correct scopes - 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. --- .../rustc_hir_analysis/src/check/region.rs | 6 +- .../rustc_mir_build/src/build/expr/as_temp.rs | 9 ++- .../rustc_mir_build/src/build/matches/mod.rs | 5 ++ compiler/rustc_mir_build/src/build/mod.rs | 8 +++ tests/ui/drop/dynamic-drop.rs | 14 +++++ .../rfcs/rfc-2294-if-let-guard/drop-order.rs | 59 +++++++++++++++++++ .../rfc-2294-if-let-guard/loop-mutability.rs | 19 ++++++ .../scoping-consistency-async.rs | 32 ++++++++++ .../scoping-consistency.rs | 24 ++++++++ tests/ui/thir-print/thir-tree-match.stdout | 12 ++-- 10 files changed, 178 insertions(+), 10 deletions(-) create mode 100644 tests/ui/rfcs/rfc-2294-if-let-guard/drop-order.rs create mode 100644 tests/ui/rfcs/rfc-2294-if-let-guard/loop-mutability.rs create mode 100644 tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency-async.rs create mode 100644 tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency.rs diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index 37b308f9f88d3..34d3f20d0cfa9 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -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); diff --git a/compiler/rustc_mir_build/src/build/expr/as_temp.rs b/compiler/rustc_mir_build/src/build/expr/as_temp.rs index 4be73e6f03c4e..27e66e7f54d0a 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_temp.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_temp.rs @@ -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(); @@ -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 { diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 5d5084c0b66f8..570a7ff36f99e 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -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 @@ -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; } diff --git a/compiler/rustc_mir_build/src/build/mod.rs b/compiler/rustc_mir_build/src/build/mod.rs index bef5c83d4a1c2..a6336ec63b215 100644 --- a/compiler/rustc_mir_build/src/build/mod.rs +++ b/compiler/rustc_mir_build/src/build/mod.rs @@ -210,6 +210,12 @@ struct Builder<'a, 'tcx> { /// finish building it. guard_context: Vec, + /// Temporaries with fixed indexes. Used so that if-let guards on arms + /// with an or-pattern are only created once. + fixed_temps: FxHashMap, + /// Scope of temporaries that should be deduplicated using [Self::fixed_temps]. + fixed_temps_scope: Option, + /// 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, @@ -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(), diff --git a/tests/ui/drop/dynamic-drop.rs b/tests/ui/drop/dynamic-drop.rs index 5bf2cc30e7fd6..d35913ed6412c 100644 --- a/tests/ui/drop/dynamic-drop.rs +++ b/tests/ui/drop/dynamic-drop.rs @@ -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]; @@ -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); diff --git a/tests/ui/rfcs/rfc-2294-if-let-guard/drop-order.rs b/tests/ui/rfcs/rfc-2294-if-let-guard/drop-order.rs new file mode 100644 index 0000000000000..9bb25a66f0927 --- /dev/null +++ b/tests/ui/rfcs/rfc-2294-if-let-guard/drop-order.rs @@ -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> = 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); +} diff --git a/tests/ui/rfcs/rfc-2294-if-let-guard/loop-mutability.rs b/tests/ui/rfcs/rfc-2294-if-let-guard/loop-mutability.rs new file mode 100644 index 0000000000000..349a24579a473 --- /dev/null +++ b/tests/ui/rfcs/rfc-2294-if-let-guard/loop-mutability.rs @@ -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() {} diff --git a/tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency-async.rs b/tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency-async.rs new file mode 100644 index 0000000000000..86a170141f8a0 --- /dev/null +++ b/tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency-async.rs @@ -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() {} diff --git a/tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency.rs b/tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency.rs new file mode 100644 index 0000000000000..37fe610637e9d --- /dev/null +++ b/tests/ui/rfcs/rfc-2294-if-let-guard/scoping-consistency.rs @@ -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() {} diff --git a/tests/ui/thir-print/thir-tree-match.stdout b/tests/ui/thir-print/thir-tree-match.stdout index 60c9283abcff8..e752e4a870215 100644 --- a/tests/ui/thir-print/thir-tree-match.stdout +++ b/tests/ui/thir-print/thir-tree-match.stdout @@ -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 { @@ -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) @@ -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 { @@ -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) @@ -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 { @@ -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)