improve borrowck to handle some frankly rather tricky cases

- receivers of method calls are also borrowed
- by-val arguments are also borrowed (needs tests)
- assignment to components can interfere with loans

Author: nikomatsakis

src/rustc/middle/borrowck.rs

@@ -149,7 +149,7 @@ Borrowck results in two maps.
 "];
 
 import syntax::ast;
-import syntax::ast::{m_mutbl, m_imm, m_const};
+import syntax::ast::{mutability, m_mutbl, m_imm, m_const};
 import syntax::visit;
 import syntax::ast_util;
 import syntax::ast_map;
@@ -254,7 +254,8 @@ enum ptr_kind {uniq_ptr, gc_ptr, region_ptr, unsafe_ptr}
 // I am coining the term "components" to mean "pieces of a data
 // structure accessible without a dereference":
 enum comp_kind {comp_tuple, comp_res, comp_variant,
-                comp_field(str), comp_index(ty::t)}
+                comp_field(str, ast::mutability),
+                comp_index(ty::t, ast::mutability)}
 
 // We pun on *T to mean both actual deref of a ptr as well
 // as accessing of components:
@@ -422,8 +423,8 @@ impl to_str_methods for borrowck_ctxt {
 
     fn comp_to_repr(comp: comp_kind) -> str {
         alt comp {
-          comp_field(fld) { fld }
-          comp_index(_) { "[]" }
+          comp_field(fld, _) { fld }
+          comp_index(*) { "[]" }
           comp_tuple { "()" }
           comp_res { "<res>" }
           comp_variant { "<enum>" }
@@ -480,11 +481,11 @@ impl to_str_methods for borrowck_ctxt {
           cat_deref(_, _, pk) { #fmt["dereference of %s %s pointer",
                                      mut_str, self.pk_to_sigil(pk)] }
           cat_stack_upvar(_) { mut_str + " upvar" }
-          cat_comp(_, comp_field(_)) { mut_str + " field" }
+          cat_comp(_, comp_field(*)) { mut_str + " field" }
           cat_comp(_, comp_tuple) { "tuple content" }
           cat_comp(_, comp_res) { "resource content" }
           cat_comp(_, comp_variant) { "enum content" }
-          cat_comp(_, comp_index(t)) {
+          cat_comp(_, comp_index(t, _)) {
             alt ty::get(t).struct {
               ty::ty_vec(*) | ty::ty_evec(*) {
                 mut_str + " vec content"
@@ -522,3 +523,14 @@ impl to_str_methods for borrowck_ctxt {
         }
     }
 }
+
+// The inherent mutability of a component is its default mutability
+// assuming it is embedded in an immutable context.  In general, the
+// mutability can be "overridden" if the component is embedded in a
+// mutable structure.
+fn inherent_mutability(ck: comp_kind) -> mutability {
+    alt ck {
+      comp_tuple | comp_res | comp_variant {m_imm}
+      comp_field(_, m) | comp_index(_, m) {m}
+    }
+}

src/rustc/middle/borrowck/categorization.rs

@@ -30,43 +30,53 @@ then an index to jump forward to the relevant item.
 "];
 
 export public_methods;
+export opt_deref_kind;
 
 // Categorizes a derefable type.  Note that we include vectors and strings as
 // derefable (we model an index as the combination of a deref and then a
 // pointer adjustment).
-fn deref_kind(tcx: ty::ctxt, t: ty::t) -> deref_kind {
+fn opt_deref_kind(t: ty::t) -> option<deref_kind> {
     alt ty::get(t).struct {
       ty::ty_uniq(*) | ty::ty_vec(*) | ty::ty_str |
       ty::ty_evec(_, ty::vstore_uniq) |
       ty::ty_estr(ty::vstore_uniq) {
-        deref_ptr(uniq_ptr)
+        some(deref_ptr(uniq_ptr))
       }
 
       ty::ty_rptr(*) |
       ty::ty_evec(_, ty::vstore_slice(_)) |
       ty::ty_estr(ty::vstore_slice(_)) {
-        deref_ptr(region_ptr)
+        some(deref_ptr(region_ptr))
       }
 
       ty::ty_box(*) |
       ty::ty_evec(_, ty::vstore_box) |
       ty::ty_estr(ty::vstore_box) {
-        deref_ptr(gc_ptr)
+        some(deref_ptr(gc_ptr))
       }
 
       ty::ty_ptr(*) {
-        deref_ptr(unsafe_ptr)
+        some(deref_ptr(unsafe_ptr))
       }
 
       ty::ty_enum(*) {
-        deref_comp(comp_variant)
+        some(deref_comp(comp_variant))
       }
 
       ty::ty_res(*) {
-        deref_comp(comp_res)
+        some(deref_comp(comp_res))
       }
 
       _ {
+        none
+      }
+    }
+}
+
+fn deref_kind(tcx: ty::ctxt, t: ty::t) -> deref_kind {
+    alt opt_deref_kind(t) {
+      some(k) {k}
+      none {
         tcx.sess.bug(
             #fmt["deref_cat() invoked on non-derefable type %s",
                  ty_to_str(tcx, t)]);
@@ -281,11 +291,12 @@ impl public_methods for borrowck_ctxt {
           m_imm { base_cmt.mutbl } // imm: as mutable as the container
           m_mutbl | m_const { f_mutbl }
         };
+        let f_comp = comp_field(f_name, f_mutbl);
         let lp = base_cmt.lp.map { |lp|
-            @lp_comp(lp, comp_field(f_name))
+            @lp_comp(lp, f_comp)
         };
         @{id: node.id(), span: node.span(),
-          cat: cat_comp(base_cmt, comp_field(f_name)), lp:lp,
+          cat: cat_comp(base_cmt, f_comp), lp:lp,
           mutbl: m, ty: self.tcx.ty(node)}
     }
 
@@ -347,8 +358,8 @@ impl public_methods for borrowck_ctxt {
         let deref_lp = base_cmt.lp.map { |lp| @lp_deref(lp, ptr) };
         let deref_cmt = @{id:expr.id, span:expr.span,
                           cat:cat_deref(base_cmt, 0u, ptr), lp:deref_lp,
-                          mutbl:mt.mutbl, ty:mt.ty};
-        let comp = comp_index(base_cmt.ty);
+                          mutbl:m_imm, ty:mt.ty};
+        let comp = comp_index(base_cmt.ty, mt.mutbl);
         let index_lp = deref_lp.map { |lp| @lp_comp(lp, comp) };
         @{id:expr.id, span:expr.span,
           cat:cat_comp(deref_cmt, comp), lp:index_lp,

src/rustc/middle/borrowck/check_loans.rs

@@ -262,7 +262,7 @@ impl methods for check_loan_ctxt {
 
     fn is_self_field(cmt: cmt) -> bool {
         alt cmt.cat {
-          cat_comp(cmt_base, comp_field(_)) {
+          cat_comp(cmt_base, comp_field(*)) {
             alt cmt_base.cat {
               cat_special(sk_self) { true }
               _ { false }
@@ -314,31 +314,54 @@ impl methods for check_loan_ctxt {
         // which will be checked for compat separately in
         // check_for_conflicting_loans()
         if at != at_mutbl_ref {
-            let lp = alt cmt.lp {
-              none { ret; }
-              some(lp) { lp }
-            };
-            for self.walk_loans_of(ex.id, lp) { |loan|
-                alt loan.mutbl {
-                  m_mutbl | m_const { /*ok*/ }
-                  m_imm {
-                    self.bccx.span_err(
-                        ex.span,
-                        #fmt["%s prohibited due to outstanding loan",
-                             at.ing_form(self.bccx.cmt_to_str(cmt))]);
-                    self.bccx.span_note(
-                        loan.cmt.span,
-                        #fmt["loan of %s granted here",
-                             self.bccx.cmt_to_str(loan.cmt)]);
-                    ret;
-                  }
-                }
+            for cmt.lp.each { |lp|
+                self.check_for_loan_conflicting_with_assignment(
+                    at, ex, cmt, lp);
             }
         }
 
         self.bccx.add_to_mutbl_map(cmt);
     }
 
+    fn check_for_loan_conflicting_with_assignment(
+        at: assignment_type,
+        ex: @ast::expr,
+        cmt: cmt,
+        lp: @loan_path) {
+
+        for self.walk_loans_of(ex.id, lp) { |loan|
+            alt loan.mutbl {
+              m_mutbl | m_const { /*ok*/ }
+              m_imm {
+                self.bccx.span_err(
+                    ex.span,
+                    #fmt["%s prohibited due to outstanding loan",
+                         at.ing_form(self.bccx.cmt_to_str(cmt))]);
+                self.bccx.span_note(
+                    loan.cmt.span,
+                    #fmt["loan of %s granted here",
+                         self.bccx.cmt_to_str(loan.cmt)]);
+                ret;
+              }
+            }
+        }
+
+        // Subtle: if the mutability of the component being assigned
+        // is inherited from the thing that the component is embedded
+        // within, then we have to check whether that thing has been
+        // loaned out as immutable!  An example:
+        //    let mut x = {f: some(3)};
+        //    let y = &x; // x loaned out as immutable
+        //    x.f = none; // changes type of y.f, which appears to be imm
+        alt *lp {
+          lp_comp(lp_base, ck) if inherent_mutability(ck) != m_mutbl {
+            self.check_for_loan_conflicting_with_assignment(
+                at, ex, cmt, lp_base);
+          }
+          lp_comp(*) | lp_local(*) | lp_arg(*) | lp_deref(*) {}
+        }
+    }
+
     fn report_purity_error(pc: purity_cause, sp: span, msg: str) {
         alt pc {
           pc_pure_fn {

src/rustc/middle/borrowck/gather_loans.rs

@@ -6,7 +6,7 @@
 // their associated scopes.  In phase two, checking loans, we will then make
 // sure that all of these loans are honored.
 
-import categorization::public_methods;
+import categorization::{public_methods, opt_deref_kind};
 import loan::public_methods;
 import preserve::public_methods;
 
@@ -30,6 +30,8 @@ fn req_loans_in_expr(ex: @ast::expr,
     let bccx = self.bccx;
     let tcx = bccx.tcx;
 
+    #debug["req_loans_in_expr(ex=%s)", pprust::expr_to_str(ex)];
+
     // If this expression is borrowed, have to ensure it remains valid:
     for tcx.borrowings.find(ex.id).each { |borrow|
         let cmt = self.bccx.cat_borrow_of_expr(ex);
@@ -64,7 +66,39 @@ fn req_loans_in_expr(ex: @ast::expr,
                 let arg_cmt = self.bccx.cat_expr(arg);
                 self.guarantee_valid(arg_cmt, m_imm,  scope_r);
               }
-              ast::by_move | ast::by_copy | ast::by_val {}
+              ast::by_val {
+                // Rust's by-val does not actually give ownership to
+                // the callee.  This means that if a pointer type is
+                // passed, it is effectively a borrow, and so the
+                // caller must guarantee that the data remains valid.
+                //
+                // Subtle: we only guarantee that the pointer is valid
+                // and const.  Technically, we ought to pass in the
+                // mutability that the caller expects (e.g., if the
+                // formal argument has type @mut, we should guarantee
+                // validity and mutability, not validity and const).
+                // However, the type system already guarantees that
+                // the caller's mutability is compatible with the
+                // callee, so this is not necessary.  (Note that with
+                // actual borrows, typeck is more liberal and allows
+                // the pointer to be borrowed as immutable even if it
+                // is mutable in the caller's frame, thus effectively
+                // passing the buck onto us to enforce this)
+
+                alt opt_deref_kind(arg_ty.ty) {
+                  some(deref_ptr(region_ptr)) {
+                    /* region pointers are (by induction) guaranteed */
+                  }
+                  none {
+                    /* not a pointer, no worries */
+                  }
+                  some(_) {
+                    let arg_cmt = self.bccx.cat_borrow_of_expr(arg);
+                    self.guarantee_valid(arg_cmt, m_const, scope_r);
+                  }
+                }
+              }
+              ast::by_move | ast::by_copy {}
             }
         }
       }
@@ -78,6 +112,24 @@ fn req_loans_in_expr(ex: @ast::expr,
         }
       }
 
+      ast::expr_field(rcvr, _, _) |
+      ast::expr_binary(_, rcvr, _) |
+      ast::expr_unary(_, rcvr) if self.bccx.method_map.contains_key(ex.id) {
+        // Receivers in method calls are always passed by ref.
+        //
+        // FIXME--this scope is both too large and too small.  We make
+        // the scope the enclosing block, which surely includes any
+        // immediate call (a.b()) but which is too big.  OTOH, in the
+        // case of a naked field `a.b`, the value is copied
+        // anyhow. This is probably best fixed if we address the
+        // syntactic ambiguity.
+
+        // let scope_r = ty::re_scope(ex.id);
+        let scope_r = ty::re_scope(self.tcx().region_map.get(ex.id));
+        let rcvr_cmt = self.bccx.cat_expr(rcvr);
+        self.guarantee_valid(rcvr_cmt, m_imm, scope_r);
+      }
+
       _ { /*ok*/ }
     }
 

src/rustc/middle/borrowck/loan.rs

@@ -55,8 +55,8 @@ impl loan_methods for loan_ctxt {
           cat_discr(base, _) {
             self.loan(base, req_mutbl)
           }
-          cat_comp(cmt_base, comp_field(_)) |
-          cat_comp(cmt_base, comp_index(_)) |
+          cat_comp(cmt_base, comp_field(*)) |
+          cat_comp(cmt_base, comp_index(*)) |
           cat_comp(cmt_base, comp_tuple) |
           cat_comp(cmt_base, comp_res) {
             // For most components, the type of the embedded data is

src/rustc/middle/borrowck/preserve.rs

@@ -29,8 +29,8 @@ impl public_methods for borrowck_ctxt {
             // This is basically a deref of a region ptr.
             ok(())
           }
-          cat_comp(cmt_base, comp_field(_)) |
-          cat_comp(cmt_base, comp_index(_)) |
+          cat_comp(cmt_base, comp_field(*)) |
+          cat_comp(cmt_base, comp_index(*)) |
           cat_comp(cmt_base, comp_tuple) |
           cat_comp(cmt_base, comp_res) {
             // Most embedded components: if the base is stable, the

src/rustc/util/ppaux.rs

@@ -27,14 +27,18 @@ fn re_scope_id_to_str(cx: ctxt, node_id: ast::node_id) -> str {
       }
       ast_map::node_expr(expr) {
         alt expr.node {
-          ast::expr_call(_, _, _) {
+          ast::expr_call(*) {
             #fmt("<call at %s>",
                  codemap::span_to_str(expr.span, cx.sess.codemap))
           }
-          ast::expr_alt(_, _, _) {
+          ast::expr_alt(*) {
             #fmt("<alt at %s>",
                  codemap::span_to_str(expr.span, cx.sess.codemap))
           }
+          ast::expr_field(*) | ast::expr_unary(*) | ast::expr_binary(*) {
+            #fmt("<method at %s>",
+                 codemap::span_to_str(expr.span, cx.sess.codemap))
+          }
           _ { cx.sess.bug(
               #fmt["re_scope refers to %s",
                    ast_map::node_id_to_str(cx.items, node_id)]) }

src/test/compile-fail/borrowck-assign-comp-idx.rs

@@ -0,0 +1,38 @@
+// xfail-fast  (compile-flags unsupported on windows)
+// compile-flags:--borrowck=err
+
+type point = { x: int, y: int };
+
+fn a() {
+    let mut p = [mut 1];
+
+    // Create an immutable pointer into p's contents:
+    let _q: &int = &p[0]; //! NOTE loan of mutable vec content granted here
+
+    p[0] = 5; //! ERROR assigning to mutable vec content prohibited due to outstanding loan
+}
+
+fn borrow(_x: [int]/&, _f: fn()) {}
+
+fn b() {
+    // here we alias the mutable vector into an imm slice and try to
+    // modify the original:
+
+    let mut p = [mut 1];
+
+    borrow(p) {|| //! NOTE loan of mutable vec content granted here
+        p[0] = 5; //! ERROR assigning to mutable vec content prohibited due to outstanding loan
+    }
+}
+
+fn c() {
+    // Legal because the scope of the borrow does not include the
+    // modification:
+    let mut p = [mut 1];
+    borrow(p, {||});
+    p[0] = 5;
+}
+
+fn main() {
+}
+