From e0a5370ef00938db0e76f6d7845befb51be629ff Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 14 Jan 2022 17:45:00 -0800 Subject: [PATCH] Respond to code review comments --- .../check/generator_interior/drop_ranges.rs | 48 +++-- .../drop_ranges/cfg_build.rs | 167 ++++++++++++++++-- .../drop_ranges/record_consumed_borrow.rs | 12 +- src/test/ui/generator/partial-drop.rs | 21 ++- src/test/ui/generator/partial-drop.stderr | 52 +++++- 5 files changed, 265 insertions(+), 35 deletions(-) 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 681cd7cf935..21a8d7b5634 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -21,6 +21,7 @@ use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; +use rustc_middle::hir::map::Map; use rustc_middle::hir::place::{PlaceBase, PlaceWithHirId}; use rustc_middle::ty; use std::collections::BTreeMap; @@ -53,15 +54,18 @@ pub fn compute_drop_ranges<'a, 'tcx>( DropRanges { tracked_value_map: drop_ranges.tracked_value_map, nodes: drop_ranges.nodes } } -/// Applies `f` to consumable portion of a HIR node. +/// Applies `f` to consumable node in the HIR subtree pointed to by `place`. /// -/// The `node` parameter should be the result of calling `Map::find(place)`. -fn for_each_consumable( - place: TrackedValue, - node: Option>, - mut f: impl FnMut(TrackedValue), -) { +/// This includes the place itself, and if the place is a reference to a local +/// variable then `f` is also called on the HIR node for that variable as well. +/// +/// For example, if `place` points to `foo()`, then `f` is called once for the +/// result of `foo`. On the other hand, if `place` points to `x` then `f` will +/// be called both on the `ExprKind::Path` node that represents the expression +/// as well as the HirId of the local `x` itself. +fn for_each_consumable<'tcx>(hir: Map<'tcx>, place: TrackedValue, mut f: impl FnMut(TrackedValue)) { f(place); + let node = hir.find(place.hir_id()); if let Some(Node::Expr(expr)) = node { match expr.kind { hir::ExprKind::Path(hir::QPath::Resolved( @@ -108,15 +112,37 @@ impl TrackedValue { } } -impl From<&PlaceWithHirId<'_>> for TrackedValue { - fn from(place_with_id: &PlaceWithHirId<'_>) -> Self { +/// Represents a reason why we might not be able to convert a HirId or Place +/// into a tracked value. +#[derive(Debug)] +enum TrackedValueConversionError { + /// Place projects are not currently supported. + /// + /// The reasoning around these is kind of subtle, so we choose to be more + /// conservative around these for now. There is not reason in theory we + /// cannot support these, we just have not implemented it yet. + PlaceProjectionsNotSupported, +} + +impl TryFrom<&PlaceWithHirId<'_>> for TrackedValue { + type Error = TrackedValueConversionError; + + fn try_from(place_with_id: &PlaceWithHirId<'_>) -> Result { + if !place_with_id.place.projections.is_empty() { + debug!( + "TrackedValue from PlaceWithHirId: {:?} has projections, which are not supported.", + place_with_id + ); + return Err(TrackedValueConversionError::PlaceProjectionsNotSupported); + } + match place_with_id.place.base { PlaceBase::Rvalue | PlaceBase::StaticItem => { - TrackedValue::Temporary(place_with_id.hir_id) + Ok(TrackedValue::Temporary(place_with_id.hir_id)) } PlaceBase::Local(hir_id) | PlaceBase::Upvar(ty::UpvarId { var_path: ty::UpvarPath { hir_id }, .. }) => { - TrackedValue::Variable(hir_id) + Ok(TrackedValue::Variable(hir_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 dfe8ed54b21..d7305957f94 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 @@ -43,6 +43,41 @@ pub(super) fn build_control_flow_graph<'tcx>( /// We are interested in points where a variables is dropped or initialized, and the control flow /// of the code. We identify locations in code by their post-order traversal index, so it is /// important for this traversal to match that in `RegionResolutionVisitor` and `InteriorVisitor`. +/// +/// We make several simplifying assumptions, with the goal of being more conservative than +/// necessary rather than less conservative (since being less conservative is unsound, but more +/// conservative is still safe). These assumptions are: +/// +/// 1. Moving a variable `a` counts as a move of the whole variable. +/// 2. Moving a partial path like `a.b.c` is ignored. +/// 3. Reinitializing through a field (e.g. `a.b.c = 5`) counds as a reinitialization of all of +/// `a`. +/// +/// Some examples: +/// +/// Rule 1: +/// ```rust +/// let mut a = (vec![0], vec![0]); +/// drop(a); +/// // `a` is not considered initialized. +/// ``` +/// +/// Rule 2: +/// ```rust +/// let mut a = (vec![0], vec![0]); +/// drop(a.0); +/// drop(a.1); +/// // `a` is still considered initialized. +/// ``` +/// +/// Rule 3: +/// ```rust +/// let mut a = (vec![0], vec![0]); +/// drop(a); +/// a.1 = vec![1]; +/// // all of `a` is considered initialized +/// ``` + struct DropRangeVisitor<'a, 'tcx> { hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, @@ -89,23 +124,76 @@ impl<'a, 'tcx> DropRangeVisitor<'a, 'tcx> { .get(&expr.hir_id) .map_or(vec![], |places| places.iter().cloned().collect()); for place in places { - for_each_consumable(place, self.hir.find(place.hir_id()), |value| { - self.record_drop(value) - }); + for_each_consumable(self.hir, place, |value| self.record_drop(value)); } } + /// Marks an expression as being reinitialized. + /// + /// Note that we always approximated on the side of things being more + /// initialized than they actually are, as opposed to less. In cases such + /// as `x.y = ...`, we would consider all of `x` as being initialized + /// instead of just the `y` field. + /// + /// This is because it is always safe to consider something initialized + /// even when it is not, but the other way around will cause problems. + /// + /// In the future, we will hopefully tighten up these rules to be more + /// precise. fn reinit_expr(&mut self, expr: &hir::Expr<'_>) { - if let ExprKind::Path(hir::QPath::Resolved( - _, - hir::Path { res: hir::def::Res::Local(hir_id), .. }, - )) = expr.kind - { - let location = self.expr_index; - debug!("reinitializing {:?} at {:?}", hir_id, location); - self.drop_ranges.reinit_at(TrackedValue::Variable(*hir_id), location); - } else { - debug!("reinitializing {:?} is not supported", expr); + // Walk the expression to find the base. For example, in an expression + // like `*a[i].x`, we want to find the `a` and mark that as + // reinitialized. + match expr.kind { + ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(hir_id), .. }, + )) => { + // This is the base case, where we have found an actual named variable. + + let location = self.expr_index; + debug!("reinitializing {:?} at {:?}", hir_id, location); + self.drop_ranges.reinit_at(TrackedValue::Variable(*hir_id), location); + } + + ExprKind::Field(base, _) => self.reinit_expr(base), + + // Most expressions do not refer to something where we need to track + // reinitializations. + // + // Some of these may be interesting in the future + ExprKind::Path(..) + | ExprKind::Box(_) + | ExprKind::ConstBlock(_) + | ExprKind::Array(_) + | ExprKind::Call(_, _) + | ExprKind::MethodCall(_, _, _, _) + | ExprKind::Tup(_) + | ExprKind::Binary(_, _, _) + | ExprKind::Unary(_, _) + | ExprKind::Lit(_) + | ExprKind::Cast(_, _) + | ExprKind::Type(_, _) + | ExprKind::DropTemps(_) + | ExprKind::Let(_, _, _) + | ExprKind::If(_, _, _) + | ExprKind::Loop(_, _, _, _) + | ExprKind::Match(_, _, _) + | ExprKind::Closure(_, _, _, _, _) + | ExprKind::Block(_, _) + | ExprKind::Assign(_, _, _) + | ExprKind::AssignOp(_, _, _) + | ExprKind::Index(_, _) + | ExprKind::AddrOf(_, _, _) + | ExprKind::Break(_, _) + | ExprKind::Continue(_) + | ExprKind::Ret(_) + | ExprKind::InlineAsm(_) + | ExprKind::LlvmInlineAsm(_) + | ExprKind::Struct(_, _, _) + | ExprKind::Repeat(_, _) + | ExprKind::Yield(_, _) + | ExprKind::Err => (), } } @@ -158,13 +246,47 @@ impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { self.drop_ranges.add_control_edge(true_end, self.expr_index + 1); } ExprKind::Match(scrutinee, arms, ..) => { + // We walk through the match expression almost like a chain of if expressions. + // Here's a diagram to follow along with: + // + // ┌─┐ + // match │A│ { + // ┌───┴─┘ + // │ + // ┌▼┌───►┌─┐ ┌─┐ + // │B│ if │C│ =>│D│, + // └─┘ ├─┴──►└─┴──────┐ + // ┌──┘ │ + // ┌──┘ │ + // │ │ + // ┌▼┌───►┌─┐ ┌─┐ │ + // │E│ if │F│ =>│G│, │ + // └─┘ ├─┴──►└─┴┐ │ + // │ │ │ + // } ▼ ▼ │ + // ┌─┐◄───────────────────┘ + // │H│ + // └─┘ + // + // The order we want is that the scrutinee (A) flows into the first pattern (B), + // which flows into the guard (C). Then the guard either flows into the arm body + // (D) or into the start of the next arm (E). Finally, the body flows to the end + // of the match block (H). + // + // The subsequent arms follow the same ordering. First we go to the pattern, then + // the guard (if present, otherwise it flows straight into the body), then into + // the body and then to the end of the match expression. + // + // The comments below show which edge is being added. self.visit_expr(scrutinee); let (guard_exit, arm_end_ids) = arms.iter().fold( (self.expr_index, vec![]), |(incoming_edge, mut arm_end_ids), hir::Arm { pat, body, guard, .. }| { + // A -> B, or C -> E self.drop_ranges.add_control_edge(incoming_edge, self.expr_index + 1); self.visit_pat(pat); + // B -> C and E -> F are added implicitly due to the traversal order. match guard { Some(Guard::If(expr)) => self.visit_expr(expr), Some(Guard::IfLet(pat, expr)) => { @@ -173,17 +295,34 @@ impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { } None => (), } + // Likewise, C -> D and F -> G are added implicitly. + + // Save C, F, so we can add the other outgoing edge. let to_next_arm = self.expr_index; + // The default edge does not get added since we also have an explicit edge, // so we also need to add an edge to the next node as well. + // + // This adds C -> D, F -> G self.drop_ranges.add_control_edge(self.expr_index, self.expr_index + 1); self.visit_expr(body); + + // Save the end of the body so we can add the exit edge once we know where + // the exit is. arm_end_ids.push(self.expr_index); + + // Pass C to the next iteration, as well as vec![D] + // + // On the last round through, we pass F and vec![D, G] so that we can + // add all the exit edges. (to_next_arm, arm_end_ids) }, ); + // F -> H self.drop_ranges.add_control_edge(guard_exit, self.expr_index + 1); + arm_end_ids.into_iter().for_each(|arm_end| { + // D -> H, G -> H self.drop_ranges.add_control_edge(arm_end, self.expr_index + 1) }); } @@ -275,7 +414,7 @@ impl DropRangesBuilder { let mut tracked_value_map = FxHashMap::<_, TrackedValueIndex>::default(); let mut next = <_>::from(0u32); for value in tracked_values { - for_each_consumable(value, hir.find(value.hir_id()), |value| { + for_each_consumable(hir, value, |value| { if !tracked_value_map.contains_key(&value) { tracked_value_map.insert(value, next); next = next + 1; 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 845cd01a44e..059a135a6fb 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 @@ -85,11 +85,9 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { "consume {:?}; diag_expr_id={:?}, using parent {:?}", place_with_id, diag_expr_id, parent ); - // We do not currently support partial drops or reinits, so just ignore - // any places with projections. - if place_with_id.place.projections.is_empty() { - self.mark_consumed(parent, place_with_id.into()); - } + place_with_id + .try_into() + .map_or((), |tracked_value| self.mark_consumed(parent, tracked_value)); } fn borrow( @@ -98,7 +96,9 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { _diag_expr_id: HirId, _bk: rustc_middle::ty::BorrowKind, ) { - self.places.borrowed.insert(place_with_id.into()); + place_with_id + .try_into() + .map_or(false, |tracked_value| self.places.borrowed.insert(tracked_value)); } fn mutate( diff --git a/src/test/ui/generator/partial-drop.rs b/src/test/ui/generator/partial-drop.rs index c8c07ba41c7..36f6e78cb3b 100644 --- a/src/test/ui/generator/partial-drop.rs +++ b/src/test/ui/generator/partial-drop.rs @@ -15,7 +15,26 @@ fn main() { let guard = Bar { foo: Foo, x: 42 }; drop(guard.foo); yield; - }) + }); + + assert_send(|| { + //~^ ERROR generator cannot be sent between threads safely + // FIXME: it would be nice to make this work. + let guard = Bar { foo: Foo, x: 42 }; + drop(guard); + guard.foo = Foo; + guard.x = 23; + yield; + }); + + assert_send(|| { + //~^ ERROR generator cannot be sent between threads safely + // FIXME: it would be nice to make this work. + let guard = Bar { foo: Foo, x: 42 }; + let Bar { foo, x } = guard; + drop(foo); + yield; + }); } fn assert_send(_: T) {} diff --git a/src/test/ui/generator/partial-drop.stderr b/src/test/ui/generator/partial-drop.stderr index 93112f52208..9a1b0734d8c 100644 --- a/src/test/ui/generator/partial-drop.stderr +++ b/src/test/ui/generator/partial-drop.stderr @@ -13,13 +13,59 @@ LL | let guard = Bar { foo: Foo, x: 42 }; LL | drop(guard.foo); LL | yield; | ^^^^^ yield occurs here, with `guard` maybe used later -LL | }) +LL | }); | - `guard` is later dropped here note: required by a bound in `assert_send` - --> $DIR/partial-drop.rs:21:19 + --> $DIR/partial-drop.rs:40:19 | LL | fn assert_send(_: T) {} | ^^^^ required by this bound in `assert_send` -error: aborting due to previous error +error: generator cannot be sent between threads safely + --> $DIR/partial-drop.rs:20: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` +note: generator is not `Send` as this value is used across a yield + --> $DIR/partial-drop.rs:27:9 + | +LL | let guard = Bar { foo: Foo, x: 42 }; + | ----- has type `Bar` which is not `Send` +... +LL | yield; + | ^^^^^ yield occurs here, with `guard` maybe used later +LL | }); + | - `guard` is later dropped here +note: required by a bound in `assert_send` + --> $DIR/partial-drop.rs:40: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 + | +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` +note: generator is not `Send` as this value is used across a yield + --> $DIR/partial-drop.rs:36:9 + | +LL | let guard = Bar { foo: Foo, x: 42 }; + | ----- has type `Bar` which is not `Send` +... +LL | yield; + | ^^^^^ yield occurs here, with `guard` maybe used later +LL | }); + | - `guard` is later dropped here +note: required by a bound in `assert_send` + --> $DIR/partial-drop.rs:40:19 + | +LL | fn assert_send(_: T) {} + | ^^^^ required by this bound in `assert_send` + +error: aborting due to 3 previous errors