From add169d4140c7583b876619b2e19ba76f4aa76e4 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 4 Mar 2022 11:03:24 -0800 Subject: [PATCH] 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 f2907bb2634..40ee6d863b5 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 db1c80ae433..8c19bbd3214 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 00000000000..fa3f3303677 --- /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(); +}