Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor suggestion diagnostic API to allow for multiple suggestions #41876

Merged
merged 7 commits into from
May 13, 2017
Merged

Refactor suggestion diagnostic API to allow for multiple suggestions #41876

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
67d762d
Refactor suggestion diagnostic API to allow for multiple suggestions
oli-obk May 9, 2017
e2f781c
Example usage of multiple suggestions
oli-obk May 10, 2017
9d51d6b
Fix tidy issues
oli-obk May 10, 2017
f859f25
Update a compile-fail test
oli-obk May 10, 2017
644ce5e
Address PR reviews
oli-obk May 11, 2017
a54bbf2
Weave the span of an import through the resolve code
oli-obk May 12, 2017
3f2bbe3
Don't use a DUMMY_SP for reporting issues with crate imports
oli-obk May 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/librustc_errors/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub struct Diagnostic {
pub code: Option<String>,
pub span: MultiSpan,
pub children: Vec<SubDiagnostic>,
pub suggestion: Option<CodeSuggestion>,
pub suggestions: Vec<CodeSuggestion>,
}

/// For example a note attached to an error.
Expand Down Expand Up @@ -87,7 +87,7 @@ impl Diagnostic {
code: code,
span: MultiSpan::new(),
children: vec![],
suggestion: None,
suggestions: vec![],
}
}

Expand Down Expand Up @@ -204,10 +204,16 @@ impl Diagnostic {
///
/// See `diagnostic::CodeSuggestion` for more information.
pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
assert!(self.suggestion.is_none());
self.suggestion = Some(CodeSuggestion {
msp: sp.into(),
substitutes: vec![suggestion],
self.suggestions.push(CodeSuggestion {
substitutes: vec![(sp, vec![suggestion])],
msg: msg.to_owned(),
});
self
}

pub fn span_suggestions(&mut self, sp: Span, msg: &str, suggestions: Vec<String>) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitutes: vec![(sp, suggestions)],
msg: msg.to_owned(),
});
self
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_errors/diagnostic_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ impl<'a> DiagnosticBuilder<'a> {
msg: &str,
suggestion: String)
-> &mut Self);
forward!(pub fn span_suggestions(&mut self,
sp: Span,
msg: &str,
suggestions: Vec<String>)
-> &mut Self);
forward!(pub fn set_span<S: Into<MultiSpan>>(&mut self, sp: S) -> &mut Self);
forward!(pub fn code(&mut self, s: String) -> &mut Self);

Expand Down
81 changes: 53 additions & 28 deletions src/librustc_errors/emitter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,37 @@ impl Emitter for EmitterWriter {
let mut primary_span = db.span.clone();
let mut children = db.children.clone();

if let Some(sugg) = db.suggestion.clone() {
assert_eq!(sugg.msp.primary_spans().len(), sugg.substitutes.len());
if db.suggestions.len() == 1 {
let sugg = &db.suggestions[0];
// don't display multispans as labels
if sugg.substitutes.len() == 1 &&
// don't display multi-suggestions as labels
sugg.substitutes[0].1.len() == 1 &&
// don't display long messages as labels
sugg.msg.split_whitespace().count() < 10 &&
// don't display multiline suggestions as labels
sugg.substitutes[0].find('\n').is_none() {
let msg = format!("help: {} `{}`", sugg.msg, sugg.substitutes[0]);
primary_span.push_span_label(sugg.msp.primary_spans()[0], msg);
sugg.substitutes[0].1[0].find('\n').is_none() {
let msg = format!("help: {} `{}`", sugg.msg, sugg.substitutes[0].1[0]);
primary_span.push_span_label(sugg.substitutes[0].0, msg);
} else {
children.push(SubDiagnostic {
level: Level::Help,
message: Vec::new(),
span: MultiSpan::new(),
render_span: Some(Suggestion(sugg)),
render_span: Some(Suggestion(sugg.clone())),
});
}
} else {
// if there are multiple suggestions, print them all in full
// to be consistent. We could try to figure out if we can
// make one (or the first one) inline, but that would give
// undue importance to a semi-random suggestion
for sugg in &db.suggestions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the code somewhat. Consider doing this instead:

if let Some((sugg, rest)) = db.suggestions.split_first() {
    if rest.is_empty() && sugg.substitutes.len() == 1 && ... { 
        show `sugg` as label 
    } else {
        for sugg in &db.suggestions {
        show `sugg` proper
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know about split_first. That's a great method.

children.push(SubDiagnostic {
level: Level::Help,
message: Vec::new(),
span: MultiSpan::new(),
render_span: Some(Suggestion(sugg.clone())),
});
}
}
Expand All @@ -66,6 +81,10 @@ impl Emitter for EmitterWriter {

/// maximum number of lines we will print for each error; arbitrary.
pub const MAX_HIGHLIGHT_LINES: usize = 6;
/// maximum number of suggestions to be shown
///
/// Arbitrary, but taken from trait import suggestion limit
pub const MAX_SUGGESTIONS: usize = 4;

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum ColorConfig {
Expand Down Expand Up @@ -1054,38 +1073,44 @@ impl EmitterWriter {
-> io::Result<()> {
use std::borrow::Borrow;

let primary_span = suggestion.msp.primary_span().unwrap();
let primary_span = suggestion.substitutes[0].0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be abstracted into a method IMO.

if let Some(ref cm) = self.cm {
let mut buffer = StyledBuffer::new();

buffer.append(0, &level.to_string(), Style::Level(level.clone()));
buffer.append(0, ": ", Style::HeaderMsg);
self.msg_to_buffer(&mut buffer,
&[(suggestion.msg.to_owned(), Style::NoStyle)],
max_line_num_len,
"suggestion",
Some(Style::HeaderMsg));

let lines = cm.span_to_lines(primary_span).unwrap();

assert!(!lines.lines.is_empty());

let complete = suggestion.splice_lines(cm.borrow());
buffer.append(0, &level.to_string(), Style::Level(level.clone()));
buffer.append(0, ": ", Style::HeaderMsg);
self.msg_to_buffer(&mut buffer,
&[(suggestion.msg.to_owned(), Style::NoStyle)],
max_line_num_len,
"suggestion",
Some(Style::HeaderMsg));

// print the suggestion without any line numbers, but leave
// space for them. This helps with lining up with previous
// snippets from the actual error being reported.
let mut lines = complete.lines();
let suggestions = suggestion.splice_lines(cm.borrow());
let mut row_num = 1;
for line in lines.by_ref().take(MAX_HIGHLIGHT_LINES) {
draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
buffer.append(row_num, line, Style::NoStyle);
row_num += 1;
}
for complete in suggestions.iter().take(MAX_SUGGESTIONS) {

// print the suggestion without any line numbers, but leave
// space for them. This helps with lining up with previous
// snippets from the actual error being reported.
let mut lines = complete.lines();
for line in lines.by_ref().take(MAX_HIGHLIGHT_LINES) {
draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
buffer.append(row_num, line, Style::NoStyle);
row_num += 1;
}

// if we elided some lines, add an ellipsis
if let Some(_) = lines.next() {
buffer.append(row_num, "...", Style::NoStyle);
// if we elided some lines, add an ellipsis
if let Some(_) = lines.next() {
buffer.append(row_num, "...", Style::NoStyle);
}
}
if suggestions.len() > MAX_SUGGESTIONS {
let msg = format!("and {} other candidates", suggestions.len() - MAX_SUGGESTIONS);
buffer.append(row_num, &msg, Style::NoStyle);
}
emit_to_destination(&buffer.render(), level, &mut self.dst)?;
}
Expand Down
88 changes: 57 additions & 31 deletions src/librustc_errors/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,25 @@ pub enum RenderSpan {

#[derive(Clone, Debug, PartialEq, RustcEncodable, RustcDecodable)]
pub struct CodeSuggestion {
pub msp: MultiSpan,
pub substitutes: Vec<String>,
/// Each substitute can have multiple variants due to multiple
/// applicable suggestions
///
/// `foo.bar` might be replaced with `a.b` or `x.y` by replacing
/// `foo` and `bar` on their own:
///
/// ```
/// vec![
/// (0..3, vec!["a", "x"]),
/// (4..7, vec!["b", "y"]),
/// ]
/// ```
///
/// or by replacing the entire span:
///
/// ```
/// vec![(0..7, vec!["a.b", "x.y"])]
/// ```
pub substitutes: Vec<(Span, Vec<String>)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes code fairly ugly. Can we just get a struct with named fields for this?

pub msg: String,
}

Expand All @@ -79,8 +96,8 @@ pub trait CodeMapper {
}

impl CodeSuggestion {
/// Returns the assembled code suggestion.
pub fn splice_lines(&self, cm: &CodeMapper) -> String {
/// Returns the assembled code suggestions.
pub fn splice_lines(&self, cm: &CodeMapper) -> Vec<String> {
use syntax_pos::{CharPos, Loc, Pos};

fn push_trailing(buf: &mut String,
Expand All @@ -102,20 +119,22 @@ impl CodeSuggestion {
}
}

let mut primary_spans = self.msp.primary_spans().to_owned();

assert_eq!(primary_spans.len(), self.substitutes.len());
if primary_spans.is_empty() {
return format!("");
if self.substitutes.is_empty() {
return vec![String::new()];
}

let mut primary_spans: Vec<_> = self.substitutes
.iter()
.map(|&(sp, ref sub)| (sp, sub))
.collect();

// Assumption: all spans are in the same file, and all spans
// are disjoint. Sort in ascending order.
primary_spans.sort_by_key(|sp| sp.lo);
primary_spans.sort_by_key(|sp| sp.0.lo);

// Find the bounding span.
let lo = primary_spans.iter().map(|sp| sp.lo).min().unwrap();
let hi = primary_spans.iter().map(|sp| sp.hi).min().unwrap();
let lo = primary_spans.iter().map(|sp| sp.0.lo).min().unwrap();
let hi = primary_spans.iter().map(|sp| sp.0.hi).min().unwrap();
let bounding_span = Span {
lo: lo,
hi: hi,
Expand All @@ -138,33 +157,40 @@ impl CodeSuggestion {
prev_hi.col = CharPos::from_usize(0);

let mut prev_line = fm.get_line(lines.lines[0].line_index);
let mut buf = String::new();
let mut bufs = vec![String::new(); self.substitutes[0].1.len()];

for (sp, substitute) in primary_spans.iter().zip(self.substitutes.iter()) {
for (sp, substitutes) in primary_spans {
let cur_lo = cm.lookup_char_pos(sp.lo);
if prev_hi.line == cur_lo.line {
push_trailing(&mut buf, prev_line, &prev_hi, Some(&cur_lo));
} else {
push_trailing(&mut buf, prev_line, &prev_hi, None);
// push lines between the previous and current span (if any)
for idx in prev_hi.line..(cur_lo.line - 1) {
if let Some(line) = fm.get_line(idx) {
buf.push_str(line);
buf.push('\n');
for (buf, substitute) in bufs.iter_mut().zip(substitutes) {
if prev_hi.line == cur_lo.line {
push_trailing(buf, prev_line, &prev_hi, Some(&cur_lo));
} else {
push_trailing(buf, prev_line, &prev_hi, None);
// push lines between the previous and current span (if any)
for idx in prev_hi.line..(cur_lo.line - 1) {
if let Some(line) = fm.get_line(idx) {
buf.push_str(line);
buf.push('\n');
}
}
if let Some(cur_line) = fm.get_line(cur_lo.line - 1) {
buf.push_str(&cur_line[..cur_lo.col.to_usize()]);
}
}
if let Some(cur_line) = fm.get_line(cur_lo.line - 1) {
buf.push_str(&cur_line[..cur_lo.col.to_usize()]);
}
buf.push_str(substitute);
}
buf.push_str(substitute);
prev_hi = cm.lookup_char_pos(sp.hi);
prev_line = fm.get_line(prev_hi.line - 1);
}
push_trailing(&mut buf, prev_line, &prev_hi, None);
// remove trailing newline
buf.pop();
buf
for buf in &mut bufs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is weird, mostly due to how push_trailing is used in the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean. The code is exactly the same as it was before, but produces multiple suggestions instead of one. The only change i made other than the multiple suggestion part is to not add the trailing line if the replacement already produced a newline character and is thus finished.

// if the replacement already ends with a newline, don't print the next line
if !buf.ends_with('\n') {
push_trailing(buf, prev_line, &prev_hi, None);
}
// remove trailing newline
buf.pop();
}
bufs
}
}

Expand Down
Loading