From 1855ab742458cc4359e27deadbdf3d8747ce361d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 16 Jan 2018 10:52:52 +0100 Subject: [PATCH] Restrict two-phase borrows to solely borrows introduced via autoref. Added `-Z two-phase-beyond-autoref` to bring back old behavior (mainly to allow demonstration of desugared examples). Updated tests to use aforementioned flag when necessary. (But in each case where I added the flag, I made sure to also include a revision without the flag so that one can readily see what the actual behavior we expect is for the initial deployment of NLL.) --- src/librustc/mir/mod.rs | 9 +++++ src/librustc/session/config.rs | 2 ++ src/librustc_mir/borrow_check/mod.rs | 13 +++++-- ...o-phase-activation-sharing-interference.rs | 34 ++++++++++++++----- ...o-phase-allow-access-during-reservation.rs | 23 ++++++++++--- ...-phase-reservation-sharing-interference.rs | 28 ++++++++++++--- 6 files changed, 89 insertions(+), 20 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 70bfc1e2d327..c035d02a6123 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -420,6 +420,15 @@ pub enum BorrowKind { } } +impl BorrowKind { + pub fn allows_two_phase_borrow(&self) -> bool { + match *self { + BorrowKind::Shared | BorrowKind::Unique => false, + BorrowKind::Mut { allow_two_phase_borrow } => allow_two_phase_borrow, + } + } +} + /////////////////////////////////////////////////////////////////////////// // Variables and temps diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index c56575f432d1..76564b9b7d2a 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1085,6 +1085,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "select which borrowck is used (`ast`, `mir`, or `compare`)"), two_phase_borrows: bool = (false, parse_bool, [UNTRACKED], "use two-phase reserved/active distinction for `&mut` borrows in MIR borrowck"), + two_phase_beyond_autoref: bool = (false, parse_bool, [UNTRACKED], + "when using two-phase-borrows, allow two phases even for non-autoref `&mut` borrows"), time_passes: bool = (false, parse_bool, [UNTRACKED], "measure time of each rustc pass"), count_llvm_insns: bool = (false, parse_bool, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 647cf178310a..217dfdf8d416 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -707,6 +707,15 @@ impl InitializationRequiringAction { } impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { + /// Returns true if the borrow represented by `kind` is + /// allowed to be split into separate Reservation and + /// Activation phases. + fn allow_two_phase_borrow(&self, kind: BorrowKind) -> bool { + self.tcx.sess.two_phase_borrows() && + (kind.allows_two_phase_borrow() || + self.tcx.sess.opts.debugging_opts.two_phase_beyond_autoref) + } + /// Checks an access to the given place to see if it is allowed. Examines the set of borrows /// that are in scope, as well as which paths have been initialized, to ensure that (a) the /// place is initialized and (b) it is not borrowed in some way that would prevent this @@ -799,7 +808,7 @@ 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 this.tcx.sess.two_phase_borrows() && index.is_reservation() + if this.allow_two_phase_borrow(borrow.kind) && index.is_reservation() { return Control::Continue; } @@ -947,7 +956,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))), BorrowKind::Unique | BorrowKind::Mut { .. } => { let wk = WriteKind::MutableBorrow(bk); - if self.tcx.sess.two_phase_borrows() { + if self.allow_two_phase_borrow(bk) { (Deep, Reservation(wk)) } else { (Deep, Write(wk)) 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 index a9797e4d215a..709c00ba8464 100644 --- a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs +++ b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs @@ -8,9 +8,13 @@ // 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 +// ignore-tidy-linelength + +// revisions: lxl_beyond nll_beyond nll_target + +//[lxl_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref +//[nll_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref -Z nll +//[nll_target] 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 @@ -18,6 +22,14 @@ // // FIXME: for clarity, diagnostics for these cases might be better off // if they specifically said "cannot activate mutable borrow of `x`" +// +// The convention for the listed revisions: "lxl" means lexical +// lifetimes (which can be easier to reason about). "nll" means +// non-lexical lifetimes. "nll_target" means the initial conservative +// two-phase borrows that only applies to autoref-introduced borrows. +// "nll_beyond" means the generalization of two-phase borrows to all +// `&mut`-borrows (doing so makes it easier to write code for specific +// corner cases). #![allow(dead_code)] @@ -27,6 +39,7 @@ fn ok() { let mut x = 3; let y = &mut x; { let z = &x; read(z); } + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable *y += 1; } @@ -34,9 +47,11 @@ fn not_ok() { let mut x = 3; let y = &mut x; let z = &x; + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable *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 + //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[nll_beyond]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[nll_target]~^^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable read(z); } @@ -44,18 +59,21 @@ fn should_be_ok_with_nll() { let mut x = 3; let y = &mut x; let z = &x; + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable read(z); *y += 1; - //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable - // (okay with nll today) + //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + // (okay with (generalized) nll today) } fn should_also_eventually_be_ok_with_nll() { let mut x = 3; let y = &mut x; let _z = &x; + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable *y += 1; - //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + // (okay with (generalized) nll today) } fn main() { } diff --git a/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs b/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs index 7695bd3e4652..dd174981fb1e 100644 --- a/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs +++ b/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs @@ -8,9 +8,12 @@ // 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 +// ignore-tidy-linelength + +// revisions: lxl_beyond nll_beyond nll_target +//[lxl_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two_phase_beyond_autoref +//[nll_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two_phase_beyond_autoref -Z nll +//[nll_target] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is the second counter-example from Niko's blog post // smallcultfollowing.com/babysteps/blog/2017/03/01/nested-method-calls-via-two-phase-borrowing/ @@ -18,6 +21,14 @@ // It is "artificial". It is meant to illustrate directly that we // should allow an aliasing access during reservation, but *not* while // the mutable borrow is active. +// +// The convention for the listed revisions: "lxl" means lexical +// lifetimes (which can be easier to reason about). "nll" means +// non-lexical lifetimes. "nll_target" means the initial conservative +// two-phase borrows that only applies to autoref-introduced borrows. +// "nll_beyond" means the generalization of two-phase borrows to all +// `&mut`-borrows (doing so makes it easier to write code for specific +// corner cases). fn main() { /*0*/ let mut i = 0; @@ -25,11 +36,13 @@ fn main() { /*1*/ let p = &mut i; // (reservation of `i` starts here) /*2*/ let j = i; // OK: `i` is only reserved here + //[nll_target]~^ ERROR cannot use `i` because it was mutably borrowed [E0503] /*3*/ *p += 1; // (mutable borrow of `i` starts here, since `p` is used) - /*4*/ let k = i; //[lxl]~ ERROR cannot use `i` because it was mutably borrowed [E0503] - //[nll]~^ ERROR cannot use `i` because it was mutably borrowed [E0503] + /*4*/ let k = i; //[lxl_beyond]~ ERROR cannot use `i` because it was mutably borrowed [E0503] + //[nll_beyond]~^ ERROR cannot use `i` because it was mutably borrowed [E0503] + //[nll_target]~^^ ERROR cannot use `i` because it was mutably borrowed [E0503] /*5*/ *p += 1; diff --git a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs index cc85315263a4..b5fda4985f23 100644 --- a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs +++ b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs @@ -8,9 +8,13 @@ // 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 +// ignore-tidy-linelength + +// revisions: lxl_beyond nll_beyond nll_target + +//[lxl_beyond]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref +//[nll_beyond]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref -Z nll +//[nll_target]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is a corner case that the current implementation is (probably) // treating more conservatively than is necessary. But it also does @@ -19,6 +23,18 @@ // 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. +// +// The convention for the listed revisions: "lxl" means lexical +// lifetimes (which can be easier to reason about). "nll" means +// non-lexical lifetimes. "nll_target" means the initial conservative +// two-phase borrows that only applies to autoref-introduced borrows. +// "nll_beyond" means the generalization of two-phase borrows to all +// `&mut`-borrows (doing so makes it easier to write code for specific +// corner cases). +// +// FIXME: in "nll_target", we currently see the same error reported +// twice. This is injected by `-Z two-phase-borrows`; not sure why as +// of yet. fn main() { let mut vec = vec![0, 1]; @@ -30,8 +46,10 @@ fn main() { // with the shared borrow. But in the current implementation, // its an error. delay = &mut vec; - //[lxl]~^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable - //[nll]~^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[lxl_beyond]~^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll_beyond]~^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll_target]~^^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll_target]~| ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable shared[0]; }