Collect move errors before reporting

This commit changes the way move errors are reported when some value is
captured by a PatIdent. First, we collect all of the "cannot move out
of" errors before reporting them, and those errors with the same "move
source" are reported together. If the move is caused by a PatIdent (that
binds by value), we add a note indicating where it is and suggest the
user to put `ref` if they don't want the value to move. This makes the
"cannot move out of" error in match expression nicer (though the extra
note may not feel that helpful in other places :P). For example, with
the following code snippet,

```rust
enum Foo {
    Foo1(~u32, ~u32),
    Foo2(~u32),
    Foo3,
}

fn main() {
    let f = &Foo1(~1u32, ~2u32);
    match *f {
        Foo1(num1, num2) => (),
        Foo2(num) => (),
        Foo3 => ()
    }
}
```

Errors before the change:

```rust
test.rs:10:9: 10:25 error: cannot move out of dereference of `&`-pointer
test.rs:10         Foo1(num1, num2) => (),
                   ^~~~~~~~~~~~~~~~
test.rs:10:9: 10:25 error: cannot move out of dereference of `&`-pointer
test.rs:10         Foo1(num1, num2) => (),
                   ^~~~~~~~~~~~~~~~
test.rs:11:9: 11:18 error: cannot move out of dereference of `&`-pointer
test.rs:11         Foo2(num) => (),
                   ^~~~~~~~~
```

After:

```rust
test.rs:9:11: 9:13 error: cannot move out of dereference of `&`-pointer
test.rs:9     match *f {
                    ^~
test.rs:10:14: 10:18 note: attempting to move value to here (to prevent the move, you can use `ref num1` to capture value by reference)
test.rs:10         Foo1(num1, num2) => (),
                        ^~~~
test.rs:10:20: 10:24 note: and here (use `ref num2`)
test.rs:10         Foo1(num1, num2) => (),
                              ^~~~
test.rs:11:14: 11:17 note: and here (use `ref num`)
test.rs:11         Foo2(num) => (),
                        ^~~
```

Close #8064

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

@@ -14,12 +14,21 @@
 
 use mc = middle::mem_categorization;
 use middle::borrowck::*;
+use middle::borrowck::gather_loans::move_error::{MoveError, MoveErrorCollector};
+use middle::borrowck::gather_loans::move_error::MoveSpanAndPath;
 use middle::borrowck::move_data::*;
 use middle::moves;
 use middle::ty;
 use syntax::ast;
 use syntax::codemap::Span;
-use util::ppaux::{Repr, UserString};
+use util::ppaux::Repr;
+
+struct GatherMoveInfo {
+    id: ast::NodeId,
+    kind: MoveKind,
+    cmt: mc::cmt,
+    span_path_opt: Option<MoveSpanAndPath>
+}
 
 pub fn gather_decl(bccx: &BorrowckCtxt,
                    move_data: &MoveData,
@@ -32,28 +41,56 @@ pub fn gather_decl(bccx: &BorrowckCtxt,
 
 pub fn gather_move_from_expr(bccx: &BorrowckCtxt,
                              move_data: &MoveData,
+                             move_error_collector: &MoveErrorCollector,
                              move_expr: &ast::Expr,
                              cmt: mc::cmt) {
-    gather_move(bccx, move_data, move_expr.id, MoveExpr, cmt);
+    let move_info = GatherMoveInfo {
+        id: move_expr.id,
+        kind: MoveExpr,
+        cmt: cmt,
+        span_path_opt: None,
+    };
+    gather_move(bccx, move_data, move_error_collector, move_info);
 }
 
 pub fn gather_move_from_pat(bccx: &BorrowckCtxt,
                             move_data: &MoveData,
+                            move_error_collector: &MoveErrorCollector,
                             move_pat: &ast::Pat,
                             cmt: mc::cmt) {
-    gather_move(bccx, move_data, move_pat.id, MovePat, cmt);
+    let pat_span_path_opt = match move_pat.node {
+        ast::PatIdent(_, ref path, _) => {
+            Some(MoveSpanAndPath::with_span_and_path(move_pat.span,
+                                                     (*path).clone()))
+        },
+        _ => None,
+    };
+    let move_info = GatherMoveInfo {
+        id: move_pat.id,
+        kind: MovePat,
+        cmt: cmt,
+        span_path_opt: pat_span_path_opt,
+    };
+    gather_move(bccx, move_data, move_error_collector, move_info);
 }
 
 pub fn gather_captures(bccx: &BorrowckCtxt,
                        move_data: &MoveData,
+                       move_error_collector: &MoveErrorCollector,
                        closure_expr: &ast::Expr) {
     for captured_var in bccx.capture_map.get(&closure_expr.id).iter() {
         match captured_var.mode {
             moves::CapMove => {
                 let cmt = bccx.cat_captured_var(closure_expr.id,
                                                 closure_expr.span,
                                                 captured_var);
-                gather_move(bccx, move_data, closure_expr.id, Captured, cmt);
+                let move_info = GatherMoveInfo {
+                    id: closure_expr.id,
+                    kind: Captured,
+                    cmt: cmt,
+                    span_path_opt: None
+                };
+                gather_move(bccx, move_data, move_error_collector, move_info);
             }
             moves::CapCopy | moves::CapRef => {}
         }
@@ -62,19 +99,27 @@ pub fn gather_captures(bccx: &BorrowckCtxt,
 
 fn gather_move(bccx: &BorrowckCtxt,
                move_data: &MoveData,
-               move_id: ast::NodeId,
-               move_kind: MoveKind,
-               cmt: mc::cmt) {
+               move_error_collector: &MoveErrorCollector,
+               move_info: GatherMoveInfo) {
     debug!("gather_move(move_id={}, cmt={})",
-           move_id, cmt.repr(bccx.tcx));
-
-    if !check_is_legal_to_move_from(bccx, cmt, cmt) {
-        return;
+           move_info.id, move_info.cmt.repr(bccx.tcx));
+
+    let potentially_illegal_move =
+                check_and_get_illegal_move_origin(bccx, move_info.cmt);
+    match potentially_illegal_move {
+        Some(illegal_move_origin) => {
+            let error = MoveError::with_move_info(illegal_move_origin,
+                                                  move_info.span_path_opt);
+            move_error_collector.add_error(error);
+            return
+        }
+        None => ()
     }
 
-    match opt_loan_path(cmt) {
+    match opt_loan_path(move_info.cmt) {
         Some(loan_path) => {
-            move_data.add_move(bccx.tcx, loan_path, move_id, move_kind);
+            move_data.add_move(bccx.tcx, loan_path,
+                               move_info.id, move_info.kind);
         }
         None => {
             // move from rvalue or unsafe pointer, hence ok
@@ -110,59 +155,49 @@ pub fn gather_move_and_assignment(bccx: &BorrowckCtxt,
                              true);
 }
 
-fn check_is_legal_to_move_from(bccx: &BorrowckCtxt,
-                               cmt0: mc::cmt,
-                               cmt: mc::cmt) -> bool {
+fn check_and_get_illegal_move_origin(bccx: &BorrowckCtxt,
+                                     cmt: mc::cmt) -> Option<mc::cmt> {
     match cmt.cat {
         mc::cat_deref(_, _, mc::BorrowedPtr(..)) |
         mc::cat_deref(_, _, mc::GcPtr) |
         mc::cat_deref(_, _, mc::UnsafePtr(..)) |
         mc::cat_upvar(..) | mc::cat_static_item |
         mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => {
-            bccx.span_err(
-                cmt0.span,
-                format!("cannot move out of {}",
-                        bccx.cmt_to_str(cmt)));
-            false
+            Some(cmt)
         }
 
         // Can move out of captured upvars only if the destination closure
         // type is 'once'. 1-shot stack closures emit the copied_upvar form
         // (see mem_categorization.rs).
         mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Once, .. }) => {
-            true
+            None
         }
 
         mc::cat_rvalue(..) |
         mc::cat_local(..) |
         mc::cat_arg(..) => {
-            true
+            None
         }
 
         mc::cat_downcast(b) |
         mc::cat_interior(b, _) => {
             match ty::get(b.ty).sty {
                 ty::ty_struct(did, _) | ty::ty_enum(did, _) => {
                     if ty::has_dtor(bccx.tcx, did) {
-                        bccx.span_err(
-                            cmt0.span,
-                            format!("cannot move out of type `{}`, \
-                                  which defines the `Drop` trait",
-                                 b.ty.user_string(bccx.tcx)));
-                        false
+                        Some(cmt)
                     } else {
-                        check_is_legal_to_move_from(bccx, cmt0, b)
+                        check_and_get_illegal_move_origin(bccx, b)
                     }
                 }
                 _ => {
-                    check_is_legal_to_move_from(bccx, cmt0, b)
+                    check_and_get_illegal_move_origin(bccx, b)
                 }
             }
         }
 
         mc::cat_deref(b, _, mc::OwnedPtr) |
         mc::cat_discr(b, _) => {
-            check_is_legal_to_move_from(bccx, cmt0, b)
+            check_and_get_illegal_move_origin(bccx, b)
         }
     }
 }

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

@@ -1,4 +1,4 @@
-// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
+// Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT
 // file at the top-level directory of this distribution and at
 // http://rust-lang.org/COPYRIGHT.
 //
@@ -39,6 +39,7 @@ use syntax::ast::{Expr, FnDecl, Block, NodeId, Stmt, Pat, Local};
 mod lifetime;
 mod restrictions;
 mod gather_moves;
+mod move_error;
 
 /// Context used while gathering loans:
 ///
@@ -70,6 +71,7 @@ struct GatherLoanCtxt<'a> {
     bccx: &'a BorrowckCtxt<'a>,
     id_range: IdRange,
     move_data: move_data::MoveData,
+    move_error_collector: move_error::MoveErrorCollector,
     all_loans: Vec<Loan>,
     item_ub: ast::NodeId,
     repeating_ids: Vec<ast::NodeId> }
@@ -121,11 +123,13 @@ pub fn gather_loans_in_fn(bccx: &BorrowckCtxt, decl: &ast::FnDecl, body: &ast::B
         all_loans: Vec::new(),
         item_ub: body.id,
         repeating_ids: vec!(body.id),
-        move_data: MoveData::new()
+        move_data: MoveData::new(),
+        move_error_collector: move_error::MoveErrorCollector::new(),
     };
     glcx.gather_fn_arg_patterns(decl, body);
 
     glcx.visit_block(body, ());
+    glcx.report_potential_errors();
     let GatherLoanCtxt { id_range, all_loans, move_data, .. } = glcx;
     (id_range, all_loans, move_data)
 }
@@ -180,7 +184,7 @@ fn gather_loans_in_expr(this: &mut GatherLoanCtxt,
     if this.bccx.is_move(ex.id) {
         let cmt = this.bccx.cat_expr(ex);
         gather_moves::gather_move_from_expr(
-            this.bccx, &this.move_data, ex, cmt);
+            this.bccx, &this.move_data, &this.move_error_collector, ex, cmt);
     }
 
     // Special checks for various kinds of expressions:
@@ -270,7 +274,8 @@ fn gather_loans_in_expr(this: &mut GatherLoanCtxt,
       }
 
       ast::ExprFnBlock(..) | ast::ExprProc(..) => {
-          gather_moves::gather_captures(this.bccx, &this.move_data, ex);
+          gather_moves::gather_captures(this.bccx, &this.move_data,
+                                        &this.move_error_collector, ex);
           this.guarantee_captures(ex);
           visit::walk_expr(this, ex, ());
       }
@@ -874,7 +879,8 @@ impl<'a> GatherLoanCtxt<'a> {
                       // No borrows here, but there may be moves
                       if self.bccx.is_move(pat.id) {
                           gather_moves::gather_move_from_pat(
-                              self.bccx, &self.move_data, pat, cmt);
+                              self.bccx, &self.move_data,
+                              &self.move_error_collector, pat, cmt);
                       }
                   }
                 }
@@ -925,6 +931,10 @@ impl<'a> GatherLoanCtxt<'a> {
     pub fn pat_is_binding(&self, pat: &ast::Pat) -> bool {
         pat_util::pat_is_binding(self.bccx.tcx.def_map, pat)
     }
+
+    pub fn report_potential_errors(&self) {
+        self.move_error_collector.report_potential_errors(self.bccx);
+    }
 }
 
 /// Context used while gathering loans on static initializers