Reconcile docs and code, adding examples and adding RESTR_CLAIM

Author: nikomatsakis

src/librustc/middle/borrowck/check_loans.rs

@@ -65,7 +65,7 @@ pub fn check_loans(bccx: @BorrowckCtxt,
 
 enum MoveError {
     MoveOk,
-    MoveWhileBorrowed(/*loan*/@LoanPath, /*loan*/span)
+    MoveWhileBorrowed(/*move*/@LoanPath, /*loan*/@LoanPath, /*loan*/span)
 }
 
 pub impl<'self> CheckLoanCtxt<'self> {
@@ -200,9 +200,9 @@ pub impl<'self> CheckLoanCtxt<'self> {
                loan1.repr(self.tcx()),
                loan2.repr(self.tcx()));
 
-        // Restrictions that would cause the new loan to be immutable:
+        // Restrictions that would cause the new loan to be illegal:
         let illegal_if = match loan2.mutbl {
-            m_mutbl => RESTR_ALIAS | RESTR_FREEZE | RESTR_MUTATE,
+            m_mutbl => RESTR_ALIAS | RESTR_FREEZE | RESTR_CLAIM,
             m_imm =>   RESTR_ALIAS | RESTR_FREEZE,
             m_const => RESTR_ALIAS,
         };
@@ -557,12 +557,12 @@ pub impl<'self> CheckLoanCtxt<'self> {
                 let cmt = self.bccx.cat_expr(ex);
                 match self.analyze_move_out_from_cmt(cmt) {
                     MoveOk => {}
-                    MoveWhileBorrowed(loan_path, loan_span) => {
+                    MoveWhileBorrowed(move_path, loan_path, loan_span) => {
                         self.bccx.span_err(
                             cmt.span,
                             fmt!("cannot move out of `%s` \
                                   because it is borrowed",
-                                 self.bccx.loan_path_to_str(loan_path)));
+                                 self.bccx.loan_path_to_str(move_path)));
                         self.bccx.span_note(
                             loan_span,
                             fmt!("borrow of `%s` occurs here",
@@ -582,7 +582,7 @@ pub impl<'self> CheckLoanCtxt<'self> {
         for opt_loan_path(cmt).each |&lp| {
             for self.each_in_scope_restriction(cmt.id, lp) |loan, _| {
                 // Any restriction prevents moves.
-                return MoveWhileBorrowed(loan.loan_path, loan.span);
+                return MoveWhileBorrowed(lp, loan.loan_path, loan.span);
             }
         }
 

src/librustc/middle/borrowck/doc.rs

@@ -580,15 +580,27 @@ borrow, `LV` remains the *sole pointer with mutable access* to `*LV`.
 Restrictions against mutations and claims are necessary because if the
 pointer in `LV` were to be somehow copied or moved to a different
 location, then the restriction issued for `*LV` would not apply to the
-new location. Consider this example, where `*t0` is frozen, but then
-`t0` and `t1` are swapped, so by mutating `*t1` the user can mutate
-the frozen memory that was originally found at `*t0`:
-
-    fn foo(t0: &mut T,
-           t1: &mut T) {
-        let p: &T = &*t0;     // Freezes `*t0`
-        t0 <-> t1;
-        *t1 = ...;            // Invalidates `p`
+new location. Note that because `&mut` values are non-copyable, a
+simple attempt to move the base pointer will fail due to the
+(implicit) restriction against moves:
+
+    // src/test/compile-fail/borrowck-move-mut-base-ptr.rs
+    fn foo(t0: &mut int) {
+        let p: &int = &*t0; // Freezes `*t0`
+        let t1 = t0;        //~ ERROR cannot move out of `t0`
+        *t1 = 22;
+    }
+
+However, the additional restrictions against mutation mean that even a
+clever attempt to use a swap to circumvent the type system will
+encounter an error:
+
+    // src/test/compile-fail/borrowck-swap-mut-base-ptr.rs
+    fn foo<'a>(mut t0: &'a mut int,
+               mut t1: &'a mut int) {
+        let p: &int = &*t0;     // Freezes `*t0`
+        swap(&mut t0, &mut t1); //~ ERROR cannot borrow `t0`
+        *t1 = 22;
     }
 
 The restriction against *aliasing* (and, in turn, freezing) is
@@ -598,12 +610,32 @@ pointee. Since we are only issuing restrictions against `*LV`, these
 other aliases would be unrestricted, and the result would be
 unsound. For example:
 
-    fn foo(t0: &mut T) {
-        let p: &T = &*t0;     // Freezes *t0
-        let q: &&mut T = &t0;
-        **q = ...;            // Invalidates `p`
+    // src/test/compile-fail/borrowck-alias-mut-base-ptr.rs
+    fn foo(t0: &mut int) {
+        let p: &int = &*t0; // Freezes `*t0`
+        let q: &const &mut int = &const t0; //~ ERROR cannot borrow `t0`
+        **q = 22; // (*)
     }
 
+Note that the current rules also report an error at the assignment in
+`(*)`, because we only permit `&mut` poiners to be assigned if they
+are located in a non-aliasable location. However, I do not believe
+this restriction is strictly necessary. It was added, I believe, to
+discourage `&mut` from being placed in aliasable locations in the
+first place. One (desirable) side-effect of restricting aliasing on
+`LV` is that borrowing an `&mut` pointee found inside an aliasable
+pointee yields an error:
+
+    // src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc:
+    fn foo(t0: & &mut int) {
+        let t1 = t0;
+        let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&` pointer
+        **t1 = 22; // (*)
+    }
+
+Here at the line `(*)` you will also see the error I referred to
+above, which I do not believe is strictly necessary.
+
 The second rule for `&mut` handles the case where we are not adding
 any restrictions (beyond the default of "no move"):
 
@@ -622,4 +654,22 @@ that way if we *can* find a simple static error, we will:
     RESTRICTIONS(*LV, ACTIONS) = [*LV, ACTIONS]   // R-Deref-Managed-Borrowed
       TYPE(LV) = @mut Ty
 
+# Some notes for future work
+
+While writing up these docs, I encountered some rules I believe to be
+stricter than necessary:
+
+- I think the restriction against mutating `&mut` pointers found in an
+  aliasable location is unnecessary. They cannot be reborrowed, to be sure,
+  so it should be safe to mutate them. Lifting this might cause some common
+  cases (`&mut int`) to work just fine, but might lead to further confusion
+  in other cases, so maybe it's best to leave it as is.
+- I think restricting the `&mut` LV against moves and `ALIAS` is sufficient,
+  `MUTATE` and `CLAIM` are overkill. `MUTATE` was necessary when swap was
+  a built-in operator, but as it is not, it is implied by `CLAIM`,
+  and `CLAIM` is implied by `ALIAS`. The only net effect of this is an
+  extra error message in some cases, though.
+- I have not described how closures interact. Current code is unsound.
+  I am working on describing and implementing the fix.
+
 */

src/librustc/middle/borrowck/gather_loans/mod.rs

@@ -517,8 +517,8 @@ pub impl GatherLoanCtxt {
     fn restriction_set(&self, req_mutbl: ast::mutability) -> RestrictionSet {
         match req_mutbl {
             m_const => RESTR_EMPTY,
-            m_imm   => RESTR_EMPTY | RESTR_MUTATE,
-            m_mutbl => RESTR_EMPTY | RESTR_MUTATE | RESTR_FREEZE
+            m_imm   => RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM,
+            m_mutbl => RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM | RESTR_FREEZE
         }
     }
 

src/librustc/middle/borrowck/gather_loans/restrictions.rs

@@ -86,7 +86,7 @@ impl RestrictionsContext {
                 // When we borrow the interior of an enum, we have to
                 // ensure the enum itself is not mutated, because that
                 // could cause the type of the memory to change.
-                self.compute(cmt_base, restrictions | RESTR_MUTATE)
+                self.compute(cmt_base, restrictions | RESTR_MUTATE | RESTR_CLAIM)
             }
 
             mc::cat_interior(cmt_base, i) => {
@@ -105,7 +105,9 @@ impl RestrictionsContext {
                 // When we borrow the interior of an owned pointer, we
                 // cannot permit the base to be mutated, because that
                 // would cause the unique pointer to be freed.
-                let result = self.compute(cmt_base, restrictions | RESTR_MUTATE);
+                let result = self.compute(
+                    cmt_base,
+                    restrictions | RESTR_MUTATE | RESTR_CLAIM);
                 self.extend(result, cmt.mutbl, LpDeref, restrictions)
             }
 
@@ -178,7 +180,9 @@ impl RestrictionsContext {
                 // mutability, we can only prevent mutation or prevent
                 // freezing if it is not aliased. Therefore, in such
                 // cases we restrict aliasing on `cmt_base`.
-                if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE) {
+                if restrictions.intersects(RESTR_MUTATE |
+                                           RESTR_CLAIM |
+                                           RESTR_FREEZE) {
                     // R-Deref-Mut-Borrowed-1
                     let result = self.compute(cmt_base, restrictions | RESTR_ALIAS);
                     self.extend(result, cmt.mutbl, LpDeref, restrictions)
@@ -244,7 +248,7 @@ impl RestrictionsContext {
     fn check_no_mutability_control(&self,
                                    cmt: mc::cmt,
                                    restrictions: RestrictionSet) {
-        if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE) {
+        if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE | RESTR_CLAIM) {
             self.bccx.report(BckError {span: self.span,
                                        cmt: cmt,
                                        code: err_freeze_aliasable_const});

src/librustc/middle/borrowck/mod.rs

@@ -301,10 +301,10 @@ pub fn opt_loan_path(cmt: mc::cmt) -> Option<@LoanPath> {
 // Borrowing an lvalue often results in *restrictions* that limit what
 // can be done with this lvalue during the scope of the loan:
 //
-// - `RESTR_MUTATE`: The lvalue may not be modified and mutable pointers to
-//                   the value cannot be created.
-// - `RESTR_FREEZE`: Immutable pointers to the value cannot be created.
-// - `RESTR_ALIAS`: The lvalue may not be aliased in any way.
+// - `RESTR_MUTATE`: The lvalue may not be modified.
+// - `RESTR_CLAIM`: `&mut` borrows of the lvalue are forbidden.
+// - `RESTR_FREEZE`: `&` borrows of the lvalue are forbidden.
+// - `RESTR_ALIAS`: All borrows of the lvalue are forbidden.
 //
 // In addition, no value which is restricted may be moved. Therefore,
 // restrictions are meaningful even if the RestrictionSet is empty,
@@ -319,10 +319,11 @@ pub struct RestrictionSet {
     bits: u32
 }
 
-pub static RESTR_EMPTY: RestrictionSet  = RestrictionSet {bits: 0b000};
-pub static RESTR_MUTATE: RestrictionSet = RestrictionSet {bits: 0b001};
-pub static RESTR_FREEZE: RestrictionSet = RestrictionSet {bits: 0b010};
-pub static RESTR_ALIAS: RestrictionSet  = RestrictionSet {bits: 0b100};
+pub static RESTR_EMPTY: RestrictionSet  = RestrictionSet {bits: 0b0000};
+pub static RESTR_MUTATE: RestrictionSet = RestrictionSet {bits: 0b0001};
+pub static RESTR_CLAIM: RestrictionSet  = RestrictionSet {bits: 0b0010};
+pub static RESTR_FREEZE: RestrictionSet = RestrictionSet {bits: 0b0100};
+pub static RESTR_ALIAS: RestrictionSet  = RestrictionSet {bits: 0b1000};
 
 pub impl RestrictionSet {
     fn intersects(&self, restr: RestrictionSet) -> bool {

src/test/compile-fail/borrowck-alias-mut-base-ptr.rs

@@ -0,0 +1,15 @@
+// Test that attempt to alias `&mut` pointer while pointee is borrowed
+// yields an error.
+//
+// Example from src/middle/borrowck/doc.rs
+
+use std::util::swap;
+
+fn foo(t0: &mut int) {
+    let p: &int = &*t0; // Freezes `*t0`
+    let q: &const &mut int = &const t0; //~ ERROR cannot borrow `t0`
+    **q = 22; //~ ERROR cannot assign to an `&mut` in a `&const` pointer
+}
+
+fn main() {
+}

src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs

@@ -0,0 +1,31 @@
+// Test that attempt to reborrow an `&mut` pointer in an aliasable
+// location yields an error.
+//
+// Example from src/middle/borrowck/doc.rs
+
+use std::util::swap;
+
+fn foo(t0: & &mut int) {
+    let t1 = t0;
+    let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&` pointer
+    **t1 = 22; //~ ERROR cannot assign
+}
+
+fn foo2(t0: &const &mut int) {
+    // Note: reborrowing from an &const actually yields two errors, since it
+    // is unsafe in two ways: we can't control the aliasing, and we can't
+    // control the mutation.
+    let t1 = t0;
+    let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&const` pointer
+    //~^ ERROR unsafe borrow of aliasable, const value
+    **t1 = 22; //~ ERROR cannot assign
+}
+
+fn foo3(t0: &mut &mut int) {
+    let t1 = &mut *t0;
+    let p: &int = &**t0; //~ ERROR cannot borrow
+    **t1 = 22;
+}
+
+fn main() {
+}

src/test/compile-fail/borrowck-move-mut-base-ptr.rs

@@ -0,0 +1,15 @@
+// Test that attempt to move `&mut` pointer while pointee is borrowed
+// yields an error.
+//
+// Example from src/middle/borrowck/doc.rs
+
+use std::util::swap;
+
+fn foo(t0: &mut int) {
+    let p: &int = &*t0; // Freezes `*t0`
+    let t1 = t0;        //~ ERROR cannot move out of `t0`
+    *t1 = 22;
+}
+
+fn main() {
+}

src/test/compile-fail/borrowck-swap-mut-base-ptr.rs

@@ -0,0 +1,16 @@
+// Test that attempt to swap `&mut` pointer while pointee is borrowed
+// yields an error.
+//
+// Example from src/middle/borrowck/doc.rs
+
+use std::util::swap;
+
+fn foo<'a>(mut t0: &'a mut int,
+           mut t1: &'a mut int) {
+    let p: &int = &*t0;     // Freezes `*t0`
+    swap(&mut t0, &mut t1); //~ ERROR cannot borrow `t0`
+    *t1 = 22;
+}
+
+fn main() {
+}