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 internal suggestion API #45741

Merged
merged 2 commits into from
Nov 9, 2017
Merged

Refactor internal suggestion API #45741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
443332a
Refactor internal suggestion API
oli-obk Nov 3, 2017
dfe218a
Do not highlight surrounding whitespace
oli-obk Nov 6, 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
19 changes: 13 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 @@ -9,6 +9,7 @@
// except according to those terms.

use CodeSuggestion;
use SubstitutionPart;
use Substitution;
use Level;
use RenderSpan;
Expand Down Expand Up @@ -217,9 +218,11 @@ impl Diagnostic {
/// See `CodeSuggestion` for more information.
pub fn span_suggestion_short(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitution_parts: vec![Substitution {
substitutions: vec![Substitution {
parts: vec![SubstitutionPart {
snippet: suggestion,
span: sp,
substitutions: vec![suggestion],
}],
}],
msg: msg.to_owned(),
show_code_when_inline: false,
Expand All @@ -245,9 +248,11 @@ impl Diagnostic {
/// See `CodeSuggestion` for more information.
pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitution_parts: vec![Substitution {
substitutions: vec![Substitution {
parts: vec![SubstitutionPart {
snippet: suggestion,
span: sp,
substitutions: vec![suggestion],
}],
}],
msg: msg.to_owned(),
show_code_when_inline: true,
Expand All @@ -258,10 +263,12 @@ impl Diagnostic {
/// Prints out a message with multiple suggested edits of the code.
pub fn span_suggestions(&mut self, sp: Span, msg: &str, suggestions: Vec<String>) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitution_parts: vec![Substitution {
substitutions: suggestions.into_iter().map(|snippet| Substitution {
parts: vec![SubstitutionPart {
snippet,
span: sp,
substitutions: suggestions,
}],
}).collect(),
msg: msg.to_owned(),
show_code_when_inline: true,
});
Expand Down
43 changes: 24 additions & 19 deletions src/librustc_errors/emitter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,23 @@ impl Emitter for EmitterWriter {

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 &&
sugg.substitutions.len() == 1 &&
// don't display multipart suggestions as labels
sugg.substitutions[0].parts.len() == 1 &&
// don't display long messages as labels
sugg.msg.split_whitespace().count() < 10 &&
// don't display multiline suggestions as labels
sugg.substitution_parts[0].substitutions[0].find('\n').is_none() {
let substitution = &sugg.substitution_parts[0].substitutions[0];
!sugg.substitutions[0].parts[0].snippet.contains('\n') {
let substitution = &sugg.substitutions[0].parts[0].snippet;
let msg = if substitution.len() == 0 || !sugg.show_code_when_inline {
// This substitution is only removal or we explicitly don't want to show the
// code inline, don't show it
format!("help: {}", sugg.msg)
} else {
format!("help: {}: `{}`", sugg.msg, substitution)
};
primary_span.push_span_label(sugg.substitution_spans().next().unwrap(), msg);
primary_span.push_span_label(sugg.substitutions[0].parts[0].span, msg);
} else {
// if there are multiple suggestions, print them all in full
// to be consistent. We could try to figure out if we can
Expand Down Expand Up @@ -1098,14 +1098,10 @@ impl EmitterWriter {
-> io::Result<()> {
use std::borrow::Borrow;

let primary_sub = &suggestion.substitution_parts[0];
if let Some(ref cm) = self.cm {
let mut buffer = StyledBuffer::new();

let lines = cm.span_to_lines(primary_sub.span).unwrap();

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

// Render the suggestion message
buffer.append(0, &level.to_string(), Style::Level(level.clone()));
buffer.append(0, ": ", Style::HeaderMsg);
self.msg_to_buffer(&mut buffer,
Expand All @@ -1114,14 +1110,22 @@ impl EmitterWriter {
"suggestion",
Some(Style::HeaderMsg));

// Render the replacements for each suggestion
let suggestions = suggestion.splice_lines(cm.borrow());
let span_start_pos = cm.lookup_char_pos(primary_sub.span.lo());

let mut row_num = 2;
for &(ref complete, ref parts) in suggestions.iter().take(MAX_SUGGESTIONS) {
let show_underline = parts.len() == 1
&& complete.lines().count() == 1
&& parts[0].snippet.trim() != complete.trim();

let lines = cm.span_to_lines(parts[0].span).unwrap();

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

let span_start_pos = cm.lookup_char_pos(parts[0].span.lo());
let line_start = span_start_pos.line;
draw_col_separator_no_space(&mut buffer, 1, max_line_num_len + 1);
let mut row_num = 2;
for (&(ref complete, show_underline), ref sub) in suggestions
.iter().zip(primary_sub.substitutions.iter()).take(MAX_SUGGESTIONS)
{
let mut line_pos = 0;
// Only show underline if there's a single suggestion and it is a single line
let mut lines = complete.lines();
Expand All @@ -1136,12 +1140,14 @@ impl EmitterWriter {
buffer.append(row_num, line, Style::NoStyle);
line_pos += 1;
row_num += 1;
}
// Only show an underline in the suggestions if the suggestion is not the
// entirety of the code being shown and the displayed code is not multiline.
if show_underline {
draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
let sub_len = sub.trim_right().len();
Copy link
Contributor

Choose a reason for hiding this comment

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

To other reviewers, this line's change is the one that causes the output change.

let underline_start = span_start_pos.col.0;
let start = parts[0].snippet.len() - parts[0].snippet.trim_left().len();
let sub_len = parts[0].snippet.trim().len();
let underline_start = span_start_pos.col.0 + start;
let underline_end = span_start_pos.col.0 + sub_len;
for p in underline_start..underline_end {
buffer.putc(row_num,
Expand All @@ -1151,7 +1157,6 @@ impl EmitterWriter {
}
row_num += 1;
}
}

// if we elided some lines, add an ellipsis
if let Some(_) = lines.next() {
Expand Down
82 changes: 30 additions & 52 deletions src/librustc_errors/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,26 +76,34 @@ pub struct CodeSuggestion {
///
/// ```
/// vec![
/// (0..3, vec!["a", "x"]),
/// (4..7, vec!["b", "y"]),
/// Substitution { parts: vec![(0..3, "a"), (4..7, "b")] },
/// Substitution { parts: vec![(0..3, "x"), (4..7, "y")] },
/// ]
/// ```
///
/// or by replacing the entire span:
///
/// ```
/// vec![(0..7, vec!["a.b", "x.y"])]
/// vec![
/// Substitution { parts: vec![(0..7, "a.b")] },
/// Substitution { parts: vec![(0..7, "x.y")] },
/// ]
/// ```
pub substitution_parts: Vec<Substitution>,
pub substitutions: Vec<Substitution>,
pub msg: String,
pub show_code_when_inline: bool,
}

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

#[derive(Clone, Debug, PartialEq, Hash, RustcEncodable, RustcDecodable)]
pub struct SubstitutionPart {
pub span: Span,
pub substitutions: Vec<String>,
pub snippet: String,
}

pub trait CodeMapper {
Expand All @@ -109,18 +117,8 @@ pub trait CodeMapper {
}

impl CodeSuggestion {
/// Returns the number of substitutions
fn substitutions(&self) -> usize {
self.substitution_parts[0].substitutions.len()
}

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

/// Returns the assembled code suggestions and wether they should be shown with an underline.
pub fn splice_lines(&self, cm: &CodeMapper) -> Vec<(String, bool)> {
/// Returns the assembled code suggestions and whether they should be shown with an underline.
pub fn splice_lines(&self, cm: &CodeMapper) -> Vec<(String, Vec<SubstitutionPart>)> {
use syntax_pos::{CharPos, Loc, Pos};

fn push_trailing(buf: &mut String,
Expand All @@ -142,22 +140,16 @@ impl CodeSuggestion {
}
}

if self.substitution_parts.is_empty() {
return vec![(String::new(), false)];
}

let mut primary_spans: Vec<_> = self.substitution_parts
.iter()
.map(|sub| (sub.span, &sub.substitutions))
.collect();
assert!(!self.substitutions.is_empty());

self.substitutions.iter().cloned().map(|mut substitution| {
// Assumption: all spans are in the same file, and all spans
// are disjoint. Sort in ascending order.
primary_spans.sort_by_key(|sp| sp.0.lo());
substitution.parts.sort_by_key(|part| part.span.lo());

// Find the bounding span.
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 lo = substitution.parts.iter().map(|part| part.span.lo()).min().unwrap();
let hi = substitution.parts.iter().map(|part| part.span.hi()).min().unwrap();
let bounding_span = Span::new(lo, hi, NO_EXPANSION);
let lines = cm.span_to_lines(bounding_span).unwrap();
assert!(!lines.lines.is_empty());
Expand All @@ -176,26 +168,14 @@ impl CodeSuggestion {
prev_hi.col = CharPos::from_usize(0);

let mut prev_line = fm.get_line(lines.lines[0].line_index);
let mut bufs = vec![(String::new(), false); self.substitutions()];
let mut buf = String::new();

for (sp, substitutes) in primary_spans {
let cur_lo = cm.lookup_char_pos(sp.lo());
for (&mut (ref mut buf, ref mut underline), substitute) in bufs.iter_mut()
.zip(substitutes) {
for part in &substitution.parts {
let cur_lo = cm.lookup_char_pos(part.span.lo());
if prev_hi.line == cur_lo.line {
push_trailing(buf, prev_line.as_ref(), &prev_hi, Some(&cur_lo));

// Only show an underline in the suggestions if the suggestion is not the
// entirety of the code being shown and the displayed code is not multiline.
if prev_line.as_ref().unwrap().trim().len() > 0
&& !substitute.ends_with('\n')
&& substitute.lines().count() == 1
{
*underline = true;
}
push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, Some(&cur_lo));
} else {
*underline = false;
push_trailing(buf, prev_line.as_ref(), &prev_hi, None);
push_trailing(&mut buf, prev_line.as_ref(), &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) {
Expand All @@ -207,22 +187,20 @@ impl CodeSuggestion {
buf.push_str(&cur_line[..cur_lo.col.to_usize()]);
}
}
buf.push_str(substitute);
}
prev_hi = cm.lookup_char_pos(sp.hi());
buf.push_str(&part.snippet);
prev_hi = cm.lookup_char_pos(part.span.hi());
prev_line = fm.get_line(prev_hi.line - 1);
}
for &mut (ref mut 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.as_ref(), &prev_hi, None);
push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, None);
}
// remove trailing newlines
while buf.ends_with('\n') {
buf.pop();
}
}
bufs
(buf, substitution.parts)
}).collect()
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/libsyntax/json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,17 +278,17 @@ impl DiagnosticSpan {

fn from_suggestion(suggestion: &CodeSuggestion, je: &JsonEmitter)
-> Vec<DiagnosticSpan> {
suggestion.substitution_parts
suggestion.substitutions
.iter()
.flat_map(|substitution| {
substitution.substitutions.iter().map(move |suggestion| {
substitution.parts.iter().map(move |suggestion| {
let span_label = SpanLabel {
span: substitution.span,
span: suggestion.span,
is_primary: true,
label: None,
};
DiagnosticSpan::from_span_label(span_label,
Some(suggestion),
Some(&suggestion.snippet),
je)
})
})
Expand Down