Auto merge of #56649 - davidtwco:issue-46589, r=pnkfelix

MIR borrowck doesn't accept the example of iterating and updating a mutable reference

Fixes #46589.

r? @pnkfelix or @nikomatsakis

Author: bors

src/librustc_mir/borrow_check/flows.rs

@@ -33,19 +33,19 @@ use std::rc::Rc;
 
 // (forced to be `pub` due to its use as an associated type below.)
 crate struct Flows<'b, 'gcx: 'tcx, 'tcx: 'b> {
-    borrows: FlowAtLocation<Borrows<'b, 'gcx, 'tcx>>,
-    pub uninits: FlowAtLocation<MaybeUninitializedPlaces<'b, 'gcx, 'tcx>>,
-    pub ever_inits: FlowAtLocation<EverInitializedPlaces<'b, 'gcx, 'tcx>>,
+    borrows: FlowAtLocation<'tcx, Borrows<'b, 'gcx, 'tcx>>,
+    pub uninits: FlowAtLocation<'tcx, MaybeUninitializedPlaces<'b, 'gcx, 'tcx>>,
+    pub ever_inits: FlowAtLocation<'tcx, EverInitializedPlaces<'b, 'gcx, 'tcx>>,
 
     /// Polonius Output
     pub polonius_output: Option<Rc<Output<RegionVid, BorrowIndex, LocationIndex>>>,
 }
 
 impl<'b, 'gcx, 'tcx> Flows<'b, 'gcx, 'tcx> {
     crate fn new(
-        borrows: FlowAtLocation<Borrows<'b, 'gcx, 'tcx>>,
-        uninits: FlowAtLocation<MaybeUninitializedPlaces<'b, 'gcx, 'tcx>>,
-        ever_inits: FlowAtLocation<EverInitializedPlaces<'b, 'gcx, 'tcx>>,
+        borrows: FlowAtLocation<'tcx, Borrows<'b, 'gcx, 'tcx>>,
+        uninits: FlowAtLocation<'tcx, MaybeUninitializedPlaces<'b, 'gcx, 'tcx>>,
+        ever_inits: FlowAtLocation<'tcx, EverInitializedPlaces<'b, 'gcx, 'tcx>>,
         polonius_output: Option<Rc<Output<RegionVid, BorrowIndex, LocationIndex>>>,
     ) -> Self {
         Flows {

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;
 
@@ -1369,7 +1369,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

src/librustc_mir/borrow_check/nll/mod.rs

@@ -85,7 +85,7 @@ pub(in borrow_check) fn compute_regions<'cx, 'gcx, 'tcx>(
     mir: &Mir<'tcx>,
     location_table: &LocationTable,
     param_env: ty::ParamEnv<'gcx>,
-    flow_inits: &mut FlowAtLocation<MaybeInitializedPlaces<'cx, 'gcx, 'tcx>>,
+    flow_inits: &mut FlowAtLocation<'tcx, MaybeInitializedPlaces<'cx, 'gcx, 'tcx>>,
     move_data: &MoveData<'tcx>,
     borrow_set: &BorrowSet<'tcx>,
     errors_buffer: &mut Vec<Diagnostic>,

src/librustc_mir/borrow_check/nll/type_check/liveness/mod.rs

@@ -39,7 +39,7 @@ pub(super) fn generate<'gcx, 'tcx>(
     typeck: &mut TypeChecker<'_, 'gcx, 'tcx>,
     mir: &Mir<'tcx>,
     elements: &Rc<RegionValueElements>,
-    flow_inits: &mut FlowAtLocation<MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
+    flow_inits: &mut FlowAtLocation<'tcx, MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
     move_data: &MoveData<'tcx>,
     location_table: &LocationTable,
 ) {

src/librustc_mir/borrow_check/nll/type_check/liveness/trace.rs

@@ -46,7 +46,7 @@ pub(super) fn trace(
     typeck: &mut TypeChecker<'_, 'gcx, 'tcx>,
     mir: &Mir<'tcx>,
     elements: &Rc<RegionValueElements>,
-    flow_inits: &mut FlowAtLocation<MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
+    flow_inits: &mut FlowAtLocation<'tcx, MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
     move_data: &MoveData<'tcx>,
     liveness_map: &NllLivenessMap,
     location_table: &LocationTable,
@@ -99,7 +99,7 @@ where
 
     /// Results of dataflow tracking which variables (and paths) have been
     /// initialized.
-    flow_inits: &'me mut FlowAtLocation<MaybeInitializedPlaces<'flow, 'gcx, 'tcx>>,
+    flow_inits: &'me mut FlowAtLocation<'tcx, MaybeInitializedPlaces<'flow, 'gcx, 'tcx>>,
 
     /// Index indicating where each variable is assigned, used, or
     /// dropped.

src/librustc_mir/borrow_check/nll/type_check/mod.rs

@@ -123,7 +123,7 @@ pub(crate) fn type_check<'gcx, 'tcx>(
     location_table: &LocationTable,
     borrow_set: &BorrowSet<'tcx>,
     all_facts: &mut Option<AllFacts>,
-    flow_inits: &mut FlowAtLocation<MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
+    flow_inits: &mut FlowAtLocation<'tcx, MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
     move_data: &MoveData<'tcx>,
     elements: &Rc<RegionValueElements>,
 ) -> MirTypeckResults<'tcx> {

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. {:?}/{:?}",

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.
@@ -190,7 +230,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;
@@ -328,6 +368,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)) => {
@@ -445,10 +486,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 })

src/librustc_mir/dataflow/at_location.rs

@@ -70,19 +70,19 @@ pub trait FlowsAtLocation {
 /// (e.g., via `reconstruct_statement_effect` and
 /// `reconstruct_terminator_effect`; don't forget to call
 /// `apply_local_effect`).
-pub struct FlowAtLocation<BD>
+pub struct FlowAtLocation<'tcx, BD>
 where
-    BD: BitDenotation,
+    BD: BitDenotation<'tcx>,
 {
-    base_results: DataflowResults<BD>,
+    base_results: DataflowResults<'tcx, BD>,
     curr_state: BitSet<BD::Idx>,
     stmt_gen: HybridBitSet<BD::Idx>,
     stmt_kill: HybridBitSet<BD::Idx>,
 }
 
-impl<BD> FlowAtLocation<BD>
+impl<'tcx, BD> FlowAtLocation<'tcx, BD>
 where
-    BD: BitDenotation,
+    BD: BitDenotation<'tcx>,
 {
     /// Iterate over each bit set in the current state.
     pub fn each_state_bit<F>(&self, f: F)
@@ -102,7 +102,7 @@ where
         self.stmt_gen.iter().for_each(f)
     }
 
-    pub fn new(results: DataflowResults<BD>) -> Self {
+    pub fn new(results: DataflowResults<'tcx, BD>) -> Self {
         let bits_per_block = results.sets().bits_per_block();
         let curr_state = BitSet::new_empty(bits_per_block);
         let stmt_gen = HybridBitSet::new_empty(bits_per_block);
@@ -143,8 +143,8 @@ where
     }
 }
 
-impl<BD> FlowsAtLocation for FlowAtLocation<BD>
-    where BD: BitDenotation
+impl<'tcx, BD> FlowsAtLocation for FlowAtLocation<'tcx, BD>
+    where BD: BitDenotation<'tcx>
 {
     fn reset_to_entry_of(&mut self, bb: BasicBlock) {
         self.curr_state.overwrite(self.base_results.sets().on_entry_set_for(bb.index()));
@@ -213,9 +213,9 @@ impl<BD> FlowsAtLocation for FlowAtLocation<BD>
 }
 
 
-impl<'tcx, T> FlowAtLocation<T>
+impl<'tcx, T> FlowAtLocation<'tcx, T>
 where
-    T: HasMoveData<'tcx> + BitDenotation<Idx = MovePathIndex>,
+    T: HasMoveData<'tcx> + BitDenotation<'tcx, Idx = MovePathIndex>,
 {
     pub fn has_any_child_of(&self, mpi: T::Idx) -> Option<T::Idx> {
         // We process `mpi` before the loop below, for two reasons:

src/librustc_mir/dataflow/graphviz.rs

@@ -25,19 +25,19 @@ use super::DataflowBuilder;
 use super::DebugFormatted;
 
 pub trait MirWithFlowState<'tcx> {
-    type BD: BitDenotation;
+    type BD: BitDenotation<'tcx>;
     fn node_id(&self) -> NodeId;
     fn mir(&self) -> &Mir<'tcx>;
-    fn flow_state(&self) -> &DataflowState<Self::BD>;
+    fn flow_state(&self) -> &DataflowState<'tcx, Self::BD>;
 }
 
 impl<'a, 'tcx, BD> MirWithFlowState<'tcx> for DataflowBuilder<'a, 'tcx, BD>
-    where BD: BitDenotation
+    where BD: BitDenotation<'tcx>
 {
     type BD = BD;
     fn node_id(&self) -> NodeId { self.node_id }
     fn mir(&self) -> &Mir<'tcx> { self.flow_state.mir() }
-    fn flow_state(&self) -> &DataflowState<Self::BD> { &self.flow_state.flow_state }
+    fn flow_state(&self) -> &DataflowState<'tcx, Self::BD> { &self.flow_state.flow_state }
 }
 
 struct Graph<'a, 'tcx, MWF:'a, P> where
@@ -53,8 +53,8 @@ pub(crate) fn print_borrowck_graph_to<'a, 'tcx, BD, P>(
     path: &Path,
     render_idx: P)
     -> io::Result<()>
-    where BD: BitDenotation,
-          P: Fn(&BD, BD::Idx) -> DebugFormatted
+    where BD: BitDenotation<'tcx>,
+          P: Fn(&BD, BD::Idx) -> DebugFormatted,
 {
     let g = Graph { mbcx, phantom: PhantomData, render_idx };
     let mut v = Vec::new();
@@ -76,7 +76,7 @@ fn outgoing(mir: &Mir, bb: BasicBlock) -> Vec<Edge> {
 
 impl<'a, 'tcx, MWF, P> dot::Labeller<'a> for Graph<'a, 'tcx, MWF, P>
     where MWF: MirWithFlowState<'tcx>,
-          P: Fn(&MWF::BD, <MWF::BD as BitDenotation>::Idx) -> DebugFormatted,
+          P: Fn(&MWF::BD, <MWF::BD as BitDenotation<'tcx>>::Idx) -> DebugFormatted,
 {
     type Node = Node;
     type Edge = Edge;
@@ -128,7 +128,7 @@ impl<'a, 'tcx, MWF, P> dot::Labeller<'a> for Graph<'a, 'tcx, MWF, P>
 
 impl<'a, 'tcx, MWF, P> Graph<'a, 'tcx, MWF, P>
 where MWF: MirWithFlowState<'tcx>,
-      P: Fn(&MWF::BD, <MWF::BD as BitDenotation>::Idx) -> DebugFormatted,
+      P: Fn(&MWF::BD, <MWF::BD as BitDenotation<'tcx>>::Idx) -> DebugFormatted,
 {
     /// Generate the node label
     fn node_label_internal<W: io::Write>(&self,