Rollup merge of rust-lang#94460 - eholk:reenable-drop-tracking-tests,…

… r=tmiasko Reenable generator drop tracking tests and fix mutation handling The previous PR, rust-lang#94068, was overly zealous in counting mutations as borrows, which effectively nullified drop tracking. We would have caught this except the drop tracking tests were still ignored, despite having the option of using the `-Zdrop-tracking` flag now. This PR fixes the issue introduced by rust-lang#94068 by only counting mutations as borrows the mutated place has a project. This is sufficient to distinguish `x.y = 42` (which should count as a borrow of `x`) from `x = 42` (which is not a borrow of `x` because the whole variable is overwritten). This PR also re-enables the drop tracking regression tests using the `-Zdrop-tracking` flag so we will avoid introducing these sorts of issues in the future. Thanks to ``@tmiasko`` for noticing this problem and pointing it out! r? ``@tmiasko``
oribenshir · Mar 5, 2022 · c7d2004 · c7d2004
2 parents 3e1e9b4 + add169d
commit c7d2004
Show file tree

Hide file tree

Showing 10 changed files with 75 additions and 52 deletions.
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
@@ -6,14 +6,14 @@ 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::map::Map;
+use rustc_middle::ty::{ParamEnv, TyCtxt};
 
 pub(super) fn find_consumed_and_borrowed<'a, 'tcx>(
     fcx: &'a FnCtxt<'a, 'tcx>,
     def_id: DefId,
     body: &'tcx Body<'tcx>,
 ) -> ConsumedAndBorrowedPlaces {
-    let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx.hir());
+    let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx, fcx.param_env);
     expr_use_visitor.consume_body(fcx, def_id, body);
     expr_use_visitor.places
 }
@@ -36,14 +36,16 @@ pub(super) struct ConsumedAndBorrowedPlaces {
 /// Interesting values are those that are either dropped or borrowed. For dropped values, we also
 /// record the parent expression, which is the point where the drop actually takes place.
 struct ExprUseDelegate<'tcx> {
-    hir: Map<'tcx>,
+    tcx: TyCtxt<'tcx>,
+    param_env: ParamEnv<'tcx>,
     places: ConsumedAndBorrowedPlaces,
 }
 
 impl<'tcx> ExprUseDelegate<'tcx> {
-    fn new(hir: Map<'tcx>) -> Self {
+    fn new(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Self {
         Self {
-            hir,
+            tcx,
+            param_env,
             places: ConsumedAndBorrowedPlaces {
                 consumed: <_>::default(),
                 borrowed: <_>::default(),
@@ -77,7 +79,7 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
         place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
         diag_expr_id: HirId,
     ) {
-        let parent = match self.hir.find_parent_node(place_with_id.hir_id) {
+        let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) {
             Some(parent) => parent,
             None => place_with_id.hir_id,
         };
@@ -107,11 +109,22 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
         assignee_place: &expr_use_visitor::PlaceWithHirId<'tcx>,
         diag_expr_id: HirId,
     ) {
-        debug!("mutate {:?}; diag_expr_id={:?}", assignee_place, diag_expr_id);
-        // Count mutations as a borrow.
-        self.places
-            .borrowed
-            .insert(TrackedValue::from_place_with_projections_allowed(assignee_place));
+        debug!("mutate {assignee_place:?}; diag_expr_id={diag_expr_id:?}");
+        // If the type being assigned needs dropped, then the mutation counts as a borrow
+        // since it is essentially doing `Drop::drop(&mut x); x = new_value;`.
+        if assignee_place.place.base_ty.needs_drop(self.tcx, self.param_env) {
+            self.places
+                .borrowed
+                .insert(TrackedValue::from_place_with_projections_allowed(assignee_place));
+        }
+    }
+
+    fn bind(
+        &mut self,
+        binding_place: &expr_use_visitor::PlaceWithHirId<'tcx>,
+        diag_expr_id: HirId,
+    ) {
+        debug!("bind {binding_place:?}; diag_expr_id={diag_expr_id:?}");
     }
 
     fn fake_read(

diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs
@@ -51,6 +51,15 @@ pub trait Delegate<'tcx> {
     /// `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);
 
+    /// The path at `binding_place` is a binding that is being initialized.
+    ///
+    /// This covers cases such as `let x = 42;`
+    fn bind(&mut self, binding_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) {
+        // Bindings can normally be treated as a regular assignment, so by default we
+        // forward this to the mutate callback.
+        self.mutate(binding_place, diag_expr_id)
+    }
+
     /// 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);
 }
@@ -648,11 +657,9 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                     let pat_ty = return_if_err!(mc.node_ty(pat.hir_id));
                     debug!("walk_pat: pat_ty={:?}", pat_ty);
 
-                    // Each match binding is effectively an assignment to the
-                    // binding being produced.
                     let def = Res::Local(canonical_id);
                     if let Ok(ref binding_place) = mc.cat_res(pat.hir_id, pat.span, pat_ty, def) {
-                        delegate.mutate(binding_place, binding_place.hir_id);
+                        delegate.bind(binding_place, binding_place.hir_id);
                     }
 
                     // It is also a borrow or copy/move of the value being matched.

diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs
@@ -1,9 +1,5 @@
 // edition:2018
-// compile-flags: --crate-type lib
-
-// FIXME(eholk): temporarily disabled while drop range tracking is disabled
-// (see generator_interior.rs:27)
-// ignore-test
+// compile-flags: --crate-type lib -Zdrop-tracking
 
 use std::{cell::RefCell, fmt::Debug, rc::Rc};
 

diff --git a/src/test/ui/async-await/drop-and-assign.rs b/src/test/ui/async-await/drop-and-assign.rs
@@ -0,0 +1,19 @@
+// edition:2021
+// compile-flags: -Zdrop-tracking
+// build-pass
+
+struct A;
+impl Drop for A { fn drop(&mut self) {} }
+
+pub async fn f() {
+    let mut a = A;
+    a = A;
+    drop(a);
+    async {}.await;
+}
+
+fn assert_send<T: Send>(_: T) {}
+
+fn main() {
+    let _ = f();
+}
diff --git a/src/test/ui/async-await/unresolved_type_param.rs b/src/test/ui/async-await/unresolved_type_param.rs
@@ -2,10 +2,7 @@
 // Error message should pinpoint the type parameter T as needing to be bound
 // (rather than give a general error message)
 // edition:2018
-
-// FIXME(eholk): temporarily disabled while drop range tracking is disabled
-// (see generator_interior.rs:27)
-// ignore-test
+// compile-flags: -Zdrop-tracking
 
 async fn bar<T>() -> () {}
 

diff --git a/src/test/ui/async-await/unresolved_type_param.stderr b/src/test/ui/async-await/unresolved_type_param.stderr
@@ -1,35 +1,35 @@
 error[E0698]: type inside `async fn` body must be known in this context
-  --> $DIR/unresolved_type_param.rs:9:5
+  --> $DIR/unresolved_type_param.rs:10:5
    |
 LL |     bar().await;
    |     ^^^ cannot infer type for type parameter `T` declared on the function `bar`
    |
 note: the type is part of the `async fn` body because of this `await`
-  --> $DIR/unresolved_type_param.rs:9:10
+  --> $DIR/unresolved_type_param.rs:10:10
    |
 LL |     bar().await;
    |          ^^^^^^
 
 error[E0698]: type inside `async fn` body must be known in this context
-  --> $DIR/unresolved_type_param.rs:9:5
+  --> $DIR/unresolved_type_param.rs:10:5
    |
 LL |     bar().await;
    |     ^^^ cannot infer type for type parameter `T` declared on the function `bar`
    |
 note: the type is part of the `async fn` body because of this `await`
-  --> $DIR/unresolved_type_param.rs:9:10
+  --> $DIR/unresolved_type_param.rs:10:10
    |
 LL |     bar().await;
    |          ^^^^^^
 
 error[E0698]: type inside `async fn` body must be known in this context
-  --> $DIR/unresolved_type_param.rs:9:5
+  --> $DIR/unresolved_type_param.rs:10:5
    |
 LL |     bar().await;
    |     ^^^ cannot infer type for type parameter `T` declared on the function `bar`
    |
 note: the type is part of the `async fn` body because of this `await`
-  --> $DIR/unresolved_type_param.rs:9:10
+  --> $DIR/unresolved_type_param.rs:10:10
    |
 LL |     bar().await;
    |          ^^^^^^

diff --git a/src/test/ui/generator/drop-control-flow.rs b/src/test/ui/generator/drop-control-flow.rs
@@ -1,10 +1,6 @@
 // build-pass
 // compile-flags: -Zdrop-tracking
 
-// FIXME(eholk): temporarily disabled while drop range tracking is disabled
-// (see generator_interior.rs:27)
-// ignore-test
-
 // A test to ensure generators capture values that were conditionally dropped,
 // and also that values that are dropped along all paths to a yield do not get
 // included in the generator type.

diff --git a/src/test/ui/generator/issue-57478.rs b/src/test/ui/generator/issue-57478.rs
@@ -1,8 +1,5 @@
 // check-pass
-
-// FIXME(eholk): temporarily disabled while drop range tracking is disabled
-// (see generator_interior.rs:27)
-// ignore-test
+// compile-flags: -Zdrop-tracking
 
 #![feature(negative_impls, generators)]
 

diff --git a/src/test/ui/generator/partial-drop.rs b/src/test/ui/generator/partial-drop.rs
@@ -1,6 +1,4 @@
-// FIXME(eholk): temporarily disabled while drop range tracking is disabled
-// (see generator_interior.rs:27)
-// ignore-test
+// compile-flags: -Zdrop-tracking
 
 #![feature(negative_impls, generators)]
 

diff --git a/src/test/ui/generator/partial-drop.stderr b/src/test/ui/generator/partial-drop.stderr
@@ -1,12 +1,12 @@
 error: generator cannot be sent between threads safely
-  --> $DIR/partial-drop.rs:12:5
+  --> $DIR/partial-drop.rs:14:5
    |
 LL |     assert_send(|| {
    |     ^^^^^^^^^^^ generator is not `Send`
    |
-   = help: within `[generator@$DIR/partial-drop.rs:12:17: 18:6]`, the trait `Send` is not implemented for `Foo`
+   = help: within `[generator@$DIR/partial-drop.rs:14:17: 20:6]`, the trait `Send` is not implemented for `Foo`
 note: generator is not `Send` as this value is used across a yield
-  --> $DIR/partial-drop.rs:17:9
+  --> $DIR/partial-drop.rs:19:9
    |
 LL |         let guard = Bar { foo: Foo, x: 42 };
    |             ----- has type `Bar` which is not `Send`
@@ -16,20 +16,20 @@ LL |         yield;
 LL |     });
    |     - `guard` is later dropped here
 note: required by a bound in `assert_send`
-  --> $DIR/partial-drop.rs:40:19
+  --> $DIR/partial-drop.rs:42:19
    |
 LL | fn assert_send<T: Send>(_: T) {}
    |                   ^^^^ required by this bound in `assert_send`
 
 error: generator cannot be sent between threads safely
-  --> $DIR/partial-drop.rs:20:5
+  --> $DIR/partial-drop.rs:22:5
    |
 LL |     assert_send(|| {
    |     ^^^^^^^^^^^ generator is not `Send`
    |
-   = help: within `[generator@$DIR/partial-drop.rs:20:17: 28:6]`, the trait `Send` is not implemented for `Foo`
+   = help: within `[generator@$DIR/partial-drop.rs:22:17: 30:6]`, the trait `Send` is not implemented for `Foo`
 note: generator is not `Send` as this value is used across a yield
-  --> $DIR/partial-drop.rs:27:9
+  --> $DIR/partial-drop.rs:29:9
    |
 LL |         let guard = Bar { foo: Foo, x: 42 };
    |             ----- has type `Bar` which is not `Send`
@@ -39,20 +39,20 @@ LL |         yield;
 LL |     });
    |     - `guard` is later dropped here
 note: required by a bound in `assert_send`
-  --> $DIR/partial-drop.rs:40:19
+  --> $DIR/partial-drop.rs:42:19
    |
 LL | fn assert_send<T: Send>(_: T) {}
    |                   ^^^^ required by this bound in `assert_send`
 
 error: generator cannot be sent between threads safely
-  --> $DIR/partial-drop.rs:30:5
+  --> $DIR/partial-drop.rs:32:5
    |
 LL |     assert_send(|| {
    |     ^^^^^^^^^^^ generator is not `Send`
    |
-   = help: within `[generator@$DIR/partial-drop.rs:30:17: 37:6]`, the trait `Send` is not implemented for `Foo`
+   = help: within `[generator@$DIR/partial-drop.rs:32:17: 39:6]`, the trait `Send` is not implemented for `Foo`
 note: generator is not `Send` as this value is used across a yield
-  --> $DIR/partial-drop.rs:36:9
+  --> $DIR/partial-drop.rs:38:9
    |
 LL |         let guard = Bar { foo: Foo, x: 42 };
    |             ----- has type `Bar` which is not `Send`
@@ -62,7 +62,7 @@ LL |         yield;
 LL |     });
    |     - `guard` is later dropped here
 note: required by a bound in `assert_send`
-  --> $DIR/partial-drop.rs:40:19
+  --> $DIR/partial-drop.rs:42:19
    |
 LL | fn assert_send<T: Send>(_: T) {}
    |                   ^^^^ required by this bound in `assert_send`