add borrowck info inline in main snippet

This uses the new `span_label` APIs

4 files changed, 146 insertions, 187 deletions

diff --git a/src/librustc/diagnostics.rs b/src/librustc/diagnostics.rs
index e230836ef45..efef259dcad 100644
--- a/src/librustc/diagnostics.rs
+++ b/src/librustc/diagnostics.rs
@@ -1569,5 +1569,5 @@ register_diagnostics! {
     E0490, // a value of type `..` is borrowed for too long
     E0491, // in type `..`, reference has a longer lifetime than the data it...
     E0495, // cannot infer an appropriate lifetime due to conflicting requirements
-    E0524, // expected a closure that implements `..` but this closure only implements `..`
+    E0525, // expected a closure that implements `..` but this closure only implements `..`
 }
diff --git a/src/librustc_borrowck/borrowck/check_loans.rs b/src/librustc_borrowck/borrowck/check_loans.rs
index d79ba213aca..bbd9cd4526d 100644
--- a/src/librustc_borrowck/borrowck/check_loans.rs
+++ b/src/librustc_borrowck/borrowck/check_loans.rs
@@ -447,22 +447,24 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
             //     borrow ends
 
             let common = new_loan.loan_path.common(&old_loan.loan_path);
-            let (nl, ol, new_loan_msg, old_loan_msg) =
+            let (nl, ol, new_loan_msg, old_loan_msg) = {
                 if new_loan.loan_path.has_fork(&old_loan.loan_path) && common.is_some() {
                     let nl = self.bccx.loan_path_to_string(&common.unwrap());
                     let ol = nl.clone();
-                    let new_loan_msg = format!(" (here through borrowing `{}`)",
+                    let new_loan_msg = format!(" (via `{}`)",
                                                self.bccx.loan_path_to_string(
                                                    &new_loan.loan_path));
-                    let old_loan_msg = format!(" (through borrowing `{}`)",
+                    let old_loan_msg = format!(" (via `{}`)",
                                                self.bccx.loan_path_to_string(
                                                    &old_loan.loan_path));
                     (nl, ol, new_loan_msg, old_loan_msg)
                 } else {
                     (self.bccx.loan_path_to_string(&new_loan.loan_path),
                      self.bccx.loan_path_to_string(&old_loan.loan_path),
-                     String::new(), String::new())
-                };
+                     String::new(),
+                     String::new())
+                }
+            };
 
             let ol_pronoun = if new_loan.loan_path == old_loan.loan_path {
                 "it".to_string()
@@ -470,12 +472,48 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
                 format!("`{}`", ol)
             };
 
+            // We want to assemble all the relevant locations for the error.
+            //
+            // 1. Where did the new loan occur.
+            //    - if due to closure creation, where was the variable used in closure?
+            // 2. Where did old loan occur.
+            // 3. Where does old loan expire.
+
+            let previous_end_span =
+                self.tcx().map.span(old_loan.kill_scope.node_id(&self.tcx().region_maps))
+                              .end_point();
+
             let mut err = match (new_loan.kind, old_loan.kind) {
                 (ty::MutBorrow, ty::MutBorrow) => {
                     struct_span_err!(self.bccx, new_loan.span, E0499,
                                      "cannot borrow `{}`{} as mutable \
                                       more than once at a time",
                                      nl, new_loan_msg)
+                        .span_label(
+                            old_loan.span,
+                            &format!("first mutable borrow occurs here{}", old_loan_msg))
+                        .span_label(
+                            new_loan.span,
+                            &format!("second mutable borrow occurs here{}", new_loan_msg))
+                        .span_label(
+                            previous_end_span,
+                            &format!("first borrow ends here"))
+                }
+
+                (ty::UniqueImmBorrow, ty::UniqueImmBorrow) => {
+                    struct_span_err!(self.bccx, new_loan.span, E0524,
+                                     "two closures require unique access to `{}` \
+                                      at the same time",
+                                     nl)
+                        .span_label(
+                            old_loan.span,
+                            &format!("first closure is constructed here"))
+                        .span_label(
+                            new_loan.span,
+                            &format!("second closure is constructed here"))
+                        .span_label(
+                            previous_end_span,
+                            &format!("borrow from first closure ends here"))
                 }
 
                 (ty::UniqueImmBorrow, _) => {
@@ -483,6 +521,15 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
                                      "closure requires unique access to `{}` \
                                       but {} is already borrowed{}",
                                      nl, ol_pronoun, old_loan_msg)
+                        .span_label(
+                            new_loan.span,
+                            &format!("closure construction occurs here{}", new_loan_msg))
+                        .span_label(
+                            old_loan.span,
+                            &format!("borrow occurs here{}", old_loan_msg))
+                        .span_label(
+                            previous_end_span,
+                            &format!("borrow ends here"))
                 }
 
                 (_, ty::UniqueImmBorrow) => {
@@ -490,6 +537,15 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
                                      "cannot borrow `{}`{} as {} because \
                                       previous closure requires unique access",
                                      nl, new_loan_msg, new_loan.kind.to_user_str())
+                        .span_label(
+                            new_loan.span,
+                            &format!("borrow occurs here{}", new_loan_msg))
+                        .span_label(
+                            old_loan.span,
+                            &format!("closure construction occurs here{}", old_loan_msg))
+                        .span_label(
+                            previous_end_span,
+                            &format!("borrow from closure ends here"))
                 }
 
                 (_, _) => {
@@ -502,70 +558,42 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
                                      ol_pronoun,
                                      old_loan.kind.to_user_str(),
                                      old_loan_msg)
+                        .span_label(
+                            new_loan.span,
+                            &format!("{} borrow occurs here{}",
+                                     new_loan.kind.to_user_str(),
+                                     new_loan_msg))
+                        .span_label(
+                            old_loan.span,
+                            &format!("{} borrow occurs here{}",
+                                     old_loan.kind.to_user_str(),
+                                     old_loan_msg))
+                        .span_label(
+                            previous_end_span,
+                            &format!("{} borrow ends here",
+                                     old_loan.kind.to_user_str()))
                 }
             };
 
             match new_loan.cause {
                 euv::ClosureCapture(span) => {
-                    err.span_note(
+                    err = err.span_label(
                         span,
-                        &format!("borrow occurs due to use of `{}` in closure",
-                                nl));
+                        &format!("borrow occurs due to use of `{}` in closure", nl));
                 }
                 _ => { }
             }
 
-            let rule_summary = match old_loan.kind {
-                ty::MutBorrow => {
-                    format!("the mutable borrow prevents subsequent \
-                            moves, borrows, or modification of `{0}` \
-                            until the borrow ends",
-                            ol)
-                }
-
-                ty::ImmBorrow => {
-                    format!("the immutable borrow prevents subsequent \
-                            moves or mutable borrows of `{0}` \
-                            until the borrow ends",
-                            ol)
-                }
-
-                ty::UniqueImmBorrow => {
-                    format!("the unique capture prevents subsequent \
-                            moves or borrows of `{0}` \
-                            until the borrow ends",
-                            ol)
-                }
-            };
-
-            let borrow_summary = match old_loan.cause {
-                euv::ClosureCapture(_) => {
-                    format!("previous borrow of `{}` occurs here{} due to \
-                            use in closure",
-                            ol, old_loan_msg)
-                }
-
-                euv::OverloadedOperator |
-                euv::AddrOf |
-                euv::AutoRef |
-                euv::AutoUnsafe |
-                euv::ClosureInvocation |
-                euv::ForLoop |
-                euv::RefBinding |
-                euv::MatchDiscriminant => {
-                    format!("previous borrow of `{}` occurs here{}",
-                            ol, old_loan_msg)
+            match old_loan.cause {
+                euv::ClosureCapture(span) => {
+                    err = err.span_label(
+                        span,
+                        &format!("previous borrow occurs due to use of `{}` in closure",
+                                 ol));
                 }
-            };
-
-            err.span_note(
-                old_loan.span,
-                &format!("{}; {}", borrow_summary, rule_summary));
+                _ => { }
+            }
 
-            let old_loan_span = self.tcx().map.span(
-                old_loan.kill_scope.node_id(&self.tcx().region_maps));
-            err.span_end_note(old_loan_span,
-                              "previous borrow ends here");
             err.emit();
             return false;
         }
diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs
index 15db356b1ba..87000749598 100644
--- a/src/librustc_borrowck/borrowck/mod.rs
+++ b/src/librustc_borrowck/borrowck/mod.rs
@@ -34,14 +34,14 @@ use rustc::middle::free_region::FreeRegionMap;
 use rustc::middle::mem_categorization as mc;
 use rustc::middle::mem_categorization::Categorization;
 use rustc::middle::region;
-use rustc::ty::{self, Ty, TyCtxt};
+use rustc::ty::{self, TyCtxt};
 
 use std::fmt;
 use std::mem;
 use std::rc::Rc;
 use syntax::ast;
 use syntax::attr::AttrMetaMethods;
-use syntax::codemap::Span;
+use syntax::codemap::{MultiSpan, Span};
 use syntax::errors::DiagnosticBuilder;
 
 use rustc::hir;
@@ -633,23 +633,22 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                                          lp: &LoanPath<'tcx>,
                                          the_move: &move_data::Move,
                                          moved_lp: &LoanPath<'tcx>,
-                                         param_env: &ty::ParameterEnvironment<'b,'tcx>) {
-        let verb = match use_kind {
-            MovedInUse => "use",
-            MovedInCapture => "capture",
+                                         _param_env: &ty::ParameterEnvironment<'b,'tcx>) {
+        let (verb, verb_participle) = match use_kind {
+            MovedInUse => ("use", "used"),
+            MovedInCapture => ("capture", "captured"),
         };
 
-        let (ol, moved_lp_msg, mut err) = match the_move.kind {
+        let (_ol, _moved_lp_msg, mut err) = match the_move.kind {
             move_data::Declared => {
-                let err = struct_span_err!(
+                // If this is an uninitialized variable, just emit a simple warning
+                // and return.
+                struct_span_err!(
                     self.tcx.sess, use_span, E0381,
                     "{} of possibly uninitialized variable: `{}`",
                     verb,
-                    self.loan_path_to_string(lp));
-
-                (self.loan_path_to_string(moved_lp),
-                 String::new(),
-                 err)
+                    self.loan_path_to_string(lp)).emit();
+                return;
             }
             _ => {
                 // If moved_lp is something like `x.a`, and lp is something like `x.b`, we would
@@ -688,122 +687,52 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                     self.tcx.sess, use_span, E0382,
                     "{} of {}moved value: `{}`",
                     verb, msg, nl);
-                (ol, moved_lp_msg, err)
+                (ol, moved_lp_msg, err)}
+        };
+
+        // Get type of value and span where it was previously
+        // moved.
+        let (move_span, move_note) = match the_move.kind {
+            move_data::Declared => {
+                unreachable!();
             }
+
+            move_data::MoveExpr |
+            move_data::MovePat =>
+                (self.tcx.map.span(the_move.id), ""),
+
+            move_data::Captured =>
+                (match self.tcx.map.expect_expr(the_move.id).node {
+                    hir::ExprClosure(_, _, _, fn_decl_span) => fn_decl_span,
+                    ref r => bug!("Captured({}) maps to non-closure: {:?}",
+                                  the_move.id, r),
+                }, " (into closure)"),
         };
 
-        match the_move.kind {
-            move_data::Declared => {}
+        // Annotate the use and the move in the span. Watch out for
+        // the case where the use and the move are the same. This
+        // means the use is in a loop.
+        err = if use_span == move_span {
+            err.span_label(
+                use_span,
+                &format!("value moved{} here in previous iteration of loop",
+                         move_note))
+        } else {
+            err.span_label(use_span, &format!("value {} here after move", verb_participle))
+               .span_label(move_span, &format!("value moved{} here", move_note))
+        };
 
-            move_data::MoveExpr => {
-                let (expr_ty, expr_span) = match self.tcx
-                                                     .map
-                                                     .find(the_move.id) {
-                    Some(hir_map::NodeExpr(expr)) => {
-                        (self.tcx.expr_ty_adjusted(&expr), expr.span)
-                    }
-                    r => {
-                        bug!("MoveExpr({}) maps to {:?}, not Expr",
-                             the_move.id,
-                             r)
-                    }
-                };
-                let (suggestion, _) =
-                    move_suggestion(param_env, expr_span, expr_ty, ("moved by default", ""));
-                // If the two spans are the same, it's because the expression will be evaluated
-                // multiple times. Avoid printing the same span and adjust the wording so it makes
-                // more sense that it's from multiple evalutations.
-                if expr_span == use_span {
-                    err.note(
-                        &format!("`{}` was previously moved here{} because it has type `{}`, \
-                                  which is {}",
-                                 ol,
-                                 moved_lp_msg,
-                                 expr_ty,
-                                 suggestion));
-                } else {
-                    err.span_note(
-                        expr_span,
-                        &format!("`{}` moved here{} because it has type `{}`, which is {}",
-                                 ol,
-                                 moved_lp_msg,
-                                 expr_ty,
-                                 suggestion));
-                }
-            }
+        err.note(&format!("move occurs because `{}` has type `{}`, \
+                           which does not implement the `Copy` trait",
+                          self.loan_path_to_string(moved_lp),
+                          moved_lp.ty));
 
-            move_data::MovePat => {
-                let pat_ty = self.tcx.node_id_to_type(the_move.id);
-                let span = self.tcx.map.span(the_move.id);
-                err.span_note(span,
-                    &format!("`{}` moved here{} because it has type `{}`, \
-                             which is moved by default",
-                            ol,
-                            moved_lp_msg,
-                            pat_ty));
-                match self.tcx.sess.codemap().span_to_snippet(span) {
-                    Ok(string) => {
-                        err.span_suggestion(
-                            span,
-                            &format!("if you would like to borrow the value instead, \
-                                      use a `ref` binding as shown:"),
-                            format!("ref {}", string));
-                    },
-                    Err(_) => {
-                        err.fileline_help(span,
-                            "use `ref` to override");
-                    },
-                }
-            }
+        // Note: we used to suggest adding a `ref binding` or calling
+        // `clone` but those suggestions have been removed because
+        // they are often not what you actually want to do, and were
+        // not considered particularly helpful.
 
-            move_data::Captured => {
-                let (expr_ty, expr_span) = match self.tcx
-                                                     .map
-                                                     .find(the_move.id) {
-                    Some(hir_map::NodeExpr(expr)) => {
-                        (self.tcx.expr_ty_adjusted(&expr), expr.span)
-                    }
-                    r => {
-                        bug!("Captured({}) maps to {:?}, not Expr",
-                             the_move.id,
-                             r)
-                    }
-                };
-                let (suggestion, help) =
-                    move_suggestion(param_env,
-                                    expr_span,
-                                    expr_ty,
-                                    ("moved by default",
-                                     "make a copy and capture that instead to override"));
-                err.span_note(
-                    expr_span,
-                    &format!("`{}` moved into closure environment here{} because it \
-                            has type `{}`, which is {}",
-                            ol,
-                            moved_lp_msg,
-                            moved_lp.ty,
-                            suggestion));
-                err.fileline_help(expr_span, help);
-            }
-        }
         err.emit();
-
-        fn move_suggestion<'a,'tcx>(param_env: &ty::ParameterEnvironment<'a,'tcx>,
-                                    span: Span,
-                                    ty: Ty<'tcx>,
-                                    default_msgs: (&'static str, &'static str))
-                                    -> (&'static str, &'static str) {
-            match ty.sty {
-                _ => {
-                    if ty.moves_by_default(param_env, span) {
-                        ("non-copyable",
-                         "perhaps you meant to use `clone()`?")
-                    } else {
-                        default_msgs
-                    }
-                }
-            }
-        }
     }
 
     pub fn report_partial_reinitialization_of_uninitialized_structure(
@@ -833,19 +762,20 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
         self.tcx.sess.span_err(s, m);
     }
 
-    pub fn struct_span_err(&self, s: Span, m: &str) -> DiagnosticBuilder<'a> {
+    pub fn struct_span_err<S: Into<MultiSpan>>(&self, s: S, m: &str)
+                                              -> DiagnosticBuilder<'a> {
         self.tcx.sess.struct_span_err(s, m)
     }
 
-    pub fn struct_span_err_with_code(&self,
-                                     s: Span,
-                                     msg: &str,
-                                     code: &str)
-                                     -> DiagnosticBuilder<'a> {
+    pub fn struct_span_err_with_code<S: Into<MultiSpan>>(&self,
+                                                         s: S,
+                                                         msg: &str,
+                                                         code: &str)
+                                                         -> DiagnosticBuilder<'a> {
         self.tcx.sess.struct_span_err_with_code(s, msg, code)
     }
 
-    pub fn span_err_with_code(&self, s: Span, msg: &str, code: &str) {
+    pub fn span_err_with_code<S: Into<MultiSpan>>(&self, s: S, msg: &str, code: &str) {
         self.tcx.sess.span_err_with_code(s, msg, code);
     }
 
diff --git a/src/librustc_borrowck/diagnostics.rs b/src/librustc_borrowck/diagnostics.rs
index 7f6fd9de3d2..c7ad0b6a6c6 100644
--- a/src/librustc_borrowck/diagnostics.rs
+++ b/src/librustc_borrowck/diagnostics.rs
@@ -444,4 +444,5 @@ register_diagnostics! {
     E0506, // cannot assign to `..` because it is borrowed
     E0508, // cannot move out of type `..`, a non-copy fixed-size array
     E0509, // cannot move out of type `..`, which defines the `Drop` trait
+    E0524, // two closures require unique access to `..` at the same time
 }