Treat two-phase borrow reservations as mutable accesses

Author: matthewjasper

src/librustc_mir/borrow_check/borrow_set.rs

@@ -52,7 +52,7 @@ crate enum TwoPhaseActivation {
     ActivatedAt(Location),
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 crate struct BorrowData<'tcx> {
     /// Location where the borrow reservation starts.
     /// In many cases, this will be equal to the activation location but not always.

src/librustc_mir/borrow_check/error_reporting.rs

@@ -318,7 +318,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         context: Context,
         (place, _span): (&Place<'tcx>, Span),
         borrow: &BorrowData<'tcx>,
-    ) {
+    ) -> DiagnosticBuilder<'cx> {
         let tcx = self.infcx.tcx;
 
         let borrow_spans = self.retrieve_borrow_spans(borrow);
@@ -347,7 +347,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
 
         self.explain_why_borrow_contains_point(context, borrow, None)
             .add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, "", None);
-        err.buffer(&mut self.errors_buffer);
+        err
     }
 
     pub(super) fn report_conflicting_borrow(
@@ -356,7 +356,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         (place, span): (&Place<'tcx>, Span),
         gen_borrow_kind: BorrowKind,
         issued_borrow: &BorrowData<'tcx>,
-    ) {
+    ) -> DiagnosticBuilder<'cx> {
         let issued_spans = self.retrieve_borrow_spans(issued_borrow);
         let issued_span = issued_spans.args_or_use();
 
@@ -460,9 +460,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                         "borrow occurs due to use of `{}`{}", desc_place, borrow_spans.describe()
                     ),
                 );
-                err.buffer(&mut self.errors_buffer);
 
-                return;
+                return err;
             }
 
             (BorrowKind::Unique, _, _, _, _, _) => {
@@ -563,7 +562,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             None,
         );
 
-        err.buffer(&mut self.errors_buffer);
+        err
     }
 
     /// Returns the description of the root place for a conflicting borrow and the full

src/librustc_mir/borrow_check/mod.rs

@@ -18,12 +18,13 @@ use rustc::ty::{self, TyCtxt};
 
 use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, Level};
 use rustc_data_structures::bit_set::BitSet;
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::graph::dominators::Dominators;
 use smallvec::SmallVec;
 
-use std::rc::Rc;
 use std::collections::BTreeMap;
+use std::mem;
+use std::rc::Rc;
 
 use syntax_pos::Span;
 
@@ -238,6 +239,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
         locals_are_invalidated_at_exit,
         access_place_error_reported: Default::default(),
         reservation_error_reported: Default::default(),
+        reservation_warnings: Default::default(),
         move_error_reported: BTreeMap::new(),
         uninitialized_error_reported: Default::default(),
         errors_buffer,
@@ -260,6 +262,14 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
     }
     mbcx.analyze_results(&mut state); // entry point for DataflowResultsConsumer
 
+    // Buffer any reservation warnings.
+    let reservation_warnings = mem::replace(&mut mbcx.reservation_warnings, Default::default());
+    for (_, (place, span, context, bk, borrow)) in reservation_warnings {
+        let mut diag = mbcx.report_conflicting_borrow(context, (&place, span), bk, &borrow);
+        downgrade_if_error(&mut diag);
+        diag.buffer(&mut mbcx.errors_buffer);
+    }
+
     // For each non-user used mutable variable, check if it's been assigned from
     // a user-declared local. If so, then put that local into the used_mut set.
     // Note that this set is expected to be small - only upvars from closures
@@ -341,18 +351,9 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
                     // if AST-borrowck signalled no errors, then
                     // downgrade all the buffered MIR-borrowck errors
                     // to warnings.
-                    for err in &mut mbcx.errors_buffer {
-                        if err.is_error() {
-                            err.level = Level::Warning;
-                            err.warn(
-                                "this error has been downgraded to a warning for backwards \
-                                 compatibility with previous releases",
-                            );
-                            err.warn(
-                                "this represents potential undefined behavior in your code and \
-                                 this warning will become a hard error in the future",
-                            );
-                        }
+
+                    for err in mbcx.errors_buffer.iter_mut() {
+                        downgrade_if_error(err);
                     }
                 }
                 SignalledError::SawSomeError => {
@@ -378,6 +379,20 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
     result
 }
 
+fn downgrade_if_error(diag: &mut Diagnostic) {
+    if diag.is_error() {
+        diag.level = Level::Warning;
+        diag.warn(
+            "this error has been downgraded to a warning for backwards \
+            compatibility with previous releases",
+        );
+        diag.warn(
+            "this represents potential undefined behavior in your code and \
+            this warning will become a hard error in the future",
+        );
+    }
+}
+
 pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
     infcx: &'cx InferCtxt<'cx, 'gcx, 'tcx>,
     mir: &'cx Mir<'tcx>,
@@ -410,6 +425,13 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
     // 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>>,
+    /// Migration warnings to be reported for #56254. We delay reporting these
+    /// so that we can suppress the warning if there's a corresponding error
+    /// for the activation of the borrow.
+    reservation_warnings: FxHashMap<
+        BorrowIndex,
+        (Place<'tcx>, Span, Context, BorrowKind, BorrowData<'tcx>)
+    >,
     /// This field keeps track of move errors that are to be reported for given move indicies.
     ///
     /// There are situations where many errors can be reported for a single move out (see #53807)
@@ -921,11 +943,18 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         let conflict_error =
             self.check_access_for_conflict(context, place_span, sd, rw, flow_state);
 
+        if let (Activation(_, borrow_idx), true) = (kind.1, conflict_error) {
+            // Suppress this warning when there's an error being emited for the
+            // same borrow: fixing the error is likely to fix the warning.
+            self.reservation_warnings.remove(&borrow_idx);
+        }
+
         if conflict_error || mutability_error {
             debug!(
                 "access_place: logging error place_span=`{:?}` kind=`{:?}`",
                 place_span, kind
             );
+
             self.access_place_error_reported
                 .insert((place_span.0.clone(), place_span.1));
         }
@@ -976,8 +1005,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     Control::Continue
                 }
 
-                (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared)
-                | (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow)
+                (Read(_), BorrowKind::Shared)
+                | (Read(_), BorrowKind::Shallow)
                 | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Unique)
                 | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Mut { .. }) => {
                     Control::Continue
@@ -991,28 +1020,53 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut { .. }) => {
                     // Reading from mere reservations of mutable-borrows is OK.
                     if !is_active(&this.dominators, borrow, context.loc) {
-                        assert!(allow_two_phase_borrow(&this.infcx.tcx, borrow.kind));
+                        assert!(allow_two_phase_borrow(&tcx, borrow.kind));
                         return Control::Continue;
                     }
 
                     error_reported = true;
                     match kind {
                         ReadKind::Copy  => {
                             this.report_use_while_mutably_borrowed(context, place_span, borrow)
+                                .buffer(&mut this.errors_buffer);
                         }
                         ReadKind::Borrow(bk) => {
-                            this.report_conflicting_borrow(context, place_span, bk, &borrow)
+                            this.report_conflicting_borrow(context, place_span, bk, borrow)
+                                .buffer(&mut this.errors_buffer);
                         }
                     }
                     Control::Break
                 }
 
-                (Reservation(kind), BorrowKind::Unique)
-                | (Reservation(kind), BorrowKind::Mut { .. })
+                (Reservation(WriteKind::MutableBorrow(bk)), BorrowKind::Shallow)
+                | (Reservation(WriteKind::MutableBorrow(bk)), BorrowKind::Shared) if {
+                    tcx.migrate_borrowck()
+                } => {
+                    let bi = this.borrow_set.location_map[&context.loc];
+                    debug!(
+                        "recording invalid reservation of place: {:?} with \
+                         borrow index {:?} as warning",
+                        place_span.0,
+                        bi,
+                    );
+                    // rust-lang/rust#56254 - This was previously permitted on
+                    // the 2018 edition so we emit it as a warning. We buffer
+                    // these sepately so that we only emit a warning if borrow
+                    // checking was otherwise successful.
+                    this.reservation_warnings.insert(
+                        bi,
+                        (place_span.0.clone(), place_span.1, context, bk, borrow.clone()),
+                    );
+
+                    // Don't suppress actual errors.
+                    Control::Continue
+                }
+
+                (Reservation(kind), _)
                 | (Activation(kind, _), _)
                 | (Write(kind), _) => {
                     match rw {
-                        Reservation(_) => {
+                        Reservation(..) => {
                             debug!(
                                 "recording invalid reservation of \
                                  place: {:?}",
@@ -1033,7 +1087,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     error_reported = true;
                     match kind {
                         WriteKind::MutableBorrow(bk) => {
-                            this.report_conflicting_borrow(context, place_span, bk, &borrow)
+                            this.report_conflicting_borrow(context, place_span, bk, borrow)
+                                .buffer(&mut this.errors_buffer);
                         }
                         WriteKind::StorageDeadOrDrop => {
                             this.report_borrowed_value_does_not_live_long_enough(
@@ -1046,7 +1101,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                             this.report_illegal_mutation_of_borrowed(context, place_span, borrow)
                         }
                         WriteKind::Move => {
-                            this.report_move_out_while_borrowed(context, place_span, &borrow)
+                            this.report_move_out_while_borrowed(context, place_span, borrow)
                         }
                     }
                     Control::Break

src/librustc_mir/borrow_check/nll/invalidation.rs

@@ -428,8 +428,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> {
                         // have already taken the reservation
                     }
 
-                    (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow)
-                    | (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared)
+                    (Read(_), BorrowKind::Shallow)
+                    | (Read(_), BorrowKind::Shared)
                     | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Unique)
                     | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Mut { .. }) => {
                         // Reads/reservations don't invalidate shared or shallow borrows
@@ -448,16 +448,15 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> {
                         this.generate_invalidates(borrow_index, context.loc);
                     }
 
-                    (Reservation(_), BorrowKind::Unique)
-                        | (Reservation(_), BorrowKind::Mut { .. })
-                        | (Activation(_, _), _)
-                        | (Write(_), _) => {
-                            // unique or mutable borrows are invalidated by writes.
-                            // Reservations count as writes since we need to check
-                            // that activating the borrow will be OK
-                            // FIXME(bob_twinkles) is this actually the right thing to do?
-                            this.generate_invalidates(borrow_index, context.loc);
-                        }
+                    (Reservation(_), _)
+                    | (Activation(_, _), _)
+                    | (Write(_), _) => {
+                        // unique or mutable borrows are invalidated by writes.
+                        // Reservations count as writes since we need to check
+                        // that activating the borrow will be OK
+                        // FIXME(bob_twinkles) is this actually the right thing to do?
+                        this.generate_invalidates(borrow_index, context.loc);
+                    }
                 }
                 Control::Continue
             },

src/test/ui/borrowck/two-phase-reservation-sharing-interference-2.ast.stderr

@@ -0,0 +1,36 @@
+error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
+  --> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
+   |
+LL |     let shared = &v;
+   |                   - immutable borrow occurs here
+LL | 
+LL |     v.extend(shared);
+   |     ^ mutable borrow occurs here
+...
+LL | }
+   | - immutable borrow ends here
+
+error[E0502]: cannot borrow `v` as immutable because it is also borrowed as mutable
+  --> $DIR/two-phase-reservation-sharing-interference-2.rs:30:15
+   |
+LL |     v.extend(&v);
+   |     -         ^- mutable borrow ends here
+   |     |         |
+   |     |         immutable borrow occurs here
+   |     mutable borrow occurs here
+
+error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
+  --> $DIR/two-phase-reservation-sharing-interference-2.rs:42:5
+   |
+LL |     let shared = &v;
+   |                   - immutable borrow occurs here
+LL | 
+LL |     v.push(shared.len());
+   |     ^ mutable borrow occurs here
+...
+LL | }
+   | - immutable borrow ends here
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0502`.

src/test/ui/borrowck/two-phase-reservation-sharing-interference-2.migrate2015.stderr

@@ -0,0 +1,39 @@
+error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
+  --> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
+   |
+LL |     let shared = &v;
+   |                  -- immutable borrow occurs here
+LL | 
+LL |     v.extend(shared);
+   |     ^^------^^^^^^^^
+   |     | |
+   |     | immutable borrow later used by call
+   |     mutable borrow occurs here
+
+error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
+  --> $DIR/two-phase-reservation-sharing-interference-2.rs:30:5
+   |
+LL |     v.extend(&v);
+   |     ^^------^--^
+   |     | |      |
+   |     | |      immutable borrow occurs here
+   |     | immutable borrow later used by call
+   |     mutable borrow occurs here
+
+warning[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
+  --> $DIR/two-phase-reservation-sharing-interference-2.rs:42:5
+   |
+LL |     let shared = &v;
+   |                  -- immutable borrow occurs here
+LL | 
+LL |     v.push(shared.len());
+   |     ^      ------ immutable borrow later used here
+   |     |
+   |     mutable borrow occurs here
+   |
+   = warning: this error has been downgraded to a warning for backwards compatibility with previous releases
+   = warning: this represents potential undefined behavior in your code and this warning will become a hard error in the future
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0502`.