Auto merge of #40851 - oli-obk:multisugg, r=jonathandturner

Minimize single span suggestions into a label

changes

```
14 |     println!("☃{}", tup[0]);
   |                     ^^^^^^
   |
help: to access tuple elements, use tuple indexing syntax as shown
   |     println!("☃{}", tup.0);
```

into

```
14 |     println!("☃{}", tup[0]);
   |                     ^^^^^^ to access tuple elements, use `tup.0`
```

Also makes suggestions explicit in the backend in preparation of adding multiple suggestions to a single diagnostic. Currently that's already possible, but results in a full help message + modified code snippet per suggestion, and has no rate limit (might show 100+ suggestions).

src/librustc_errors/diagnostic.rs

@@ -11,7 +11,6 @@
 use CodeSuggestion;
 use Level;
 use RenderSpan;
-use RenderSpan::Suggestion;
 use std::fmt;
 use syntax_pos::{MultiSpan, Span};
 use snippet::Style;
@@ -24,6 +23,7 @@ pub struct Diagnostic {
     pub code: Option<String>,
     pub span: MultiSpan,
     pub children: Vec<SubDiagnostic>,
+    pub suggestion: Option<CodeSuggestion>,
 }
 
 /// For example a note attached to an error.
@@ -87,6 +87,7 @@ impl Diagnostic {
             code: code,
             span: MultiSpan::new(),
             children: vec![],
+            suggestion: None,
         }
     }
 
@@ -202,19 +203,14 @@ impl Diagnostic {
 
     /// Prints out a message with a suggested edit of the code.
     ///
-    /// See `diagnostic::RenderSpan::Suggestion` for more information.
+    /// See `diagnostic::CodeSuggestion` for more information.
-    pub fn span_suggestion<S: Into<MultiSpan>>(&mut self,
+    pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
-                                               sp: S,
+        assert!(self.suggestion.is_none());
-                                               msg: &str,
+        self.suggestion = Some(CodeSuggestion {
-                                               suggestion: String)
+            msp: sp.into(),
-                                               -> &mut Self {
+            substitutes: vec![suggestion],
-        self.sub(Level::Help,
+            msg: msg.to_owned(),
-                 msg,
+        });
-                 MultiSpan::new(),
-                 Some(Suggestion(CodeSuggestion {
-                     msp: sp.into(),
-                     substitutes: vec![suggestion],
-                 })));
         self
     }
 

src/librustc_errors/diagnostic_builder.rs

@@ -141,11 +141,11 @@ impl<'a> DiagnosticBuilder<'a> {
                                                   sp: S,
                                                   msg: &str)
                                                   -> &mut Self);
-    forward!(pub fn span_suggestion<S: Into<MultiSpan>>(&mut self,
+    forward!(pub fn span_suggestion(&mut self,
-                                                        sp: S,
+                                    sp: Span,
-                                                        msg: &str,
+                                    msg: &str,
-                                                        suggestion: String)
+                                    suggestion: String)
-                                                        -> &mut Self);
+                                    -> &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

@@ -34,6 +34,27 @@ impl Emitter for EmitterWriter {
     fn emit(&mut self, db: &DiagnosticBuilder) {
         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());
+            // don't display multispans as labels
+            if sugg.substitutes.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);
+            } else {
+                children.push(SubDiagnostic {
+                    level: Level::Help,
+                    message: Vec::new(),
+                    span: MultiSpan::new(),
+                    render_span: Some(Suggestion(sugg)),
+                });
+            }
+        }
+
         self.fix_multispans_in_std_macros(&mut primary_span, &mut children);
         self.emit_messages_default(&db.level,
                                    &db.styled_message(),
@@ -756,7 +777,7 @@ impl EmitterWriter {
     /// displayed, keeping the provided highlighting.
     fn msg_to_buffer(&self,
                      buffer: &mut StyledBuffer,
-                     msg: &Vec<(String, Style)>,
+                     msg: &[(String, Style)],
                      padding: usize,
                      label: &str,
                      override_style: Option<Style>) {
@@ -1022,7 +1043,6 @@ impl EmitterWriter {
     fn emit_suggestion_default(&mut self,
                                suggestion: &CodeSuggestion,
                                level: &Level,
-                               msg: &Vec<(String, Style)>,
                                max_line_num_len: usize)
                                -> io::Result<()> {
         use std::borrow::Borrow;
@@ -1034,7 +1054,7 @@ impl EmitterWriter {
             buffer.append(0, &level.to_string(), Style::Level(level.clone()));
             buffer.append(0, ": ", Style::HeaderMsg);
             self.msg_to_buffer(&mut buffer,
-                               msg,
+                               &[(suggestion.msg.to_owned(), Style::NoStyle)],
                                max_line_num_len,
                                "suggestion",
                                Some(Style::HeaderMsg));
@@ -1099,7 +1119,6 @@ impl EmitterWriter {
                         Some(Suggestion(ref cs)) => {
                             match self.emit_suggestion_default(cs,
                                                                &child.level,
-                                                               &child.styled_message(),
                                                                max_line_num_len) {
                                 Err(e) => panic!("failed to emit error: {}", e),
                                 _ => ()

src/librustc_errors/lib.rs

@@ -67,6 +67,7 @@ pub enum RenderSpan {
 pub struct CodeSuggestion {
     pub msp: MultiSpan,
     pub substitutes: Vec<String>,
+    pub msg: String,
 }
 
 pub trait CodeMapper {

src/librustc_typeck/check/mod.rs

@@ -3883,9 +3883,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                                       let snip = tcx.sess.codemap().span_to_snippet(base.span);
                                       if let Ok(snip) = snip {
                                           err.span_suggestion(expr.span,
-                                                              "to access tuple elements, \
+                                                              "to access tuple elements, use",
-                                                               use tuple indexing syntax \
-                                                               as shown",
                                                               format!("{}.{}", snip, i));
                                           needs_note = false;
                                       }

src/librustc_typeck/check/op.rs

@@ -197,7 +197,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                 // error types are considered "builtin"
                 if !lhs_ty.references_error() {
                     if let IsAssign::Yes = is_assign {
-                        struct_span_err!(self.tcx.sess, lhs_expr.span, E0368,
+                        struct_span_err!(self.tcx.sess, expr.span, E0368,
                                          "binary assignment operation `{}=` \
                                           cannot be applied to type `{}`",
                                          op.node.as_str(),
@@ -207,7 +207,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                                         op.node.as_str(), lhs_ty))
                             .emit();
                     } else {
-                        let mut err = struct_span_err!(self.tcx.sess, lhs_expr.span, E0369,
+                        let mut err = struct_span_err!(self.tcx.sess, expr.span, E0369,
                             "binary operation `{}` cannot be applied to type `{}`",
                             op.node.as_str(),
                             lhs_ty);
@@ -245,7 +245,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                         if let Some(missing_trait) = missing_trait {
                             if missing_trait == "std::ops::Add" &&
                                 self.check_str_addition(expr, lhs_expr, lhs_ty,
-                                                         rhs_expr, rhs_ty, &mut err) {
+                                                        rhs_ty, &mut err) {
                                 // This has nothing here because it means we did string
                                 // concatenation (e.g. "Hello " + "World!"). This means
                                 // we don't want the note in the else clause to be emitted
@@ -269,7 +269,6 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                           expr: &'gcx hir::Expr,
                           lhs_expr: &'gcx hir::Expr,
                           lhs_ty: Ty<'tcx>,
-                          rhs_expr: &'gcx hir::Expr,
                           rhs_ty: Ty<'tcx>,
                           mut err: &mut errors::DiagnosticBuilder) -> bool {
         // If this function returns true it means a note was printed, so we don't need
@@ -278,17 +277,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         if let TyRef(_, l_ty) = lhs_ty.sty {
             if let TyRef(_, r_ty) = rhs_ty.sty {
                 if l_ty.ty.sty == TyStr && r_ty.ty.sty == TyStr {
-                    err.note("`+` can't be used to concatenate two `&str` strings");
+                    err.span_label(expr.span,
+                        &"`+` can't be used to concatenate two `&str` strings");
                     let codemap = self.tcx.sess.codemap();
                     let suggestion =
-                        match (codemap.span_to_snippet(lhs_expr.span),
+                        match codemap.span_to_snippet(lhs_expr.span) {
-                                codemap.span_to_snippet(rhs_expr.span)) {
+                            Ok(lstring) => format!("{}.to_owned()", lstring),
-                            (Ok(lstring), Ok(rstring)) =>
-                                format!("{}.to_owned() + {}", lstring, rstring),
                             _ => format!("<expression>")
                         };
-                    err.span_suggestion(expr.span,
+                    err.span_suggestion(lhs_expr.span,
-                        &format!("to_owned() can be used to create an owned `String` \
+                        &format!("`to_owned()` can be used to create an owned `String` \
                                   from a string reference. String concatenation \
                                   appends the string on the right to the string \
                                   on the left and may require reallocation. This \

src/librustc_typeck/diagnostics.rs

@@ -3181,6 +3181,13 @@ x << 2; // ok!
 
 It is also possible to overload most operators for your own type by
 implementing traits from `std::ops`.
+
+String concatenation appends the string on the right to the string on the
+left and may require reallocation. This requires ownership of the string
+on the left. If something should be added to a string literal, move the
+literal to the heap by allocating it with `to_owned()` like in
+`"Your text".to_owned()`.
+
 "##,
 
 E0370: r##"

src/libsyntax/json.rs

@@ -22,8 +22,9 @@
 use codemap::{CodeMap, FilePathMapping};
 use syntax_pos::{self, MacroBacktrace, Span, SpanLabel, MultiSpan};
 use errors::registry::Registry;
-use errors::{DiagnosticBuilder, SubDiagnostic, RenderSpan, CodeSuggestion, CodeMapper};
+use errors::{Level, DiagnosticBuilder, SubDiagnostic, RenderSpan, CodeSuggestion, CodeMapper};
 use errors::emitter::Emitter;
+use errors::snippet::Style;
 
 use std::rc::Rc;
 use std::io::{self, Write};
@@ -153,12 +154,21 @@ impl Diagnostic {
     fn from_diagnostic_builder(db: &DiagnosticBuilder,
                                je: &JsonEmitter)
                                -> Diagnostic {
+        let sugg = db.suggestion.as_ref().map(|sugg| {
+            SubDiagnostic {
+                level: Level::Help,
+                message: vec![(sugg.msg.clone(), Style::NoStyle)],
+                span: MultiSpan::new(),
+                render_span: Some(RenderSpan::Suggestion(sugg.clone())),
+            }
+        });
+        let sugg = sugg.as_ref();
         Diagnostic {
             message: db.message(),
             code: DiagnosticCode::map_opt_string(db.code.clone(), je),
             level: db.level.to_str(),
             spans: DiagnosticSpan::from_multispan(&db.span, je),
-            children: db.children.iter().map(|c| {
+            children: db.children.iter().chain(sugg).map(|c| {
                 Diagnostic::from_sub_diagnostic(c, je)
             }).collect(),
             rendered: None,

src/libsyntax/parse/parser.rs

@@ -1492,9 +1492,8 @@ impl<'a> Parser<'a> {
         let bounds = self.parse_ty_param_bounds()?;
         let sum_span = ty.span.to(self.prev_span);
 
-        let mut err = struct_span_err!(self.sess.span_diagnostic, ty.span, E0178,
+        let mut err = struct_span_err!(self.sess.span_diagnostic, sum_span, E0178,
             "expected a path on the left-hand side of `+`, not `{}`", pprust::ty_to_string(&ty));
-        err.span_label(ty.span, &format!("expected a path"));
 
         match ty.node {
             TyKind::Rptr(ref lifetime, ref mut_ty) => {
@@ -1513,9 +1512,11 @@ impl<'a> Parser<'a> {
                 err.span_suggestion(sum_span, "try adding parentheses:", sum_with_parens);
             }
             TyKind::Ptr(..) | TyKind::BareFn(..) => {
-                help!(&mut err, "perhaps you forgot parentheses?");
+                err.span_label(sum_span, &"perhaps you forgot parentheses?");
             }
-            _ => {}
+            _ => {
+                err.span_label(sum_span, &"expected a path");
+            },
         }
         err.emit();
         Ok(())
@@ -5131,7 +5132,6 @@ impl<'a> Parser<'a> {
         }
 
         if self.check(&token::OpenDelim(token::Paren)) {
-            let start_span = self.span;
             // We don't `self.bump()` the `(` yet because this might be a struct definition where
             // `()` or a tuple might be allowed. For example, `struct Struct(pub (), pub (usize));`.
             // Because of this, we only `bump` the `(` if we're assured it is appropriate to do so
@@ -5170,12 +5170,9 @@ impl<'a> Parser<'a> {
 `pub(in path::to::module)`: visible only on the specified path"##;
                 let path = self.parse_path(PathStyle::Mod)?;
                 let path_span = self.prev_span;
-                let help_msg = format!("to make this visible only to module `{}`, add `in` before \
+                let help_msg = format!("make this visible only to module `{}` with `in`:", path);
-                                       the path:",
-                                       path);
                 self.expect(&token::CloseDelim(token::Paren))?;  // `)`
-                let sp = start_span.to(self.prev_span);
+                let mut err = self.span_fatal_help(path_span, &msg, &suggestion);
-                let mut err = self.span_fatal_help(sp, &msg, &suggestion);
                 err.span_suggestion(path_span, &help_msg, format!("in {}", path));
                 err.emit();  // emit diagnostic, but continue with public visibility
             }

src/test/compile-fail/issue-27842.rs

@@ -13,7 +13,7 @@ fn main() {
     // the case where we show a suggestion
     let _ = tup[0];
     //~^ ERROR cannot index a value of type
-    //~| HELP to access tuple elements, use tuple indexing syntax as shown
+    //~| HELP to access tuple elements, use
     //~| SUGGESTION let _ = tup.0
 
     // the case where we show just a general hint

src/test/parse-fail/trait-object-polytrait-priority.rs

@@ -13,7 +13,6 @@ trait Trait<'a> {}
 fn main() {
     let _: &for<'a> Trait<'a> + 'static;
     //~^ ERROR expected a path on the left-hand side of `+`, not `& for<'a>Trait<'a>`
-    //~| NOTE expected a path
     //~| HELP try adding parentheses
     //~| SUGGESTION &( for<'a>Trait<'a> + 'static)
 }

src/test/ui/did_you_mean/E0178.rs

@@ -12,17 +12,9 @@ trait Foo {}
 
 struct Bar<'a> {
     w: &'a Foo + Copy,
-    //~^ ERROR E0178
-    //~| NOTE expected a path
     x: &'a Foo + 'a,
-    //~^ ERROR E0178
-    //~| NOTE expected a path
     y: &'a mut Foo + 'a,
-    //~^ ERROR E0178
-    //~| NOTE expected a path
     z: fn() -> Foo + 'a,
-    //~^ ERROR E0178
-    //~| NOTE expected a path
 }
 
 fn main() {

src/test/ui/did_you_mean/E0178.stderr

@@ -0,0 +1,26 @@
+error[E0178]: expected a path on the left-hand side of `+`, not `&'a Foo`
+  --> $DIR/E0178.rs:14:8
+   |
+14 |     w: &'a Foo + Copy,
+   |        ^^^^^^^^^^^^^^ help: try adding parentheses: `&'a (Foo + Copy)`
+
+error[E0178]: expected a path on the left-hand side of `+`, not `&'a Foo`
+  --> $DIR/E0178.rs:15:8
+   |
+15 |     x: &'a Foo + 'a,
+   |        ^^^^^^^^^^^^ help: try adding parentheses: `&'a (Foo + 'a)`
+
+error[E0178]: expected a path on the left-hand side of `+`, not `&'a mut Foo`
+  --> $DIR/E0178.rs:16:8
+   |
+16 |     y: &'a mut Foo + 'a,
+   |        ^^^^^^^^^^^^^^^^ help: try adding parentheses: `&'a mut (Foo + 'a)`
+
+error[E0178]: expected a path on the left-hand side of `+`, not `fn() -> Foo`
+  --> $DIR/E0178.rs:17:8
+   |
+17 |     z: fn() -> Foo + 'a,
+   |        ^^^^^^^^^^^^^^^^ perhaps you forgot parentheses?
+
+error: aborting due to 4 previous errors
+

src/test/ui/did_you_mean/trait-object-reference-without-parens-suggestion.rs

@@ -10,12 +10,5 @@
 
 fn main() {
     let _: &Copy + 'static;
-    //~^ ERROR expected a path
-    //~| HELP try adding parentheses
-    //~| SUGGESTION let _: &(Copy + 'static);
-    //~| ERROR the trait `std::marker::Copy` cannot be made into an object
     let _: &'static Copy + 'static;
-    //~^ ERROR expected a path
-    //~| HELP try adding parentheses
-    //~| SUGGESTION let _: &'static (Copy + 'static);
 }

src/test/ui/did_you_mean/trait-object-reference-without-parens-suggestion.stderr

@@ -0,0 +1,22 @@
+error[E0178]: expected a path on the left-hand side of `+`, not `&Copy`
+  --> $DIR/trait-object-reference-without-parens-suggestion.rs:12:12
+   |
+12 |     let _: &Copy + 'static;
+   |            ^^^^^^^^^^^^^^^ help: try adding parentheses: `&(Copy + 'static)`
+
+error[E0178]: expected a path on the left-hand side of `+`, not `&'static Copy`
+  --> $DIR/trait-object-reference-without-parens-suggestion.rs:13:12
+   |
+13 |     let _: &'static Copy + 'static;
+   |            ^^^^^^^^^^^^^^^^^^^^^^^ help: try adding parentheses: `&'static (Copy + 'static)`
+
+error[E0038]: the trait `std::marker::Copy` cannot be made into an object
+  --> $DIR/trait-object-reference-without-parens-suggestion.rs:12:12
+   |
+12 |     let _: &Copy + 'static;
+   |            ^^^^^ the trait `std::marker::Copy` cannot be made into an object
+   |
+   = note: the trait cannot require that `Self : Sized`
+
+error: aborting due to previous error
+

src/test/ui/pub/pub-restricted.rs

@@ -34,4 +34,8 @@ mod y {
     }
 }
 
-fn main() {}
+fn main() {}
+
+// test multichar names
+mod xyz {}
+pub (xyz) fn xyz() {}

src/test/ui/pub/pub-restricted.stderr

@@ -1,41 +1,46 @@
 error: incorrect visibility restriction
-  --> $DIR/pub-restricted.rs:15:5
+  --> $DIR/pub-restricted.rs:15:6
    |
 15 | pub (a) fn afn() {}
-   |     ^^^
+   |      ^ help: make this visible only to module `a` with `in`: `in a`
    |
    = help: some possible visibility restrictions are:
            `pub(crate)`: visible only on the current crate
            `pub(super)`: visible only in the current module's parent
            `pub(in path::to::module)`: visible only on the specified path
-help: to make this visible only to module `a`, add `in` before the path:
-   | pub (in a) fn afn() {}
 
 error: incorrect visibility restriction
-  --> $DIR/pub-restricted.rs:16:5
+  --> $DIR/pub-restricted.rs:16:6
    |
 16 | pub (b) fn bfn() {}
-   |     ^^^
+   |      ^ help: make this visible only to module `b` with `in`: `in b`
    |
    = help: some possible visibility restrictions are:
            `pub(crate)`: visible only on the current crate
            `pub(super)`: visible only in the current module's parent
            `pub(in path::to::module)`: visible only on the specified path
-help: to make this visible only to module `b`, add `in` before the path:
-   | pub (in b) fn bfn() {}
 
 error: incorrect visibility restriction
-  --> $DIR/pub-restricted.rs:32:13
+  --> $DIR/pub-restricted.rs:32:14
    |
 32 |         pub (a) invalid: usize,
-   |             ^^^
+   |              ^ help: make this visible only to module `a` with `in`: `in a`
+   |
+   = help: some possible visibility restrictions are:
+           `pub(crate)`: visible only on the current crate
+           `pub(super)`: visible only in the current module's parent
+           `pub(in path::to::module)`: visible only on the specified path
+
+error: incorrect visibility restriction
+  --> $DIR/pub-restricted.rs:41:6
+   |
+41 | pub (xyz) fn xyz() {}
+   |      ^^^ help: make this visible only to module `xyz` with `in`: `in xyz`
    |
    = help: some possible visibility restrictions are:
            `pub(crate)`: visible only on the current crate
            `pub(super)`: visible only in the current module's parent
            `pub(in path::to::module)`: visible only on the specified path
-help: to make this visible only to module `a`, add `in` before the path:
-   |         pub (in a) invalid: usize,
 
 error: visibilities can only be restricted to ancestor modules
   --> $DIR/pub-restricted.rs:33:17
@@ -43,5 +48,5 @@ error: visibilities can only be restricted to ancestor modules
 33 |         pub (in x) non_parent_invalid: usize,
    |                 ^
 
-error: aborting due to 4 previous errors
+error: aborting due to 5 previous errors
 

src/test/ui/span/issue-39018.stderr

@@ -2,17 +2,16 @@ error[E0369]: binary operation `+` cannot be applied to type `&'static str`
   --> $DIR/issue-39018.rs:12:13
    |
 12 |     let x = "Hello " + "World!";
-   |             ^^^^^^^^
+   |             ^^^^^^^^^^^^^^^^^^^ `+` can't be used to concatenate two `&str` strings
    |
-   = note: `+` can't be used to concatenate two `&str` strings
+help: `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left.
-help: to_owned() can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left.
    |     let x = "Hello ".to_owned() + "World!";
 
 error[E0369]: binary operation `+` cannot be applied to type `World`
   --> $DIR/issue-39018.rs:17:13
    |
 17 |     let y = World::Hello + World::Goodbye;
-   |             ^^^^^^^^^^^^
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: an implementation of `std::ops::Add` might be missing for `World`
 

src/test/ui/span/suggestion-non-ascii.stderr

@@ -2,10 +2,7 @@ error: cannot index a value of type `({integer},)`
   --> $DIR/suggestion-non-ascii.rs:14:21
    |
 14 |     println!("☃{}", tup[0]);
-   |                     ^^^^^^
+   |                     ^^^^^^ help: to access tuple elements, use `tup.0`
-   |
-help: to access tuple elements, use tuple indexing syntax as shown
-   |     println!("☃{}", tup.0);
 
 error: aborting due to previous error
 

src/test/ui/suggestions/tuple-float-index.stderr

@@ -2,10 +2,10 @@ error: unexpected token: `1.1`
   --> $DIR/tuple-float-index.rs:14:17
    |
 14 |     (1, (2, 3)).1.1;
-   |                 ^^^ unexpected token
+   |     ------------^^^
-   |
+   |     |           |
-help: try parenthesizing the first index
+   |     |           unexpected token
-   |     ((1, (2, 3)).1).1;
+   |     help: try parenthesizing the first index `((1, (2, 3)).1).1`
 
 error: aborting due to previous error