Auto merge of rust-lang#96923 - eholk:fix-fake-read, r=nikomatsakis

Drop Tracking: Implement `fake_read` callback

This PR updates drop tracking's use of `ExprUseVisitor` so that we treat `fake_read` events as borrows. Without doing this, we were not handling match expressions correctly, which showed up as a breakage in the `addassign-yield.rs` test. We did not previously notice this because we still had rather large temporary scopes that we held borrows for, which changed in rust-lang#94309.

This PR also includes a variant of the `addassign-yield.rs` test case to make sure we continue to have correct behavior here with drop tracking.

r? `@nikomatsakis`

Author: bors

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs

@@ -77,38 +77,8 @@ impl<'tcx> ExprUseDelegate<'tcx> {
         }
         self.places.consumed.get_mut(&consumer).map(|places| places.insert(target));
     }
-}
-
-impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
-    fn consume(
-        &mut self,
-        place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
-        diag_expr_id: HirId,
-    ) {
-        let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) {
-            Some(parent) => parent,
-            None => place_with_id.hir_id,
-        };
-        debug!(
-            "consume {:?}; diag_expr_id={:?}, using parent {:?}",
-            place_with_id, diag_expr_id, parent
-        );
-        place_with_id
-            .try_into()
-            .map_or((), |tracked_value| self.mark_consumed(parent, tracked_value));
-    }
-
-    fn borrow(
-        &mut self,
-        place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
-        diag_expr_id: HirId,
-        bk: rustc_middle::ty::BorrowKind,
-    ) {
-        debug!(
-            "borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}, \
-            borrow_kind={bk:?}"
-        );
 
+    fn borrow_place(&mut self, place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>) {
         self.places
             .borrowed
             .insert(TrackedValue::from_place_with_projections_allowed(place_with_id));
@@ -158,6 +128,40 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
             self.places.borrowed_temporaries.insert(place_with_id.hir_id);
         }
     }
+}
+
+impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
+    fn consume(
+        &mut self,
+        place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
+        diag_expr_id: HirId,
+    ) {
+        let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) {
+            Some(parent) => parent,
+            None => place_with_id.hir_id,
+        };
+        debug!(
+            "consume {:?}; diag_expr_id={:?}, using parent {:?}",
+            place_with_id, diag_expr_id, parent
+        );
+        place_with_id
+            .try_into()
+            .map_or((), |tracked_value| self.mark_consumed(parent, tracked_value));
+    }
+
+    fn borrow(
+        &mut self,
+        place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
+        diag_expr_id: HirId,
+        bk: rustc_middle::ty::BorrowKind,
+    ) {
+        debug!(
+            "borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}, \
+            borrow_kind={bk:?}"
+        );
+
+        self.borrow_place(place_with_id);
+    }
 
     fn copy(
         &mut self,
@@ -208,9 +212,18 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
 
     fn fake_read(
         &mut self,
-        _place: expr_use_visitor::Place<'tcx>,
-        _cause: rustc_middle::mir::FakeReadCause,
-        _diag_expr_id: HirId,
+        place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
+        cause: rustc_middle::mir::FakeReadCause,
+        diag_expr_id: HirId,
     ) {
+        debug!(
+            "fake_read place_with_id={place_with_id:?}; cause={cause:?}; diag_expr_id={diag_expr_id:?}"
+        );
+
+        // fake reads happen in places like the scrutinee of a match expression.
+        // we treat those as a borrow, much like a copy: the idea is that we are
+        // transiently creating a `&T` ref that we can read from to observe the current
+        // value (this `&T` is immediately dropped afterwards).
+        self.borrow_place(place_with_id);
     }
 }

compiler/rustc_typeck/src/check/upvar.rs

@@ -1755,14 +1755,19 @@ struct InferBorrowKind<'a, 'tcx> {
 }
 
 impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
-    fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId) {
-        let PlaceBase::Upvar(_) = place.base else { return };
+    fn fake_read(
+        &mut self,
+        place: &PlaceWithHirId<'tcx>,
+        cause: FakeReadCause,
+        diag_expr_id: hir::HirId,
+    ) {
+        let PlaceBase::Upvar(_) = place.place.base else { return };
 
         // We need to restrict Fake Read precision to avoid fake reading unsafe code,
         // such as deref of a raw pointer.
         let dummy_capture_kind = ty::UpvarCapture::ByRef(ty::BorrowKind::ImmBorrow);
 
-        let (place, _) = restrict_capture_precision(place, dummy_capture_kind);
+        let (place, _) = restrict_capture_precision(place.place.clone(), dummy_capture_kind);
 
         let (place, _) = restrict_repr_packed_field_ref_capture(
             self.fcx.tcx,

compiler/rustc_typeck/src/expr_use_visitor.rs

@@ -70,7 +70,12 @@ pub trait Delegate<'tcx> {
     }
 
     /// The `place` should be a fake read because of specified `cause`.
-    fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId);
+    fn fake_read(
+        &mut self,
+        place_with_id: &PlaceWithHirId<'tcx>,
+        cause: FakeReadCause,
+        diag_expr_id: hir::HirId,
+    );
 }
 
 #[derive(Copy, Clone, PartialEq, Debug)]
@@ -328,7 +333,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                     };
 
                     self.delegate.fake_read(
-                        discr_place.place.clone(),
+                        &discr_place,
                         FakeReadCause::ForMatchedPlace(closure_def_id),
                         discr_place.hir_id,
                     );
@@ -618,7 +623,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
         };
 
         self.delegate.fake_read(
-            discr_place.place.clone(),
+            discr_place,
             FakeReadCause::ForMatchedPlace(closure_def_id),
             discr_place.hir_id,
         );
@@ -642,7 +647,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
         };
 
         self.delegate.fake_read(
-            discr_place.place.clone(),
+            discr_place,
             FakeReadCause::ForLet(closure_def_id),
             discr_place.hir_id,
         );
@@ -777,7 +782,11 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                         );
                     }
                 };
-                self.delegate.fake_read(fake_read.clone(), *cause, *hir_id);
+                self.delegate.fake_read(
+                    &PlaceWithHirId { place: fake_read.clone(), hir_id: *hir_id },
+                    *cause,
+                    *hir_id,
+                );
             }
         }
 

src/test/ui/generator/drop-track-addassign-yield.rs

@@ -0,0 +1,41 @@
+// run-pass
+// compile-flags: -Zdrop-tracking
+
+// Based on addassign-yield.rs, but with drop tracking enabled. Originally we did not implement
+// the fake_read callback on ExprUseVisitor which caused this case to break.
+
+#![feature(generators)]
+
+fn foo() {
+    let _y = static || {
+        let x = &mut 0;
+        *{
+            yield;
+            x
+        } += match String::new() {
+            _ => 0,
+        };
+    };
+
+    // Please don't ever actually write something like this
+    let _z = static || {
+        let x = &mut 0;
+        *{
+            let inner = &mut 1;
+            *{
+                yield ();
+                inner
+            } += match String::new() {
+                _ => 1,
+            };
+            yield;
+            x
+        } += match String::new() {
+            _ => 2,
+        };
+    };
+}
+
+fn main() {
+    foo()
+}

src/tools/clippy/clippy_lints/src/escape.rs

@@ -187,7 +187,7 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
         }
     }
 
-    fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
+    fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
 }
 
 impl<'a, 'tcx> EscapeDelegate<'a, 'tcx> {

src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs

@@ -114,7 +114,7 @@ impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> {
         }
     }
 
-    fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
+    fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
 }
 
 impl MutatePairDelegate<'_, '_> {

src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs

@@ -343,5 +343,5 @@ impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt {
 
     fn mutate(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId) {}
 
-    fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
+    fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
 }

src/tools/clippy/clippy_utils/src/sugg.rs

@@ -1033,7 +1033,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
 
     fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
 
-    fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
+    fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
 }
 
 #[cfg(test)]

src/tools/clippy/clippy_utils/src/usage.rs

@@ -74,7 +74,7 @@ impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
         self.update(cmt);
     }
 
-    fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
+    fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
 }
 
 pub struct ParamBindingIdCollector {