Address PR reviews

Author: oli-obk

src/librustc_errors/diagnostic.rs

@@ -9,6 +9,7 @@
 // except according to those terms.
 
 use CodeSuggestion;
+use Substitution;
 use Level;
 use RenderSpan;
 use std::fmt;
@@ -205,15 +206,21 @@ impl Diagnostic {
     /// See `diagnostic::CodeSuggestion` for more information.
     pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
         self.suggestions.push(CodeSuggestion {
-            substitutes: vec![(sp, vec![suggestion])],
+            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 {
-            substitutes: vec![(sp, suggestions)],
+            substitution_parts: vec![Substitution {
+                span: sp,
+                substitutions: suggestions,
+            }],
             msg: msg.to_owned(),
         });
         self

src/librustc_errors/emitter.rs

@@ -35,38 +35,32 @@ impl Emitter for EmitterWriter {
         let mut primary_span = db.span.clone();
         let mut children = db.children.clone();
 
-        if db.suggestions.len() == 1 {
-            let sugg = &db.suggestions[0];
-            // 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.substitutes[0].1.len() == 1 &&
+               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].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);
+               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.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())),
-                });
+                // 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())),
+                    });
+                }
             }
         }
 
@@ -1073,7 +1067,7 @@ impl EmitterWriter {
                                -> io::Result<()> {
         use std::borrow::Borrow;
 
-        let primary_span = suggestion.substitutes[0].0;
+        let primary_span = suggestion.substitution_spans().next().unwrap();
         if let Some(ref cm) = self.cm {
             let mut buffer = StyledBuffer::new();
 

src/librustc_errors/lib.rs

@@ -23,6 +23,7 @@
 #![feature(staged_api)]
 #![feature(range_contains)]
 #![feature(libc)]
+#![feature(conservative_impl_trait)]
 
 extern crate term;
 extern crate libc;
@@ -83,10 +84,17 @@ pub struct CodeSuggestion {
     /// ```
     /// vec![(0..7, vec!["a.b", "x.y"])]
     /// ```
-    pub substitutes: Vec<(Span, Vec<String>)>,
+    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;
@@ -96,6 +104,16 @@ 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
+    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};
@@ -119,13 +137,13 @@ impl CodeSuggestion {
             }
         }
 
-        if self.substitutes.is_empty() {
+        if self.substitution_parts.is_empty() {
             return vec![String::new()];
         }
 
-        let mut primary_spans: Vec<_> = self.substitutes
+        let mut primary_spans: Vec<_> = self.substitution_parts
             .iter()
-            .map(|&(sp, ref sub)| (sp, sub))
+            .map(|sub| (sub.span, &sub.substitutions))
             .collect();
 
         // Assumption: all spans are in the same file, and all spans
@@ -157,7 +175,7 @@ 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(); self.substitutes[0].1.len()];
+        let mut bufs = vec![String::new(); self.substitutions()];
 
         for (sp, substitutes) in primary_spans {
             let cur_lo = cm.lookup_char_pos(sp.lo);

src/libsyntax/json.rs

@@ -279,12 +279,12 @@ impl DiagnosticSpan {
 
     fn from_suggestion(suggestion: &CodeSuggestion, je: &JsonEmitter)
                        -> Vec<DiagnosticSpan> {
-        suggestion.substitutes
+        suggestion.substitution_parts
                       .iter()
-                      .flat_map(|&(span, ref suggestion)| {
-                          suggestion.iter().map(move |suggestion| {
+                      .flat_map(|substitution| {
+                          substitution.substitutions.iter().map(move |suggestion| {
                               let span_label = SpanLabel {
-                                  span,
+                                  span: substitution.span,
                                   is_primary: true,
                                   label: None,
                               };
@@ -301,7 +301,7 @@ impl DiagnosticSpan {
             RenderSpan::FullSpan(ref msp) =>
                 DiagnosticSpan::from_multispan(msp, je),
             // regular diagnostics don't produce this anymore
-            // will be removed in a later commit
+            // FIXME(oli_obk): remove it entirely
             RenderSpan::Suggestion(_) => unreachable!(),
         }
     }