From 43b10fa8ed3dd96684ec89812dc7810431cd0d01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 7 Jan 2017 21:12:33 -0800 Subject: [PATCH 1/5] Teach diagnostics to have correctly padded lists Make the suggestion list have a correct padding: ``` error[E0308]: mismatched types --> file.rs:3:20 | 3 | let x: usize = ""; | ^^ expected usize, found reference | = note: expected type `usize` = note: found type `&'static str` = help: here are some functions which might fulfill your needs: - .len() - .foo() - .bar() ``` --- src/librustc_errors/diagnostic.rs | 18 ++++++++++++++++++ src/librustc_errors/diagnostic_builder.rs | 1 + src/librustc_errors/emitter.rs | 19 +++++++++++++++++-- src/librustc_typeck/check/demand.rs | 10 ++++------ .../ui/resolve/token-error-correct-3.stderr | 6 +++--- src/test/ui/span/coerce-suggestions.stderr | 12 ++++++------ 6 files changed, 49 insertions(+), 17 deletions(-) diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs index 730ca8f9e2e44..73a8343eafc45 100644 --- a/src/librustc_errors/diagnostic.rs +++ b/src/librustc_errors/diagnostic.rs @@ -32,6 +32,7 @@ pub struct SubDiagnostic { pub message: String, pub span: MultiSpan, pub render_span: Option, + pub list: Vec, } impl Diagnostic { @@ -132,6 +133,11 @@ impl Diagnostic { self } + pub fn help_with_list(&mut self , msg: &str, list: Vec) -> &mut Self { + self.sub_with_list(Level::Help, msg, MultiSpan::new(), None, list); + self + } + pub fn span_help>(&mut self, sp: S, msg: &str) @@ -191,11 +197,23 @@ impl Diagnostic { message: &str, span: MultiSpan, render_span: Option) { + self.sub_with_list(level, message, span, render_span, vec![]); + } + + /// Convenience function for internal use, clients should use one of the + /// public methods above. + fn sub_with_list(&mut self, + level: Level, + message: &str, + span: MultiSpan, + render_span: Option, + list: Vec) { let sub = SubDiagnostic { level: level, message: message.to_owned(), span: span, render_span: render_span, + list: list, }; self.children.push(sub); } diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs index 7dfea6b8951b0..24f1b86739da6 100644 --- a/src/librustc_errors/diagnostic_builder.rs +++ b/src/librustc_errors/diagnostic_builder.rs @@ -135,6 +135,7 @@ impl<'a> DiagnosticBuilder<'a> { forward!(pub fn warn(&mut self, msg: &str) -> &mut Self); forward!(pub fn span_warn>(&mut self, sp: S, msg: &str) -> &mut Self); forward!(pub fn help(&mut self , msg: &str) -> &mut Self); + forward!(pub fn help_with_list(&mut self , msg: &str, list: Vec) -> &mut Self); forward!(pub fn span_help>(&mut self, sp: S, msg: &str) diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 808fe504b95cd..015997211bb3f 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -699,6 +699,7 @@ impl EmitterWriter { .to_string(), span: MultiSpan::new(), render_span: None, + list: vec![], }); } } @@ -923,14 +924,28 @@ impl EmitterWriter { } }, None => { + let msg = if child.list.len() == 0 { + child.message.to_owned() + } else { + format!("{}\n{}", + &child.message, + &child.list.iter().map(|item| { + format!("{} - {}", + (0..max_line_num_len) + .map(|_| " ") + .collect::(), + item) + }).collect::>() + .join("\n")) + }; match self.emit_message_default(&child.span, - &child.message, + &msg, &None, &child.level, max_line_num_len, true) { Err(e) => panic!("failed to emit error: {}", e), - _ => () + _ => (), } } } diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 393d9341a0843..06d629c173256 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -70,9 +70,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ast::DUMMY_NODE_ID); let mut err = self.report_mismatched_types(&cause, expected, expr_ty, e); if suggestions.len() > 0 { - err.help(&format!("here are some functions which \ - might fulfill your needs:\n - {}", - self.get_best_match(&suggestions))); + err.help_with_list("here are some functions which might fulfill your needs:", + self.get_best_match(&suggestions)); }; err.emit(); } @@ -88,15 +87,14 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { }) } - fn display_suggested_methods(&self, methods: &[AssociatedItem]) -> String { + fn display_suggested_methods(&self, methods: &[AssociatedItem]) -> Vec { methods.iter() .take(5) .map(|method| self.format_method_suggestion(&*method)) .collect::>() - .join("\n - ") } - fn get_best_match(&self, methods: &[AssociatedItem]) -> String { + fn get_best_match(&self, methods: &[AssociatedItem]) -> Vec { let no_argument_methods: Vec<_> = methods.iter() .filter(|ref x| self.has_no_input_arg(&*x)) diff --git a/src/test/ui/resolve/token-error-correct-3.stderr b/src/test/ui/resolve/token-error-correct-3.stderr index 0b15c23909ca6..bb6f310fec813 100644 --- a/src/test/ui/resolve/token-error-correct-3.stderr +++ b/src/test/ui/resolve/token-error-correct-3.stderr @@ -37,9 +37,9 @@ error[E0308]: mismatched types = note: expected type `()` = note: found type `std::result::Result` = help: here are some functions which might fulfill your needs: - - .unwrap() - - .unwrap_err() - - .unwrap_or_default() + - .unwrap() + - .unwrap_err() + - .unwrap_or_default() error: aborting due to previous error diff --git a/src/test/ui/span/coerce-suggestions.stderr b/src/test/ui/span/coerce-suggestions.stderr index e316466150703..ed1bcd318a4f6 100644 --- a/src/test/ui/span/coerce-suggestions.stderr +++ b/src/test/ui/span/coerce-suggestions.stderr @@ -7,8 +7,8 @@ error[E0308]: mismatched types = note: expected type `usize` = note: found type `std::string::String` = help: here are some functions which might fulfill your needs: - - .capacity() - - .len() + - .capacity() + - .len() error[E0308]: mismatched types --> $DIR/coerce-suggestions.rs:23:19 @@ -19,10 +19,10 @@ error[E0308]: mismatched types = note: expected type `&str` = note: found type `std::string::String` = help: here are some functions which might fulfill your needs: - - .as_str() - - .trim() - - .trim_left() - - .trim_right() + - .as_str() + - .trim() + - .trim_left() + - .trim_right() error[E0308]: mismatched types --> $DIR/coerce-suggestions.rs:30:10 From f65a907ef9e21a350118d2efa323ebf49c3ec1cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 8 Jan 2017 12:56:34 -0800 Subject: [PATCH 2/5] Use fold instead of collect/join and add comments --- src/librustc_errors/emitter.rs | 49 +++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 015997211bb3f..88373bf988da7 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -924,19 +924,48 @@ impl EmitterWriter { } }, None => { + // Diagnostic with lists need to render the list items at the + // appropriate depth and composed into the body of the message. let msg = if child.list.len() == 0 { + // Diagnostics without lists just need the original message child.message.to_owned() } else { - format!("{}\n{}", - &child.message, - &child.list.iter().map(|item| { - format!("{} - {}", - (0..max_line_num_len) - .map(|_| " ") - .collect::(), - item) - }).collect::>() - .join("\n")) + // Diagnostic with a list of items needs to be rendered with the + // appropriate padding at the left to have a consistent margin with + // the `note: ` text. + + // Add as many ` ` chars at the beggining to align the `- item` + // text to the beggining of the `note: ` text. The extra 9 ` ` is + // the padding that's always needed to align to the `note: `. + let padding = (0..max_line_num_len + 9) + .map(|_| " ") + .collect::(); + + // Concatenate the message and all the list items, properly aligned + child.list.iter().fold(child.message.to_owned(), |mut acc, x| { + acc.push_str("\n"); + acc.push_str(&padding); + acc.push_str("- "); + acc.push_str(x); + acc + }) + // msg will now be: + // + // child.message's content + // - item 1 + // - item 2 + // + // and the diagnostic will look like + // + // error: message + // --> file.rs:3:20 + // | + // 3 | + // | ^^^^ highlight + // | + // = help: child.message's content + // - item 1 + // - item 2 }; match self.emit_message_default(&child.span, &msg, From b206064fc86b356759e4cbfc60407b426c362cac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 8 Jan 2017 14:00:57 -0800 Subject: [PATCH 3/5] Teach diagnostics to correct margin on multiline messages Make any diagnostic line to have the correct margin to align with the first line: ``` error: message --> file.rs:3:20 | 3 | | ^^^^ | = note: this is a multiline note with a correct margin = note: this is a single line note = help: here are some functions which might fulfill your needs: - .len() - .foo() - .bar() = suggestion: this is a multiline suggestion with a correct margin ``` --- src/librustc_errors/diagnostic.rs | 18 ------ src/librustc_errors/diagnostic_builder.rs | 1 - src/librustc_errors/emitter.rs | 71 ++++++++--------------- src/librustc_typeck/check/demand.rs | 7 ++- 4 files changed, 28 insertions(+), 69 deletions(-) diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs index 73a8343eafc45..730ca8f9e2e44 100644 --- a/src/librustc_errors/diagnostic.rs +++ b/src/librustc_errors/diagnostic.rs @@ -32,7 +32,6 @@ pub struct SubDiagnostic { pub message: String, pub span: MultiSpan, pub render_span: Option, - pub list: Vec, } impl Diagnostic { @@ -133,11 +132,6 @@ impl Diagnostic { self } - pub fn help_with_list(&mut self , msg: &str, list: Vec) -> &mut Self { - self.sub_with_list(Level::Help, msg, MultiSpan::new(), None, list); - self - } - pub fn span_help>(&mut self, sp: S, msg: &str) @@ -197,23 +191,11 @@ impl Diagnostic { message: &str, span: MultiSpan, render_span: Option) { - self.sub_with_list(level, message, span, render_span, vec![]); - } - - /// Convenience function for internal use, clients should use one of the - /// public methods above. - fn sub_with_list(&mut self, - level: Level, - message: &str, - span: MultiSpan, - render_span: Option, - list: Vec) { let sub = SubDiagnostic { level: level, message: message.to_owned(), span: span, render_span: render_span, - list: list, }; self.children.push(sub); } diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs index 24f1b86739da6..7dfea6b8951b0 100644 --- a/src/librustc_errors/diagnostic_builder.rs +++ b/src/librustc_errors/diagnostic_builder.rs @@ -135,7 +135,6 @@ impl<'a> DiagnosticBuilder<'a> { forward!(pub fn warn(&mut self, msg: &str) -> &mut Self); forward!(pub fn span_warn>(&mut self, sp: S, msg: &str) -> &mut Self); forward!(pub fn help(&mut self , msg: &str) -> &mut Self); - forward!(pub fn help_with_list(&mut self , msg: &str, list: Vec) -> &mut Self); forward!(pub fn span_help>(&mut self, sp: S, msg: &str) diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 88373bf988da7..dbef287f11337 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -699,11 +699,25 @@ impl EmitterWriter { .to_string(), span: MultiSpan::new(), render_span: None, - list: vec![], }); } } + fn msg_with_padding(&self, msg: &str, padding: usize) -> String { + let padding = (0..padding) + .map(|_| " ") + .collect::(); + msg.split('\n').enumerate().fold("".to_owned(), |mut acc, x| { + if x.0 != 0 { + acc.push_str("\n"); + // Align every line with first one. + acc.push_str(&padding); + } + acc.push_str(&x.1); + acc + }) + } + fn emit_message_default(&mut self, msp: &MultiSpan, msg: &str, @@ -722,7 +736,10 @@ impl EmitterWriter { draw_note_separator(&mut buffer, 0, max_line_num_len + 1); buffer.append(0, &level.to_string(), Style::HeaderMsg); buffer.append(0, ": ", Style::NoStyle); - buffer.append(0, msg, Style::NoStyle); + + // The extra 9 ` ` is the padding that's always needed to align to the `note: `. + let message = self.msg_with_padding(msg, max_line_num_len + 9); + buffer.append(0, &message, Style::NoStyle); } else { buffer.append(0, &level.to_string(), Style::Level(level.clone())); match code { @@ -855,7 +872,10 @@ impl EmitterWriter { buffer.append(0, &level.to_string(), Style::Level(level.clone())); buffer.append(0, ": ", Style::HeaderMsg); - buffer.append(0, msg, Style::HeaderMsg); + + // The extra 15 ` ` is the padding that's always needed to align to the `suggestion: `. + let message = self.msg_with_padding(msg, max_line_num_len + 15); + buffer.append(0, &message, Style::HeaderMsg); let lines = cm.span_to_lines(primary_span).unwrap(); @@ -924,51 +944,8 @@ impl EmitterWriter { } }, None => { - // Diagnostic with lists need to render the list items at the - // appropriate depth and composed into the body of the message. - let msg = if child.list.len() == 0 { - // Diagnostics without lists just need the original message - child.message.to_owned() - } else { - // Diagnostic with a list of items needs to be rendered with the - // appropriate padding at the left to have a consistent margin with - // the `note: ` text. - - // Add as many ` ` chars at the beggining to align the `- item` - // text to the beggining of the `note: ` text. The extra 9 ` ` is - // the padding that's always needed to align to the `note: `. - let padding = (0..max_line_num_len + 9) - .map(|_| " ") - .collect::(); - - // Concatenate the message and all the list items, properly aligned - child.list.iter().fold(child.message.to_owned(), |mut acc, x| { - acc.push_str("\n"); - acc.push_str(&padding); - acc.push_str("- "); - acc.push_str(x); - acc - }) - // msg will now be: - // - // child.message's content - // - item 1 - // - item 2 - // - // and the diagnostic will look like - // - // error: message - // --> file.rs:3:20 - // | - // 3 | - // | ^^^^ highlight - // | - // = help: child.message's content - // - item 1 - // - item 2 - }; match self.emit_message_default(&child.span, - &msg, + &child.message, &None, &child.level, max_line_num_len, diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 06d629c173256..3bc3d1a2c970e 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -70,15 +70,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ast::DUMMY_NODE_ID); let mut err = self.report_mismatched_types(&cause, expected, expr_ty, e); if suggestions.len() > 0 { - err.help_with_list("here are some functions which might fulfill your needs:", - self.get_best_match(&suggestions)); + err.help(&format!("here are some functions which \ + might fulfill your needs:\n{}", + self.get_best_match(&suggestions).join("\n"))); }; err.emit(); } } fn format_method_suggestion(&self, method: &AssociatedItem) -> String { - format!(".{}({})", + format!("- .{}({})", method.name, if self.has_no_input_arg(method) { "" From 690476191db427ab8603876ef9a8a929222e71aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 8 Jan 2017 21:18:24 -0800 Subject: [PATCH 4/5] Remove magic number --- src/librustc_errors/emitter.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index dbef287f11337..2640ff62d5b6d 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -737,8 +737,8 @@ impl EmitterWriter { buffer.append(0, &level.to_string(), Style::HeaderMsg); buffer.append(0, ": ", Style::NoStyle); - // The extra 9 ` ` is the padding that's always needed to align to the `note: `. - let message = self.msg_with_padding(msg, max_line_num_len + 9); + // The extra 3 ` ` is the padding that's always needed to align to the `note: `. + let message = self.msg_with_padding(msg, max_line_num_len + "note: ".len() + 3); buffer.append(0, &message, Style::NoStyle); } else { buffer.append(0, &level.to_string(), Style::Level(level.clone())); @@ -873,8 +873,8 @@ impl EmitterWriter { buffer.append(0, &level.to_string(), Style::Level(level.clone())); buffer.append(0, ": ", Style::HeaderMsg); - // The extra 15 ` ` is the padding that's always needed to align to the `suggestion: `. - let message = self.msg_with_padding(msg, max_line_num_len + 15); + // The extra 3 ` ` is the padding that's always needed to align to the `suggestion: `. + let message = self.msg_with_padding(msg, max_line_num_len + "suggestion: ".len() + 3); buffer.append(0, &message, Style::HeaderMsg); let lines = cm.span_to_lines(primary_span).unwrap(); From 04e4a60b45ec4debd20be2327cb6859271502c05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 9 Jan 2017 09:07:34 -0800 Subject: [PATCH 5/5] Deduplicate and document logic --- src/librustc_errors/emitter.rs | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 2640ff62d5b6d..77c6c36836417 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -703,10 +703,29 @@ impl EmitterWriter { } } - fn msg_with_padding(&self, msg: &str, padding: usize) -> String { - let padding = (0..padding) + /// Add a left margin to every line but the first, given a padding length and the label being + /// displayed. + fn msg_with_padding(&self, msg: &str, padding: usize, label: &str) -> String { + // The extra 5 ` ` is padding that's always needed to align to the `note: `: + // + // error: message + // --> file.rs:13:20 + // | + // 13 | + // | ^^^^ + // | + // = note: multiline + // message + // ++^^^----xx + // | | | | + // | | | magic `2` + // | | length of label + // | magic `3` + // `max_line_num_len` + let padding = (0..padding + label.len() + 5) .map(|_| " ") .collect::(); + msg.split('\n').enumerate().fold("".to_owned(), |mut acc, x| { if x.0 != 0 { acc.push_str("\n"); @@ -737,8 +756,7 @@ impl EmitterWriter { buffer.append(0, &level.to_string(), Style::HeaderMsg); buffer.append(0, ": ", Style::NoStyle); - // The extra 3 ` ` is the padding that's always needed to align to the `note: `. - let message = self.msg_with_padding(msg, max_line_num_len + "note: ".len() + 3); + let message = self.msg_with_padding(msg, max_line_num_len, "note"); buffer.append(0, &message, Style::NoStyle); } else { buffer.append(0, &level.to_string(), Style::Level(level.clone())); @@ -873,8 +891,7 @@ impl EmitterWriter { buffer.append(0, &level.to_string(), Style::Level(level.clone())); buffer.append(0, ": ", Style::HeaderMsg); - // The extra 3 ` ` is the padding that's always needed to align to the `suggestion: `. - let message = self.msg_with_padding(msg, max_line_num_len + "suggestion: ".len() + 3); + let message = self.msg_with_padding(msg, max_line_num_len, "suggestion"); buffer.append(0, &message, Style::HeaderMsg); let lines = cm.span_to_lines(primary_span).unwrap();