diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 1b02bbbc152..e9d5bd365e2 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -36,6 +36,7 @@ use dataflow::{MaybeInitializedLvals, MaybeUninitializedLvals}; use dataflow::{EverInitializedLvals, MovingOutStatements}; use dataflow::{Borrows, BorrowData, ReserveOrActivateIndex}; use dataflow::{ActiveBorrows, Reservations}; +use dataflow::indexes::{BorrowIndex}; use dataflow::move_paths::{IllegalMoveOriginKind, MoveError}; use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MovePathIndex}; use util::borrowck_errors::{BorrowckErrors, Origin}; @@ -222,6 +223,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( }, storage_dead_or_drop_error_reported_l: FxHashSet(), storage_dead_or_drop_error_reported_s: FxHashSet(), + reservation_error_reported: FxHashSet(), }; let borrows = Borrows::new(tcx, mir, opt_regioncx, def_id, body_id); @@ -287,6 +289,14 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { storage_dead_or_drop_error_reported_l: FxHashSet, /// Same as the above, but for statics (thread-locals) storage_dead_or_drop_error_reported_s: FxHashSet, + /// 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>, } // Check that: @@ -318,6 +328,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx flow_state ); 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 @@ -424,6 +437,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx flow_state ); let span = term.source_info.span; + + self.check_activations(location, span, flow_state); + match term.kind { TerminatorKind::SwitchInt { ref discr, @@ -557,7 +573,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 { @@ -592,6 +608,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 @@ -680,6 +702,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) -> AccessErrorsReported { let (sd, rw) = kind; + if let Activation(_, borrow_index) = rw { + if self.reservation_error_reported.contains(&place_span.0) { + debug!("skipping access_place for activation of invalid reservation \ + place: {:?} borrow_index: {:?}", place_span.0, borrow_index); + return AccessErrorsReported { mutability_error: false, conflict_error: false }; + } + } + let mutability_error = self.check_access_permissions(place_span, rw, is_local_mutation_allowed); let conflict_error = @@ -702,9 +732,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { (sd, place_span.0), flow_state, |this, index, borrow| match (rw, borrow.kind) { - (Read(_), BorrowKind::Shared) => Control::Continue, + // 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(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut) => { + (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() @@ -734,13 +771,32 @@ 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 .operator() .opt_region_end_span(&borrow.region); + error_reported = true; this.report_conflicting_borrow( context, @@ -827,9 +883,15 @@ 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), @@ -837,6 +899,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { LocalMutationIsAllowed::No, flow_state, ); + self.check_if_path_is_moved( context, InitializationRequiringAction::Borrow, @@ -1007,6 +1070,49 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) } } + + fn check_activations(&mut self, + location: Location, + span: Span, + flow_state: &Flows<'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.operator(); + let data = domain.borrows(); + flow_state.borrows.each_gen_bit(|gen| { + if gen.is_activation() && // must be activation, + !flow_state.borrows.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 => return, + 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> { @@ -1250,11 +1356,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_mutable(place, LocalMutationIsAllowed::Yes) { 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) { @@ -1277,6 +1385,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; @@ -1298,6 +1407,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)) => { @@ -1312,6 +1424,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 +2007,7 @@ struct Context { #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum ContextKind { + Activation, AssignLhs, AssignRhs, SetDiscrim, diff --git a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs new file mode 100644 index 00000000000..b6f5e17f1f6 --- /dev/null +++ b/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 or the MIT license +// , 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() { } diff --git a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs index 397b3dafa7d..fc9100c8a9a 100644 --- a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs +++ b/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. -fn main() { +#![feature(rustc_attrs)] + +#[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]); }