diff --git a/Cargo.lock b/Cargo.lock index 0f344dc920e57..dfe35e20a4bcb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4134,6 +4134,7 @@ dependencies = [ "rustc_session", "rustc_span", "rustc_target", + "smallvec", "tracing", ] diff --git a/compiler/rustc_middle/src/middle/region.rs b/compiler/rustc_middle/src/middle/region.rs index 75dd223d014d4..5f9ddae5f8abc 100644 --- a/compiler/rustc_middle/src/middle/region.rs +++ b/compiler/rustc_middle/src/middle/region.rs @@ -234,6 +234,15 @@ pub struct ScopeTree { /// escape into 'static and should have no local cleanup scope. rvalue_scopes: FxHashMap>, + /// Sometimes destruction scopes are too overarching in the sense that + /// all temporaries are dropped while it may be the case that + /// only a subset of temporaries is requested to be dropped. + /// For instance, `x[y.z()]` indexing operation can discard the temporaries + /// generated in evaluating `y.z()`. + /// `eager_scopes` is a map from items to a region smaller than a destruction scope + /// so that the temporaries do not need to linger unnecessarily. + eager_scopes: FxHashMap, + /// If there are any `yield` nested within a scope, this map /// stores the `Span` of the last one and its index in the /// postorder of the Visitor traversal on the HIR. @@ -358,6 +367,38 @@ impl ScopeTree { self.rvalue_scopes.insert(var, lifetime); } + pub fn record_local_access_scope(&mut self, var: hir::ItemLocalId, proposed_lifetime: Scope) { + debug!( + "record_local_access_scope(var={:?}, proposed_lifetime={:?})", + var, proposed_lifetime + ); + let mut id = Scope { id: var, data: ScopeData::Node }; + + while let Some(&(p, _)) = self.parent_map.get(&id) { + match p.data { + ScopeData::Destruction => return, + _ => id = p, + } + if id == proposed_lifetime { + // proposed lifetime outlives the destruction lifetime + self.record_rvalue_scope(var, Some(proposed_lifetime)); + return; + } + } + } + + pub fn record_eager_scope(&mut self, var: hir::ItemLocalId, proposed_lifetime: Scope) { + debug!("record_eager_scope(var={:?}, proposed_lifetime={:?})", var, proposed_lifetime); + if let Some(destruction) = self.temporary_scope(var) { + if self.is_subscope_of(destruction, proposed_lifetime) { + // If the current temporary scope is already a subset of the proposed region, + // it is safe to keep this scope and discard the proposal. + return; + } + } + self.eager_scopes.insert(var, proposed_lifetime); + } + /// Returns the narrowest scope that encloses `id`, if any. pub fn opt_encl_scope(&self, id: Scope) -> Option { self.parent_map.get(&id).cloned().map(|(p, _)| p) @@ -393,6 +434,9 @@ impl ScopeTree { } _ => id = p, } + if let Some(&eager_scope) = self.eager_scopes.get(&id.item_local_id()) { + return Some(eager_scope); + } } debug!("temporary_scope({:?}) = None", expr_id); @@ -444,6 +488,7 @@ impl<'a> HashStable> for ScopeTree { ref var_map, ref destruction_scopes, ref rvalue_scopes, + ref eager_scopes, ref yield_in_scope, } = *self; @@ -456,6 +501,7 @@ impl<'a> HashStable> for ScopeTree { var_map.hash_stable(hcx, hasher); destruction_scopes.hash_stable(hcx, hasher); rvalue_scopes.hash_stable(hcx, hasher); + eager_scopes.hash_stable(hcx, hasher); yield_in_scope.hash_stable(hcx, hasher); } } diff --git a/compiler/rustc_passes/Cargo.toml b/compiler/rustc_passes/Cargo.toml index 39e578bce7ef6..1ed7c26f056f7 100644 --- a/compiler/rustc_passes/Cargo.toml +++ b/compiler/rustc_passes/Cargo.toml @@ -5,6 +5,7 @@ edition = "2021" [dependencies] tracing = "0.1" +smallvec = { version = "1.6.1", features = ["union", "may_dangle"] } rustc_middle = { path = "../rustc_middle" } rustc_attr = { path = "../rustc_attr" } rustc_data_structures = { path = "../rustc_data_structures" } diff --git a/compiler/rustc_passes/src/region.rs b/compiler/rustc_passes/src/region.rs index fdf93e5893247..de26585e9014e 100644 --- a/compiler/rustc_passes/src/region.rs +++ b/compiler/rustc_passes/src/region.rs @@ -18,6 +18,7 @@ use rustc_middle::ty::query::Providers; use rustc_middle::ty::TyCtxt; use rustc_span::source_map; use rustc_span::Span; +use smallvec::SmallVec; use std::mem; @@ -203,6 +204,105 @@ fn resolve_stmt<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, stmt: &'tcx h visitor.cx.parent = prev_parent; } +fn mark_local_terminating_scopes<'tcx>(expr: &'tcx hir::Expr<'tcx>) -> FxHashSet { + struct LocalAccessResolutionVisitor<'a> { + locals: &'a mut FxHashSet, + } + impl<'a> LocalAccessResolutionVisitor<'a> { + fn probe<'b>(&mut self, expr: &'b Expr<'b>) { + if self.locals.contains(&expr.hir_id.local_id) { + return; + } + let mut nested_expr = expr; + let mut ops = SmallVec::<[_; 4]>::new(); + enum OpTy { + Ref, + Deref, + Project, + } + // Here we are looking for a chain of access to extract a place reference for + // a local variable. + // For instance, given `&*(&*x[y]).z().u`, `probe` will pick out + // the sub-expressions `&*x[y]` and `y` as expressions making accesses to locals. + // The place references generated can be discarded earlier as MIR allows, + // and, thus, can be assigned a smaller region. + loop { + match nested_expr.kind { + hir::ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(_), .. }, + )) => { + // We have reached the end of the access path + // because a local variable access is encountered + break; + } + hir::ExprKind::AddrOf(_, _, subexpr) => { + ops.push((nested_expr, OpTy::Ref)); + nested_expr = subexpr; + } + hir::ExprKind::Unary(hir::UnOp::Deref, subexpr) => { + ops.push((nested_expr, OpTy::Deref)); + nested_expr = subexpr; + } + hir::ExprKind::Field(subexpr, _) | hir::ExprKind::Index(subexpr, _) => { + ops.push((nested_expr, OpTy::Project)); + nested_expr = subexpr; + } + _ => { + drop(ops); + intravisit::walk_expr(self, expr); + return; + } + } + } + let mut ref_level = SmallVec::<[_; 4]>::new(); + for (expr, op) in ops.into_iter().rev() { + match op { + OpTy::Ref => { + ref_level.push(expr); + } + OpTy::Deref => { + if let Some(ref_expr) = ref_level.pop() { + self.locals.insert(ref_expr.hir_id.local_id); + } + } + OpTy::Project => { + // The generated MIR produces temporaries for the each of the nested references, + // such as `&x`, `&&x` in `(&&&x).y` for the field access of `y`. + // These temporaries must stay alive even after the field access. + ref_level.clear(); + } + } + } + self.locals.insert(expr.hir_id.local_id); + } + } + impl<'a, 'b> Visitor<'a> for LocalAccessResolutionVisitor<'b> { + fn visit_expr(&mut self, expr: &'a Expr<'a>) { + match expr.kind { + hir::ExprKind::AddrOf(..) + | hir::ExprKind::Unary(hir::UnOp::Deref, _) + | hir::ExprKind::Field(..) + | hir::ExprKind::Index(..) + | hir::ExprKind::Path(..) => self.probe(expr), + + // We do not probe into other function bodies or blocks, + // neither `if`s and `match`es because they will be covered in deeper visits + hir::ExprKind::If(..) + | hir::ExprKind::Match(..) + | hir::ExprKind::Block(..) + | hir::ExprKind::Closure(..) + | hir::ExprKind::ConstBlock(..) => {} + _ => intravisit::walk_expr(self, expr), + } + } + } + let mut locals = Default::default(); + let mut resolution_visitor = LocalAccessResolutionVisitor { locals: &mut locals }; + resolution_visitor.visit_expr(expr); + locals +} + fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx hir::Expr<'tcx>) { debug!("resolve_expr - pre-increment {} expr = {:?}", visitor.expr_and_pat_count, expr); @@ -396,7 +496,15 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h let expr_cx = visitor.cx; visitor.enter_scope(Scope { id: then.hir_id.local_id, data: ScopeData::IfThen }); visitor.cx.var_parent = visitor.cx.parent; - visitor.visit_expr(cond); + { + visitor.visit_expr(cond); + let lifetime = Scope { id: cond.hir_id.local_id, data: ScopeData::Node }; + for local in mark_local_terminating_scopes(cond) { + if local != cond.hir_id.local_id { + visitor.scope_tree.record_local_access_scope(local, lifetime); + } + } + } visitor.visit_expr(then); visitor.cx = expr_cx; visitor.visit_expr(otherwise); @@ -406,11 +514,39 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h let expr_cx = visitor.cx; visitor.enter_scope(Scope { id: then.hir_id.local_id, data: ScopeData::IfThen }); visitor.cx.var_parent = visitor.cx.parent; - visitor.visit_expr(cond); + { + visitor.visit_expr(cond); + let lifetime = Scope { id: cond.hir_id.local_id, data: ScopeData::Node }; + for local in mark_local_terminating_scopes(cond) { + if local != cond.hir_id.local_id { + visitor.scope_tree.record_local_access_scope(local, lifetime); + } + } + } visitor.visit_expr(then); visitor.cx = expr_cx; } + hir::ExprKind::Match(subexpr, arms, _) => { + visitor.visit_expr(subexpr); + let lifetime = Scope { id: subexpr.hir_id.local_id, data: ScopeData::Node }; + for local in mark_local_terminating_scopes(subexpr) { + if local != subexpr.hir_id.local_id { + visitor.scope_tree.record_local_access_scope(local, lifetime); + } + } + walk_list!(visitor, visit_arm, arms); + } + + hir::ExprKind::Index(subexpr, idxexpr) => { + visitor.visit_expr(subexpr); + visitor.visit_expr(idxexpr); + visitor.scope_tree.record_eager_scope( + idxexpr.hir_id.local_id, + Scope { id: expr.hir_id.local_id, data: ScopeData::Node }, + ); + } + _ => intravisit::walk_expr(visitor, expr), } @@ -683,10 +819,17 @@ fn resolve_local<'tcx>( visitor.scope_tree.record_rvalue_scope(expr.hir_id.local_id, blk_scope); match expr.kind { - hir::ExprKind::AddrOf(_, _, ref subexpr) - | hir::ExprKind::Unary(hir::UnOp::Deref, ref subexpr) - | hir::ExprKind::Field(ref subexpr, _) - | hir::ExprKind::Index(ref subexpr, _) => { + hir::ExprKind::AddrOf(_, _, subexpr) + | hir::ExprKind::Unary(hir::UnOp::Deref, subexpr) + | hir::ExprKind::Field(subexpr, _) + | hir::ExprKind::Index(subexpr, _) => { + if let hir::ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(_), .. }, + )) = &subexpr.kind + { + return; + } expr = &subexpr; } _ => { diff --git a/src/test/mir-opt/remove_storage_markers.main.RemoveStorageMarkers.diff b/src/test/mir-opt/remove_storage_markers.main.RemoveStorageMarkers.diff index 2dfc94f21868e..124017463d06a 100644 --- a/src/test/mir-opt/remove_storage_markers.main.RemoveStorageMarkers.diff +++ b/src/test/mir-opt/remove_storage_markers.main.RemoveStorageMarkers.diff @@ -71,7 +71,6 @@ - StorageDead(_13); // scope 3 at $DIR/remove_storage_markers.rs:9:16: 9:17 _6 = const (); // scope 3 at $DIR/remove_storage_markers.rs:8:20: 10:6 - StorageDead(_12); // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6 -- StorageDead(_9); // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6 - StorageDead(_7); // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6 - StorageDead(_6); // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6 _5 = const (); // scope 2 at $DIR/remove_storage_markers.rs:8:5: 10:6 @@ -80,7 +79,6 @@ bb3: { _0 = const (); // scope 2 at $DIR/remove_storage_markers.rs:8:5: 10:6 -- StorageDead(_9); // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6 - StorageDead(_7); // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6 - StorageDead(_6); // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6 - StorageDead(_4); // scope 1 at $DIR/remove_storage_markers.rs:10:5: 10:6 @@ -91,6 +89,7 @@ bb4: { - StorageDead(_14); // scope 5 at $DIR/remove_storage_markers.rs:8:14: 8:19 +- StorageDead(_9); // scope 2 at $DIR/remove_storage_markers.rs:8:18: 8:19 - StorageDead(_8); // scope 2 at $DIR/remove_storage_markers.rs:8:18: 8:19 _10 = discriminant(_7); // scope 2 at $DIR/remove_storage_markers.rs:8:14: 8:19 switchInt(move _10) -> [0_isize: bb3, otherwise: bb2]; // scope 2 at $DIR/remove_storage_markers.rs:8:14: 8:19 diff --git a/src/test/ui/async-await/issue-64130-4-async-move.rs b/src/test/ui/async-await/issue-64130-4-async-move.rs index 2538f34351e5a..99a3b28c5e00f 100644 --- a/src/test/ui/async-await/issue-64130-4-async-move.rs +++ b/src/test/ui/async-await/issue-64130-4-async-move.rs @@ -10,16 +10,15 @@ impl Client { } } -async fn get() { } +async fn get() {} pub fn foo() -> impl Future + Send { //~^ ERROR future cannot be sent between threads safely - let client = Client(Box::new(true)); - async move { - match client.status() { + async { + match Client(Box::new(true)).status() { 200 => { let _x = get().await; - }, + } _ => (), } } diff --git a/src/test/ui/async-await/issue-64130-4-async-move.stderr b/src/test/ui/async-await/issue-64130-4-async-move.stderr index d631e6dc7f7e9..0eeed986cd7eb 100644 --- a/src/test/ui/async-await/issue-64130-4-async-move.stderr +++ b/src/test/ui/async-await/issue-64130-4-async-move.stderr @@ -6,21 +6,21 @@ LL | pub fn foo() -> impl Future + Send { | = help: the trait `Sync` is not implemented for `(dyn Any + Send + 'static)` note: future is not `Send` as this value is used across an await - --> $DIR/issue-64130-4-async-move.rs:21:31 + --> $DIR/issue-64130-4-async-move.rs:20:31 | -LL | match client.status() { - | ------ has type `&Client` which is not `Send` +LL | match Client(Box::new(true)).status() { + | ---------------------- has type `&Client` which is not `Send` LL | 200 => { LL | let _x = get().await; - | ^^^^^^ await occurs here, with `client` maybe used later + | ^^^^^^ await occurs here, with `Client(Box::new(true))` maybe used later ... LL | } - | - `client` is later dropped here + | - `Client(Box::new(true))` is later dropped here help: consider moving this into a `let` binding to create a shorter lived borrow - --> $DIR/issue-64130-4-async-move.rs:19:15 + --> $DIR/issue-64130-4-async-move.rs:18:15 | -LL | match client.status() { - | ^^^^^^^^^^^^^^^ +LL | match Client(Box::new(true)).status() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to previous error diff --git a/src/test/ui/async-await/issue-69663-lingering-local-borrows.rs b/src/test/ui/async-await/issue-69663-lingering-local-borrows.rs new file mode 100644 index 0000000000000..7dfde370f31d8 --- /dev/null +++ b/src/test/ui/async-await/issue-69663-lingering-local-borrows.rs @@ -0,0 +1,51 @@ +// edition:2018 +// build-pass + +use std::ops::Index; + +/// A `Send + !Sync` for demonstration purposes. +struct Banana(*mut ()); +unsafe impl Send for Banana {} + +impl Banana { + /// Make a static mutable reference to Banana for convenience purposes. + /// + /// Any potential unsoundness here is not super relevant to the issue at hand. + fn new() -> &'static mut Banana { + static mut BANANA: Banana = Banana(std::ptr::null_mut()); + unsafe { &mut BANANA } + } +} + +// Peach is still Send (because `impl Send for &mut T where T: Send`) +struct Peach<'a>(&'a mut Banana); + +impl<'a> std::ops::Index for Peach<'a> { + type Output = (); + fn index(&self, _: usize) -> &() { + &() + } +} + +async fn baz(_: &()) {} + +async fn bar() { + let peach = Peach(Banana::new()); + let r = &*peach.index(0); + baz(r).await; + peach.index(0); // make sure peach is retained across yield point +} + +async fn bat() { + let peach = Peach(Banana::new()); + let r = &peach[0]; + baz(r).await; + peach[0]; // make sure peach is retained across yield point +} + +fn assert_send(_: T) {} + +pub fn main() { + assert_send(bar()); + assert_send(bat()); +} diff --git a/src/test/ui/generator/refined-region-for-local-accesses.rs b/src/test/ui/generator/refined-region-for-local-accesses.rs new file mode 100644 index 0000000000000..67daf6bc6aac5 --- /dev/null +++ b/src/test/ui/generator/refined-region-for-local-accesses.rs @@ -0,0 +1,40 @@ +// edition:2018 +// build-pass +#![feature(generators, generator_trait, negative_impls)] + +fn assert_send(_: T) {} + +struct Client; + +impl Client { + fn zero(&self) -> usize { + 0 + } + + fn status(&self) -> i16 { + 200 + } +} + +impl !Sync for Client {} + +fn status(_: &Client) -> i16 { + 200 +} + +fn main() { + let g = || { + let x = Client; + match status(&x) { + _ => yield, + } + match (&*&x).status() { + _ => yield, + } + let a = [0]; + match a[Client.zero()] { + _ => yield, + } + }; + assert_send(g); +}