From f9ccd39ae38051463771495e03889bf3aeff616a Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Thu, 6 Aug 2020 15:36:57 +0800 Subject: [PATCH 1/7] documentation fix --- compiler/rustc_middle/src/middle/region.rs | 10 +++++++--- compiler/rustc_mir_build/src/build/matches/simplify.rs | 5 +++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_middle/src/middle/region.rs b/compiler/rustc_middle/src/middle/region.rs index 4c6ac82060485..38cb3c1701f92 100644 --- a/compiler/rustc_middle/src/middle/region.rs +++ b/compiler/rustc_middle/src/middle/region.rs @@ -283,23 +283,27 @@ pub struct ScopeTree { /// To see that this method works, consider: /// /// Let `D` be our binding/temporary and `U` be our other HIR node, with - /// `HIR-postorder(U) < HIR-postorder(D)` (in our example, U would be - /// the yield and D would be one of the calls). Let's show that - /// `D` is storage-dead at `U`. + /// `HIR-postorder(U) < HIR-postorder(D)`. Suppose, as in our example, + /// U is the yield and D is one of the calls. + /// Let's show that `D` is storage-dead at `U`. /// /// Remember that storage-live/storage-dead refers to the state of /// the *storage*, and does not consider moves/drop flags. /// /// Then: + /// /// 1. From the ordering guarantee of HIR visitors (see /// `rustc_hir::intravisit`), `D` does not dominate `U`. + /// /// 2. Therefore, `D` is *potentially* storage-dead at `U` (because /// we might visit `U` without ever getting to `D`). + /// /// 3. However, we guarantee that at each HIR point, each /// binding/temporary is always either always storage-live /// or always storage-dead. This is what is being guaranteed /// by `terminating_scopes` including all blocks where the /// count of executions is not guaranteed. + /// /// 4. By `2.` and `3.`, `D` is *statically* storage-dead at `U`, /// QED. /// diff --git a/compiler/rustc_mir_build/src/build/matches/simplify.rs b/compiler/rustc_mir_build/src/build/matches/simplify.rs index a28a181e93504..e46274770be17 100644 --- a/compiler/rustc_mir_build/src/build/matches/simplify.rs +++ b/compiler/rustc_mir_build/src/build/matches/simplify.rs @@ -28,8 +28,9 @@ use std::mem; impl<'a, 'tcx> Builder<'a, 'tcx> { /// Simplify a candidate so that all match pairs require a test. /// - /// This method will also split a candidate where the only match-pair is an - /// or-pattern into multiple candidates. This is so that + /// This method will also split a candidate, in which the only + /// match-pair is an or-pattern, into multiple candidates. + /// This is so that /// /// match x { /// 0 | 1 => { ... }, From 7f5721c3f470f9c393db239c01dc051ec84430c8 Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Thu, 6 Aug 2020 15:37:12 +0800 Subject: [PATCH 2/7] also record the types of borrows from the pattern locals in match guards so that it reflects the fact that borrowing these pattern locals is happening before any yield points in match guards --- .../src/check/generator_interior.rs | 70 ++++++++++++++++--- 1 file changed, 61 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 93fdf93e9e394..25268b554e1ce 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -9,10 +9,12 @@ use rustc_hir as hir; use rustc_hir::def::{CtorKind, DefKind, Res}; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; -use rustc_hir::{Expr, ExprKind, Pat, PatKind}; +use rustc_hir::{Expr, ExprKind, Pat, PatKind, Arm, Guard, HirId}; +use rustc_hir::hir_id::HirIdSet; use rustc_middle::middle::region::{self, YieldData}; use rustc_middle::ty::{self, Ty}; use rustc_span::Span; +use smallvec::SmallVec; struct InteriorVisitor<'a, 'tcx> { fcx: &'a FnCtxt<'a, 'tcx>, @@ -21,6 +23,14 @@ struct InteriorVisitor<'a, 'tcx> { expr_count: usize, kind: hir::GeneratorKind, prev_unresolved_span: Option, + /// Match arm guards have temporary borrows from the pattern bindings. + /// In case there is a yield point in a guard with a reference to such bindings, + /// such borrows can span across this yield point. + /// As such, we need to track these borrows and record them despite of the fact + /// that they may succeed the said yield point in the post-order. + nested_scope_of_guards: SmallVec<[SmallVec<[HirId; 4]>; 4]>, + current_scope_of_guards: HirIdSet, + arm_has_guard: bool, } impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { @@ -30,6 +40,7 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { scope: Option, expr: Option<&'tcx Expr<'tcx>>, source_span: Span, + guard_borrowing_from_pattern: bool, ) { use rustc_span::DUMMY_SP; @@ -53,7 +64,7 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { yield_data.expr_and_pat_count, self.expr_count, source_span ); - if yield_data.expr_and_pat_count >= self.expr_count { + if guard_borrowing_from_pattern || yield_data.expr_and_pat_count >= self.expr_count { Some(yield_data) } else { None @@ -134,6 +145,9 @@ pub fn resolve_interior<'a, 'tcx>( expr_count: 0, kind, prev_unresolved_span: None, + nested_scope_of_guards: <_>::default(), + current_scope_of_guards: <_>::default(), + arm_has_guard: false, }; intravisit::walk_body(&mut visitor, body); @@ -210,19 +224,46 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { NestedVisitorMap::None } + fn visit_arm(&mut self, arm: &'tcx Arm<'tcx>) { + if arm.guard.is_some() { + self.nested_scope_of_guards.push(<_>::default()); + self.arm_has_guard = true; + } + self.visit_pat(&arm.pat); + if let Some(ref g) = arm.guard { + match g { + Guard::If(ref e) => { + self.visit_expr(e); + } + } + let mut scope_var_ids = + self.nested_scope_of_guards.pop().expect("should have pushed at least one earlier"); + for var_id in scope_var_ids.drain(..) { + assert!(self.current_scope_of_guards.remove(&var_id), "variable should be placed in scope earlier"); + } + self.arm_has_guard = false; + } + self.visit_expr(&arm.body); + } + fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) { intravisit::walk_pat(self, pat); self.expr_count += 1; - if let PatKind::Binding(..) = pat.kind { + if let PatKind::Binding(_, id, ..) = pat.kind { let scope = self.region_scope_tree.var_scope(pat.hir_id.local_id); let ty = self.fcx.typeck_results.borrow().pat_ty(pat); - self.record(ty, Some(scope), None, pat.span); + self.record(ty, Some(scope), None, pat.span, false); + if self.arm_has_guard { + self.nested_scope_of_guards.as_mut_slice().last_mut().unwrap().push(id); + self.current_scope_of_guards.insert(id); + } } } fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + let mut guard_borrowing_from_pattern = false; match &expr.kind { ExprKind::Call(callee, args) => match &callee.kind { ExprKind::Path(qpath) => { @@ -248,7 +289,18 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { } } _ => intravisit::walk_expr(self, expr), - }, + } + ExprKind::Path(qpath) => { + intravisit::walk_expr(self, expr); + let res = self.fcx.typeck_results.borrow().qpath_res(qpath, expr.hir_id); + match res { + Res::Local(id) if self.current_scope_of_guards.contains(&id) => { + debug!("a borrow in guard from pattern local is detected"); + guard_borrowing_from_pattern = true; + } + _ => {} + } + } _ => intravisit::walk_expr(self, expr), } @@ -259,7 +311,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { // If there are adjustments, then record the final type -- // this is the actual value that is being produced. if let Some(adjusted_ty) = self.fcx.typeck_results.borrow().expr_ty_adjusted_opt(expr) { - self.record(adjusted_ty, scope, Some(expr), expr.span); + self.record(adjusted_ty, scope, Some(expr), expr.span, guard_borrowing_from_pattern); } // Also record the unadjusted type (which is the only type if @@ -267,10 +319,10 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { // unadjusted value is sometimes a "temporary" that would wind // up in a MIR temporary. // - // As an example, consider an expression like `vec![].push()`. + // As an example, consider an expression like `vec![].push(x)`. // Here, the `vec![]` would wind up MIR stored into a // temporary variable `t` which we can borrow to invoke - // `>::push(&mut t)`. + // `>::push(&mut t, x)`. // // Note that an expression can have many adjustments, and we // are just ignoring those intermediate types. This is because @@ -287,7 +339,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { // The type table might not have information for this expression // if it is in a malformed scope. (#66387) if let Some(ty) = self.fcx.typeck_results.borrow().expr_ty_opt(expr) { - self.record(ty, scope, Some(expr), expr.span); + self.record(ty, scope, Some(expr), expr.span, guard_borrowing_from_pattern); } else { self.fcx.tcx.sess.delay_span_bug(expr.span, "no type for node"); } From 323f0794c09a545881180a391c9c68765216df5d Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Thu, 6 Aug 2020 16:18:46 +0800 Subject: [PATCH 3/7] test derived from #74961 --- .../ui/generator/yielding-in-match-guards.rs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 src/test/ui/generator/yielding-in-match-guards.rs diff --git a/src/test/ui/generator/yielding-in-match-guards.rs b/src/test/ui/generator/yielding-in-match-guards.rs new file mode 100644 index 0000000000000..88c1424ce14f0 --- /dev/null +++ b/src/test/ui/generator/yielding-in-match-guards.rs @@ -0,0 +1,29 @@ +// check-pass +// edition:2018 + +// This test is derived from +// https://github.com/rust-lang/rust/issues/74961#issuecomment-666893845 +// by @SNCPlay42 + +// This test demonstrates that, in `async fn g()`, +// indeed a temporary borrow `y` from `x` is live +// while `f().await` is being evaluated. +// Thus, `&'_ A` should be included in type signature +// of the underlying generator. + +#[derive(PartialEq, Eq)] +struct A; + +async fn f() -> A { + A +} + +async fn g() { + let x = A; + match x { + y if f().await == y => {} + _ => {} + } +} + +fn main() {} \ No newline at end of file From 66345d93592cda1d8f01d4cd0331a95e8feee45c Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Thu, 6 Aug 2020 16:20:06 +0800 Subject: [PATCH 4/7] rustfmt --- .../rustc_typeck/src/check/generator_interior.rs | 15 ++++++++++----- src/test/ui/generator/yielding-in-match-guards.rs | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 25268b554e1ce..973e9797ccbc4 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -8,9 +8,9 @@ use rustc_data_structures::fx::{FxHashSet, FxIndexSet}; use rustc_hir as hir; use rustc_hir::def::{CtorKind, DefKind, Res}; use rustc_hir::def_id::DefId; -use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; -use rustc_hir::{Expr, ExprKind, Pat, PatKind, Arm, Guard, HirId}; use rustc_hir::hir_id::HirIdSet; +use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; +use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind}; use rustc_middle::middle::region::{self, YieldData}; use rustc_middle::ty::{self, Ty}; use rustc_span::Span; @@ -64,7 +64,9 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { yield_data.expr_and_pat_count, self.expr_count, source_span ); - if guard_borrowing_from_pattern || yield_data.expr_and_pat_count >= self.expr_count { + if guard_borrowing_from_pattern + || yield_data.expr_and_pat_count >= self.expr_count + { Some(yield_data) } else { None @@ -239,7 +241,10 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { let mut scope_var_ids = self.nested_scope_of_guards.pop().expect("should have pushed at least one earlier"); for var_id in scope_var_ids.drain(..) { - assert!(self.current_scope_of_guards.remove(&var_id), "variable should be placed in scope earlier"); + assert!( + self.current_scope_of_guards.remove(&var_id), + "variable should be placed in scope earlier" + ); } self.arm_has_guard = false; } @@ -289,7 +294,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { } } _ => intravisit::walk_expr(self, expr), - } + }, ExprKind::Path(qpath) => { intravisit::walk_expr(self, expr); let res = self.fcx.typeck_results.borrow().qpath_res(qpath, expr.hir_id); diff --git a/src/test/ui/generator/yielding-in-match-guards.rs b/src/test/ui/generator/yielding-in-match-guards.rs index 88c1424ce14f0..12ef07ed032d7 100644 --- a/src/test/ui/generator/yielding-in-match-guards.rs +++ b/src/test/ui/generator/yielding-in-match-guards.rs @@ -26,4 +26,4 @@ async fn g() { } } -fn main() {} \ No newline at end of file +fn main() {} From df127b86cb90470d8ceebdd880db8ad4c372f91b Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Wed, 19 Aug 2020 09:04:40 +0800 Subject: [PATCH 5/7] switch the test to an actual MCVE --- .../ui/generator/yielding-in-match-guards.rs | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/test/ui/generator/yielding-in-match-guards.rs b/src/test/ui/generator/yielding-in-match-guards.rs index 12ef07ed032d7..d8aa354b1c604 100644 --- a/src/test/ui/generator/yielding-in-match-guards.rs +++ b/src/test/ui/generator/yielding-in-match-guards.rs @@ -2,28 +2,23 @@ // edition:2018 // This test is derived from -// https://github.com/rust-lang/rust/issues/74961#issuecomment-666893845 -// by @SNCPlay42 +// https://github.com/rust-lang/rust/issues/72651#issuecomment-668720468 // This test demonstrates that, in `async fn g()`, // indeed a temporary borrow `y` from `x` is live // while `f().await` is being evaluated. -// Thus, `&'_ A` should be included in type signature +// Thus, `&'_ u8` should be included in type signature // of the underlying generator. -#[derive(PartialEq, Eq)] -struct A; +async fn f() -> u8 { 1 } -async fn f() -> A { - A -} - -async fn g() { - let x = A; +pub async fn g(x: u8) { match x { - y if f().await == y => {} - _ => {} + y if f().await == y => (), + _ => (), } } -fn main() {} +fn main() { + let _ = g(10); +} From 4a8ba7b031728229cd30fe30e4d9f3cce6ece14c Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Wed, 7 Oct 2020 12:58:02 +0800 Subject: [PATCH 6/7] dedicated visitor for arm patterns --- .../src/check/generator_interior.rs | 59 +++++++++++++------ 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 973e9797ccbc4..677070c08e35e 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -28,9 +28,8 @@ struct InteriorVisitor<'a, 'tcx> { /// such borrows can span across this yield point. /// As such, we need to track these borrows and record them despite of the fact /// that they may succeed the said yield point in the post-order. - nested_scope_of_guards: SmallVec<[SmallVec<[HirId; 4]>; 4]>, - current_scope_of_guards: HirIdSet, - arm_has_guard: bool, + guard_bindings: SmallVec<[SmallVec<[HirId; 4]>; 1]>, + guard_bindings_set: HirIdSet, } impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { @@ -147,9 +146,8 @@ pub fn resolve_interior<'a, 'tcx>( expr_count: 0, kind, prev_unresolved_span: None, - nested_scope_of_guards: <_>::default(), - current_scope_of_guards: <_>::default(), - arm_has_guard: false, + guard_bindings: <_>::default(), + guard_bindings_set: <_>::default(), }; intravisit::walk_body(&mut visitor, body); @@ -228,25 +226,34 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { fn visit_arm(&mut self, arm: &'tcx Arm<'tcx>) { if arm.guard.is_some() { - self.nested_scope_of_guards.push(<_>::default()); - self.arm_has_guard = true; + self.guard_bindings.push(<_>::default()); } self.visit_pat(&arm.pat); if let Some(ref g) = arm.guard { + self.guard_bindings.push(<_>::default()); + ArmPatCollector { + guard_bindings_set: &mut self.guard_bindings_set, + guard_bindings: self + .guard_bindings + .last_mut() + .expect("should have pushed at least one earlier"), + } + .visit_pat(&arm.pat); + match g { Guard::If(ref e) => { self.visit_expr(e); } } + let mut scope_var_ids = - self.nested_scope_of_guards.pop().expect("should have pushed at least one earlier"); + self.guard_bindings.pop().expect("should have pushed at least one earlier"); for var_id in scope_var_ids.drain(..) { assert!( - self.current_scope_of_guards.remove(&var_id), + self.guard_bindings_set.remove(&var_id), "variable should be placed in scope earlier" ); } - self.arm_has_guard = false; } self.visit_expr(&arm.body); } @@ -256,14 +263,10 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { self.expr_count += 1; - if let PatKind::Binding(_, id, ..) = pat.kind { + if let PatKind::Binding(..) = pat.kind { let scope = self.region_scope_tree.var_scope(pat.hir_id.local_id); let ty = self.fcx.typeck_results.borrow().pat_ty(pat); self.record(ty, Some(scope), None, pat.span, false); - if self.arm_has_guard { - self.nested_scope_of_guards.as_mut_slice().last_mut().unwrap().push(id); - self.current_scope_of_guards.insert(id); - } } } @@ -299,8 +302,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { intravisit::walk_expr(self, expr); let res = self.fcx.typeck_results.borrow().qpath_res(qpath, expr.hir_id); match res { - Res::Local(id) if self.current_scope_of_guards.contains(&id) => { - debug!("a borrow in guard from pattern local is detected"); + Res::Local(id) if self.guard_bindings_set.contains(&id) => { guard_borrowing_from_pattern = true; } _ => {} @@ -350,3 +352,24 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { } } } + +struct ArmPatCollector<'a> { + guard_bindings_set: &'a mut HirIdSet, + guard_bindings: &'a mut SmallVec<[HirId; 4]>, +} + +impl<'a, 'tcx> Visitor<'tcx> for ArmPatCollector<'a> { + type Map = intravisit::ErasedMap<'tcx>; + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } + + fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) { + intravisit::walk_pat(self, pat); + if let PatKind::Binding(_, id, ..) = pat.kind { + self.guard_bindings.push(id); + self.guard_bindings_set.insert(id); + } + } +} From 50627a39c178b30a1bf2796201e442a61bdec369 Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Wed, 14 Oct 2020 00:50:30 +0800 Subject: [PATCH 7/7] explanatory comments and fix guard binding stack --- .../rustc_typeck/src/check/generator_interior.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 677070c08e35e..3fc5f02a4a47d 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -63,6 +63,9 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { yield_data.expr_and_pat_count, self.expr_count, source_span ); + // If it is a borrowing happening in the guard, + // it needs to be recorded regardless because they + // do live across this yield point. if guard_borrowing_from_pattern || yield_data.expr_and_pat_count >= self.expr_count { @@ -225,11 +228,9 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { } fn visit_arm(&mut self, arm: &'tcx Arm<'tcx>) { - if arm.guard.is_some() { - self.guard_bindings.push(<_>::default()); - } - self.visit_pat(&arm.pat); - if let Some(ref g) = arm.guard { + let Arm { guard, pat, body, .. } = arm; + self.visit_pat(pat); + if let Some(ref g) = guard { self.guard_bindings.push(<_>::default()); ArmPatCollector { guard_bindings_set: &mut self.guard_bindings_set, @@ -238,7 +239,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { .last_mut() .expect("should have pushed at least one earlier"), } - .visit_pat(&arm.pat); + .visit_pat(pat); match g { Guard::If(ref e) => { @@ -255,7 +256,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { ); } } - self.visit_expr(&arm.body); + self.visit_expr(body); } fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) {