Modify borrow checker to visit irrefutable patterns that appear in

let and function arguments; modify type checker to store type
information for all patterns and handles some missing cases.

Author: nikomatsakis

src/librustc/middle/borrowck/check_loans.rs

@@ -65,7 +65,7 @@ pub fn check_loans(bccx: @BorrowckCtxt,
 
 enum MoveError {
     MoveOk,
-    MoveWhileBorrowed(/*move*/@LoanPath, /*loan*/@LoanPath, /*loan*/span)
+    MoveWhileBorrowed(/*loan*/@LoanPath, /*loan*/span)
 }
 
 impl<'self> CheckLoanCtxt<'self> {
@@ -348,7 +348,7 @@ impl<'self> CheckLoanCtxt<'self> {
                         cmt = b;
                     }
 
-                    mc::cat_rvalue |
+                    mc::cat_rvalue(*) |
                     mc::cat_static_item |
                     mc::cat_implicit_self |
                     mc::cat_copied_upvar(*) |
@@ -547,45 +547,50 @@ impl<'self> CheckLoanCtxt<'self> {
                  self.bccx.loan_path_to_str(loan_path)));
     }
 
-    pub fn check_move_out_from_expr(&self, ex: @ast::expr) {
-        match ex.node {
-            ast::expr_paren(*) => {
-                /* In the case of an expr_paren(), the expression inside
-                 * the parens will also be marked as being moved.  Ignore
-                 * the parents then so as not to report duplicate errors. */
+    fn check_move_out_from_expr(&self, expr: @ast::expr) {
+        match expr.node {
+            ast::expr_fn_block(*) => {
+                // moves due to capture clauses are checked
+                // in `check_loans_in_fn`, so that we can
+                // give a better error message
             }
             _ => {
-                let cmt = self.bccx.cat_expr(ex);
-                match self.analyze_move_out_from_cmt(cmt) {
-                    MoveOk => {}
-                    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(move_path)));
-                        self.bccx.span_note(
-                            loan_span,
-                            fmt!("borrow of `%s` occurs here",
-                                 self.bccx.loan_path_to_str(loan_path)));
-                    }
+                self.check_move_out_from_id(expr.id, expr.span)
+            }
+        }
+    }
+
+    fn check_move_out_from_id(&self, id: ast::node_id, span: span) {
+        for self.move_data.each_path_moved_by(id) |_, move_path| {
+            match self.analyze_move_out_from(id, move_path) {
+                MoveOk => {}
+                MoveWhileBorrowed(loan_path, loan_span) => {
+                    self.bccx.span_err(
+                        span,
+                        fmt!("cannot move out of `%s` \
+                              because it is borrowed",
+                             self.bccx.loan_path_to_str(move_path)));
+                    self.bccx.span_note(
+                        loan_span,
+                        fmt!("borrow of `%s` occurs here",
+                             self.bccx.loan_path_to_str(loan_path)));
                 }
             }
         }
     }
 
-    pub fn analyze_move_out_from_cmt(&self, cmt: mc::cmt) -> MoveError {
-        debug!("analyze_move_out_from_cmt(cmt=%s)", cmt.repr(self.tcx()));
+    pub fn analyze_move_out_from(&self,
+                                 expr_id: ast::node_id,
+                                 move_path: @LoanPath) -> MoveError {
+        debug!("analyze_move_out_from(expr_id=%?, move_path=%s)",
+               expr_id, move_path.repr(self.tcx()));
 
         // FIXME(#4384) inadequare if/when we permit `move a.b`
 
         // check for a conflicting loan:
-        let r = opt_loan_path(cmt);
-        for r.iter().advance |&lp| {
-            for self.each_in_scope_restriction(cmt.id, lp) |loan, _| {
-                // Any restriction prevents moves.
-                return MoveWhileBorrowed(lp, loan.loan_path, loan.span);
-            }
+        for self.each_in_scope_restriction(expr_id, move_path) |loan, _| {
+            // Any restriction prevents moves.
+            return MoveWhileBorrowed(loan.loan_path, loan.span);
         }
 
         MoveOk
@@ -652,13 +657,11 @@ fn check_loans_in_fn<'a>(fk: &visit::fn_kind,
                                  closure_id: ast::node_id,
                                  cap_var: &moves::CaptureVar) {
             let var_id = ast_util::def_id_of_def(cap_var.def).node;
-            let ty = ty::node_id_to_type(this.tcx(), var_id);
-            let cmt = this.bccx.cat_def(closure_id, cap_var.span,
-                                        ty, cap_var.def);
-            let move_err = this.analyze_move_out_from_cmt(cmt);
+            let move_path = @LpVar(var_id);
+            let move_err = this.analyze_move_out_from(closure_id, move_path);
             match move_err {
                 MoveOk => {}
-                MoveWhileBorrowed(move_path, loan_path, loan_span) => {
+                MoveWhileBorrowed(loan_path, loan_span) => {
                     this.bccx.span_err(
                         cap_var.span,
                         fmt!("cannot move `%s` into closure \
@@ -689,10 +692,7 @@ fn check_loans_in_expr<'a>(expr: @ast::expr,
            expr.repr(this.tcx()));
 
     this.check_for_conflicting_loans(expr.id);
-
-    if this.bccx.moves_map.contains(&expr.id) {
-        this.check_move_out_from_expr(expr);
-    }
+    this.check_move_out_from_expr(expr);
 
     match expr.node {
       ast::expr_self |
@@ -742,18 +742,7 @@ fn check_loans_in_pat<'a>(pat: @ast::pat,
                                        visit::vt<@mut CheckLoanCtxt<'a>>))
 {
     this.check_for_conflicting_loans(pat.id);
-
-    // Note: moves out of pattern bindings are not checked by
-    // the borrow checker, at least not directly.  What happens
-    // is that if there are any moved bindings, the discriminant
-    // will be considered a move, and this will be checked as
-    // normal.  Then, in `middle::check_match`, we will check
-    // that no move occurs in a binding that is underneath an
-    // `@` or `&`.  Together these give the same guarantees as
-    // `check_move_out_from_expr()` without requiring us to
-    // rewalk the patterns and rebuild the pattern
-    // categorizations.
-
+    this.check_move_out_from_id(pat.id, pat.span);
     visit::visit_pat(pat, (this, vt));
 }
 

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

@@ -67,7 +67,7 @@ impl GuaranteeLifetimeContext {
         //! Main routine. Walks down `cmt` until we find the "guarantor".
 
         match cmt.cat {
-            mc::cat_rvalue |
+            mc::cat_rvalue(*) |
             mc::cat_implicit_self |
             mc::cat_copied_upvar(*) |                  // L-Local
             mc::cat_local(*) |                         // L-Local
@@ -179,7 +179,7 @@ impl GuaranteeLifetimeContext {
         //! lvalue.
 
         cmt.mutbl.is_immutable() || match cmt.guarantor().cat {
-            mc::cat_rvalue => true,
+            mc::cat_rvalue(*) => true,
             _ => false
         }
     }
@@ -299,7 +299,7 @@ impl GuaranteeLifetimeContext {
             mc::cat_arg(id) => {
                 self.bccx.moved_variables_set.contains(&id)
             }
-            mc::cat_rvalue |
+            mc::cat_rvalue(*) |
             mc::cat_static_item |
             mc::cat_implicit_self |
             mc::cat_copied_upvar(*) |
@@ -325,8 +325,8 @@ impl GuaranteeLifetimeContext {
         // See the SCOPE(LV) function in doc.rs
 
         match cmt.cat {
-            mc::cat_rvalue => {
-                ty::re_scope(self.bccx.tcx.region_maps.cleanup_scope(cmt.id))
+            mc::cat_rvalue(cleanup_scope_id) => {
+                ty::re_scope(cleanup_scope_id)
             }
             mc::cat_implicit_self |
             mc::cat_copied_upvar(_) => {

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

@@ -73,6 +73,7 @@ struct GatherLoanCtxt {
 }
 
 pub fn gather_loans(bccx: @BorrowckCtxt,
+                    decl: &ast::fn_decl,
                     body: &ast::blk)
                     -> (id_range, @mut ~[Loan], @mut move_data::MoveData) {
     let glcx = @mut GatherLoanCtxt {
@@ -83,6 +84,7 @@ pub fn gather_loans(bccx: @BorrowckCtxt,
         repeating_ids: ~[body.node.id],
         move_data: @mut MoveData::new()
     };
+    glcx.gather_fn_arg_patterns(decl, body);
     let v = visit::mk_vt(@visit::Visitor {visit_expr: gather_loans_in_expr,
                                           visit_block: gather_loans_in_block,
                                           visit_fn: gather_loans_in_fn,
@@ -124,6 +126,7 @@ fn gather_loans_in_fn(fk: &visit::fn_kind,
             this.push_repeating_id(body.node.id);
             visit::visit_fn(fk, decl, body, sp, id, (this, v));
             this.pop_repeating_id(body.node.id);
+            this.gather_fn_arg_patterns(decl, body);
         }
     }
 }
@@ -138,26 +141,33 @@ fn gather_loans_in_block(blk: &ast::blk,
 fn gather_loans_in_local(local: @ast::local,
                          (this, vt): (@mut GatherLoanCtxt,
                                       visit::vt<@mut GatherLoanCtxt>)) {
-    if local.node.init.is_none() {
-        // Variable declarations without initializers are considered "moves":
-        let tcx = this.bccx.tcx;
-        do pat_util::pat_bindings(tcx.def_map, local.node.pat) |_, id, span, _| {
-            gather_moves::gather_decl(this.bccx,
-                                      this.move_data,
-                                      id,
-                                      span,
-                                      id);
+    match local.node.init {
+        None => {
+            // Variable declarations without initializers are considered "moves":
+            let tcx = this.bccx.tcx;
+            do pat_util::pat_bindings(tcx.def_map, local.node.pat)
+                |_, id, span, _| {
+                gather_moves::gather_decl(this.bccx,
+                                          this.move_data,
+                                          id,
+                                          span,
+                                          id);
+            }
         }
-    } else {
-        // Variable declarations with initializers are considered "assigns":
-        let tcx = this.bccx.tcx;
-        do pat_util::pat_bindings(tcx.def_map, local.node.pat) |_, id, span, _| {
-            gather_moves::gather_assignment(this.bccx,
-                                            this.move_data,
-                                            id,
-                                            span,
-                                            @LpVar(id),
-                                            id);
+        Some(init) => {
+            // Variable declarations with initializers are considered "assigns":
+            let tcx = this.bccx.tcx;
+            do pat_util::pat_bindings(tcx.def_map, local.node.pat)
+                |_, id, span, _| {
+                gather_moves::gather_assignment(this.bccx,
+                                                this.move_data,
+                                                id,
+                                                span,
+                                                @LpVar(id),
+                                                id);
+            }
+            let init_cmt = this.bccx.cat_expr(init);
+            this.gather_pat(init_cmt, local.node.pat, None);
         }
     }
 
@@ -230,7 +240,7 @@ fn gather_loans_in_expr(ex: @ast::expr,
         let cmt = this.bccx.cat_expr(ex_v);
         for arms.iter().advance |arm| {
             for arm.pats.iter().advance |pat| {
-                this.gather_pat(cmt, *pat, arm.body.node.id, ex.id);
+                this.gather_pat(cmt, *pat, Some((arm.body.node.id, ex.id)));
             }
         }
         visit::visit_expr(ex, (this, vt));
@@ -596,11 +606,40 @@ impl GatherLoanCtxt {
         }
     }
 
-    pub fn gather_pat(&mut self,
-                      discr_cmt: mc::cmt,
-                      root_pat: @ast::pat,
-                      arm_body_id: ast::node_id,
-                      match_id: ast::node_id) {
+    fn gather_fn_arg_patterns(&mut self,
+                              decl: &ast::fn_decl,
+                              body: &ast::blk) {
+        /*!
+         * Walks the patterns for fn arguments, checking that they
+         * do not attempt illegal moves or create refs that outlive
+         * the arguments themselves. Just a shallow wrapper around
+         * `gather_pat()`.
+         */
+
+        let mc_ctxt = self.bccx.mc_ctxt();
+        for decl.inputs.each |arg| {
+            let arg_ty = ty::node_id_to_type(self.tcx(), arg.pat.id);
+
+            let arg_cmt = mc_ctxt.cat_rvalue(
+                arg.id,
+                arg.pat.span,
+                body.node.id, // Arguments live only as long as the fn body.
+                arg_ty);
+
+            self.gather_pat(arg_cmt, arg.pat, None);
+        }
+    }
+
+    fn gather_pat(&mut self,
+                  discr_cmt: mc::cmt,
+                  root_pat: @ast::pat,
+                  arm_match_ids: Option<(ast::node_id, ast::node_id)>) {
+        /*!
+         * Walks patterns, examining the bindings to determine if they
+         * cause borrows (`ref` bindings, vector patterns) or
+         * moves (non-`ref` bindings with linear type).
+         */
+
         do self.bccx.cat_pattern(discr_cmt, root_pat) |cmt, pat| {
             match pat.node {
               ast::pat_ident(bm, _, _) if self.pat_is_binding(pat) => {
@@ -621,15 +660,19 @@ impl GatherLoanCtxt {
                     // with a cat_discr() node.  There is a detailed
                     // discussion of the function of this node in
                     // `lifetime.rs`:
-                    let arm_scope = ty::re_scope(arm_body_id);
-                    if self.bccx.is_subregion_of(scope_r, arm_scope) {
-                        let cmt_discr = self.bccx.cat_discr(cmt, match_id);
-                        self.guarantee_valid(pat.id, pat.span,
-                                             cmt_discr, mutbl, scope_r);
-                    } else {
-                        self.guarantee_valid(pat.id, pat.span,
-                                             cmt, mutbl, scope_r);
-                    }
+                    let cmt_discr = match arm_match_ids {
+                        None => cmt,
+                        Some((arm_id, match_id)) => {
+                            let arm_scope = ty::re_scope(arm_id);
+                            if self.bccx.is_subregion_of(scope_r, arm_scope) {
+                                self.bccx.cat_discr(cmt, match_id)
+                            } else {
+                                cmt
+                            }
+                        }
+                    };
+                    self.guarantee_valid(pat.id, pat.span,
+                                         cmt_discr, mutbl, scope_r);
                   }
                   ast::bind_infer => {
                       // No borrows here, but there may be moves
@@ -652,6 +695,24 @@ impl GatherLoanCtxt {
                       self.vec_slice_info(slice_pat, slice_ty);
                   let mcx = self.bccx.mc_ctxt();
                   let cmt_index = mcx.cat_index(slice_pat, cmt, 0);
+
+                  // Note: We declare here that the borrow occurs upon
+                  // entering the `[...]` pattern. This implies that
+                  // something like `[a, ..b]` where `a` is a move is
+                  // illegal, because the borrow is already in effect.
+                  // In fact such a move would be safe-ish, but it
+                  // effectively *requires* that we use the nulling
+                  // out semantics to indicate when a value has been
+                  // moved, which we are trying to move away from.
+                  // Otherwise, how can we indicate that the first
+                  // element in the vector has been moved?
+                  // Eventually, we could perhaps modify this rule to
+                  // permit `[..a, b]` where `b` is a move, because in
+                  // that case we can adjust the length of the
+                  // original vec accordingly, but we'd have to make
+                  // trans do the right thing, and it would only work
+                  // for `~` vectors. It seems simpler to just require
+                  // that people call `vec.pop()` or `vec.unshift()`.
                   self.guarantee_valid(pat.id, pat.span,
                                        cmt_index, slice_mutbl, slice_r);
               }

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

@@ -64,7 +64,7 @@ impl RestrictionsContext {
         }
 
         match cmt.cat {
-            mc::cat_rvalue => {
+            mc::cat_rvalue(*) => {
                 // Effectively, rvalues are stored into a
                 // non-aliasable temporary on the stack. Since they
                 // are inherently non-aliasable, they can only be