Kill borrows from a projection after assignment.

davidtwco · davidtwco · commit db635fc5664c · 2018-12-17T14:49:52.000+01:00
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.
diff --git 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
@@ -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
@@ -17,17 +17,55 @@ 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>,
     borrow_place: &Place<'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
@@ -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<BorrowIndex>,
-                             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<BorrowIndex>,
+        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<BorrowIndex>, 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
@@ -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
@@ -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 2 previous errors
+error: aborting due to previous error
 
-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
@@ -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
@@ -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
@@ -35,7 +35,7 @@ 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:43:5
+  --> $DIR/loan_ends_mid_block_pair.rs:41:5
    |
 LL |     let c = &mut data.0;
    |                  ------ borrow of `data.0` occurs here
@@ -44,7 +44,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:45:5
+  --> $DIR/loan_ends_mid_block_pair.rs:43:5
    |
 LL |     let c = &mut data.0;
    |                  ------ borrow of `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`.

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,6 @@ impl Foo {`
`31`	`31`	`};`
`32`	`32`
`33`	`33`	`let c = other;`
`34`		- //~^ ERROR cannot move out of `other` because it is borrowed [E0505]
`35`	`34`	`}`
`36`	`35`	`}`
`37`	`36`