Auto merge of #47489 - pnkfelix:limit-2pb-issue-46747, r=nikomatsakis

NLL: Limit two-phase borrows to autoref-introduced borrows

This imposes a restriction on two-phase borrows so that it only applies to autoref-introduced borrows.

The goal is to ensure that our initial deployment of two-phase borrows is very conservative. We want it to still cover the `v.push(v.len());` example, but we do not want it to cover cases like `let imm = &v; let mu = &mut v; mu.push(imm.len());`

(Why do we want it to be conservative? Because when you are not conservative, then the results you get, at least with the current analysis, are tightly coupled to details of the MIR construction that we would rather remain invisible to the end user.)

Fix #46747

I decided, for this PR, to add a debug-flag `-Z two-phase-beyond-autoref`, to re-enable the more general approach. But my intention here is *not* that we would eventually turn on that debugflag by default; the main reason I added it was that I thought it was useful for writing tests to be able to write source that looks like desugared MIR.

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() { }

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;
 

src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs

@@ -0,0 +1,260 @@
+// 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 g2p
+//[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows
+//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll
+//[g2p]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll -Z two-phase-beyond-autoref
+
+// This is a test checking that when we limit two-phase borrows to
+// method receivers, we do not let other kinds of auto-ref to leak
+// through.
+//
+// The g2p revision illustrates the "undesirable" behavior you would
+// otherwise observe without limiting the phasing to autoref on method
+// receivers (namely, in many cases demonstrated below, the error
+// would not arise).
+
+// (If we revise the compiler or this test so that the g2p revision
+// passes, turn the `rustc_attrs` feature back on and tag the `fn
+// main` with `#[rustc_error]` so that this remains a valid
+// compile-fail test.)
+//
+// #![feature(rustc_attrs)]
+
+use std::ops::{Index, IndexMut};
+use std::ops::{AddAssign, SubAssign, MulAssign, DivAssign, RemAssign};
+use std::ops::{BitAndAssign, BitOrAssign, BitXorAssign, ShlAssign, ShrAssign};
+
+// This is case outlined by Niko that we want to ensure we reject
+// (at least initially).
+
+fn foo(x: &mut u32, y: u32) {
+    *x += y;
+}
+
+fn deref_coercion(x: &mut u32) {
+    foo(x, *x);
+    //[lxl]~^ ERROR cannot use `*x` because it was mutably borrowed [E0503]
+    //[nll]~^^ ERROR cannot use `*x` because it was mutably borrowed [E0503]
+}
+
+// While adding a flag to adjustments (indicating whether they
+// should support two-phase borrows, here are the cases I
+// encountered:
+//
+// - [x] Resolving overloaded_call_traits (call, call_mut, call_once)
+// - [x] deref_coercion (shown above)
+// - [x] coerce_unsized e.g. `&[T; n]`, `&mut [T; n] -> &[T]`,
+//                      `&mut [T; n] -> &mut [T]`, `&Concrete -> &Trait`
+// - [x] Method Call Receivers (the case we want to support!)
+// - [x] ExprIndex and ExprUnary Deref; only need to handle coerce_index_op
+// - [x] overloaded_binops
+
+fn overloaded_call_traits() {
+    // Regarding overloaded call traits, note that there is no
+    // scenario where adding two-phase borrows should "fix" these
+    // cases, because either we will resolve both invocations to
+    // `call_mut` (in which case the inner call requires a mutable
+    // borrow which will conflict with the outer reservation), or we
+    // will resolve both to `call` (which will just work, regardless
+    // of two-phase borrow support), or we will resolve both to
+    // `call_once` (in which case the inner call requires moving the
+    // receiver, invalidating the outer call).
+
+    fn twice_ten_sm<F: FnMut(i32) -> i32>(f: &mut F) {
+        f(f(10));
+        //[lxl]~^     ERROR cannot borrow `*f` as mutable more than once at a time
+        //[lxl]~|     ERROR cannot borrow `*f` as mutable more than once at a time
+        //[nll]~^^^   ERROR cannot borrow `*f` as mutable more than once at a time
+        //[nll]~|     ERROR cannot borrow `*f` as mutable more than once at a time
+        //[g2p]~^^^^^ ERROR cannot borrow `*f` as mutable more than once at a time
+    }
+    fn twice_ten_si<F: Fn(i32) -> i32>(f: &mut F) {
+        f(f(10));
+    }
+    fn twice_ten_so<F: FnOnce(i32) -> i32>(f: Box<F>) {
+        f(f(10));
+        //[lxl]~^   ERROR use of moved value: `*f`
+        //[nll]~^^  ERROR use of moved value: `*f`
+        //[g2p]~^^^ ERROR use of moved value: `*f`
+    }
+
+    fn twice_ten_om(f: &mut FnMut(i32) -> i32) {
+        f(f(10));
+        //[lxl]~^     ERROR cannot borrow `*f` as mutable more than once at a time
+        //[lxl]~|     ERROR cannot borrow `*f` as mutable more than once at a time
+        //[nll]~^^^   ERROR cannot borrow `*f` as mutable more than once at a time
+        //[nll]~|     ERROR cannot borrow `*f` as mutable more than once at a time
+        //[g2p]~^^^^^ ERROR cannot borrow `*f` as mutable more than once at a time
+    }
+    fn twice_ten_oi(f: &mut Fn(i32) -> i32) {
+        f(f(10));
+    }
+    fn twice_ten_oo(f: Box<FnOnce(i32) -> i32>) {
+        f(f(10));
+        //[lxl]~^             ERROR cannot move a value of type
+        //[lxl]~^^            ERROR cannot move a value of type
+        //[lxl]~^^^           ERROR use of moved value: `*f`
+        //[nll]~^^^^          ERROR cannot move a value of type
+        //[nll]~^^^^^         ERROR cannot move a value of type
+        //[nll]~^^^^^^        ERROR cannot move a value of type
+        //[nll]~^^^^^^^       ERROR cannot move a value of type
+        //[nll]~^^^^^^^^      ERROR use of moved value: `*f`
+        //[g2p]~^^^^^^^^^     ERROR cannot move a value of type
+        //[g2p]~^^^^^^^^^^    ERROR cannot move a value of type
+        //[g2p]~^^^^^^^^^^^   ERROR cannot move a value of type
+        //[g2p]~^^^^^^^^^^^^  ERROR cannot move a value of type
+        //[g2p]~^^^^^^^^^^^^^ ERROR use of moved value: `*f`
+    }
+
+    twice_ten_sm(&mut |x| x + 1);
+    twice_ten_si(&mut |x| x + 1);
+    twice_ten_so(Box::new(|x| x + 1));
+    twice_ten_om(&mut |x| x + 1);
+    twice_ten_oi(&mut |x| x + 1);
+    twice_ten_oo(Box::new(|x| x + 1));
+}
+
+trait TwoMethods {
+    fn m(&mut self, x: i32) -> i32 { x + 1 }
+    fn i(&self, x: i32) -> i32 { x + 1 }
+}
+
+struct T;
+
+impl TwoMethods for T { }
+
+struct S;
+
+impl S {
+    fn m(&mut self, x: i32) -> i32 { x + 1 }
+    fn i(&self, x: i32) -> i32 { x + 1 }
+}
+
+impl TwoMethods for [i32; 3] { }
+
+fn double_access<X: Copy>(m: &mut [X], s: &[X]) {
+    m[0] = s[1];
+}
+
+fn coerce_unsized() {
+    let mut a = [1, 2, 3];
+
+    // This is not okay.
+    double_access(&mut a, &a);
+    //[lxl]~^   ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502]
+    //[nll]~^^  ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502]
+    //[g2p]~^^^ ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502]
+
+    // But this is okay.
+    a.m(a.i(10));
+}
+
+struct I(i32);
+
+impl Index<i32> for I {
+    type Output = i32;
+    fn index(&self, _: i32) -> &i32 {
+        &self.0
+    }
+}
+
+impl IndexMut<i32> for I {
+    fn index_mut(&mut self, _: i32) -> &mut i32 {
+        &mut self.0
+    }
+}
+
+fn coerce_index_op() {
+    let mut i = I(10);
+    i[i[3]] = 4;
+    //[lxl]~^  ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502]
+    //[nll]~^^ ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502]
+
+    i[3] = i[4];
+
+    i[i[3]] = i[4];
+    //[lxl]~^  ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502]
+    //[nll]~^^ ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502]
+}
+
+struct A(i32);
+
+macro_rules! trivial_binop {
+    ($Trait:ident, $m:ident) => {
+        impl $Trait<i32> for A { fn $m(&mut self, rhs: i32) { self.0 = rhs; } }
+    }
+}
+
+trivial_binop!(AddAssign, add_assign);
+trivial_binop!(SubAssign, sub_assign);
+trivial_binop!(MulAssign, mul_assign);
+trivial_binop!(DivAssign, div_assign);
+trivial_binop!(RemAssign, rem_assign);
+trivial_binop!(BitAndAssign, bitand_assign);
+trivial_binop!(BitOrAssign, bitor_assign);
+trivial_binop!(BitXorAssign, bitxor_assign);
+trivial_binop!(ShlAssign, shl_assign);
+trivial_binop!(ShrAssign, shr_assign);
+
+fn overloaded_binops() {
+    let mut a = A(10);
+    a += a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a -= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a *= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a /= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a &= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a |= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a ^= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a <<= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+    a >>= a.0;
+    //[lxl]~^   ERROR cannot use `a.0` because it was mutably borrowed
+    //[nll]~^^  ERROR cannot use `a.0` because it was mutably borrowed
+}
+
+fn main() {
+
+    // As a reminder, this is the basic case we want to ensure we handle.
+    let mut v = vec![1, 2, 3];
+    v.push(v.len());
+
+    // (as a rule, pnkfelix does not like to write tests with dead code.)
+
+    deref_coercion(&mut 5);
+    overloaded_call_traits();
+
+
+    let mut s = S;
+    s.m(s.i(10));
+
+    let mut t = T;
+    t.m(t.i(10));
+
+    coerce_unsized();
+    coerce_index_op();
+    overloaded_binops();
+}

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];
     }