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

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 1 commit
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
27 changes: 17 additions & 10 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 {
span: sp,
substitutions: vec![suggestion],
substitutions: vec![Substitution {
parts: vec![SubstitutionPart {
snippet: suggestion,
span: sp,
}],
}],
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 {
span: sp,
substitutions: vec![suggestion],
substitutions: vec![Substitution {
parts: vec![SubstitutionPart {
snippet: suggestion,
span: sp,
}],
}],
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 {
span: sp,
substitutions: suggestions,
}],
substitutions: suggestions.into_iter().map(|snippet| Substitution {
parts: vec![SubstitutionPart {
snippet,
span: sp,
}],
}).collect(),
msg: msg.to_owned(),
show_code_when_inline: true,
});
Expand Down
74 changes: 39 additions & 35 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,30 +1098,34 @@ 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,
&[(suggestion.msg.to_owned(), Style::NoStyle)],
max_line_num_len,
"suggestion",
Some(Style::HeaderMsg));
&[(suggestion.msg.to_owned(), Style::NoStyle)],
max_line_num_len,
"suggestion",
Some(Style::HeaderMsg));
Copy link
Contributor

Choose a reason for hiding this comment

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

Realign to opening paren.


// 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 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)
{
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 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,21 +1140,21 @@ 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 underline_end = span_start_pos.col.0 + sub_len;
for p in underline_start..underline_end {
buffer.putc(row_num,
max_line_num_len + 3 + p,
'^',
Style::UnderlinePrimary);
}
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 = parts[0].snippet.len();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to include trailing whitespace in the suggestions? I feel it looks slightly cleaner when looking at it in the terminal as it is pointing at the code you need to write only, while it is still there for the RLS to suggest the properly formatted code.

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 thought it was less correct, but you're right, there's no gain in knowing about the whitespace in the terminal output. I'll fix it.

let underline_start = span_start_pos.col.0;
let underline_end = span_start_pos.col.0 + sub_len;
for p in underline_start..underline_end {
buffer.putc(row_num,
max_line_num_len + 3 + p,
'^',
Style::UnderlinePrimary);
}
row_num += 1;
}

// if we elided some lines, add an ellipsis
Expand Down
132 changes: 55 additions & 77 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,60 +140,42 @@ 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();

// 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());

// 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 bounding_span = Span::new(lo, hi, NO_EXPANSION);
let lines = cm.span_to_lines(bounding_span).unwrap();
assert!(!lines.lines.is_empty());

// To build up the result, we do this for each span:
// - push the line segment trailing the previous span
// (at the beginning a "phantom" span pointing at the start of the line)
// - push lines between the previous and current span (if any)
// - if the previous and current span are not on the same line
// push the line segment leading up to the current span
// - splice in the span substitution
//
// Finally push the trailing line segment of the last span
let fm = &lines.file;
let mut prev_hi = cm.lookup_char_pos(bounding_span.lo());
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()];

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) {
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.
substitution.parts.sort_by_key(|part| part.span.lo());

// Find the bounding span.
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());

// To build up the result, we do this for each span:
// - push the line segment trailing the previous span
// (at the beginning a "phantom" span pointing at the start of the line)
// - push lines between the previous and current span (if any)
// - if the previous and current span are not on the same line
// push the line segment leading up to the current span
// - splice in the span substitution
//
// Finally push the trailing line segment of the last span
let fm = &lines.file;
let mut prev_hi = cm.lookup_char_pos(bounding_span.lo());
prev_hi.col = CharPos::from_usize(0);

let mut prev_line = fm.get_line(lines.lines[0].line_index);
let mut buf = String::new();

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);
buf.push_str(&part.snippet);
prev_hi = cm.lookup_char_pos(part.span.hi());
prev_line = fm.get_line(prev_hi.line - 1);
}
prev_hi = cm.lookup_char_pos(sp.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
2 changes: 1 addition & 1 deletion src/test/ui/block-result/unexpected-return-on-unit.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ help: try adding a semicolon
help: try adding a return type
|
18 | fn bar() -> usize {
| ^^^^^^^^
| ^^^^^^^^^

error: aborting due to previous error