From 3f73491589d99ad60d785add0309637423fe7368 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 28 Feb 2022 12:54:07 -0800 Subject: [PATCH 1/4] Only count mutations with projections as borrows Because bindings also count as a mutation, the previous behavior counted all variables as borrowed, completely negating the effect of drop tracking. --- .../drop_ranges/record_consumed_borrow.rs | 18 +++++++++++++----- compiler/rustc_typeck/src/expr_use_visitor.rs | 6 +++++- 2 files changed, 18 insertions(+), 6 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 index 03d3b23bb23d..f2907bb2634a 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 @@ -107,11 +107,19 @@ 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:?}"); + // Count mutations as a borrow when done through a projection. + // + // The goal here is to catch cases such as `x.y = 42`, since MIR will count this + // as a borrow of `x`, and we need to match that behavior. + // + // FIXME(eholk): this is probably still more conservative than we need to be. For example, + // we may need to count `*x = 42` as a borrow of `x`, since it overwrites all of `x`. + if !assignee_place.place.projections.is_empty() { + self.places + .borrowed + .insert(TrackedValue::from_place_with_projections_allowed(assignee_place)); + } } fn fake_read( diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index db1c80ae433b..9f4502709418 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -49,7 +49,11 @@ pub trait Delegate<'tcx> { /// 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); + fn mutate( + &mut self, + assignee_place: &PlaceWithHirId<'tcx>, + diag_expr_id: hir::HirId + ); /// 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); From 09aa09f1f75a3fc1b71c65201a7c228c9f4e4225 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 28 Feb 2022 12:55:50 -0800 Subject: [PATCH 2/4] Enable drop-tracking tests behind -Zdrop-tracking These were still disabled from the soft revert of drop tracking, which meant we were not catching regressions that were introduced while trying to fix drop tracking. --- src/test/ui/async-await/async-fn-nonsend.rs | 6 +---- .../ui/async-await/unresolved_type_param.rs | 5 +--- .../async-await/unresolved_type_param.stderr | 12 +++++----- src/test/ui/generator/drop-control-flow.rs | 4 ---- src/test/ui/generator/issue-57478.rs | 5 +--- src/test/ui/generator/partial-drop.rs | 4 +--- src/test/ui/generator/partial-drop.stderr | 24 +++++++++---------- 7 files changed, 22 insertions(+), 38 deletions(-) diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index a1a05c0acba4..d7f8d7ac546c 100644 --- 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/unresolved_type_param.rs b/src/test/ui/async-await/unresolved_type_param.rs index 187356ca1402..6d6d80614910 100644 --- 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() -> () {} diff --git a/src/test/ui/async-await/unresolved_type_param.stderr b/src/test/ui/async-await/unresolved_type_param.stderr index d19a3226ef9a..7236c681f341 100644 --- 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 index 914a7d71dc42..d383680002f4 100644 --- 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 index 5c23ecbae327..91407ea1844f 100644 --- 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 index e89e4b61bbff..c872fb7f3e63 100644 --- 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 index 9a1b0734d8c8..16b34c917ece 100644 --- 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) {} | ^^^^ 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) {} | ^^^^ 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) {} | ^^^^ required by this bound in `assert_send` From 1c12c92286375dd91e283a6b0cbfc32f67d045b3 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 28 Feb 2022 13:03:13 -0800 Subject: [PATCH 3/4] Fix formatting --- compiler/rustc_typeck/src/expr_use_visitor.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index 9f4502709418..db1c80ae433b 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -49,11 +49,7 @@ pub trait Delegate<'tcx> { /// 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 - ); + fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId); /// 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); From add169d4140c7583b876619b2e19ba76f4aa76e4 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 4 Mar 2022 11:03:24 -0800 Subject: [PATCH 4/4] Distinguish binding assignments, use Ty::needs_drop This better captures the actual behavior, rather than using hacks around whether the assignment has any projections. --- .../drop_ranges/record_consumed_borrow.rs | 33 +++++++++++-------- compiler/rustc_typeck/src/expr_use_visitor.rs | 13 ++++++-- src/test/ui/async-await/drop-and-assign.rs | 19 +++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 src/test/ui/async-await/drop-and-assign.rs 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 f2907bb2634a..40ee6d863b5a 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,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, }; @@ -108,20 +110,23 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { diag_expr_id: HirId, ) { debug!("mutate {assignee_place:?}; diag_expr_id={diag_expr_id:?}"); - // Count mutations as a borrow when done through a projection. - // - // The goal here is to catch cases such as `x.y = 42`, since MIR will count this - // as a borrow of `x`, and we need to match that behavior. - // - // FIXME(eholk): this is probably still more conservative than we need to be. For example, - // we may need to count `*x = 42` as a borrow of `x`, since it overwrites all of `x`. - if !assignee_place.place.projections.is_empty() { + // 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( &mut self, _place: expr_use_visitor::Place<'tcx>, diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index db1c80ae433b..8c19bbd3214e 100644 --- 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/drop-and-assign.rs b/src/test/ui/async-await/drop-and-assign.rs new file mode 100644 index 000000000000..fa3f3303677d --- /dev/null +++ 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) {} + +fn main() { + let _ = f(); +}