Make some borrow checker errors more user friendly

A mutable and immutable borrow place some restrictions on what you can
with the variable until the borrow ends. This commit attempts to convey
to the user what those restrictions are. Also, if the original borrow is
a mutable borrow, the error message has been changed (more specifically,
i. "cannot borrow `x` as immutable because it is also borrowed as
mutable" and ii. "cannot borrow `x` as mutable more than once" have
been changed to "cannot borrow `x` because it is already borrowed as
mutable").

In addition, this adds a (custom) span note to communicate where the
original borrow ends.

Author: ktt3ja

src/librustc/driver/session.rs

@@ -248,6 +248,9 @@ impl Session_ {
     pub fn span_note(&self, sp: Span, msg: &str) {
         self.span_diagnostic.span_note(sp, msg)
     }
+    pub fn span_end_note(&self, sp: Span, msg: &str) {
+        self.span_diagnostic.span_end_note(sp, msg)
+    }
     pub fn note(&self, msg: &str) {
         self.span_diagnostic.handler().note(msg)
     }

src/librustc/middle/borrowck/check_loans.rs

@@ -231,34 +231,47 @@ impl<'a> CheckLoanCtxt<'a> {
             if restr.loan_path != loan2.loan_path { continue; }
 
             match (new_loan.mutbl, old_loan.mutbl) {
-                (MutableMutability, MutableMutability) => {
+                (_, MutableMutability) => {
+                    let var = self.bccx.loan_path_to_str(new_loan.loan_path);
                     self.bccx.span_err(
                         new_loan.span,
-                        format!("cannot borrow `{}` as mutable \
-                              more than once at a time",
-                             self.bccx.loan_path_to_str(new_loan.loan_path)));
+                        format!("cannot borrow `{}` because it is already \
+                                 borrowed as mutable", var));
                     self.bccx.span_note(
                         old_loan.span,
-                        format!("previous borrow of `{}` as mutable occurs here",
-                             self.bccx.loan_path_to_str(new_loan.loan_path)));
-                    return false;
+                        format!("previous borrow of `{0}` as mutable occurs \
+                                 here; the mutable borrow prevents subsequent \
+                                 moves, borrows, or modification of `{0}` \
+                                 until the borrow ends", var));
                 }
 
-                _ => {
+                (_, mutability) => {
                     self.bccx.span_err(
                         new_loan.span,
                         format!("cannot borrow `{}` as {} because \
-                              it is also borrowed as {}",
+                              it is already borrowed as {}",
                              self.bccx.loan_path_to_str(new_loan.loan_path),
                              self.bccx.mut_to_str(new_loan.mutbl),
                              self.bccx.mut_to_str(old_loan.mutbl)));
-                    self.bccx.span_note(
-                        old_loan.span,
-                        format!("previous borrow of `{}` occurs here",
-                             self.bccx.loan_path_to_str(new_loan.loan_path)));
-                    return false;
+
+                    let var = self.bccx.loan_path_to_str(new_loan.loan_path);
+                    let mut note = format!("previous borrow of `{}` occurs \
+                                            here", var);
+                    if mutability == ImmutableMutability {
+                        note.push_str(format!("; the immutable borrow prevents \
+                                               subsequent moves or mutable
+                                               borrows of `{}` until the
+                                               borrow ends", var));
+                    }
+                    self.bccx.span_note(old_loan.span, note);
                 }
             }
+
+            let old_loan_span = ast_map::node_span(self.tcx().items,
+                                                   old_loan.kill_scope);
+            self.bccx.span_end_note(old_loan_span,
+                                    "previous borrow ends here");
+            return false;
         }
 
         true

src/librustc/middle/borrowck/mod.rs

@@ -631,6 +631,10 @@ impl BorrowckCtxt {
         self.tcx.sess.span_note(s, m);
     }
 
+    pub fn span_end_note(&self, s: Span, m: &str) {
+        self.tcx.sess.span_end_note(s, m);
+    }
+
     pub fn bckerr_to_str(&self, err: BckError) -> ~str {
         match err.code {
             err_mutbl(lk) => {

src/libsyntax/codemap.rs

@@ -311,10 +311,6 @@ impl CodeMap {
         }
     }
 
-    pub fn adjust_span(&self, sp: Span) -> Span {
-        sp
-    }
-
     pub fn span_to_str(&self, sp: Span) -> ~str {
         {
             let files = self.files.borrow();

src/libsyntax/diagnostic.rs

@@ -19,12 +19,16 @@ use extra::term;
 
 static BUG_REPORT_URL: &'static str =
     "http://static.rust-lang.org/doc/master/complement-bugreport.html";
+// maximum number of lines we will print for each error; arbitrary.
+static MAX_LINES: uint = 6u;
 
 pub trait Emitter {
     fn emit(&self,
             cmsp: Option<(&codemap::CodeMap, Span)>,
             msg: &str,
             lvl: Level);
+    fn custom_emit(&self, cm: &codemap::CodeMap,
+                   sp: Span, msg: &str, lvl: Level);
 }
 
 /// This structure is used to signify that a task has failed with a fatal error
@@ -55,6 +59,9 @@ impl SpanHandler {
     pub fn span_note(@self, sp: Span, msg: &str) {
         self.handler.emit(Some((&*self.cm, sp)), msg, Note);
     }
+    pub fn span_end_note(@self, sp: Span, msg: &str) {
+        self.handler.custom_emit(&*self.cm, sp, msg, Note);
+    }
     pub fn span_bug(@self, sp: Span, msg: &str) -> ! {
         self.span_fatal(sp, ice_msg(msg));
     }
@@ -122,6 +129,10 @@ impl Handler {
             lvl: Level) {
         self.emit.emit(cmsp, msg, lvl);
     }
+    pub fn custom_emit(@self, cm: &codemap::CodeMap,
+                       sp: Span, msg: &str, lvl: Level) {
+        self.emit.custom_emit(cm, sp, msg, lvl);
+    }
 }
 
 pub fn ice_msg(msg: &str) -> ~str {
@@ -239,17 +250,34 @@ impl Emitter for DefaultEmitter {
             msg: &str,
             lvl: Level) {
         match cmsp {
-            Some((cm, sp)) => {
-                let sp = cm.adjust_span(sp);
-                let ss = cm.span_to_str(sp);
-                let lines = cm.span_to_lines(sp);
-                print_diagnostic(ss, lvl, msg);
-                highlight_lines(cm, sp, lvl, lines);
-                print_macro_backtrace(cm, sp);
-            }
+            Some((cm, sp)) => emit(cm, sp, msg, lvl, false),
             None => print_diagnostic("", lvl, msg),
         }
     }
+
+    fn custom_emit(&self, cm: &codemap::CodeMap,
+                   sp: Span, msg: &str, lvl: Level) {
+        emit(cm, sp, msg, lvl, true);
+    }
+}
+
+fn emit(cm: &codemap::CodeMap, sp: Span,
+        msg: &str, lvl: Level, custom: bool) {
+    let ss = cm.span_to_str(sp);
+    let lines = cm.span_to_lines(sp);
+    if custom {
+        // we want to tell compiletest/runtest to look at the last line of the
+        // span (since `custom_highlight_lines` displays an arrow to the end of
+        // the span)
+        let span_end = Span { lo: sp.hi, hi: sp.hi, expn_info: sp.expn_info};
+        let ses = cm.span_to_str(span_end);
+        print_diagnostic(ses, lvl, msg);
+        custom_highlight_lines(cm, sp, lvl, lines);
+    } else {
+        print_diagnostic(ss, lvl, msg);
+        highlight_lines(cm, sp, lvl, lines);
+    }
+    print_macro_backtrace(cm, sp);
 }
 
 fn highlight_lines(cm: &codemap::CodeMap,
@@ -260,12 +288,10 @@ fn highlight_lines(cm: &codemap::CodeMap,
     let mut err = io::stderr();
     let err = &mut err as &mut io::Writer;
 
-    // arbitrarily only print up to six lines of the error
-    let max_lines = 6u;
     let mut elided = false;
     let mut display_lines = lines.lines.as_slice();
-    if display_lines.len() > max_lines {
-        display_lines = display_lines.slice(0u, max_lines);
+    if display_lines.len() > MAX_LINES {
+        display_lines = display_lines.slice(0u, MAX_LINES);
         elided = true;
     }
     // Print the offending lines
@@ -319,6 +345,44 @@ fn highlight_lines(cm: &codemap::CodeMap,
     }
 }
 
+// Here are the differences between this and the normal `highlight_lines`:
+// `custom_highlight_lines` will always put arrow on the last byte of the
+// span (instead of the first byte). Also, when the span is too long (more
+// than 6 lines), `custom_highlight_lines` will print the first line, then
+// dot dot dot, then last line, whereas `highlight_lines` prints the first
+// six lines.
+fn custom_highlight_lines(cm: &codemap::CodeMap,
+                          sp: Span,
+                          lvl: Level,
+                          lines: &codemap::FileLines) {
+    let fm = lines.file;
+    let mut err = io::stderr();
+    let err = &mut err as &mut io::Writer;
+
+    let lines = lines.lines.as_slice();
+    if lines.len() > MAX_LINES {
+        write!(err, "{}:{} {}\n", fm.name,
+               lines[0] + 1, fm.get_line(lines[0] as int));
+        write!(err, "...\n");
+        let last_line = lines[lines.len()-1];
+        write!(err, "{}:{} {}\n", fm.name,
+               last_line + 1, fm.get_line(last_line as int));
+    } else {
+        for line in lines.iter() {
+            write!(err, "{}:{} {}\n", fm.name,
+                   *line + 1, fm.get_line(*line as int));
+        }
+    }
+    let last_line_start = format!("{}:{} ", fm.name, lines[lines.len()-1]+1);
+    let hi = cm.lookup_char_pos(sp.hi);
+    // Span seems to use half-opened interval, so subtract 1
+    let skip = last_line_start.len() + hi.col.to_uint() - 1;
+    let mut s = ~"";
+    skip.times(|| s.push_char(' '));
+    s.push_char('^');
+    print_maybe_styled(s + "\n", term::attr::ForegroundColor(lvl.color()));
+}
+
 fn print_macro_backtrace(cm: &codemap::CodeMap, sp: Span) {
     for ei in sp.expn_info.iter() {
         let ss = ei.callee.span.as_ref().map_or(~"", |span| cm.span_to_str(*span));

src/test/compile-fail/borrowck-borrow-mut-object-twice.rs

@@ -18,7 +18,7 @@ trait Foo {
 
 fn test(x: &mut Foo) {
     let _y = x.f1();
-    x.f2(); //~ ERROR cannot borrow `*x` as mutable more than once at a time
+    x.f2(); //~ ERROR cannot borrow `*x` because it is already borrowed as mutable
 }
 
 fn main() {}

src/test/compile-fail/borrowck-report-with-custom-diagnostic.rs

@@ -0,0 +1,31 @@
+#[allow(dead_code)];
+fn main() {
+    // Original borrow ends at end of function
+    let mut x = 1u;
+    let y = &mut x;
+    let z = &x; //~ ERROR cannot borrow
+}
+//~^ NOTE previous borrow ends here
+
+fn foo() {
+    match true {
+        true => {
+            // Original borrow ends at end of match arm
+            let mut x = 1u;
+            let y = &x;
+            let z = &mut x; //~ ERROR cannot borrow
+        }
+     //~^ NOTE previous borrow ends here
+        false => ()
+    }
+}
+
+fn bar() {
+    // Original borrow ends at end of closure
+    || {
+        let mut x = 1u;
+        let y = &mut x;
+        let z = &mut x; //~ ERROR cannot borrow
+    };
+ //~^ NOTE previous borrow ends here
+}

src/test/compile-fail/mut-cant-alias.rs

@@ -14,5 +14,5 @@ fn main() {
     let m = RefCell::new(0);
     let mut b = m.borrow_mut();
     let b1 = b.get();
-    let b2 = b.get(); //~ ERROR cannot borrow `b` as mutable more than once at a time
+    let b2 = b.get(); //~ ERROR cannot borrow `b` because it is already borrowed as mutable
 }

src/test/compile-fail/vec-mut-iter-borrow.rs

@@ -12,6 +12,6 @@ fn main() {
     let mut xs = ~[1, 2, 3, 4];
 
     for x in xs.mut_iter() {
-        xs.push(1) //~ ERROR cannot borrow `xs` as mutable
+        xs.push(1) //~ ERROR cannot borrow `xs` because it is already borrowed as mutable
     }
 }