Auto merge of #61140 - estebank:attr-diagnostics, r=michaelwoerister

Reword malformed attribute input diagnostics

- Handle empty `cfg_attr` attribute
- Reword empty `derive` attribute error
- Use consistend error message: "malformed `attrname` attribute input"
- Provide suggestions when possible
- Move note/help to label/suggestion
- Use consistent wording "ill-formed" -> "malformed"
- Move diagnostic logic out of parser

Split up from #61026, where there's prior conversation.

Author: bors

src/librustc/lint/levels.rs

@@ -191,7 +191,7 @@ impl<'a> LintLevelsBuilder<'a> {
         let store = self.sess.lint_store.borrow();
         let sess = self.sess;
         let bad_attr = |span| {
-            struct_span_err!(sess, span, E0452, "malformed lint attribute")
+            struct_span_err!(sess, span, E0452, "malformed lint attribute input")
         };
         for attr in attrs {
             let level = match Level::from_symbol(attr.name_or_empty()) {
@@ -238,18 +238,20 @@ impl<'a> LintLevelsBuilder<'a> {
                                 }
                                 reason = Some(rationale);
                             } else {
-                                let mut err = bad_attr(name_value.span);
-                                err.help("reason must be a string literal");
-                                err.emit();
+                                bad_attr(name_value.span)
+                                    .span_label(name_value.span, "reason must be a string literal")
+                                    .emit();
                             }
                         } else {
-                            let mut err = bad_attr(item.span);
-                            err.emit();
+                            bad_attr(item.span)
+                                .span_label(item.span, "bad attribute argument")
+                                .emit();
                         }
                     },
                     ast::MetaItemKind::List(_) => {
-                        let mut err = bad_attr(item.span);
-                        err.emit();
+                        bad_attr(item.span)
+                            .span_label(item.span, "bad attribute argument")
+                            .emit();
                     }
                 }
             }
@@ -258,14 +260,20 @@ impl<'a> LintLevelsBuilder<'a> {
                 let meta_item = match li.meta_item() {
                     Some(meta_item) if meta_item.is_word() => meta_item,
                     _ => {
-                        let mut err = bad_attr(li.span());
+                        let sp = li.span();
+                        let mut err = bad_attr(sp);
+                        let mut add_label = true;
                         if let Some(item) = li.meta_item() {
                             if let ast::MetaItemKind::NameValue(_) = item.node {
                                 if item.path == sym::reason {
-                                    err.help("reason in lint attribute must come last");
+                                    err.span_label(sp, "reason in lint attribute must come last");
+                                    add_label = false;
                                 }
                             }
                         }
+                        if add_label {
+                            err.span_label(sp, "bad attribute argument");
+                        }
                         err.emit();
                         continue;
                     }
@@ -318,15 +326,14 @@ impl<'a> LintLevelsBuilder<'a> {
                                      Also `cfg_attr(cargo-clippy)` won't be necessary anymore",
                                     name
                                 );
-                                let mut err = lint::struct_lint_level(
+                                lint::struct_lint_level(
                                     self.sess,
                                     lint,
                                     lvl,
                                     src,
                                     Some(li.span().into()),
                                     &msg,
-                                );
-                                err.span_suggestion(
+                                ).span_suggestion(
                                     li.span(),
                                     "change it to",
                                     new_lint_name.to_string(),

src/librustc_plugin/load.rs

@@ -10,7 +10,7 @@ use std::env;
 use std::mem;
 use std::path::PathBuf;
 use syntax::ast;
-use syntax::span_err;
+use syntax::struct_span_err;
 use syntax::symbol::{Symbol, kw, sym};
 use syntax_pos::{Span, DUMMY_SP};
 
@@ -29,8 +29,10 @@ struct PluginLoader<'a> {
     plugins: Vec<PluginRegistrar>,
 }
 
-fn call_malformed_plugin_attribute(a: &Session, b: Span) {
-    span_err!(a, b, E0498, "malformed plugin attribute");
+fn call_malformed_plugin_attribute(sess: &Session, span: Span) {
+    struct_span_err!(sess, span, E0498, "malformed `plugin` attribute")
+        .span_label(span, "malformed attribute")
+        .emit();
 }
 
 /// Read plugin metadata and dynamically load registrar functions.

src/librustc_typeck/collect.rs

@@ -48,6 +48,8 @@ use rustc::hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc::hir::GenericParamKind;
 use rustc::hir::{self, CodegenFnAttrFlags, CodegenFnAttrs, Unsafety};
 
+use errors::Applicability;
+
 use std::iter;
 
 struct OnlySelfBounds(bool);
@@ -2400,23 +2402,26 @@ fn from_target_feature(
         Some(list) => list,
         None => return,
     };
+    let bad_item = |span| {
+        let msg = "malformed `target_feature` attribute input";
+        let code = "enable = \"..\"".to_owned();
+        tcx.sess.struct_span_err(span, &msg)
+            .span_suggestion(span, "must be of the form", code, Applicability::HasPlaceholders)
+            .emit();
+    };
     let rust_features = tcx.features();
     for item in list {
         // Only `enable = ...` is accepted in the meta item list
         if !item.check_name(sym::enable) {
-            let msg = "#[target_feature(..)] only accepts sub-keys of `enable` \
-                       currently";
-            tcx.sess.span_err(item.span(), &msg);
+            bad_item(item.span());
             continue;
         }
 
         // Must be of the form `enable = "..."` ( a string)
         let value = match item.value_str() {
             Some(value) => value,
             None => {
-                let msg = "#[target_feature] attribute must be of the form \
-                           #[target_feature(enable = \"..\")]";
-                tcx.sess.span_err(item.span(), &msg);
+                bad_item(item.span());
                 continue;
             }
         };
@@ -2428,12 +2433,14 @@ fn from_target_feature(
                 Some(g) => g,
                 None => {
                     let msg = format!(
-                        "the feature named `{}` is not valid for \
-                         this target",
+                        "the feature named `{}` is not valid for this target",
                         feature
                     );
                     let mut err = tcx.sess.struct_span_err(item.span(), &msg);
-
+                    err.span_label(
+                        item.span(),
+                        format!("`{}` is not valid for this target", feature),
+                    );
                     if feature.starts_with("+") {
                         let valid = whitelist.contains_key(&feature[1..]);
                         if valid {
@@ -2571,9 +2578,11 @@ fn codegen_fn_attrs<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, id: DefId) -> Codegen
             }
         } else if attr.check_name(sym::target_feature) {
             if tcx.fn_sig(id).unsafety() == Unsafety::Normal {
-                let msg = "#[target_feature(..)] can only be applied to \
-                           `unsafe` function";
-                tcx.sess.span_err(attr.span, msg);
+                let msg = "#[target_feature(..)] can only be applied to `unsafe` functions";
+                tcx.sess.struct_span_err(attr.span, msg)
+                    .span_label(attr.span, "can only be applied to `unsafe` functions")
+                    .span_label(tcx.def_span(id), "not an `unsafe` function")
+                    .emit();
             }
             from_target_feature(
                 tcx,

src/libsyntax/attr/builtin.rs

@@ -92,7 +92,15 @@ pub fn find_unwind_attr(diagnostic: Option<&Handler>, attrs: &[Attribute]) -> Op
                     }
 
                     diagnostic.map(|d| {
-                        span_err!(d, attr.span, E0633, "malformed `#[unwind]` attribute");
+                        struct_span_err!(d, attr.span, E0633, "malformed `unwind` attribute input")
+                            .span_label(attr.span, "invalid argument")
+                            .span_suggestions(
+                                attr.span,
+                                "the allowed arguments are `allowed` and `aborts`",
+                                (vec!["allowed", "aborts"]).into_iter()
+                                    .map(|s| format!("#[unwind({})]", s)),
+                                Applicability::MachineApplicable,
+                            ).emit();
                     });
                 }
             }

src/libsyntax/config.rs

@@ -94,6 +94,22 @@ impl<'a> StripUnconfigured<'a> {
         if !attr.check_name(sym::cfg_attr) {
             return vec![attr];
         }
+        if attr.tokens.len() == 0 {
+            self.sess.span_diagnostic
+                .struct_span_err(
+                    attr.span,
+                    "malformed `cfg_attr` attribute input",
+                ).span_suggestion(
+                    attr.span,
+                    "missing condition and attribute",
+                    "#[cfg_attr(condition, attribute, other_attribute, ...)]".to_owned(),
+                    Applicability::HasPlaceholders,
+                ).note("for more information, visit \
+                       <https://doc.rust-lang.org/reference/conditional-compilation.html\
+                       #the-cfg_attr-attribute>")
+                .emit();
+            return Vec::new();
+        }
 
         let (cfg_predicate, expanded_attrs) = match attr.parse(self.sess, |parser| {
             parser.expect(&token::OpenDelim(token::Paren))?;

src/libsyntax/ext/derive.rs

@@ -5,6 +5,7 @@ use crate::ext::base::ExtCtxt;
 use crate::ext::build::AstBuilder;
 use crate::parse::parser::PathStyle;
 use crate::symbol::{Symbol, sym};
+use crate::errors::Applicability;
 
 use syntax_pos::Span;
 
@@ -17,8 +18,13 @@ pub fn collect_derives(cx: &mut ExtCtxt<'_>, attrs: &mut Vec<ast::Attribute>) ->
             return true;
         }
         if !attr.is_meta_item_list() {
-            cx.span_err(attr.span,
-                        "attribute must be of the form `#[derive(Trait1, Trait2, ...)]`");
+            cx.struct_span_err(attr.span, "malformed `derive` attribute input")
+                .span_suggestion(
+                    attr.span,
+                    "missing traits to be derived",
+                    "#[derive(Trait1, Trait2, ...)]".to_owned(),
+                    Applicability::HasPlaceholders,
+                ).emit();
             return false;
         }
 

src/libsyntax/feature_gate.rs

@@ -25,7 +25,7 @@ use crate::parse::{token, ParseSess};
 use crate::symbol::{Symbol, kw, sym};
 use crate::tokenstream::TokenTree;
 
-use errors::{DiagnosticBuilder, Handler};
+use errors::{Applicability, DiagnosticBuilder, Handler};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_target::spec::abi::Abi;
 use syntax_pos::{Span, DUMMY_SP};
@@ -1422,7 +1422,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
         Normal,
         template!(
             Word,
-            List: r#"/*opt*/ since = "version", /*opt*/ note = "reason"#,
+            List: r#"/*opt*/ since = "version", /*opt*/ note = "reason""#,
             NameValueStr: "reason"
         ),
         Ungated
@@ -1858,24 +1858,32 @@ impl<'a> PostExpansionVisitor<'a> {
 
         match attr.parse_meta(self.context.parse_sess) {
             Ok(meta) => if !should_skip(name) && !template.compatible(&meta.node) {
+                let error_msg = format!("malformed `{}` attribute input", name);
                 let mut msg = "attribute must be of the form ".to_owned();
+                let mut suggestions = vec![];
                 let mut first = true;
                 if template.word {
                     first = false;
-                    msg.push_str(&format!("`#[{}{}]`", name, ""));
+                    let code = format!("#[{}]", name);
+                    msg.push_str(&format!("`{}`", &code));
+                    suggestions.push(code);
                 }
                 if let Some(descr) = template.list {
                     if !first {
                         msg.push_str(" or ");
                     }
                     first = false;
-                    msg.push_str(&format!("`#[{}({})]`", name, descr));
+                    let code = format!("#[{}({})]", name, descr);
+                    msg.push_str(&format!("`{}`", &code));
+                    suggestions.push(code);
                 }
                 if let Some(descr) = template.name_value_str {
                     if !first {
                         msg.push_str(" or ");
                     }
-                    msg.push_str(&format!("`#[{} = \"{}\"]`", name, descr));
+                    let code = format!("#[{} = \"{}\"]", name, descr);
+                    msg.push_str(&format!("`{}`", &code));
+                    suggestions.push(code);
                 }
                 if should_warn(name) {
                     self.context.parse_sess.buffer_lint(
@@ -1885,7 +1893,17 @@ impl<'a> PostExpansionVisitor<'a> {
                         &msg,
                     );
                 } else {
-                    self.context.parse_sess.span_diagnostic.span_err(meta.span, &msg);
+                    self.context.parse_sess.span_diagnostic.struct_span_err(meta.span, &error_msg)
+                        .span_suggestions(
+                            meta.span,
+                            if suggestions.len() == 1 {
+                                "must be of the form"
+                            } else {
+                                "the following are the possible correct uses"
+                            },
+                            suggestions.into_iter(),
+                            Applicability::HasPlaceholders,
+                        ).emit();
                 }
             }
             Err(mut err) => err.emit(),
@@ -2298,6 +2316,8 @@ pub fn get_features(span_handler: &Handler, krate_attrs: &[ast::Attribute],
         let mut err = struct_span_err!(span_handler, span, E0557, "feature has been removed");
         if let Some(reason) = reason {
             err.span_note(span, reason);
+        } else {
+            err.span_label(span, "feature has been removed");
         }
         err.emit();
     }
@@ -2379,12 +2399,24 @@ pub fn get_features(span_handler: &Handler, krate_attrs: &[ast::Attribute],
             None => continue,
         };
 
+        let bad_input = |span| {
+            struct_span_err!(span_handler, span, E0556, "malformed `feature` attribute input")
+        };
+
         for mi in list {
             let name = match mi.ident() {
                 Some(ident) if mi.is_word() => ident.name,
-                _ => {
-                    span_err!(span_handler, mi.span(), E0556,
-                            "malformed feature, expected just one word");
+                Some(ident) => {
+                    bad_input(mi.span()).span_suggestion(
+                        mi.span(),
+                        "expected just one word",
+                        format!("{}", ident.name),
+                        Applicability::MaybeIncorrect,
+                    ).emit();
+                    continue
+                }
+                None => {
+                    bad_input(mi.span()).span_label(mi.span(), "expected just one word").emit();
                     continue
                 }
             };

src/test/ui/deprecation/deprecated_no_stack_check.stderr

@@ -2,7 +2,7 @@ error[E0557]: feature has been removed
   --> $DIR/deprecated_no_stack_check.rs:2:12
    |
 LL | #![feature(no_stack_check)]
-   |            ^^^^^^^^^^^^^^
+   |            ^^^^^^^^^^^^^^ feature has been removed
 
 error: aborting due to previous error
 

src/test/ui/deprecation/invalid-literal.rs

@@ -1,4 +1,4 @@
-#[deprecated = b"test"] //~ ERROR attribute must be of the form
+#[deprecated = b"test"] //~ ERROR malformed `deprecated` attribute
 fn foo() {}
 
 fn main() {}

src/test/ui/deprecation/invalid-literal.stderr

@@ -1,8 +1,16 @@
-error: attribute must be of the form `#[deprecated]` or `#[deprecated(/*opt*/ since = "version", /*opt*/ note = "reason)]` or `#[deprecated = "reason"]`
+error: malformed `deprecated` attribute input
   --> $DIR/invalid-literal.rs:1:1
    |
 LL | #[deprecated = b"test"]
    | ^^^^^^^^^^^^^^^^^^^^^^^
+help: the following are the possible correct uses
+   |
+LL | #[deprecated]
+   | ^^^^^^^^^^^^^
+LL | #[deprecated(/*opt*/ since = "version", /*opt*/ note = "reason")]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | #[deprecated = "reason"]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: aborting due to previous error
 

src/test/ui/error-codes/E0452.stderr

@@ -1,8 +1,8 @@
-error[E0452]: malformed lint attribute
+error[E0452]: malformed lint attribute input
   --> $DIR/E0452.rs:1:10
    |
 LL | #![allow(foo = "")]
-   |          ^^^^^^^^
+   |          ^^^^^^^^ bad attribute argument
 
 error: aborting due to previous error