Check activation points as the place where mutable borrows become relevant.

Since we are now checking activation points, I removed one of the
checks at the reservation point. (You can see the effect this had on
two-phase-reservation-sharing-interference-2.rs)

Also, since we now have checks at both the reservation point and the
activation point, we sometimes would observe duplicate errors (since
either one independently interferes with another mutable borrow).  To
deal with this, I used a similar strategy to one used as discussed on
issue #45360: keep a set of errors reported (in this case for
reservations), and then avoid doing the checks for the corresponding
activations. (This does mean that some errors could get masked, namely
for conflicting borrows that start after the reservation but still
conflict with the activation, which is unchecked when there was an
error for the reservation. But this seems like a reasonable price to
pay.)

Author: pnkfelix

src/librustc_mir/borrow_check/mod.rs

@@ -34,6 +34,7 @@ use dataflow::{MaybeInitializedLvals, MaybeUninitializedLvals};
 use dataflow::{EverInitializedLvals, MovingOutStatements};
 use dataflow::{Borrows, BorrowData, ResActIndex};
 use dataflow::{ActiveBorrows, Reservations};
+use dataflow::indexes::{BorrowIndex};
 use dataflow::move_paths::{IllegalMoveOriginKind, MoveError};
 use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MoveOutIndex, MovePathIndex};
 use util::borrowck_errors::{BorrowckErrors, Origin};
@@ -195,6 +196,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
         move_data: &mdpe.move_data,
         param_env: param_env,
         storage_dead_or_drop_error_reported: FxHashSet(),
+        reservation_error_reported: FxHashSet(),
     };
 
     let borrows = Borrows::new(tcx, mir, opt_regioncx);
@@ -249,6 +251,14 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
     /// in order to stop duplicate error reporting and identify the conditions required
     /// for a "temporary value dropped here while still borrowed" error. See #45360.
     storage_dead_or_drop_error_reported: FxHashSet<Local>,
+    /// This field keeps track of when borrow conflict errors are reported
+    /// for reservations, so that we don't report seemingly duplicate
+    /// errors for corresponding activations
+    ///
+    /// FIXME: Ideally this would be a set of BorrowIndex, not Places,
+    /// but it is currently inconvenient to track down the BorrowIndex
+    /// at the time we detect and report a reservation error.
+    reservation_error_reported: FxHashSet<Place<'tcx>>,
 }
 
 // (forced to be `pub` due to its use as an associated type below.)
@@ -347,6 +357,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
             summary
         );
         let span = stmt.source_info.span;
+
+        self.check_activations(location, span, flow_state);
+
         match stmt.kind {
             StatementKind::Assign(ref lhs, ref rhs) => {
                 // NOTE: NLL RFC calls for *shallow* write; using Deep
@@ -454,6 +467,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
             summary
         );
         let span = term.source_info.span;
+
+        self.check_activations(location, span, flow_state);
+
         match term.kind {
             TerminatorKind::SwitchInt {
                 ref discr,
@@ -600,7 +616,7 @@ enum Control {
 }
 
 use self::ShallowOrDeep::{Deep, Shallow};
-use self::ReadOrWrite::{Read, Write};
+use self::ReadOrWrite::{Activation, Read, Reservation, Write};
 
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 enum ArtificialField {
@@ -635,6 +651,12 @@ enum ReadOrWrite {
     /// new values or otherwise invalidated (for example, it could be
     /// de-initialized, as in a move operation).
     Write(WriteKind),
+
+    /// For two-phase borrows, we distinguish a reservation (which is treated
+    /// like a Read) from an activation (which is treated like a write), and
+    /// each of those is furthermore distinguished from Reads/Writes above.
+    Reservation(WriteKind),
+    Activation(WriteKind, BorrowIndex),
 }
 
 /// Kind of read access to a value
@@ -726,6 +748,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             }
         }
 
+        if let Activation(_, borrow_index) = rw {
+            if self.reservation_error_reported.contains(&place_span.0) {
+                debug!("skipping check_place for activation of invalid reservation \
+                        place: {:?} borrow_index: {:?}", place_span.0, borrow_index);
+                return;
+            }
+        }
+
         // Check permissions
         let mut error_reported =
             self.check_access_permissions(place_span, rw, is_local_mutation_allowed);
@@ -735,9 +765,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             (sd, place_span.0),
             flow_state,
             |this, index, borrow, common_prefix| match (rw, borrow.kind) {
-                (Read(_), BorrowKind::Shared) => Control::Continue,
 
-                (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut) => {
+                // Obviously an activation is compatible with its own reservation;
+                // so don't check if they interfere.
+                (Activation(_, activating), _) if index.is_reservation() &&
+                    activating == index.borrow_index() => Control::Continue,
+
+                (Read(_), BorrowKind::Shared) |
+                (Reservation(..), BorrowKind::Shared) => Control::Continue,
+
+                (Read(kind), BorrowKind::Unique) |
+                (Read(kind), BorrowKind::Mut) => {
                     // Reading from mere reservations of mutable-borrows is OK.
                     if this.tcx.sess.opts.debugging_opts.two_phase_borrows &&
                         index.is_reservation()
@@ -769,14 +807,33 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     }
                     Control::Break
                 }
+
+                (Reservation(kind), BorrowKind::Unique) |
+                (Reservation(kind), BorrowKind::Mut) |
+                (Activation(kind, _), _) |
                 (Write(kind), _) => {
+
+                    match rw {
+                        Reservation(_) => {
+                            debug!("recording invalid reservation of \
+                                    place: {:?}", place_span.0);
+                            this.reservation_error_reported.insert(place_span.0.clone());
+                        }
+                        Activation(_, activating) => {
+                            debug!("observing check_place for activation of \
+                                    borrow_index: {:?}", activating);
+                        }
+                        Read(..) | Write(..) => {}
+                    }
+
                     match kind {
                         WriteKind::MutableBorrow(bk) => {
                             let end_issued_loan_span = flow_state
                                 .borrows
                                 .base_results
                                 .operator()
                                 .opt_region_end_span(&borrow.region);
+
                             error_reported = true;
                             this.report_conflicting_borrow(
                                 context,
@@ -868,16 +925,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 let access_kind = match bk {
                     BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
                     BorrowKind::Unique | BorrowKind::Mut => {
-                        (Deep, Write(WriteKind::MutableBorrow(bk)))
+                        let wk = WriteKind::MutableBorrow(bk);
+                        if self.tcx.sess.opts.debugging_opts.two_phase_borrows {
+                            (Deep, Reservation(wk))
+                        } else {
+                            (Deep, Write(wk))
+                        }
                     }
                 };
+
                 self.access_place(
                     context,
                     (place, span),
                     access_kind,
                     LocalMutationIsAllowed::No,
                     flow_state,
                 );
+
                 self.check_if_path_is_moved(
                     context,
                     InitializationRequiringAction::Borrow,
@@ -1031,6 +1095,50 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         self.prefixes(place, prefix_set)
             .any(|prefix| prefix == root_place)
     }
+
+    fn check_activations(&mut self,
+                         location: Location,
+                         span: Span,
+                         flow_state: &InProgress<'cx, 'gcx, 'tcx>)
+    {
+        if !self.tcx.sess.opts.debugging_opts.two_phase_borrows {
+            return;
+        }
+
+        // Two-phase borrow support: For each activation that is newly
+        // generated at this statement, check if it interferes with
+        // another borrow.
+        let domain = flow_state.borrows.base_results.operator();
+        let data = domain.borrows();
+        let sets = &flow_state.borrows.sets;
+        for gen in sets.gen_set.iter() {
+            if gen.is_activation() && // must be activation,
+                !sets.on_entry.contains(&gen) // and newly generated.
+            {
+                let borrow_index = gen.borrow_index();
+                let borrow = &data[borrow_index];
+                // currently the flow analysis registers
+                // activations for both mutable and immutable
+                // borrows. So make sure we are talking about a
+                // mutable borrow before we check it.
+                match borrow.kind {
+                    BorrowKind::Shared => continue,
+                    BorrowKind::Unique |
+                    BorrowKind::Mut => {}
+                }
+
+                self.access_place(ContextKind::Activation.new(location),
+                                  (&borrow.borrowed_place, span),
+                                  (Deep, Activation(WriteKind::MutableBorrow(borrow.kind),
+                                                    borrow_index)),
+                                  LocalMutationIsAllowed::No,
+                                  flow_state);
+                // We do not need to call `check_if_path_is_moved`
+                // again, as we already called it when we made the
+                // initial reservation.
+            }
+        }
+    }
 }
 
 impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
@@ -1293,11 +1401,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         );
         let mut error_reported = false;
         match kind {
+            Reservation(WriteKind::MutableBorrow(BorrowKind::Unique)) |
             Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => {
                 if let Err(_place_err) = self.is_unique(place) {
                     span_bug!(span, "&unique borrow for {:?} should not fail", place);
                 }
             }
+            Reservation(WriteKind::MutableBorrow(BorrowKind::Mut)) |
             Write(WriteKind::MutableBorrow(BorrowKind::Mut)) => if let Err(place_err) =
                 self.is_mutable(place, is_local_mutation_allowed)
             {
@@ -1320,6 +1430,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
 
                 err.emit();
             },
+            Reservation(WriteKind::Mutate) |
             Write(WriteKind::Mutate) => {
                 if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) {
                     error_reported = true;
@@ -1341,6 +1452,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     err.emit();
                 }
             }
+            Reservation(WriteKind::Move) |
+            Reservation(WriteKind::StorageDeadOrDrop) |
+            Reservation(WriteKind::MutableBorrow(BorrowKind::Shared)) |
             Write(WriteKind::Move) |
             Write(WriteKind::StorageDeadOrDrop) |
             Write(WriteKind::MutableBorrow(BorrowKind::Shared)) => {
@@ -1355,6 +1469,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     );
                 }
             }
+
+            Activation(..) => {} // permission checks are done at Reservation point.
+
             Read(ReadKind::Borrow(BorrowKind::Unique)) |
             Read(ReadKind::Borrow(BorrowKind::Mut)) |
             Read(ReadKind::Borrow(BorrowKind::Shared)) |
@@ -1892,6 +2009,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         use rustc::hir::ExprClosure;
         use rustc::mir::AggregateKind;
 
+        if location.statement_index == self.mir[location.block].statements.len() {
+            // Code below hasn't been written in a manner to deal with
+            // a terminator location.
+            return None;
+        }
+
         let local = if let StatementKind::Assign(Place::Local(local), _) =
             self.mir[location.block].statements[location.statement_index].kind
         {
@@ -2355,6 +2478,7 @@ struct Context {
 
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 enum ContextKind {
+    Activation,
     AssignLhs,
     AssignRhs,
     SetDiscrim,

src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs

@@ -0,0 +1,62 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// revisions: lxl nll
+//[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows
+//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll
+
+// This is an important corner case pointed out by Niko: one is
+// allowed to initiate a shared borrow during a reservation, but it
+// *must end* before the activation occurs.
+//
+// FIXME: for clarity, diagnostics for these cases might be better off
+// if they specifically said "cannot activate mutable borrow of `x`"
+
+#![allow(dead_code)]
+
+fn read(_: &i32) { }
+
+fn ok() {
+    let mut x = 3;
+    let y = &mut x;
+    { let z = &x; read(z); }
+    *y += 1;
+}
+
+fn not_ok() {
+    let mut x = 3;
+    let y = &mut x;
+    let z = &x;
+    *y += 1;
+    //[lxl]~^  ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
+    //[nll]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
+    read(z);
+}
+
+fn should_be_ok_with_nll() {
+    let mut x = 3;
+    let y = &mut x;
+    let z = &x;
+    read(z);
+    *y += 1;
+    //[lxl]~^  ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
+    // (okay with nll today)
+}
+
+fn should_also_eventually_be_ok_with_nll() {
+    let mut x = 3;
+    let y = &mut x;
+    let _z = &x;
+    *y += 1;
+    //[lxl]~^  ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
+    //[nll]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
+}
+
+fn main() { }

src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs

@@ -15,24 +15,23 @@
 // This is similar to two-phase-reservation-sharing-interference.rs
 // in that it shows a reservation that overlaps with a shared borrow.
 //
-// However, it is also more immediately concerning because one would
-// intutively think that if run-pass/borrowck/two-phase-baseline.rs
-// works, then this *should* work too.
+// Currently, this test fails with lexical lifetimes, but succeeds
+// with non-lexical lifetimes. (The reason is because the activation
+// of the mutable borrow ends up overlapping with a lexically-scoped
+// shared borrow; but a non-lexical shared borrow can end before the
+// activation occurs.)
 //
-// As before, the current implementation is (probably) more
-// conservative than is necessary.
-//
-// So this test is just making a note of the current behavior, with
-// the caveat that in the future, the rules may be loosened, at which
-// point this test might be thrown out.
+// So this test is just making a note of the current behavior.
+
+#![feature(rustc_attrs)]
 
-fn main() {
+#[rustc_error]
+fn main() { //[nll]~ ERROR compilation successful
     let mut v = vec![0, 1, 2];
     let shared = &v;
 
     v.push(shared.len());
     //[lxl]~^  ERROR cannot borrow `v` as mutable because it is also borrowed as immutable [E0502]
-    //[nll]~^^ ERROR cannot borrow `v` as mutable because it is also borrowed as immutable [E0502]
 
     assert_eq!(v, [0, 1, 2, 3]);
 }