Make &mut borrows restrict aliasing

Fixes #11913

src/librustc/metadata/encoder.rs

@@ -681,7 +681,7 @@ fn encode_explicit_self(ebml_w: &mut writer::Encoder, explicit_self: ast::Explic
 
     ebml_w.end_tag();
 
-    fn encode_mutability(ebml_w: &writer::Encoder,
+    fn encode_mutability(ebml_w: &mut writer::Encoder,
                          m: ast::Mutability) {
         match m {
             MutImmutable => { ebml_w.writer.write(&[ 'i' as u8 ]); }

src/librustc/middle/borrowck/check_loans.rs

@@ -22,7 +22,6 @@
 use middle::borrowck::*;
 use middle::moves;
 use middle::ty;
-use syntax::ast::{MutImmutable, MutMutable};
 use syntax::ast;
 use syntax::ast_map;
 use syntax::ast_util;
@@ -220,8 +219,8 @@ pub fn report_error_if_loan_conflicts_with_restriction(&self,
 
         // Restrictions that would cause the new loan to be illegal:
         let illegal_if = match loan2.mutbl {
-            MutableMutability   => RESTR_ALIAS | RESTR_FREEZE | RESTR_CLAIM,
-            ImmutableMutability => RESTR_ALIAS | RESTR_FREEZE,
+            MutableMutability   => RESTR_FREEZE | RESTR_CLAIM,
+            ImmutableMutability => RESTR_FREEZE,
         };
         debug!("illegal_if={:?}", illegal_if);
 
@@ -423,7 +422,7 @@ fn check_for_aliasable_mutable_writes(this: &CheckLoanCtxt,
             debug!("check_for_aliasable_mutable_writes(cmt={}, guarantor={})",
                    cmt.repr(this.tcx()), guarantor.repr(this.tcx()));
             match guarantor.cat {
-                mc::cat_deref(b, _, mc::region_ptr(MutMutable, _)) => {
+                mc::cat_deref(b, _, mc::region_ptr(ast::MutMutable, _)) => {
                     // Statically prohibit writes to `&mut` when aliasable
 
                     check_for_aliasability_violation(this, expr, b);
@@ -437,43 +436,18 @@ fn check_for_aliasable_mutable_writes(this: &CheckLoanCtxt,
 
         fn check_for_aliasability_violation(this: &CheckLoanCtxt,
                                             expr: &ast::Expr,
-                                            cmt: mc::cmt) -> bool {
-            let mut cmt = cmt;
-
-            loop {
-                match cmt.cat {
-                    mc::cat_deref(b, _, mc::region_ptr(MutMutable, _)) |
-                    mc::cat_downcast(b) |
-                    mc::cat_stack_upvar(b) |
-                    mc::cat_deref(b, _, mc::uniq_ptr) |
-                    mc::cat_interior(b, _) |
-                    mc::cat_discr(b, _) => {
-                        // Aliasability depends on base cmt
-                        cmt = b;
-                    }
-
-                    mc::cat_copied_upvar(_) |
-                    mc::cat_rvalue(..) |
-                    mc::cat_local(..) |
-                    mc::cat_arg(_) |
-                    mc::cat_deref(_, _, mc::unsafe_ptr(..)) |
-                    mc::cat_static_item(..) |
-                    mc::cat_deref(_, _, mc::gc_ptr) |
-                    mc::cat_deref(_, _, mc::region_ptr(MutImmutable, _)) => {
-                        // Aliasability is independent of base cmt
-                        match cmt.freely_aliasable() {
-                            None => {
-                                return true;
-                            }
-                            Some(cause) => {
-                                this.bccx.report_aliasability_violation(
-                                    expr.span,
-                                    MutabilityViolation,
-                                    cause);
-                                return false;
-                            }
-                        }
-                    }
+                                            cmt: mc::cmt)
+                                            -> bool {
+            match cmt.freely_aliasable() {
+                None => {
+                    return true;
+                }
+                Some(cause) => {
+                    this.bccx.report_aliasability_violation(
+                        expr.span,
+                        MutabilityViolation,
+                        cause);
+                    return false;
                 }
             }
         }

src/librustc/middle/borrowck/doc.rs

@@ -151,14 +151,13 @@ struct defined in `middle::borrowck`. Formally, we define `LOAN` as
 restrictions `RS` that accompany the loan.
 
 The first restriction `((*x).f, [MUTATE, CLAIM, FREEZE])` states that
-the lender may not mutate nor freeze `(*x).f`. Mutation is illegal
-because `(*x).f` is only supposed to be mutated via the new reference,
-not by mutating the original path `(*x).f`. Freezing is
+the lender may not mutate, freeze, nor alias `(*x).f`. Mutation is
+illegal because `(*x).f` is only supposed to be mutated via the new
+reference, not by mutating the original path `(*x).f`. Freezing is
 illegal because the path now has an `&mut` alias; so even if we the
 lender were to consider `(*x).f` to be immutable, it might be mutated
-via this alias. Both of these restrictions are temporary. They will be
-enforced for the lifetime `'a` of the loan. After the loan expires,
-the restrictions no longer apply.
+via this alias. They will be enforced for the lifetime `'a` of the
+loan. After the loan expires, the restrictions no longer apply.
 
 The second restriction on `*x` is interesting because it does not
 apply to the path that was lent (`(*x).f`) but rather to a prefix of
@@ -188,11 +187,9 @@ struct defined in `middle::borrowck`. Formally, we define `LOAN` as
   against mutating `lv`;
 - *moves*: illegal if there is any in-scope restriction on `lv` at all;
 - *mutable borrows* (`&mut lv`): illegal there is an in-scope restriction
-  against mutating `lv` or aliasing `lv`;
+  against claiming `lv`;
 - *immutable borrows* (`&lv`): illegal there is an in-scope restriction
-  against freezing `lv` or aliasing `lv`;
-- *read-only borrows* (`&const lv`): illegal there is an in-scope restriction
-  against aliasing `lv`.
+  against freezing `lv`.
 
 ## Formal rules
 
@@ -238,19 +235,23 @@ struct defined in `middle::borrowck`. Formally, we define `LOAN` as
 We start with the `gather_loans` pass, which walks the AST looking for
 borrows.  For each borrow, there are three bits of information: the
 lvalue `LV` being borrowed and the mutability `MQ` and lifetime `LT`
-of the resulting pointer. Given those, `gather_loans` applies three
+of the resulting pointer. Given those, `gather_loans` applies four
 validity tests:
 
 1. `MUTABILITY(LV, MQ)`: The mutability of the reference is
 compatible with the mutability of `LV` (i.e., not borrowing immutable
 data as mutable).
 
-2. `LIFETIME(LV, LT, MQ)`: The lifetime of the borrow does not exceed
+2. `ALIASABLE(LV, MQ)`: The aliasability of the reference is
+compatible with the aliasability of `LV`. The goal is to prevent
+`&mut` borrows of aliasability data.
+
+3. `LIFETIME(LV, LT, MQ)`: The lifetime of the borrow does not exceed
 the lifetime of the value being borrowed. This pass is also
 responsible for inserting root annotations to keep managed values
 alive.
 
-3. `RESTRICTIONS(LV, LT, ACTIONS) = RS`: This pass checks and computes the
+4. `RESTRICTIONS(LV, LT, ACTIONS) = RS`: This pass checks and computes the
 restrictions to maintain memory safety. These are the restrictions
 that will go into the final loan. We'll discuss in more detail below.
 
@@ -313,6 +314,47 @@ struct defined in `middle::borrowck`. Formally, we define `LOAN` as
     MUTABILITY(*LV, MQ)                 // M-Deref-Borrowed-Mut
       TYPE(LV) = &mut Ty
 
+## Checking aliasability
+
+The goal of the aliasability check is to ensure that we never permit
+`&mut` borrows of aliasable data. Formally we define a predicate
+`ALIASABLE(LV, MQ)` which if defined means that
+"borrowing `LV` with mutability `MQ` is ok". The
+Rust code corresponding to this predicate is the function
+`check_aliasability()` in `middle::borrowck::gather_loans`.
+
+### Checking aliasability of variables
+
+Local variables are never aliasable as they are accessible only within
+the stack frame.
+
+    ALIASABLE(X, MQ)                   // M-Var-Mut
+
+### Checking aliasable of owned content
+
+Owned content is aliasable if it is found in an aliasable location:
+
+    ALIASABLE(LV.f, MQ)                // M-Field
+      ALIASABLE(LV, MQ)
+
+    ALIASABLE(*LV, MQ)                 // M-Deref-Unique
+      ALIASABLE(LV, MQ)
+
+### Checking mutability of immutable pointer types
+
+Immutable pointer types like `&T` are aliasable, and hence can only be
+borrowed immutably:
+
+    ALIASABLE(*LV, imm)                // M-Deref-Borrowed-Imm
+      TYPE(LV) = &Ty
+
+### Checking mutability of mutable pointer types
+
+`&mut T` can be frozen, so it is acceptable to borrow it as either imm or mut:
+
+    ALIASABLE(*LV, MQ)                 // M-Deref-Borrowed-Mut
+      TYPE(LV) = &mut Ty
+
 ## Checking lifetime
 
 These rules aim to ensure that no data is borrowed for a scope that exceeds

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

@@ -460,6 +460,11 @@ pub fn guarantee_valid(&mut self,
             return; // reported an error, no sense in reporting more.
         }
 
+        // Check that we don't allow mutable borrows of aliasable data.
+        if check_aliasability(self.bccx, borrow_span, cmt, req_mutbl).is_err() {
+            return; // reported an error, no sense in reporting more.
+        }
+
         // Compute the restrictions that are required to enforce the
         // loan is safe.
         let restr = restrictions::compute_restrictions(
@@ -586,15 +591,53 @@ fn check_mutability(bccx: &BorrowckCtxt,
                 }
             }
         }
+
+        fn check_aliasability(bccx: &BorrowckCtxt,
+                              borrow_span: Span,
+                              cmt: mc::cmt,
+                              req_mutbl: LoanMutability) -> Result<(),()> {
+            //! Implements the A-* rules in doc.rs.
+
+            match req_mutbl {
+                ImmutableMutability => {
+                    // both imm and mut data can be lent as imm;
+                    // for mutable data, this is a freeze
+                    Ok(())
+                }
+
+                MutableMutability => {
+                    // Check for those cases where we cannot control
+                    // the aliasing and make sure that we are not
+                    // being asked to.
+                    match cmt.freely_aliasable() {
+                        None => {
+                            Ok(())
+                        }
+                        Some(mc::AliasableStaticMut) => {
+                            // This is nasty, but we ignore the
+                            // aliasing rules if the data is based in
+                            // a `static mut`, since those are always
+                            // unsafe. At your own peril and all that.
+                            Ok(())
+                        }
+                        Some(cause) => {
+                            bccx.report_aliasability_violation(
+                                borrow_span,
+                                BorrowViolation,
+                                cause);
+                            Err(())
+                        }
+                    }
+                }
+            }
+        }
     }
 
     pub fn restriction_set(&self, req_mutbl: LoanMutability)
                            -> RestrictionSet {
         match req_mutbl {
-            ImmutableMutability => RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM,
-            MutableMutability => {
-                RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM | RESTR_FREEZE
-            }
+            ImmutableMutability => RESTR_MUTATE | RESTR_CLAIM,
+            MutableMutability => RESTR_MUTATE | RESTR_CLAIM | RESTR_FREEZE,
         }
     }
 

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

@@ -53,16 +53,6 @@ impl<'a> RestrictionsContext<'a> {
     fn restrict(&self,
                 cmt: mc::cmt,
                 restrictions: RestrictionSet) -> RestrictionResult {
-
-        // Check for those cases where we cannot control the aliasing
-        // and make sure that we are not being asked to.
-        match cmt.freely_aliasable() {
-            None => {}
-            Some(cause) => {
-                self.check_aliasing_permitted(cause, restrictions);
-            }
-        }
-
         match cmt.cat {
             mc::cat_rvalue(..) => {
                 // Effectively, rvalues are stored into a
@@ -179,28 +169,4 @@ fn extend(&self,
             }
         }
     }
-
-    fn check_aliasing_permitted(&self,
-                                cause: mc::AliasableReason,
-                                restrictions: RestrictionSet) {
-        //! This method is invoked when the current `cmt` is something
-        //! where aliasing cannot be controlled. It reports an error if
-        //! the restrictions required that it not be aliased; currently
-        //! this only occurs when re-borrowing an `&mut` pointer.
-        //!
-        //! NB: To be 100% consistent, we should report an error if
-        //! RESTR_FREEZE is found, because we cannot prevent freezing,
-        //! nor would we want to. However, we do not report such an
-        //! error, because this restriction only occurs when the user
-        //! is creating an `&mut` pointer to immutable or read-only
-        //! data, and there is already another piece of code that
-        //! checks for this condition.
-
-        if restrictions.intersects(RESTR_ALIAS) {
-            self.bccx.report_aliasability_violation(
-                self.span,
-                BorrowViolation,
-                cause);
-        }
-    }
 }

src/librustc/middle/borrowck/mod.rs

@@ -316,7 +316,6 @@ pub fn opt_loan_path(cmt: mc::cmt) -> Option<@LoanPath> {
 // - `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,
@@ -336,7 +335,6 @@ pub struct RestrictionSet {
 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};
 
 impl RestrictionSet {
     pub fn intersects(&self, restr: RestrictionSet) -> bool {
@@ -661,8 +659,8 @@ pub fn report_aliasability_violation(&self,
                                          kind: AliasableViolationKind,
                                          cause: mc::AliasableReason) {
         let prefix = match kind {
-            MutabilityViolation => "cannot assign to an `&mut`",
-            BorrowViolation => "cannot borrow an `&mut`"
+            MutabilityViolation => "cannot assign to data",
+            BorrowViolation => "cannot borrow data mutably"
         };
 
         match cause {
@@ -671,17 +669,21 @@ pub fn report_aliasability_violation(&self,
                     span,
                     format!("{} in an aliasable location", prefix));
             }
+            mc::AliasableStatic |
+            mc::AliasableStaticMut => {
+                self.tcx.sess.span_err(
+                    span,
+                    format!("{} in a static location", prefix));
+            }
             mc::AliasableManaged => {
-                self.tcx.sess.span_err(span, format!("{} in a `@` pointer",
-                                                     prefix))
+                self.tcx.sess.span_err(
+                    span,
+                    format!("{} in a `@` pointer", prefix));
             }
-            mc::AliasableBorrowed(m) => {
+            mc::AliasableBorrowed(_) => {
                 self.tcx.sess.span_err(
                     span,
-                    format!("{} in a `&{}` pointer; \
-                          try an `&mut` instead",
-                         prefix,
-                         self.mut_to_keyword(m)));
+                    format!("{} in a `&` reference", prefix));
             }
         }
     }

src/librustc/middle/mem_categorization.rs

@@ -1100,7 +1100,9 @@ pub fn field_mutbl(tcx: ty::ctxt,
 pub enum AliasableReason {
     AliasableManaged,
     AliasableBorrowed(ast::Mutability),
-    AliasableOther
+    AliasableOther,
+    AliasableStatic,
+    AliasableStaticMut,
 }
 
 impl cmt_ {
@@ -1130,10 +1132,6 @@ pub fn guarantor(self) -> cmt {
         }
     }
 
-    pub fn is_freely_aliasable(&self) -> bool {
-        self.freely_aliasable().is_some()
-    }
-
     pub fn freely_aliasable(&self) -> Option<AliasableReason> {
         /*!
          * Returns `Some(_)` if this lvalue represents a freely aliasable
@@ -1145,20 +1143,36 @@ pub fn freely_aliasable(&self) -> Option<AliasableReason> {
         // aliased and eventually recused.
 
         match self.cat {
+            cat_deref(b, _, region_ptr(MutMutable, _)) |
+            cat_downcast(b) |
+            cat_stack_upvar(b) |
+            cat_deref(b, _, uniq_ptr) |
+            cat_interior(b, _) |
+            cat_discr(b, _) => {
+                // Aliasability depends on base cmt
+                b.freely_aliasable()
+            }
+
             cat_copied_upvar(CopiedUpvar {onceness: ast::Once, ..}) |
             cat_rvalue(..) |
             cat_local(..) |
             cat_arg(_) |
-            cat_deref(_, _, unsafe_ptr(..)) | // of course it is aliasable, but...
-            cat_deref(_, _, region_ptr(MutMutable, _)) => {
+            cat_deref(_, _, unsafe_ptr(..)) => { // yes, it's aliasable, but...
                 None
             }
 
-            cat_copied_upvar(CopiedUpvar {onceness: ast::Many, ..}) |
-            cat_static_item(..) => {
+            cat_copied_upvar(CopiedUpvar {onceness: ast::Many, ..}) => {
                 Some(AliasableOther)
             }
 
+            cat_static_item(..) => {
+                if self.mutbl.is_mutable() {
+                    Some(AliasableStaticMut)
+                } else {
+                    Some(AliasableStatic)
+                }
+            }
+
             cat_deref(_, _, gc_ptr) => {
                 Some(AliasableManaged)
             }
@@ -1166,14 +1180,6 @@ pub fn freely_aliasable(&self) -> Option<AliasableReason> {
             cat_deref(_, _, region_ptr(m @ MutImmutable, _)) => {
                 Some(AliasableBorrowed(m))
             }
-
-            cat_downcast(..) |
-            cat_stack_upvar(..) |
-            cat_deref(_, _, uniq_ptr) |
-            cat_interior(..) |
-            cat_discr(..) => {
-                None
-            }
         }
     }
 }

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

@@ -27,5 +27,10 @@ fn foo3(t0: &mut &mut int) {
     **t1 = 22;
 }
 
+fn foo4(t0: & &mut int) {
+    let x:  &mut int = &mut **t0; //~ ERROR cannot borrow
+    *x += 1;
+}
+
 fn main() {
 }