auto merge of #12117 : nikomatsakis/rust/issue-11913-borrow-in-aliasable-loc, r=pcwalton

Repair a rather embarassingly obvious hole that I created as part of #9629. In particular, prevent `&mut` borrows of data in an aliasable location. This used to be prevented through the restrictions mechanism, but in #9629 I modified those rules incorrectly. 

r? @pcwalton

Fixes #11913

Author: bors

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 mc = middle::mem_categorization;
 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,9 +219,8 @@ impl<'a> CheckLoanCtxt<'a> {
 
         // 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,
-            ConstMutability     => RESTR_ALIAS,
+            MutableMutability   => RESTR_FREEZE | RESTR_CLAIM,
+            ImmutableMutability => RESTR_FREEZE,
         };
         debug!("illegal_if={:?}", illegal_if);
 
@@ -424,7 +422,7 @@ impl<'a> CheckLoanCtxt<'a> {
             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);
@@ -438,43 +436,18 @@ impl<'a> CheckLoanCtxt<'a> {
 
         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;
                 }
             }
         }
@@ -598,8 +571,7 @@ impl<'a> CheckLoanCtxt<'a> {
 
                 // Check for a non-const loan of `loan_path`
                 let cont = this.each_in_scope_loan(expr.id, |loan| {
-                    if loan.loan_path == loan_path &&
-                            loan.mutbl != ConstMutability {
+                    if loan.loan_path == loan_path {
                         this.report_illegal_mutation(expr,
                                                      full_loan_path,
                                                      loan);

src/librustc/middle/borrowck/doc.rs

@@ -151,14 +151,13 @@ that the value `(*x).f` may be mutated via the newly created reference
 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 @@ The kinds of expressions which in-scope loans can render illegal are:
   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 @@ live. (This is done via restrictions, read on.)
 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 @@ be borrowed if MQ is immutable or const:
     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 @@ impl<'a> GatherLoanCtxt<'a> {
             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(
@@ -568,11 +573,6 @@ impl<'a> GatherLoanCtxt<'a> {
             //! Implements the M-* rules in doc.rs.
 
             match req_mutbl {
-                ConstMutability => {
-                    // Data of any mutability can be lent as const.
-                    Ok(())
-                }
-
                 ImmutableMutability => {
                     // both imm and mut data can be lent as imm;
                     // for mutable data, this is a freeze
@@ -591,16 +591,53 @@ impl<'a> GatherLoanCtxt<'a> {
                 }
             }
         }
+
+        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 {
-            ConstMutability => RESTR_EMPTY,
-            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 @@ impl<'a> RestrictionsContext<'a> {
             }
         }
     }
-
-    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);
-        }
-    }
 }