auto merge of #8797 : nikomatsakis/rust/issue-8625-assign-to-andmut-in-borrowed-loc-2, r=pcwalton

Fixes for #8625 to prevent assigning to `&mut` in borrowed or aliasable locations. The old code was insufficient in that it failed to catch bizarre cases like `& &mut &mut`. 

r? @pnkfelix

src/librustc/middle/astencode.rs

@@ -383,7 +383,7 @@ fn tr(&self, xcx: @ExtendedDecodeContext) -> ast::def {
             ast::def_method(did0.tr(xcx), did1.map(|did1| did1.tr(xcx)))
           }
           ast::def_self_ty(nid) => { ast::def_self_ty(xcx.tr_id(nid)) }
-          ast::def_self(nid, i) => { ast::def_self(xcx.tr_id(nid), i) }
+          ast::def_self(nid) => { ast::def_self(xcx.tr_id(nid)) }
           ast::def_mod(did) => { ast::def_mod(did.tr(xcx)) }
           ast::def_foreign_mod(did) => { ast::def_foreign_mod(did.tr(xcx)) }
           ast::def_static(did, m) => { ast::def_static(did.tr(xcx), m) }

src/librustc/middle/borrowck/check_loans.rs

@@ -367,7 +367,6 @@ fn mark_variable_as_used_mut(this: &CheckLoanCtxt,
 
                     mc::cat_rvalue(*) |
                     mc::cat_static_item |
-                    mc::cat_implicit_self |
                     mc::cat_copied_upvar(*) |
                     mc::cat_deref(_, _, mc::unsafe_ptr(*)) |
                     mc::cat_deref(_, _, mc::gc_ptr(*)) |
@@ -406,15 +405,7 @@ fn check_for_aliasable_mutable_writes(this: &CheckLoanCtxt,
                 mc::cat_deref(b, _, mc::region_ptr(m_mutbl, _)) => {
                     // Statically prohibit writes to `&mut` when aliasable
 
-                    match b.freely_aliasable() {
-                        None => {}
-                        Some(cause) => {
-                            this.bccx.report_aliasability_violation(
-                                expr.span,
-                                MutabilityViolation,
-                                cause);
-                        }
-                    }
+                    check_for_aliasability_violation(this, expr, b);
                 }
 
                 mc::cat_deref(_, deref_count, mc::gc_ptr(ast::m_mutbl)) => {
@@ -434,6 +425,51 @@ fn check_for_aliasable_mutable_writes(this: &CheckLoanCtxt,
             return true; // no errors reported
         }
 
+        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(m_mutbl, _)) |
+                    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_self(*) |
+                    mc::cat_deref(_, _, mc::unsafe_ptr(*)) |
+                    mc::cat_static_item(*) |
+                    mc::cat_deref(_, _, mc::gc_ptr(_)) |
+                    mc::cat_deref(_, _, mc::region_ptr(m_const, _)) |
+                    mc::cat_deref(_, _, mc::region_ptr(m_imm, _)) => {
+                        // 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;
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
         fn check_for_assignment_to_restricted_or_frozen_location(
             this: &CheckLoanCtxt,
             expr: @ast::expr,
@@ -511,6 +547,12 @@ fn check_for_assignment_to_restricted_or_frozen_location(
             // path, and check that the super path was not lent out as
             // mutable or immutable (a const loan is ok).
             //
+            // Mutability of a path can be dependent on the super path
+            // in two ways. First, it might be inherited mutability.
+            // Second, the pointee of an `&mut` pointer can only be
+            // mutated if it is found in an unaliased location, so we
+            // have to check that the owner location is not borrowed.
+            //
             // Note that we are *not* checking for any and all
             // restrictions.  We are only interested in the pointers
             // that the user created, whereas we add restrictions for
@@ -528,9 +570,12 @@ fn check_for_assignment_to_restricted_or_frozen_location(
             let mut loan_path = loan_path;
             loop {
                 match *loan_path {
-                    // Peel back one layer if `loan_path` has
-                    // inherited mutability
-                    LpExtend(lp_base, mc::McInherited, _) => {
+                    // Peel back one layer if, for `loan_path` to be
+                    // mutable, `lp_base` must be mutable. This occurs
+                    // with inherited mutability and with `&mut`
+                    // pointers.
+                    LpExtend(lp_base, mc::McInherited, _) |
+                    LpExtend(lp_base, _, LpDeref(mc::region_ptr(ast::m_mutbl, _))) => {
                         loan_path = lp_base;
                     }
 

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

@@ -100,7 +100,6 @@ fn check_is_legal_to_move_from(bccx: @BorrowckCtxt,
                                cmt0: mc::cmt,
                                cmt: mc::cmt) -> bool {
     match cmt.cat {
-        mc::cat_implicit_self(*) |
         mc::cat_deref(_, _, mc::region_ptr(*)) |
         mc::cat_deref(_, _, mc::gc_ptr(*)) |
         mc::cat_deref(_, _, mc::unsafe_ptr(*)) => {

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

@@ -68,7 +68,6 @@ fn check(&self, cmt: mc::cmt, discr_scope: Option<ast::NodeId>) {
 
         match cmt.cat {
             mc::cat_rvalue(*) |
-            mc::cat_implicit_self |
             mc::cat_copied_upvar(*) |                  // L-Local
             mc::cat_local(*) |                         // L-Local
             mc::cat_arg(*) |                           // L-Local
@@ -301,7 +300,6 @@ fn is_moved(&self, cmt: mc::cmt) -> bool {
             }
             mc::cat_rvalue(*) |
             mc::cat_static_item |
-            mc::cat_implicit_self |
             mc::cat_copied_upvar(*) |
             mc::cat_deref(*) => {
                 false
@@ -328,7 +326,6 @@ fn scope(&self, cmt: mc::cmt) -> ty::Region {
             mc::cat_rvalue(cleanup_scope_id) => {
                 ty::re_scope(cleanup_scope_id)
             }
-            mc::cat_implicit_self |
             mc::cat_copied_upvar(_) => {
                 ty::re_scope(self.item_scope_id)
             }

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

@@ -101,7 +101,7 @@ fn restrict(&self,
                 self.extend(result, cmt.mutbl, LpInterior(i), restrictions)
             }
 
-            mc::cat_deref(cmt_base, _, mc::uniq_ptr) => {
+            mc::cat_deref(cmt_base, _, pk @ mc::uniq_ptr) => {
                 // R-Deref-Send-Pointer
                 //
                 // When we borrow the interior of an owned pointer, we
@@ -110,12 +110,11 @@ fn restrict(&self,
                 let result = self.restrict(
                     cmt_base,
                     restrictions | RESTR_MUTATE | RESTR_CLAIM);
-                self.extend(result, cmt.mutbl, LpDeref, restrictions)
+                self.extend(result, cmt.mutbl, LpDeref(pk), restrictions)
             }
 
             mc::cat_copied_upvar(*) | // FIXME(#2152) allow mutation of upvars
             mc::cat_static_item(*) |
-            mc::cat_implicit_self(*) |
             mc::cat_deref(_, _, mc::region_ptr(m_imm, _)) |
             mc::cat_deref(_, _, mc::gc_ptr(m_imm)) => {
                 // R-Deref-Imm-Borrowed
@@ -129,7 +128,7 @@ fn restrict(&self,
                 Safe
             }
 
-            mc::cat_deref(cmt_base, _, mc::gc_ptr(m_mutbl)) => {
+            mc::cat_deref(cmt_base, _, pk @ mc::gc_ptr(m_mutbl)) => {
                 // R-Deref-Managed-Borrowed
                 //
                 // Technically, no restrictions are *necessary* here.
@@ -170,14 +169,14 @@ fn restrict(&self,
                 match opt_loan_path(cmt_base) {
                     None => Safe,
                     Some(lp_base) => {
-                        let lp = @LpExtend(lp_base, cmt.mutbl, LpDeref);
+                        let lp = @LpExtend(lp_base, cmt.mutbl, LpDeref(pk));
                         SafeIf(lp, ~[Restriction {loan_path: lp,
                                                   set: restrictions}])
                     }
                 }
             }
 
-            mc::cat_deref(cmt_base, _, mc::region_ptr(m_mutbl, _)) => {
+            mc::cat_deref(cmt_base, _, pk @ mc::region_ptr(m_mutbl, _)) => {
                 // Because an `&mut` pointer does not inherit its
                 // mutability, we can only prevent mutation or prevent
                 // freezing if it is not aliased. Therefore, in such
@@ -187,7 +186,7 @@ fn restrict(&self,
                     let result = self.restrict(
                         cmt_base,
                         RESTR_ALIAS | RESTR_MUTATE | RESTR_CLAIM);
-                    self.extend(result, cmt.mutbl, LpDeref, restrictions)
+                    self.extend(result, cmt.mutbl, LpDeref(pk), restrictions)
                 } else {
                     // R-Deref-Mut-Borrowed-2
                     Safe

src/librustc/middle/borrowck/mod.rs

@@ -261,7 +261,7 @@ pub enum LoanPath {
 
 #[deriving(Eq, IterBytes)]
 pub enum LoanPathElem {
-    LpDeref,                     // `*LV` in doc.rs
+    LpDeref(mc::PointerKind),    // `*LV` in doc.rs
     LpInterior(mc::InteriorKind) // `LV.f` in doc.rs
 }
 
@@ -284,8 +284,7 @@ pub fn opt_loan_path(cmt: mc::cmt) -> Option<@LoanPath> {
     match cmt.cat {
         mc::cat_rvalue(*) |
         mc::cat_static_item |
-        mc::cat_copied_upvar(_) |
-        mc::cat_implicit_self => {
+        mc::cat_copied_upvar(_) => {
             None
         }
 
@@ -295,9 +294,9 @@ pub fn opt_loan_path(cmt: mc::cmt) -> Option<@LoanPath> {
             Some(@LpVar(id))
         }
 
-        mc::cat_deref(cmt_base, _, _) => {
+        mc::cat_deref(cmt_base, _, pk) => {
             do opt_loan_path(cmt_base).map_move |lp| {
-                @LpExtend(lp, cmt.mutbl, LpDeref)
+                @LpExtend(lp, cmt.mutbl, LpDeref(pk))
             }
         }
 
@@ -728,7 +727,7 @@ pub fn append_loan_path_to_str_from_interior(&self,
                                                  loan_path: &LoanPath,
                                                  out: &mut ~str) {
         match *loan_path {
-            LpExtend(_, _, LpDeref) => {
+            LpExtend(_, _, LpDeref(_)) => {
                 out.push_char('(');
                 self.append_loan_path_to_str(loan_path, out);
                 out.push_char(')');
@@ -776,7 +775,7 @@ pub fn append_loan_path_to_str(&self,
                 out.push_str("[]");
             }
 
-            LpExtend(lp_base, _, LpDeref) => {
+            LpExtend(lp_base, _, LpDeref(_)) => {
                 out.push_char('*');
                 self.append_loan_path_to_str(lp_base, out);
             }
@@ -854,7 +853,7 @@ fn repr(&self, tcx: ty::ctxt) -> ~str {
                 fmt!("$(%?)", id)
             }
 
-            &LpExtend(lp, _, LpDeref) => {
+            &LpExtend(lp, _, LpDeref(_)) => {
                 fmt!("%s.*", lp.repr(tcx))
             }
 

src/librustc/middle/mem_categorization.rs

@@ -59,18 +59,17 @@
 
 #[deriving(Eq)]
 pub enum categorization {
-    cat_rvalue(ast::NodeId),          // temporary val, argument is its scope
+    cat_rvalue(ast::NodeId),           // temporary val, argument is its scope
     cat_static_item,
-    cat_implicit_self,
     cat_copied_upvar(CopiedUpvar),     // upvar copied into @fn or ~fn env
     cat_stack_upvar(cmt),              // by ref upvar from &fn
-    cat_local(ast::NodeId),           // local variable
-    cat_arg(ast::NodeId),             // formal argument
-    cat_deref(cmt, uint, ptr_kind),    // deref of a ptr
+    cat_local(ast::NodeId),            // local variable
+    cat_arg(ast::NodeId),              // formal argument
+    cat_deref(cmt, uint, PointerKind), // deref of a ptr
     cat_interior(cmt, InteriorKind),   // something interior: field, tuple, etc
     cat_downcast(cmt),                 // selects a particular enum variant (*)
-    cat_discr(cmt, ast::NodeId),      // match discriminant (see preserve())
-    cat_self(ast::NodeId),            // explicit `self`
+    cat_discr(cmt, ast::NodeId),       // match discriminant (see preserve())
+    cat_self(ast::NodeId),             // explicit `self`
 
     // (*) downcast is only required if the enum has more than one variant
 }
@@ -82,8 +81,8 @@ pub struct CopiedUpvar {
 }
 
 // different kinds of pointers:
-#[deriving(Eq)]
-pub enum ptr_kind {
+#[deriving(Eq, IterBytes)]
+pub enum PointerKind {
     uniq_ptr,
     gc_ptr(ast::mutability),
     region_ptr(ast::mutability, ty::Region),
@@ -147,7 +146,7 @@ pub struct cmt_ {
 // We pun on *T to mean both actual deref of a ptr as well
 // as accessing of components:
 pub enum deref_kind {
-    deref_ptr(ptr_kind),
+    deref_ptr(PointerKind),
     deref_interior(InteriorKind),
 }
 
@@ -493,17 +492,11 @@ pub fn cat_def(&self,
             }
           }
 
-          ast::def_self(self_id, is_implicit) => {
-            let cat = if is_implicit {
-                cat_implicit_self
-            } else {
-                cat_self(self_id)
-            };
-
+          ast::def_self(self_id) => {
             @cmt_ {
                 id:id,
                 span:span,
-                cat:cat,
+                cat:cat_self(self_id),
                 mutbl: McImmutable,
                 ty:expr_ty
             }
@@ -1016,9 +1009,6 @@ pub fn cmt_to_str(&self, cmt: cmt) -> ~str {
           cat_static_item => {
               ~"static item"
           }
-          cat_implicit_self => {
-              ~"self reference"
-          }
           cat_copied_upvar(_) => {
               ~"captured outer variable in a heap closure"
           }
@@ -1121,7 +1111,6 @@ pub fn guarantor(@self) -> cmt {
         match self.cat {
             cat_rvalue(*) |
             cat_static_item |
-            cat_implicit_self |
             cat_copied_upvar(*) |
             cat_local(*) |
             cat_self(*) |
@@ -1146,9 +1135,10 @@ pub fn is_freely_aliasable(&self) -> bool {
     }
 
     pub fn freely_aliasable(&self) -> Option<AliasableReason> {
-        //! True if this lvalue resides in an area that is
-        //! freely aliasable, meaning that rustc cannot track
-        //! the alias//es with precision.
+        /*!
+         * Returns `Some(_)` if this lvalue represents a freely aliasable
+         * pointer type.
+         */
 
         // Maybe non-obvious: copied upvars can only be considered
         // non-aliasable in once closures, since any other kind can be
@@ -1166,8 +1156,7 @@ pub fn freely_aliasable(&self) -> Option<AliasableReason> {
             }
 
             cat_copied_upvar(CopiedUpvar {onceness: ast::Many, _}) |
-            cat_static_item(*) |
-            cat_implicit_self(*) => {
+            cat_static_item(*) => {
                 Some(AliasableOther)
             }
 
@@ -1180,12 +1169,12 @@ pub fn freely_aliasable(&self) -> Option<AliasableReason> {
                 Some(AliasableBorrowed(m))
             }
 
-            cat_downcast(b) |
-            cat_stack_upvar(b) |
-            cat_deref(b, _, uniq_ptr) |
-            cat_interior(b, _) |
-            cat_discr(b, _) => {
-                b.freely_aliasable()
+            cat_downcast(*) |
+            cat_stack_upvar(*) |
+            cat_deref(_, _, uniq_ptr) |
+            cat_interior(*) |
+            cat_discr(*) => {
+                None
             }
         }
     }
@@ -1205,7 +1194,6 @@ impl Repr for categorization {
     fn repr(&self, tcx: ty::ctxt) -> ~str {
         match *self {
             cat_static_item |
-            cat_implicit_self |
             cat_rvalue(*) |
             cat_copied_upvar(*) |
             cat_local(*) |
@@ -1233,7 +1221,7 @@ fn repr(&self, tcx: ty::ctxt) -> ~str {
     }
 }
 
-pub fn ptr_sigil(ptr: ptr_kind) -> ~str {
+pub fn ptr_sigil(ptr: PointerKind) -> ~str {
     match ptr {
         uniq_ptr => ~"~",
         gc_ptr(_) => ~"@",

src/librustc/middle/moves.rs

@@ -228,7 +228,7 @@ pub fn moved_variable_node_id_from_def(def: def) -> Option<NodeId> {
       def_binding(nid, _) |
       def_arg(nid, _) |
       def_local(nid, _) |
-      def_self(nid, _) => Some(nid),
+      def_self(nid) => Some(nid),
 
       _ => None
     }

src/librustc/middle/resolve.rs

@@ -125,7 +125,7 @@ pub enum Mutability {
 
 pub enum SelfBinding {
     NoSelfBinding,
-    HasSelfBinding(NodeId, bool /* is implicit */)
+    HasSelfBinding(NodeId)
 }
 
 struct ResolveVisitor {
@@ -3771,9 +3771,8 @@ pub fn resolve_function(@mut self,
                 NoSelfBinding => {
                     // Nothing to do.
                 }
-                HasSelfBinding(self_node_id, is_implicit) => {
-                    let def_like = dl_def(def_self(self_node_id,
-                                                   is_implicit));
+                HasSelfBinding(self_node_id) => {
+                    let def_like = dl_def(def_self(self_node_id));
                     *function_value_rib.self_binding = Some(def_like);
                 }
             }
@@ -3917,7 +3916,7 @@ pub fn resolve_method(@mut self,
         // we only have self ty if it is a non static method
         let self_binding = match method.explicit_self.node {
           sty_static => { NoSelfBinding }
-          _ => { HasSelfBinding(method.self_id, false) }
+          _ => { HasSelfBinding(method.self_id) }
         };
 
         self.resolve_function(rib_kind,

src/librustc/middle/trans/expr.rs

@@ -1066,7 +1066,7 @@ pub fn trans_local_var(bcx: @mut Block, def: ast::def) -> Datum {
         ast::def_local(nid, _) | ast::def_binding(nid, _) => {
             take_local(bcx, bcx.fcx.lllocals, nid)
         }
-        ast::def_self(nid, _) => {
+        ast::def_self(nid) => {
             let self_info: ValSelfData = match bcx.fcx.llself {
                 Some(ref self_info) => *self_info,
                 None => {

src/librustc/middle/typeck/check/mod.rs

@@ -3093,7 +3093,7 @@ pub fn ty_param_bounds_and_ty_for_def(fcx: @mut FnCtxt,
                                       defn: ast::def)
                                    -> ty_param_bounds_and_ty {
     match defn {
-      ast::def_arg(nid, _) | ast::def_local(nid, _) | ast::def_self(nid, _) |
+      ast::def_arg(nid, _) | ast::def_local(nid, _) | ast::def_self(nid) |
       ast::def_binding(nid, _) => {
           let typ = fcx.local_ty(sp, nid);
           return no_params(typ);

src/librustc/middle/typeck/check/regionck.rs

@@ -58,7 +58,7 @@ fn encl_region_of_def(fcx: @mut FnCtxt, def: ast::def) -> ty::Region {
     let tcx = fcx.tcx();
     match def {
         def_local(node_id, _) | def_arg(node_id, _) |
-        def_self(node_id, _) | def_binding(node_id, _) => {
+        def_self(node_id) | def_binding(node_id, _) => {
             tcx.region_maps.encl_region(node_id)
         }
         def_upvar(_, subdef, closure_id, body_id) => {

src/libsyntax/ast.rs

@@ -171,7 +171,7 @@ pub enum def {
     def_static_method(/* method */ def_id,
                       /* trait */  Option<def_id>,
                       purity),
-    def_self(NodeId, bool /* is_implicit */),
+    def_self(NodeId),
     def_self_ty(/* trait id */ NodeId),
     def_mod(def_id),
     def_foreign_mod(def_id),

src/libsyntax/ast_util.rs

@@ -64,7 +64,7 @@ pub fn def_id_of_def(d: def) -> def_id {
       def_use(id) | def_struct(id) | def_trait(id) | def_method(id, _) => {
         id
       }
-      def_arg(id, _) | def_local(id, _) | def_self(id, _) | def_self_ty(id)
+      def_arg(id, _) | def_local(id, _) | def_self(id) | def_self_ty(id)
       | def_upvar(id, _, _, _) | def_binding(id, _) | def_region(id)
       | def_typaram_binder(id) | def_label(id) => {
         local_def(id)

src/test/compile-fail/borrowck-assign-to-andmut-in-aliasable-loc.rs

+// Copyright 2012 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.
+
+// Test that assignments to an `&mut` pointer which is found in a
+// borrowed (but otherwise non-aliasable) location is illegal.
+
+struct S<'self> {
+    pointer: &'self mut int
+}
+
+fn a(s: &S) {
+    *s.pointer += 1; //~ ERROR cannot assign
+}
+
+fn b(s: &mut S) {
+    *s.pointer += 1;
+}
+
+fn c(s: & &mut S) {
+    *s.pointer += 1; //~ ERROR cannot assign
+}
+
+fn main() {}

src/test/compile-fail/borrowck-assign-to-andmut-in-borrowed-loc.rs

+// Copyright 2012 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.
+
+// Test that assignments to an `&mut` pointer which is found in a
+// borrowed (but otherwise non-aliasable) location is illegal.
+
+struct S<'self> {
+    pointer: &'self mut int
+}
+
+fn copy_borrowed_ptr<'a>(p: &'a mut S<'a>) -> S<'a> {
+    S { pointer: &mut *p.pointer }
+}
+
+fn main() {
+    let mut x = 1;
+
+    {
+        let mut y = S { pointer: &mut x };
+        let z = copy_borrowed_ptr(&mut y);
+        *y.pointer += 1; //~ ERROR cannot assign
+        *z.pointer += 1;
+    }
+}