Skip to content

Commit

Permalink
Rollup merge of rust-lang#41876 - oli-obk:diagnosing_diagnostics, r=n…
Browse files Browse the repository at this point in the history
…agisa

Refactor suggestion diagnostic API to allow for multiple suggestions

r? @jonathandturner

cc @nrc @petrochenkov
  • Loading branch information
Mark-Simulacrum authored May 13, 2017
2 parents 2519e90 + 3f2bbe3 commit 1f72662
Show file tree
Hide file tree
Showing 20 changed files with 392 additions and 257 deletions.
25 changes: 19 additions & 6 deletions src/librustc_errors/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// except according to those terms.

use CodeSuggestion;
use Substitution;
use Level;
use RenderSpan;
use std::fmt;
Expand All @@ -23,7 +24,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 +88,7 @@ impl Diagnostic {
code: code,
span: MultiSpan::new(),
children: vec![],
suggestion: None,
suggestions: vec![],
}
}

Expand Down Expand Up @@ -204,10 +205,22 @@ 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 {
substitution_parts: vec![Substitution {
span: sp,
substitutions: 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 {
substitution_parts: vec![Substitution {
span: sp,
substitutions: suggestions,
}],
msg: msg.to_owned(),
});
self
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_errors/diagnostic_builder.rs
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
89 changes: 54 additions & 35 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,23 +35,32 @@ 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());
// don't display multispans as labels
if sugg.substitutes.len() == 1 &&
if let Some((sugg, rest)) = db.suggestions.split_first() {
if rest.is_empty() &&
// don't display multipart suggestions as labels
sugg.substitution_parts.len() == 1 &&
// don't display multi-suggestions as labels
sugg.substitutions() == 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.substitution_parts[0].substitutions[0].find('\n').is_none() {
let substitution = &sugg.substitution_parts[0].substitutions[0];
let msg = format!("help: {} `{}`", sugg.msg, substitution);
primary_span.push_span_label(sugg.substitution_spans().next().unwrap(), msg);
} else {
children.push(SubDiagnostic {
level: Level::Help,
message: Vec::new(),
span: MultiSpan::new(),
render_span: Some(Suggestion(sugg)),
});
// 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 {
children.push(SubDiagnostic {
level: Level::Help,
message: Vec::new(),
span: MultiSpan::new(),
render_span: Some(Suggestion(sugg.clone())),
});
}
}
}

Expand All @@ -66,6 +75,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 +1067,44 @@ impl EmitterWriter {
-> io::Result<()> {
use std::borrow::Borrow;

let primary_span = suggestion.msp.primary_span().unwrap();
let primary_span = suggestion.substitution_spans().next().unwrap();
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
106 changes: 75 additions & 31 deletions src/librustc_errors/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#![feature(staged_api)]
#![feature(range_contains)]
#![feature(libc)]
#![feature(conservative_impl_trait)]

extern crate term;
extern crate libc;
Expand Down Expand Up @@ -65,11 +66,35 @@ 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 substitution_parts: Vec<Substitution>,
pub msg: String,
}

#[derive(Clone, Debug, PartialEq, RustcEncodable, RustcDecodable)]
/// See the docs on `CodeSuggestion::substitutions`
pub struct Substitution {
pub span: Span,
pub substitutions: Vec<String>,
}

pub trait CodeMapper {
fn lookup_char_pos(&self, pos: BytePos) -> Loc;
fn span_to_lines(&self, sp: Span) -> FileLinesResult;
Expand All @@ -79,8 +104,18 @@ pub trait CodeMapper {
}

impl CodeSuggestion {
/// Returns the assembled code suggestion.
pub fn splice_lines(&self, cm: &CodeMapper) -> String {
/// Returns the number of substitutions
fn substitutions(&self) -> usize {
self.substitution_parts[0].substitutions.len()
}

/// Returns the number of substitutions
pub fn substitution_spans<'a>(&'a self) -> impl Iterator<Item = Span> + 'a {
self.substitution_parts.iter().map(|sub| sub.span)
}

/// 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 +137,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.substitution_parts.is_empty() {
return vec![String::new()];
}

let mut primary_spans: Vec<_> = self.substitution_parts
.iter()
.map(|sub| (sub.span, &sub.substitutions))
.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 +175,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.substitutions()];

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 {
// 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

0 comments on commit 1f72662

Please sign in to comment.