[generator_interior] Be more precise with scopes of borrowed places

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.

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]

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

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(

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),
     }
 }
 

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);
+}