Refactor suggestion diagnostic API to allow for multiple suggestions

Author: oli-obk

src/librustc_errors/diagnostic.rs

@@ -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.
@@ -87,7 +87,7 @@ impl Diagnostic {
             code: code,
             span: MultiSpan::new(),
             children: vec![],
-            suggestion: None,
+            suggestions: vec![],
         }
     }
 
@@ -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

src/librustc_errors/diagnostic_builder.rs

@@ -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);
 

src/librustc_errors/emitter.rs

@@ -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 {
+                children.push(SubDiagnostic {
+                    level: Level::Help,
+                    message: Vec::new(),
+                    span: MultiSpan::new(),
+                    render_span: Some(Suggestion(sugg.clone())),
                 });
             }
         }
@@ -1054,38 +1069,38 @@ impl EmitterWriter {
                                -> io::Result<()> {
         use std::borrow::Borrow;
 
-        let primary_span = suggestion.msp.primary_span().unwrap();
+        let primary_span = suggestion.substitutes[0].0;
         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());
-
-            // 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 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 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 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;
+                }
 
-            // 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);
+                }
             }
             emit_to_destination(&buffer.render(), level, &mut self.dst)?;
         }

src/librustc_errors/lib.rs

@@ -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>)>,
     pub msg: String,
 }
 
@@ -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,
@@ -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,
@@ -138,33 +157,37 @@ 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 {
+            push_trailing(buf, prev_line, &prev_hi, None);
+            // remove trailing newline
+            buf.pop();
+        }
+        bufs
     }
 }