Refactor internal suggestion API

oli-obk · oli-obk · commit 443332afaffb · 2017-11-03T16:30:04.000+01:00
diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs
@@ -9,6 +9,7 @@
 // except according to those terms.
 
 use CodeSuggestion;
+use SubstitutionPart;
 use Substitution;
 use Level;
 use RenderSpan;
@@ -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,
@@ -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,
@@ -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,
         });
diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs
@@ -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
@@ -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));
 
+            // 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();
@@ -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();
-                        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();
+                    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
diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs
@@ -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 {
@@ -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,
@@ -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) {
@@ -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()
     }
 }
 
diff --git a/src/libsyntax/json.rs b/src/libsyntax/json.rs
@@ -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)
                           })
                       })
diff --git a/src/test/ui/block-result/unexpected-return-on-unit.stderr b/src/test/ui/block-result/unexpected-return-on-unit.stderr
@@ -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
 

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ help: try adding a semicolon`
`13`	`13`	`help: try adding a return type`
`14`	`14`	`\|`
`15`	`15`	`18 \| fn bar() -> usize {`
`16`		`- \| ^^^^^^^^`
	`16`	`+ \| ^^^^^^^^^`
`17`	`17`
`18`	`18`	`error: aborting due to previous error`
`19`	`19`