From b3290d322e32a1110fd31e9a9ecd246252c1c67c Mon Sep 17 00:00:00 2001 From: Kiet Tran Date: Wed, 22 Jan 2014 00:33:37 -0500 Subject: [PATCH] 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. --- src/librustc/driver/session.rs | 3 + src/librustc/middle/borrowck/check_loans.rs | 41 ++++++--- src/librustc/middle/borrowck/mod.rs | 4 + src/libsyntax/codemap.rs | 4 - src/libsyntax/diagnostic.rs | 88 ++++++++++++++++--- .../borrowck-borrow-mut-object-twice.rs | 2 +- .../borrowck-report-with-custom-diagnostic.rs | 31 +++++++ src/test/compile-fail/mut-cant-alias.rs | 2 +- src/test/compile-fail/vec-mut-iter-borrow.rs | 2 +- 9 files changed, 144 insertions(+), 33 deletions(-) create mode 100644 src/test/compile-fail/borrowck-report-with-custom-diagnostic.rs diff --git a/src/librustc/driver/session.rs b/src/librustc/driver/session.rs index ebd6c06e243..b6341374a7d 100644 --- a/src/librustc/driver/session.rs +++ b/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) } diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index fe9371af1fe..0fd92079489 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/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 diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 74845878907..f1cccab1239 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/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) => { diff --git a/src/libsyntax/codemap.rs b/src/libsyntax/codemap.rs index 15146551370..bad95374123 100644 --- a/src/libsyntax/codemap.rs +++ b/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(); diff --git a/src/libsyntax/diagnostic.rs b/src/libsyntax/diagnostic.rs index 0eca56e2691..a9d3f6fea24 100644 --- a/src/libsyntax/diagnostic.rs +++ b/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)); diff --git a/src/test/compile-fail/borrowck-borrow-mut-object-twice.rs b/src/test/compile-fail/borrowck-borrow-mut-object-twice.rs index 502d7e017b5..c338aac2ddf 100644 --- a/src/test/compile-fail/borrowck-borrow-mut-object-twice.rs +++ b/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() {} diff --git a/src/test/compile-fail/borrowck-report-with-custom-diagnostic.rs b/src/test/compile-fail/borrowck-report-with-custom-diagnostic.rs new file mode 100644 index 00000000000..3c01045369f --- /dev/null +++ b/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 +} diff --git a/src/test/compile-fail/mut-cant-alias.rs b/src/test/compile-fail/mut-cant-alias.rs index f031467328f..5b8079b832e 100644 --- a/src/test/compile-fail/mut-cant-alias.rs +++ b/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 } diff --git a/src/test/compile-fail/vec-mut-iter-borrow.rs b/src/test/compile-fail/vec-mut-iter-borrow.rs index 19c786b553f..21ffc1ae7f9 100644 --- a/src/test/compile-fail/vec-mut-iter-borrow.rs +++ b/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 } }