From 60f5cad6ebaa683ac58132b7bb64002a90deb343 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 18 Feb 2022 17:06:48 -0500 Subject: [PATCH 1/8] try to fix issue 57017, but not quite there yet Co-authored-by: Eric Holk --- .../src/check/generator_interior.rs | 14 +++++++++++-- .../check/generator_interior/drop_ranges.rs | 21 ++++++++++++++++--- .../drop_ranges/cfg_build.rs | 6 +++--- .../drop_ranges/record_consumed_borrow.rs | 17 ++++++++++++++- foo.rs | 21 +++++++++++++++++++ 5 files changed, 70 insertions(+), 9 deletions(-) create mode 100644 foo.rs diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 9684237be8313..30469e2ec7bbd 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/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,17 @@ 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)); + + 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. diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index 972dd622d6e98..4fa7ed82c6a84 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/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, nodes: IndexVec, + borrowed_temporaries: Option>, } 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] diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index cfed784ea728b..f4dd4cc010d3c 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/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) { 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 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 40ee6d863b5a7..a66f4b4558e14 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,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; 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>, + /// A set of hir-ids of values or variables that are borrowed at some point within the body. pub(super) borrowed: FxHashSet, + + /// A set of hir-ids of values or variables that are borrowed at some point within the body. + pub(super) borrowed_temporaries: FxHashSet, } /// 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(), }, } } @@ -98,10 +104,19 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { diag_expr_id: HirId, _bk: rustc_middle::ty::BorrowKind, ) { - debug!("borrow {:?}; diag_expr_id={:?}", place_with_id, diag_expr_id); + debug!("borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}"); + self.places .borrowed .insert(TrackedValue::from_place_with_projections_allowed(place_with_id)); + + // XXX -- we need to distinguish "consuming a copy" from other borrows + // + // XXX -- we need to distinguish `&*E` where `E: &T` which is not creating a temporary + // even though the place-base E is an rvalue + if let PlaceBase::Rvalue = place_with_id.place.base { + self.places.borrowed_temporaries.insert(place_with_id.hir_id); + } } fn mutate( diff --git a/foo.rs b/foo.rs new file mode 100644 index 0000000000000..e91ca1a0dd66a --- /dev/null +++ b/foo.rs @@ -0,0 +1,21 @@ +// check-pass +#![feature(generators, negative_impls)] + +struct Client; + +impl !Sync for Client {} + +fn status(_client_status: &Client) -> i16 { + 200 +} + +fn assert_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); +} From 513a9c67a59c0e1b9a7903d7e5fca4b6da974673 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 22 Feb 2022 16:02:33 -0800 Subject: [PATCH 2/8] Move test to right place --- foo.rs => src/test/ui/generator/issue-57017.rs | 1 + 1 file changed, 1 insertion(+) rename foo.rs => src/test/ui/generator/issue-57017.rs (92%) diff --git a/foo.rs b/src/test/ui/generator/issue-57017.rs similarity index 92% rename from foo.rs rename to src/test/ui/generator/issue-57017.rs index e91ca1a0dd66a..1223a3037abc7 100644 --- a/foo.rs +++ b/src/test/ui/generator/issue-57017.rs @@ -1,4 +1,5 @@ // check-pass +// compile-flags: -Zdrop-tracking #![feature(generators, negative_impls)] struct Client; From ac804f27a8ce2e02fd3a9c5eff83098258b78da6 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 22 Feb 2022 16:10:17 -0800 Subject: [PATCH 3/8] Trying to detect autorefs to avoid unnecessary borrowed temporaries This is all almost certainly wrong --- .../drop_ranges/record_consumed_borrow.rs | 10 +++++++--- compiler/rustc_typeck/src/check/upvar.rs | 3 ++- compiler/rustc_typeck/src/expr_use_visitor.rs | 13 ++++++++----- 3 files changed, 17 insertions(+), 9 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 a66f4b4558e14..3ff7d7ad010ff 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 @@ -102,9 +102,13 @@ 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, + is_autoref: bool, ) { - debug!("borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}"); + debug!( + "borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}, \ + borrow_kind={bk:?}, is_autoref={is_autoref}" + ); self.places .borrowed @@ -114,7 +118,7 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { // // XXX -- we need to distinguish `&*E` where `E: &T` which is not creating a temporary // even though the place-base E is an rvalue - if let PlaceBase::Rvalue = place_with_id.place.base { + if let (false, PlaceBase::Rvalue) = (is_autoref, place_with_id.place.base) { self.places.borrowed_temporaries.insert(place_with_id.hir_id); } } diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index 257846324b888..a3317f1cd7c7c 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/compiler/rustc_typeck/src/check/upvar.rs @@ -1792,6 +1792,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> { place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId, bk: ty::BorrowKind, + _is_autoref: bool, ) { let PlaceBase::Upvar(upvar_id) = place_with_id.place.base else { return }; assert_eq!(self.closure_def_id, upvar_id.closure_expr_id); @@ -1826,7 +1827,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> { #[instrument(skip(self), level = "debug")] fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) { - self.borrow(assignee_place, diag_expr_id, ty::BorrowKind::MutBorrow); + self.borrow(assignee_place, diag_expr_id, ty::BorrowKind::MutBorrow, false); } } diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index 8c19bbd3214ee..1cce2a1da744d 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -45,6 +45,7 @@ pub trait Delegate<'tcx> { place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId, bk: ty::BorrowKind, + is_autoref: bool, ); /// The path at `assignee_place` is being assigned to. @@ -175,7 +176,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { debug!("borrow_expr(expr={:?}, bk={:?})", expr, bk); let place_with_id = return_if_err!(self.mc.cat_expr(expr)); - self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk); + self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk, false); self.walk_expr(expr) } @@ -558,7 +559,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { // this is an autoref of `x`. adjustment::Adjust::Deref(Some(ref deref)) => { let bk = ty::BorrowKind::from_mutbl(deref.mutbl); - self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk); + self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk, true); } adjustment::Adjust::Borrow(ref autoref) => { @@ -590,13 +591,14 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { base_place, base_place.hir_id, ty::BorrowKind::from_mutbl(m.into()), + true, ); } adjustment::AutoBorrow::RawPtr(m) => { debug!("walk_autoref: expr.hir_id={} base_place={:?}", expr.hir_id, base_place); - self.delegate.borrow(base_place, base_place.hir_id, ty::BorrowKind::from_mutbl(m)); + self.delegate.borrow(base_place, base_place.hir_id, ty::BorrowKind::from_mutbl(m), true); } } } @@ -669,7 +671,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { match bm { ty::BindByReference(m) => { let bk = ty::BorrowKind::from_mutbl(m); - delegate.borrow(place, discr_place.hir_id, bk); + delegate.borrow(place, discr_place.hir_id, bk, false); } ty::BindByValue(..) => { debug!("walk_pat binding consuming pat"); @@ -799,6 +801,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { &place_with_id, place_with_id.hir_id, upvar_borrow, + false, ); } } @@ -837,7 +840,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) + delegate.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow, false) } } } From 87ad6683d6cc3c1e2597a3a37e03fe4330414fd3 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 23 Feb 2022 15:46:09 -0800 Subject: [PATCH 4/8] Distinguish borrows of copies from other borrows --- .../drop_ranges/record_consumed_borrow.rs | 22 ++++++++++++++++--- compiler/rustc_typeck/src/expr_use_visitor.rs | 19 ++++++++++++---- 2 files changed, 34 insertions(+), 7 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 3ff7d7ad010ff..b07e3a18e3fde 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 @@ -114,15 +114,31 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { .borrowed .insert(TrackedValue::from_place_with_projections_allowed(place_with_id)); - // XXX -- we need to distinguish "consuming a copy" from other borrows + // Keep track of whether this is a borrowed temporary (i.e. a borrow of an RValue) + // so that later in generator_interior we can use the correct scope. // - // XXX -- we need to distinguish `&*E` where `E: &T` which is not creating a temporary - // even though the place-base E is an rvalue + // We ignore borrows that are the result of an autoref because these will be + // immediately consumed and should not extend the temporary's lifetime. if let (false, PlaceBase::Rvalue) = (is_autoref, 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!("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( &mut self, assignee_place: &expr_use_visitor::PlaceWithHirId<'tcx>, diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index 1cce2a1da744d..b15f7cf2889f1 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -48,6 +48,14 @@ pub trait Delegate<'tcx> { is_autoref: bool, ); + /// 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, treating a copy as a borrow is the right thing, so we forward + // this to the borrow callback by default. + self.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow, false) + } + /// 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); @@ -598,7 +606,12 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { adjustment::AutoBorrow::RawPtr(m) => { debug!("walk_autoref: expr.hir_id={} base_place={:?}", expr.hir_id, base_place); - self.delegate.borrow(base_place, base_place.hir_id, ty::BorrowKind::from_mutbl(m), true); + self.delegate.borrow( + base_place, + base_place.hir_id, + ty::BorrowKind::from_mutbl(m), + true, + ); } } } @@ -839,9 +852,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, false) - } + ConsumeMode::Copy => delegate.copy(place_with_id, diag_expr_id), } } From 9f0f46fa4da35bea07c3bedd7bd9e6742d375a27 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 25 Feb 2022 17:13:53 -0800 Subject: [PATCH 5/8] Update clippy to new ExprUseVisitor delegate --- src/tools/clippy/clippy_lints/src/escape.rs | 2 +- src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs | 2 +- src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs | 2 +- src/tools/clippy/clippy_utils/src/sugg.rs | 2 +- src/tools/clippy/clippy_utils/src/usage.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/escape.rs b/src/tools/clippy/clippy_lints/src/escape.rs index af591dd71aa1d..5974b67eb7285 100644 --- a/src/tools/clippy/clippy_lints/src/escape.rs +++ b/src/tools/clippy/clippy_lints/src/escape.rs @@ -154,7 +154,7 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> { } } - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind, _is_autoref: bool) { if cmt.place.projections.is_empty() { if let PlaceBase::Local(lid) = cmt.place.base { self.set.remove(&lid); diff --git a/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs b/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs index 9d8679d77c6d0..4b3d7c1ef247a 100644 --- a/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs +++ b/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs @@ -90,7 +90,7 @@ struct MutatePairDelegate<'a, 'tcx> { impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> { fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind, _is_autoref: bool) { if bk == ty::BorrowKind::MutBorrow { if let PlaceBase::Local(id) = cmt.place.base { if Some(id) == self.hir_id_low && !BreakAfterExprVisitor::is_found(self.cx, diag_expr_id) { diff --git a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs index ebfd908a6fb74..ccd355f480a62 100644 --- a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs +++ b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs @@ -332,7 +332,7 @@ impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt { self.move_common(cmt); } - fn borrow(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {} + fn borrow(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind, _is_autoref: bool) {} fn mutate(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId) {} diff --git a/src/tools/clippy/clippy_utils/src/sugg.rs b/src/tools/clippy/clippy_utils/src/sugg.rs index 63c442e70085a..0e97d837ec59e 100644 --- a/src/tools/clippy/clippy_utils/src/sugg.rs +++ b/src/tools/clippy/clippy_utils/src/sugg.rs @@ -886,7 +886,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} #[allow(clippy::too_many_lines)] - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind, _is_autoref: bool) { if let PlaceBase::Local(id) = cmt.place.base { let map = self.cx.tcx.hir(); let span = map.span(cmt.hir_id); diff --git a/src/tools/clippy/clippy_utils/src/usage.rs b/src/tools/clippy/clippy_utils/src/usage.rs index 405e306359bc9..ae4ca77e48c37 100644 --- a/src/tools/clippy/clippy_utils/src/usage.rs +++ b/src/tools/clippy/clippy_utils/src/usage.rs @@ -64,7 +64,7 @@ impl<'tcx> MutVarsDelegate { impl<'tcx> Delegate<'tcx> for MutVarsDelegate { fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, bk: ty::BorrowKind) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, bk: ty::BorrowKind, _is_autoref: bool) { if bk == ty::BorrowKind::MutBorrow { self.update(cmt); } From 170b02702277229ccaae3ffed916bf6dc57548fc Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 1 Mar 2022 16:57:23 -0800 Subject: [PATCH 6/8] Add comments based on code review feedback --- compiler/rustc_typeck/src/check/generator_interior.rs | 8 ++++++++ compiler/rustc_typeck/src/expr_use_visitor.rs | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior.rs b/compiler/rustc_typeck/src/check/generator_interior.rs index 30469e2ec7bbd..74e98f81439d0 100644 --- a/compiler/rustc_typeck/src/check/generator_interior.rs +++ b/compiler/rustc_typeck/src/check/generator_interior.rs @@ -371,6 +371,14 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { 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 { diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index b15f7cf2889f1..e839822602dd6 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -51,8 +51,8 @@ pub trait Delegate<'tcx> { /// 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, treating a copy as a borrow is the right thing, so we forward - // this to the borrow callback by default. + // 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, false) } From 12d8ca113ca0c9b0d8e1ca43ea9bbd83c212efc5 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 10 Mar 2022 17:14:50 -0800 Subject: [PATCH 7/8] Use projections rather than is_autoref Also includes a lengthy comment arguing the correctness. Co-authored-by: Niko Matsakis --- .../drop_ranges/record_consumed_borrow.rs | 48 ++++++++++++++++--- 1 file changed, 42 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 b07e3a18e3fde..45421a57082d8 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,7 +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; +use rustc_middle::hir::place::{PlaceBase, Projection, ProjectionKind}; use rustc_middle::ty::{ParamEnv, TyCtxt}; pub(super) fn find_consumed_and_borrowed<'a, 'tcx>( @@ -114,12 +114,48 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { .borrowed .insert(TrackedValue::from_place_with_projections_allowed(place_with_id)); - // Keep track of whether this is a borrowed temporary (i.e. a borrow of an RValue) - // so that later in generator_interior we can use the correct scope. + // 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 ignore borrows that are the result of an autoref because these will be - // immediately consumed and should not extend the temporary's lifetime. - if let (false, PlaceBase::Rvalue) = (is_autoref, place_with_id.place.base) { + // 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(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); } } From 2fcd5427345154661f8d512745bdedaf072a55b6 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 10 Mar 2022 17:24:08 -0800 Subject: [PATCH 8/8] Remove is_autoref parameter --- .../drop_ranges/record_consumed_borrow.rs | 3 +-- compiler/rustc_typeck/src/check/upvar.rs | 3 +-- compiler/rustc_typeck/src/expr_use_visitor.rs | 18 +++++------------- src/tools/clippy/clippy_lints/src/escape.rs | 2 +- .../clippy_lints/src/loops/mut_range_bound.rs | 2 +- .../clippy_lints/src/needless_pass_by_value.rs | 2 +- src/tools/clippy/clippy_utils/src/sugg.rs | 2 +- src/tools/clippy/clippy_utils/src/usage.rs | 2 +- 8 files changed, 12 insertions(+), 22 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 45421a57082d8..928daba0a7b39 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 @@ -103,11 +103,10 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: rustc_middle::ty::BorrowKind, - is_autoref: bool, ) { debug!( "borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}, \ - borrow_kind={bk:?}, is_autoref={is_autoref}" + borrow_kind={bk:?}" ); self.places diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index a3317f1cd7c7c..257846324b888 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/compiler/rustc_typeck/src/check/upvar.rs @@ -1792,7 +1792,6 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> { place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId, bk: ty::BorrowKind, - _is_autoref: bool, ) { let PlaceBase::Upvar(upvar_id) = place_with_id.place.base else { return }; assert_eq!(self.closure_def_id, upvar_id.closure_expr_id); @@ -1827,7 +1826,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> { #[instrument(skip(self), level = "debug")] fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) { - self.borrow(assignee_place, diag_expr_id, ty::BorrowKind::MutBorrow, false); + self.borrow(assignee_place, diag_expr_id, ty::BorrowKind::MutBorrow); } } diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index e839822602dd6..4313a75aee89d 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -45,7 +45,6 @@ pub trait Delegate<'tcx> { place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId, bk: ty::BorrowKind, - is_autoref: bool, ); /// The value found at `place` is being copied. @@ -53,7 +52,7 @@ pub trait Delegate<'tcx> { 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, false) + self.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow) } /// The path at `assignee_place` is being assigned to. @@ -184,7 +183,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { debug!("borrow_expr(expr={:?}, bk={:?})", expr, bk); let place_with_id = return_if_err!(self.mc.cat_expr(expr)); - self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk, false); + self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk); self.walk_expr(expr) } @@ -567,7 +566,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { // this is an autoref of `x`. adjustment::Adjust::Deref(Some(ref deref)) => { let bk = ty::BorrowKind::from_mutbl(deref.mutbl); - self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk, true); + self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk); } adjustment::Adjust::Borrow(ref autoref) => { @@ -599,19 +598,13 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { base_place, base_place.hir_id, ty::BorrowKind::from_mutbl(m.into()), - true, ); } adjustment::AutoBorrow::RawPtr(m) => { debug!("walk_autoref: expr.hir_id={} base_place={:?}", expr.hir_id, base_place); - self.delegate.borrow( - base_place, - base_place.hir_id, - ty::BorrowKind::from_mutbl(m), - true, - ); + self.delegate.borrow(base_place, base_place.hir_id, ty::BorrowKind::from_mutbl(m)); } } } @@ -684,7 +677,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { match bm { ty::BindByReference(m) => { let bk = ty::BorrowKind::from_mutbl(m); - delegate.borrow(place, discr_place.hir_id, bk, false); + delegate.borrow(place, discr_place.hir_id, bk); } ty::BindByValue(..) => { debug!("walk_pat binding consuming pat"); @@ -814,7 +807,6 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { &place_with_id, place_with_id.hir_id, upvar_borrow, - false, ); } } diff --git a/src/tools/clippy/clippy_lints/src/escape.rs b/src/tools/clippy/clippy_lints/src/escape.rs index 5974b67eb7285..af591dd71aa1d 100644 --- a/src/tools/clippy/clippy_lints/src/escape.rs +++ b/src/tools/clippy/clippy_lints/src/escape.rs @@ -154,7 +154,7 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> { } } - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind, _is_autoref: bool) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { if cmt.place.projections.is_empty() { if let PlaceBase::Local(lid) = cmt.place.base { self.set.remove(&lid); diff --git a/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs b/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs index 4b3d7c1ef247a..9d8679d77c6d0 100644 --- a/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs +++ b/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs @@ -90,7 +90,7 @@ struct MutatePairDelegate<'a, 'tcx> { impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> { fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind, _is_autoref: bool) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) { if bk == ty::BorrowKind::MutBorrow { if let PlaceBase::Local(id) = cmt.place.base { if Some(id) == self.hir_id_low && !BreakAfterExprVisitor::is_found(self.cx, diag_expr_id) { diff --git a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs index ccd355f480a62..ebfd908a6fb74 100644 --- a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs +++ b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs @@ -332,7 +332,7 @@ impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt { self.move_common(cmt); } - fn borrow(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind, _is_autoref: bool) {} + fn borrow(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {} fn mutate(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId) {} diff --git a/src/tools/clippy/clippy_utils/src/sugg.rs b/src/tools/clippy/clippy_utils/src/sugg.rs index 0e97d837ec59e..63c442e70085a 100644 --- a/src/tools/clippy/clippy_utils/src/sugg.rs +++ b/src/tools/clippy/clippy_utils/src/sugg.rs @@ -886,7 +886,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} #[allow(clippy::too_many_lines)] - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind, _is_autoref: bool) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { if let PlaceBase::Local(id) = cmt.place.base { let map = self.cx.tcx.hir(); let span = map.span(cmt.hir_id); diff --git a/src/tools/clippy/clippy_utils/src/usage.rs b/src/tools/clippy/clippy_utils/src/usage.rs index ae4ca77e48c37..405e306359bc9 100644 --- a/src/tools/clippy/clippy_utils/src/usage.rs +++ b/src/tools/clippy/clippy_utils/src/usage.rs @@ -64,7 +64,7 @@ impl<'tcx> MutVarsDelegate { impl<'tcx> Delegate<'tcx> for MutVarsDelegate { fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, bk: ty::BorrowKind, _is_autoref: bool) { + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, bk: ty::BorrowKind) { if bk == ty::BorrowKind::MutBorrow { self.update(cmt); }