From db635fc5664c0a19cdb4063545ca2d2b297f0212 Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 17 Dec 2018 13:11:33 +0100 Subject: [PATCH] Kill borrows from a projection after assignment. This commit extends previous work to kill borrows from a local after assignment into that local to kill borrows from a projection after assignment into a prefix of that place. --- src/librustc_mir/borrow_check/mod.rs | 5 +- src/librustc_mir/borrow_check/path_utils.rs | 1 + .../borrow_check/places_conflict.rs | 69 ++++++++++++++--- src/librustc_mir/dataflow/impls/borrows.rs | 77 +++++++++++++------ src/test/ui/nll/issue-46589.rs | 1 - src/test/ui/nll/issue-46589.stderr | 17 +--- src/test/ui/nll/loan_ends_mid_block_pair.rs | 2 - .../ui/nll/loan_ends_mid_block_pair.stderr | 48 +++--------- 8 files changed, 132 insertions(+), 88 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 76ba6ae5de6e..c328ac49f40d 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -63,7 +63,7 @@ mod move_errors; mod mutability_errors; mod path_utils; crate mod place_ext; -mod places_conflict; +crate mod places_conflict; mod prefixes; mod used_muts; @@ -1370,7 +1370,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { place, borrow.kind, root_place, - sd + sd, + places_conflict::PlaceConflictBias::Overlap, ) { debug!("check_for_invalidation_at_exit({:?}): INVALID", place); // FIXME: should be talking about the region lifetime instead diff --git a/src/librustc_mir/borrow_check/path_utils.rs b/src/librustc_mir/borrow_check/path_utils.rs index 9250c04969f9..5e17afc3d3cd 100644 --- a/src/librustc_mir/borrow_check/path_utils.rs +++ b/src/librustc_mir/borrow_check/path_utils.rs @@ -68,6 +68,7 @@ pub(super) fn each_borrow_involving_path<'a, 'tcx, 'gcx: 'tcx, F, I, S> ( borrowed.kind, place, access, + places_conflict::PlaceConflictBias::Overlap, ) { debug!( "each_borrow_involving_path: {:?} @ {:?} vs. {:?}/{:?}", diff --git a/src/librustc_mir/borrow_check/places_conflict.rs b/src/librustc_mir/borrow_check/places_conflict.rs index 715d6e0c0d1b..d6e73419db2e 100644 --- a/src/librustc_mir/borrow_check/places_conflict.rs +++ b/src/librustc_mir/borrow_check/places_conflict.rs @@ -17,6 +17,43 @@ use rustc::mir::{Projection, ProjectionElem}; use rustc::ty::{self, TyCtxt}; use std::cmp::max; +/// When checking if a place conflicts with another place, this enum is used to influence decisions +/// where a place might be equal or disjoint with another place, such as if `a[i] == a[j]`. +/// `PlaceConflictBias::Overlap` would bias toward assuming that `i` might equal `j` and that these +/// places overlap. `PlaceConflictBias::NoOverlap` assumes that for the purposes of the predicate +/// being run in the calling context, the conservative choice is to assume the compared indices +/// are disjoint (and therefore, do not overlap). +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +crate enum PlaceConflictBias { + Overlap, + NoOverlap, +} + +/// Helper function for checking if places conflict with a mutable borrow and deep access depth. +/// This is used to check for places conflicting outside of the borrow checking code (such as in +/// dataflow). +crate fn places_conflict<'gcx, 'tcx>( + tcx: TyCtxt<'_, 'gcx, 'tcx>, + mir: &Mir<'tcx>, + borrow_place: &Place<'tcx>, + access_place: &Place<'tcx>, + bias: PlaceConflictBias, +) -> bool { + borrow_conflicts_with_place( + tcx, + mir, + borrow_place, + BorrowKind::Mut { allow_two_phase_borrow: true }, + access_place, + AccessDepth::Deep, + bias, + ) +} + +/// Checks whether the `borrow_place` conflicts with the `access_place` given a borrow kind and +/// access depth. The `bias` parameter is used to determine how the unknowable (comparing runtime +/// array indices, for example) should be interpreted - this depends on what the caller wants in +/// order to make the conservative choice and preserve soundness. pub(super) fn borrow_conflicts_with_place<'gcx, 'tcx>( tcx: TyCtxt<'_, 'gcx, 'tcx>, mir: &Mir<'tcx>, @@ -24,10 +61,11 @@ pub(super) fn borrow_conflicts_with_place<'gcx, 'tcx>( borrow_kind: BorrowKind, access_place: &Place<'tcx>, access: AccessDepth, + bias: PlaceConflictBias, ) -> bool { debug!( - "borrow_conflicts_with_place({:?},{:?},{:?})", - borrow_place, access_place, access + "borrow_conflicts_with_place({:?}, {:?}, {:?}, {:?})", + borrow_place, access_place, access, bias, ); // This Local/Local case is handled by the more general code below, but @@ -46,7 +84,8 @@ pub(super) fn borrow_conflicts_with_place<'gcx, 'tcx>( borrow_components, borrow_kind, access_components, - access + access, + bias, ) }) }) @@ -59,6 +98,7 @@ fn place_components_conflict<'gcx, 'tcx>( borrow_kind: BorrowKind, mut access_components: PlaceComponentsIter<'_, 'tcx>, access: AccessDepth, + bias: PlaceConflictBias, ) -> bool { // The borrowck rules for proving disjointness are applied from the "root" of the // borrow forwards, iterating over "similar" projections in lockstep until @@ -121,7 +161,7 @@ fn place_components_conflict<'gcx, 'tcx>( // check whether the components being borrowed vs // accessed are disjoint (as in the second example, // but not the first). - match place_element_conflict(tcx, mir, borrow_c, access_c) { + match place_element_conflict(tcx, mir, borrow_c, access_c, bias) { Overlap::Arbitrary => { // We have encountered different fields of potentially // the same union - the borrow now partially overlaps. @@ -193,7 +233,7 @@ fn place_components_conflict<'gcx, 'tcx>( bug!("Tracking borrow behind shared reference."); } (ProjectionElem::Deref, ty::Ref(_, _, hir::MutMutable), AccessDepth::Drop) => { - // Values behind a mutatble reference are not access either by Dropping a + // Values behind a mutable reference are not access either by dropping a // value, or by StorageDead debug!("borrow_conflicts_with_place: drop access behind ptr"); return false; @@ -331,6 +371,7 @@ fn place_element_conflict<'a, 'gcx: 'tcx, 'tcx>( mir: &Mir<'tcx>, elem1: &Place<'tcx>, elem2: &Place<'tcx>, + bias: PlaceConflictBias, ) -> Overlap { match (elem1, elem2) { (Place::Local(l1), Place::Local(l2)) => { @@ -448,10 +489,20 @@ fn place_element_conflict<'a, 'gcx: 'tcx, 'tcx>( | (ProjectionElem::ConstantIndex { .. }, ProjectionElem::Index(..)) | (ProjectionElem::Subslice { .. }, ProjectionElem::Index(..)) => { // Array indexes (`a[0]` vs. `a[i]`). These can either be disjoint - // (if the indexes differ) or equal (if they are the same), so this - // is the recursive case that gives "equal *or* disjoint" its meaning. - debug!("place_element_conflict: DISJOINT-OR-EQ-ARRAY-INDEX"); - Overlap::EqualOrDisjoint + // (if the indexes differ) or equal (if they are the same). + match bias { + PlaceConflictBias::Overlap => { + // If we are biased towards overlapping, then this is the recursive + // case that gives "equal *or* disjoint" its meaning. + debug!("place_element_conflict: DISJOINT-OR-EQ-ARRAY-INDEX"); + Overlap::EqualOrDisjoint + } + PlaceConflictBias::NoOverlap => { + // If we are biased towards no overlapping, then this is disjoint. + debug!("place_element_conflict: DISJOINT-ARRAY-INDEX"); + Overlap::Disjoint + } + } } (ProjectionElem::ConstantIndex { offset: o1, min_length: _, from_end: false }, ProjectionElem::ConstantIndex { offset: o2, min_length: _, from_end: false }) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 3a9e4fc9e4ab..532143af6d39 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -11,7 +11,6 @@ use borrow_check::borrow_set::{BorrowSet, BorrowData}; use borrow_check::place_ext::PlaceExt; -use rustc; use rustc::mir::{self, Location, Place, Mir}; use rustc::ty::TyCtxt; use rustc::ty::RegionVid; @@ -24,6 +23,7 @@ use dataflow::{BitDenotation, BlockSets, InitialFlow}; pub use dataflow::indexes::BorrowIndex; use borrow_check::nll::region_infer::RegionInferenceContext; use borrow_check::nll::ToRegionVid; +use borrow_check::places_conflict; use std::rc::Rc; @@ -191,12 +191,54 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { } } - fn kill_borrows_on_local(&self, - sets: &mut BlockSets, - local: &rustc::mir::Local) - { - if let Some(borrow_indexes) = self.borrow_set.local_map.get(local) { - sets.kill_all(borrow_indexes); + /// Kill any borrows that conflict with `place`. + fn kill_borrows_on_place( + &self, + sets: &mut BlockSets, + location: Location, + place: &Place<'tcx> + ) { + debug!("kill_borrows_on_place: location={:?} place={:?}", location, place); + // Handle the `Place::Local(..)` case first and exit early. + if let Place::Local(local) = place { + if let Some(borrow_indexes) = self.borrow_set.local_map.get(&local) { + debug!( + "kill_borrows_on_place: local={:?} borrow_indexes={:?}", + local, borrow_indexes, + ); + sets.kill_all(borrow_indexes); + return; + } + } + + // Otherwise, look at all borrows that are live and if they conflict with the assignment + // into our place then we can kill them. + let mut borrows = sets.on_entry.clone(); + let _ = borrows.union(sets.gen_set); + for borrow_index in borrows.iter() { + let borrow_data = &self.borrows()[borrow_index]; + debug!( + "kill_borrows_on_place: borrow_index={:?} borrow_data={:?}", + borrow_index, borrow_data, + ); + + // By passing `PlaceConflictBias::NoOverlap`, we conservatively assume that any given + // pair of array indices are unequal, so that when `places_conflict` returns true, we + // will be assured that two places being compared definitely denotes the same sets of + // locations. + if places_conflict::places_conflict( + self.tcx, + self.mir, + place, + &borrow_data.borrowed_place, + places_conflict::PlaceConflictBias::NoOverlap, + ) { + debug!( + "kill_borrows_on_place: (kill) place={:?} borrow_index={:?} borrow_data={:?}", + place, borrow_index, borrow_data, + ); + sets.kill(borrow_index); + } } } } @@ -222,7 +264,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { } fn statement_effect(&self, sets: &mut BlockSets, location: Location) { - debug!("Borrows::statement_effect sets: {:?} location: {:?}", sets, location); + debug!("Borrows::statement_effect: sets={:?} location={:?}", sets, location); let block = &self.mir.basic_blocks().get(location.block).unwrap_or_else(|| { panic!("could not find block at location {:?}", location); @@ -231,21 +273,17 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { panic!("could not find statement at location {:?}"); }); + debug!("Borrows::statement_effect: stmt={:?}", stmt); match stmt.kind { mir::StatementKind::Assign(ref lhs, ref rhs) => { // Make sure there are no remaining borrows for variables // that are assigned over. - if let Place::Local(ref local) = *lhs { - // FIXME: Handle the case in which we're assigning over - // a projection (`foo.bar`). - self.kill_borrows_on_local(sets, local); - } + self.kill_borrows_on_place(sets, location, lhs); // NOTE: if/when the Assign case is revised to inspect // the assigned_place here, make sure to also // re-consider the current implementations of the // propagate_call_return method. - if let mir::Rvalue::Ref(_, _, ref place) = **rhs { if place.ignore_borrow( self.tcx, @@ -279,19 +317,13 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { mir::StatementKind::StorageDead(local) => { // Make sure there are no remaining borrows for locals that // are gone out of scope. - self.kill_borrows_on_local(sets, &local) + self.kill_borrows_on_place(sets, location, &Place::Local(local)); } mir::StatementKind::InlineAsm { ref outputs, ref asm, .. } => { for (output, kind) in outputs.iter().zip(&asm.outputs) { if !kind.is_indirect && !kind.is_rw { - // Make sure there are no remaining borrows for direct - // output variables. - if let Place::Local(ref local) = *output { - // FIXME: Handle the case in which we're assigning over - // a projection (`foo.bar`). - self.kill_borrows_on_local(sets, local); - } + self.kill_borrows_on_place(sets, location, output); } } } @@ -342,4 +374,3 @@ impl<'a, 'gcx, 'tcx> InitialFlow for Borrows<'a, 'gcx, 'tcx> { false // bottom = nothing is reserved or activated yet } } - diff --git a/src/test/ui/nll/issue-46589.rs b/src/test/ui/nll/issue-46589.rs index 0099e44ec385..82e73651f431 100644 --- a/src/test/ui/nll/issue-46589.rs +++ b/src/test/ui/nll/issue-46589.rs @@ -31,7 +31,6 @@ impl Foo { }; let c = other; - //~^ ERROR cannot move out of `other` because it is borrowed [E0505] } } diff --git a/src/test/ui/nll/issue-46589.stderr b/src/test/ui/nll/issue-46589.stderr index ef656912b0f1..6df2983f4652 100644 --- a/src/test/ui/nll/issue-46589.stderr +++ b/src/test/ui/nll/issue-46589.stderr @@ -10,19 +10,6 @@ LL | None => (*other).new_self() | second mutable borrow occurs here | first borrow later used here -error[E0505]: cannot move out of `other` because it is borrowed - --> $DIR/issue-46589.rs:33:17 - | -LL | *other = match (*other).get_self() { - | -------- borrow of `**other` occurs here -... -LL | let c = other; - | ^^^^^ - | | - | move out of `other` occurs here - | borrow later used here +error: aborting due to previous error -error: aborting due to 2 previous errors - -Some errors occurred: E0499, E0505. -For more information about an error, try `rustc --explain E0499`. +For more information about this error, try `rustc --explain E0499`. diff --git a/src/test/ui/nll/loan_ends_mid_block_pair.rs b/src/test/ui/nll/loan_ends_mid_block_pair.rs index 97126e98cbf3..320d80438b06 100644 --- a/src/test/ui/nll/loan_ends_mid_block_pair.rs +++ b/src/test/ui/nll/loan_ends_mid_block_pair.rs @@ -27,10 +27,8 @@ fn nll_fail() { //~| ERROR (Mir) [E0506] data.0 = 'f'; //~^ ERROR (Ast) [E0506] - //~| ERROR (Mir) [E0506] data.0 = 'g'; //~^ ERROR (Ast) [E0506] - //~| ERROR (Mir) [E0506] capitalize(c); } diff --git a/src/test/ui/nll/loan_ends_mid_block_pair.stderr b/src/test/ui/nll/loan_ends_mid_block_pair.stderr index 9afae71edbe1..3ba3fa15a538 100644 --- a/src/test/ui/nll/loan_ends_mid_block_pair.stderr +++ b/src/test/ui/nll/loan_ends_mid_block_pair.stderr @@ -17,7 +17,7 @@ LL | data.0 = 'f'; | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here error[E0506]: cannot assign to `data.0` because it is borrowed (Ast) - --> $DIR/loan_ends_mid_block_pair.rs:31:5 + --> $DIR/loan_ends_mid_block_pair.rs:30:5 | LL | let c = &mut data.0; | ------ borrow of `data.0` occurs here @@ -26,7 +26,7 @@ LL | data.0 = 'g'; | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here error[E0506]: cannot assign to `data.0` because it is borrowed (Ast) - --> $DIR/loan_ends_mid_block_pair.rs:41:5 + --> $DIR/loan_ends_mid_block_pair.rs:39:5 | LL | let c = &mut data.0; | ------ borrow of `data.0` occurs here @@ -34,21 +34,21 @@ LL | capitalize(c); LL | data.0 = 'e'; | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here +error[E0506]: cannot assign to `data.0` because it is borrowed (Ast) + --> $DIR/loan_ends_mid_block_pair.rs:41:5 + | +LL | let c = &mut data.0; + | ------ borrow of `data.0` occurs here +... +LL | data.0 = 'f'; + | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here + error[E0506]: cannot assign to `data.0` because it is borrowed (Ast) --> $DIR/loan_ends_mid_block_pair.rs:43:5 | LL | let c = &mut data.0; | ------ borrow of `data.0` occurs here ... -LL | data.0 = 'f'; - | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here - -error[E0506]: cannot assign to `data.0` because it is borrowed (Ast) - --> $DIR/loan_ends_mid_block_pair.rs:45:5 - | -LL | let c = &mut data.0; - | ------ borrow of `data.0` occurs here -... LL | data.0 = 'g'; | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here @@ -64,30 +64,6 @@ LL | data.0 = 'e'; LL | capitalize(c); | - borrow later used here -error[E0506]: cannot assign to `data.0` because it is borrowed (Mir) - --> $DIR/loan_ends_mid_block_pair.rs:28:5 - | -LL | let c = &mut data.0; - | ----------- borrow of `data.0` occurs here -... -LL | data.0 = 'f'; - | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here -... -LL | capitalize(c); - | - borrow later used here - -error[E0506]: cannot assign to `data.0` because it is borrowed (Mir) - --> $DIR/loan_ends_mid_block_pair.rs:31:5 - | -LL | let c = &mut data.0; - | ----------- borrow of `data.0` occurs here -... -LL | data.0 = 'g'; - | ^^^^^^^^^^^^ assignment to borrowed `data.0` occurs here -... -LL | capitalize(c); - | - borrow later used here - -error: aborting due to 9 previous errors +error: aborting due to 7 previous errors For more information about this error, try `rustc --explain E0506`.