Rollup merge of #94309 - eholk:issue-57017, r=tmandry

[generator_interior] Be more precise with scopes of borrowed places

Previously the generator interior type checking analysis would use the nearest temporary scope as the scope of a borrowed value. This ends up being overly broad for cases such as:

```rust
fn status(_client_status: &Client) -> i16 {
    200
}

fn main() {
    let client = Client;
    let g = move || match status(&client) {
        _status => yield,
    };
    assert_send(g);
}
```

In this case, the borrow `&client` could be considered in scope for the entirety of the `match` expression, meaning it would be viewed as live across the `yield`, therefore making the generator not `Send`.

In most cases, we want to use the enclosing expression as the scope for a borrowed value which will be less than or equal to the nearest temporary scope. This PR changes the analysis to use the enclosing expression as the scope for most borrows, with the exception of borrowed RValues which are true temporary values that should have the temporary scope. There's one further exception where borrows of a copy such as happens in autoref cases also should be ignored despite being RValues.

Joint work with `@nikomatsakis`

Fixes #57017

r? `@tmandry`

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