diff options
6 files changed, 144 insertions, 13 deletions
diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 48d24170072..ca5ae20dfcf 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -13,7 +13,7 @@ use rustc_hir::def_id::DefId; use rustc_hir::hir_id::HirIdSet; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind}; -use rustc_middle::middle::region::{self, YieldData}; +use rustc_middle::middle::region::{self, Scope, ScopeData, YieldData}; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_span::symbol::sym; use rustc_span::Span; @@ -369,7 +369,25 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { self.expr_count += 1; - let scope = self.region_scope_tree.temporary_scope(expr.hir_id.local_id); + debug!("is_borrowed_temporary: {:?}", self.drop_ranges.is_borrowed_temporary(expr)); + + // Typically, the value produced by an expression is consumed by its parent in some way, + // so we only have to check if the parent contains a yield (note that the parent may, for + // example, store the value into a local variable, but then we already consider local + // variables to be live across their scope). + // + // However, in the case of temporary values, we are going to store the value into a + // temporary on the stack that is live for the current temporary scope and then return a + // reference to it. That value may be live across the entire temporary scope. + let scope = if self.drop_ranges.is_borrowed_temporary(expr) { + self.region_scope_tree.temporary_scope(expr.hir_id.local_id) + } else { + debug!("parent_node: {:?}", self.fcx.tcx.hir().find_parent_node(expr.hir_id)); + match self.fcx.tcx.hir().find_parent_node(expr.hir_id) { + Some(parent) => Some(Scope { id: parent.local_id, data: ScopeData::Node }), + None => self.region_scope_tree.temporary_scope(expr.hir_id.local_id), + } + }; // If there are adjustments, then record the final type -- // this is the actual value that is being produced. diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index 972dd622d6e..4fa7ed82c6a 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -18,6 +18,7 @@ use crate::check::FnCtxt; use hir::def_id::DefId; use hir::{Body, HirId, HirIdMap, Node}; use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::stable_set::FxHashSet; use rustc_hir as hir; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; @@ -41,7 +42,7 @@ pub fn compute_drop_ranges<'a, 'tcx>( let consumed_borrowed_places = find_consumed_and_borrowed(fcx, def_id, body); let num_exprs = fcx.tcx.region_scope_tree(def_id).body_expr_count(body.id()).unwrap_or(0); - let mut drop_ranges = build_control_flow_graph( + let (mut drop_ranges, borrowed_temporaries) = build_control_flow_graph( fcx.tcx.hir(), fcx.tcx, &fcx.typeck_results.borrow(), @@ -52,11 +53,20 @@ pub fn compute_drop_ranges<'a, 'tcx>( drop_ranges.propagate_to_fixpoint(); - DropRanges { tracked_value_map: drop_ranges.tracked_value_map, nodes: drop_ranges.nodes } + debug!("borrowed_temporaries = {borrowed_temporaries:?}"); + DropRanges { + tracked_value_map: drop_ranges.tracked_value_map, + nodes: drop_ranges.nodes, + borrowed_temporaries: Some(borrowed_temporaries), + } } else { // If drop range tracking is not enabled, skip all the analysis and produce an // empty set of DropRanges. - DropRanges { tracked_value_map: FxHashMap::default(), nodes: IndexVec::new() } + DropRanges { + tracked_value_map: FxHashMap::default(), + nodes: IndexVec::new(), + borrowed_temporaries: None, + } } } @@ -161,6 +171,7 @@ impl TryFrom<&PlaceWithHirId<'_>> for TrackedValue { pub struct DropRanges { tracked_value_map: FxHashMap<TrackedValue, TrackedValueIndex>, nodes: IndexVec<PostOrderId, NodeInfo>, + borrowed_temporaries: Option<FxHashSet<HirId>>, } impl DropRanges { @@ -174,6 +185,10 @@ impl DropRanges { }) } + pub fn is_borrowed_temporary(&self, expr: &hir::Expr<'_>) -> bool { + if let Some(b) = &self.borrowed_temporaries { b.contains(&expr.hir_id) } else { true } + } + /// Returns a reference to the NodeInfo for a node, panicking if it does not exist fn expect_node(&self, id: PostOrderId) -> &NodeInfo { &self.nodes[id] diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index cfed784ea72..f4dd4cc010d 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -6,7 +6,7 @@ use hir::{ intravisit::{self, Visitor}, Body, Expr, ExprKind, Guard, HirId, LoopIdError, }; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::{fx::FxHashMap, stable_set::FxHashSet}; use rustc_hir as hir; use rustc_index::vec::IndexVec; use rustc_middle::{ @@ -27,14 +27,14 @@ pub(super) fn build_control_flow_graph<'tcx>( consumed_borrowed_places: ConsumedAndBorrowedPlaces, body: &'tcx Body<'tcx>, num_exprs: usize, -) -> DropRangesBuilder { +) -> (DropRangesBuilder, FxHashSet<HirId>) { let mut drop_range_visitor = DropRangeVisitor::new(hir, tcx, typeck_results, consumed_borrowed_places, num_exprs); intravisit::walk_body(&mut drop_range_visitor, body); drop_range_visitor.drop_ranges.process_deferred_edges(); - drop_range_visitor.drop_ranges + (drop_range_visitor.drop_ranges, drop_range_visitor.places.borrowed_temporaries) } /// This struct is used to gather the information for `DropRanges` to determine the regions of the diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index 40ee6d863b5..928daba0a7b 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -6,6 +6,7 @@ use crate::{ use hir::{def_id::DefId, Body, HirId, HirIdMap}; use rustc_data_structures::stable_set::FxHashSet; use rustc_hir as hir; +use rustc_middle::hir::place::{PlaceBase, Projection, ProjectionKind}; use rustc_middle::ty::{ParamEnv, TyCtxt}; pub(super) fn find_consumed_and_borrowed<'a, 'tcx>( @@ -27,8 +28,12 @@ pub(super) struct ConsumedAndBorrowedPlaces { /// Note that this set excludes "partial drops" -- for example, a statement like `drop(x.y)` is /// not considered a drop of `x`, although it would be a drop of `x.y`. pub(super) consumed: HirIdMap<FxHashSet<TrackedValue>>, + /// A set of hir-ids of values or variables that are borrowed at some point within the body. pub(super) borrowed: FxHashSet<TrackedValue>, + + /// A set of hir-ids of values or variables that are borrowed at some point within the body. + pub(super) borrowed_temporaries: FxHashSet<HirId>, } /// Works with ExprUseVisitor to find interesting values for the drop range analysis. @@ -49,6 +54,7 @@ impl<'tcx> ExprUseDelegate<'tcx> { places: ConsumedAndBorrowedPlaces { consumed: <_>::default(), borrowed: <_>::default(), + borrowed_temporaries: <_>::default(), }, } } @@ -96,12 +102,76 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { &mut self, place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, diag_expr_id: HirId, - _bk: rustc_middle::ty::BorrowKind, + bk: rustc_middle::ty::BorrowKind, + ) { + debug!( + "borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}, \ + borrow_kind={bk:?}" + ); + + self.places + .borrowed + .insert(TrackedValue::from_place_with_projections_allowed(place_with_id)); + + // Ordinarily a value is consumed by it's parent, but in the special case of a + // borrowed RValue, we create a reference that lives as long as the temporary scope + // for that expression (typically, the innermost statement, but sometimes the enclosing + // block). We record this fact here so that later in generator_interior + // we can use the correct scope. + // + // We special case borrows through a dereference (`&*x`, `&mut *x` where `x` is + // some rvalue expression), since these are essentially a copy of a pointer. + // In other words, this borrow does not refer to the + // temporary (`*x`), but to the referent (whatever `x` is a borrow of). + // + // We were considering that we might encounter problems down the line if somehow, + // some part of the compiler were to look at this result and try to use it to + // drive a borrowck-like analysis (this does not currently happen, as of this writing). + // But even this should be fine, because the lifetime of the dereferenced reference + // found in the rvalue is only significant as an intermediate 'link' to the value we + // are producing, and we separately track whether that value is live over a yield. + // Example: + // + // ```notrust + // fn identity<T>(x: &mut T) -> &mut T { x } + // let a: A = ...; + // let y: &'y mut A = &mut *identity(&'a mut a); + // ^^^^^^^^^^^^^^^^^^^^^^^^^ the borrow we are talking about + // ``` + // + // The expression `*identity(...)` is a deref of an rvalue, + // where the `identity(...)` (the rvalue) produces a return type + // of `&'rv mut A`, where `'a: 'rv`. We then assign this result to + // `'y`, resulting in (transitively) `'a: 'y` (i.e., while `y` is in use, + // `a` will be considered borrowed). Other parts of the code will ensure + // that if `y` is live over a yield, `&'y mut A` appears in the generator + // state. If `'y` is live, then any sound region analysis must conclude + // that `'a` is also live. So if this causes a bug, blame some other + // part of the code! + let is_deref = place_with_id + .place + .projections + .iter() + .any(|Projection { kind, .. }| *kind == ProjectionKind::Deref); + + if let (false, PlaceBase::Rvalue) = (is_deref, place_with_id.place.base) { + self.places.borrowed_temporaries.insert(place_with_id.hir_id); + } + } + + fn copy( + &mut self, + place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, + _diag_expr_id: HirId, ) { - debug!("borrow {:?}; diag_expr_id={:?}", place_with_id, diag_expr_id); + debug!("copy: place_with_id = {place_with_id:?}"); + self.places .borrowed .insert(TrackedValue::from_place_with_projections_allowed(place_with_id)); + + // For copied we treat this mostly like a borrow except that we don't add the place + // to borrowed_temporaries because the copy is consumed. } fn mutate( diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index cb59438e343..b08e08a27d0 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -47,6 +47,14 @@ pub trait Delegate<'tcx> { bk: ty::BorrowKind, ); + /// The value found at `place` is being copied. + /// `diag_expr_id` is the id used for diagnostics (see `consume` for more details). + fn copy(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) { + // In most cases, copying data from `x` is equivalent to doing `*&x`, so by default + // we treat a copy of `x` as a borrow of `x`. + self.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow) + } + /// The path at `assignee_place` is being assigned to. /// `diag_expr_id` is the id used for diagnostics (see `consume` for more details). fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId); @@ -836,9 +844,7 @@ fn delegate_consume<'a, 'tcx>( match mode { ConsumeMode::Move => delegate.consume(place_with_id, diag_expr_id), - ConsumeMode::Copy => { - delegate.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow) - } + ConsumeMode::Copy => delegate.copy(place_with_id, diag_expr_id), } } diff --git a/src/test/ui/generator/issue-57017.rs b/src/test/ui/generator/issue-57017.rs new file mode 100644 index 00000000000..1223a3037ab --- /dev/null +++ b/src/test/ui/generator/issue-57017.rs @@ -0,0 +1,22 @@ +// check-pass +// compile-flags: -Zdrop-tracking +#![feature(generators, negative_impls)] + +struct Client; + +impl !Sync for Client {} + +fn status(_client_status: &Client) -> i16 { + 200 +} + +fn assert_send<T: Send>(_thing: T) {} + +// This is the same bug as issue 57017, but using yield instead of await +fn main() { + let client = Client; + let g = move || match status(&client) { + _status => yield, + }; + assert_send(g); +} |