Skip to content

Commit

Permalink
Check activation points as the place where mutable borrows become rel…
Browse files Browse the repository at this point in the history
…evant.

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.)
  • Loading branch information
pnkfelix committed Dec 13, 2017
1 parent 18aedf6 commit 5cae7a0
Show file tree
Hide file tree
Showing 3 changed files with 192 additions and 15 deletions.
124 changes: 120 additions & 4 deletions src/librustc_mir/borrow_check/mod.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -287,6 +289,14 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
storage_dead_or_drop_error_reported_l: FxHashSet<Local>,
/// Same as the above, but for statics (thread-locals)
storage_dead_or_drop_error_reported_s: FxHashSet<DefId>,
/// 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>>,
}

// Check that:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 =
Expand All @@ -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()
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -827,16 +883,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,
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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)
{
Expand All @@ -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;
Expand All @@ -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)) => {
Expand All @@ -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)) |
Expand Down Expand Up @@ -1892,6 +2007,7 @@ struct Context {

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
enum ContextKind {
Activation,
AssignLhs,
AssignRhs,
SetDiscrim,
Expand Down
@@ -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() { }
Expand Up @@ -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]);
}

0 comments on commit 5cae7a0

Please sign in to comment.