Rework illformed attribute check for unparsed attributes

Signed-off-by: Jonathan Brouwer <[email protected]>

Author: JonathanBrouwer

Cargo.lock

@@ -3321,6 +3321,7 @@ dependencies = [
  "rustc_hir",
  "rustc_lexer",
  "rustc_macros",
+ "rustc_parse",
  "rustc_session",
  "rustc_span",
  "thin-vec",

compiler/rustc_attr_parsing/Cargo.toml

@@ -15,6 +15,7 @@ rustc_fluent_macro = { path = "../rustc_fluent_macro" }
 rustc_hir = { path = "../rustc_hir" }
 rustc_lexer = { path = "../rustc_lexer" }
 rustc_macros = { path = "../rustc_macros" }
+rustc_parse = { path = "../rustc_parse" }
 rustc_session = { path = "../rustc_session" }
 rustc_span = { path = "../rustc_span" }
 thin-vec = "0.2.12"

compiler/rustc_attr_parsing/src/attributes/stability.rs

@@ -237,7 +237,12 @@ pub(crate) fn parse_stability<S: Stage>(
     let mut feature = None;
     let mut since = None;
 
-    for param in args.list()?.mixed() {
+    let ArgParser::List(list) = args else {
+        cx.expected_list(cx.attr_span);
+        return None;
+    };
+
+    for param in list.mixed() {
         let param_span = param.span();
         let Some(param) = param.meta_item() else {
             cx.emit_err(session_diagnostics::UnsupportedLiteral {
@@ -312,7 +317,13 @@ pub(crate) fn parse_unstability<S: Stage>(
     let mut is_soft = false;
     let mut implied_by = None;
     let mut old_name = None;
-    for param in args.list()?.mixed() {
+
+    let ArgParser::List(list) = args else {
+        cx.expected_list(cx.attr_span);
+        return None;
+    };
+
+    for param in list.mixed() {
         let Some(param) = param.meta_item() else {
             cx.emit_err(session_diagnostics::UnsupportedLiteral {
                 span: param.span(),

compiler/rustc_attr_parsing/src/context.rs

@@ -7,9 +7,10 @@ use private::Sealed;
 use rustc_ast::{self as ast, LitKind, MetaItemLit, NodeId};
 use rustc_attr_data_structures::AttributeKind;
 use rustc_attr_data_structures::lints::{AttributeLint, AttributeLintKind};
-use rustc_errors::{DiagCtxtHandle, Diagnostic};
-use rustc_feature::{AttributeTemplate, Features};
+use rustc_errors::{Applicability, DiagCtxtHandle, Diagnostic};
+use rustc_feature::{AttributeTemplate, BUILTIN_ATTRIBUTE_MAP, BuiltinAttribute, Features};
 use rustc_hir::{AttrArgs, AttrItem, AttrPath, Attribute, HashIgnoredAttrId, HirId};
+use rustc_parse::validate_attr::{is_attr_template_compatible, parse_meta};
 use rustc_session::Session;
 use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span, Symbol, sym};
 
@@ -197,7 +198,7 @@ mod private {
 #[allow(private_interfaces)]
 pub trait Stage: Sized + 'static + Sealed {
     type Id: Copy;
-    const SHOULD_EMIT_LINTS: bool;
+    const IS_LATE: bool;
 
     fn parsers() -> &'static group_type!(Self);
 
@@ -212,7 +213,7 @@ pub trait Stage: Sized + 'static + Sealed {
 #[allow(private_interfaces)]
 impl Stage for Early {
     type Id = NodeId;
-    const SHOULD_EMIT_LINTS: bool = false;
+    const IS_LATE: bool = false;
 
     fn parsers() -> &'static group_type!(Self) {
         &early::ATTRIBUTE_PARSERS
@@ -234,7 +235,7 @@ impl Stage for Early {
 #[allow(private_interfaces)]
 impl Stage for Late {
     type Id = HirId;
-    const SHOULD_EMIT_LINTS: bool = true;
+    const IS_LATE: bool = true;
 
     fn parsers() -> &'static group_type!(Self) {
         &late::ATTRIBUTE_PARSERS
@@ -284,7 +285,7 @@ impl<'f, 'sess: 'f, S: Stage> SharedContext<'f, 'sess, S> {
     /// must be delayed until after HIR is built. This method will take care of the details of
     /// that.
     pub(crate) fn emit_lint(&mut self, lint: AttributeLintKind, span: Span) {
-        if !S::SHOULD_EMIT_LINTS {
+        if !S::IS_LATE {
             return;
         }
         let id = self.target_id;
@@ -740,12 +741,25 @@ impl<'sess, S: Stage> AttributeParser<'sess, S> {
                 ast::AttrKind::Normal(n) => {
                     attr_paths.push(PathParser::Ast(&n.item.path));
 
+                    // Parse attribute using new infra
                     let parser = MetaItemParser::from_attr(n, self.dcx());
                     let path = parser.path();
                     let args = parser.args();
                     let parts = path.segments().map(|i| i.name).collect::<Vec<_>>();
 
                     if let Some(accepts) = S::parsers().0.get(parts.as_slice()) {
+                        //FIXME we call this to generate a few of the errors (such as invalid literals) that the new parses does not generate yet
+                        //Remove this when possible
+
+                        // rustc_dummy is not checked unless it is of the `Eq` arguments form
+                        let skip_parse_meta_check = attr.has_name(sym::rustc_dummy)
+                            && !matches!(n.item.args, rustc_ast::AttrArgs::Eq { .. });
+                        if !skip_parse_meta_check
+                            && let Err(err) = parse_meta(&self.sess.psess, attr)
+                        {
+                            err.emit();
+                        }
+
                         for (template, accept) in accepts {
                             let mut cx: AcceptContext<'_, 'sess, S> = AcceptContext {
                                 shared: SharedContext {
@@ -777,6 +791,46 @@ impl<'sess, S: Stage> AttributeParser<'sess, S> {
                         //     "attribute {path} wasn't parsed and isn't a know tool attribute",
                         // );
 
+                        if S::IS_LATE
+                            && !attr.has_name(sym::cfg_trace)
+                            && !attr.has_name(sym::cfg_attr_trace)
+                        {
+                            let builtin_attr_info = attr
+                                .ident()
+                                .and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name));
+                            if let Some(BuiltinAttribute { name, template, .. }) = builtin_attr_info
+                            {
+                                match parse_meta(&self.sess.psess, attr) {
+                                    // Don't check safety again, we just did that
+                                    Ok(meta) => {
+                                        if !is_attr_template_compatible(&template, &meta.kind) {
+                                            self.emit_malformed_unparsed_attribute(
+                                                attr.style,
+                                                meta.span,
+                                                *name,
+                                                *template,
+                                                target_id,
+                                                &mut emit_lint,
+                                            );
+                                        }
+                                    }
+                                    Err(err) => {
+                                        err.emit();
+                                    }
+                                }
+                            } else {
+                                if let rustc_ast::AttrArgs::Eq { .. } = n.item.args {
+                                    // All key-value attributes are restricted to meta-item syntax.
+                                    match parse_meta(&self.sess.psess, attr) {
+                                        Ok(_) => {}
+                                        Err(err) => {
+                                            err.emit();
+                                        }
+                                    }
+                                }
+                            }
+                        }
+
                         attributes.push(Attribute::Unparsed(Box::new(AttrItem {
                             path: AttrPath::from_ast(&n.item.path),
                             args: self.lower_attr_args(&n.item.args, lower_span),
@@ -809,6 +863,57 @@ impl<'sess, S: Stage> AttributeParser<'sess, S> {
         attributes
     }
 
+    fn emit_malformed_unparsed_attribute(
+        &self,
+        style: ast::AttrStyle,
+        span: Span,
+        name: Symbol,
+        template: AttributeTemplate,
+        target_id: S::Id,
+        emit_lint: &mut impl FnMut(AttributeLint<S::Id>),
+    ) {
+        // Some of previously accepted forms were used in practice,
+        // report them as warnings for now.
+        let should_warn = |name| matches!(name, sym::doc | sym::link | sym::test | sym::bench);
+
+        let error_msg = format!("malformed `{name}` attribute input");
+        let mut suggestions = vec![];
+        let inner = if style == ast::AttrStyle::Inner { "!" } else { "" };
+        if template.word {
+            suggestions.push(format!("#{inner}[{name}]"));
+        }
+        if let Some(descr) = template.list {
+            suggestions.push(format!("#{inner}[{name}({descr})]"));
+        }
+        suggestions.extend(template.one_of.iter().map(|&word| format!("#{inner}[{name}({word})]")));
+        if let Some(descr) = template.name_value_str {
+            suggestions.push(format!("#{inner}[{name} = \"{descr}\"]"));
+        }
+        if should_warn(name) {
+            emit_lint(AttributeLint {
+                id: target_id,
+                span,
+                kind: AttributeLintKind::IllFormedAttributeInput { suggestions },
+            });
+        } else {
+            suggestions.sort();
+            self.sess
+                .dcx()
+                .struct_span_err(span, error_msg)
+                .with_span_suggestions(
+                    span,
+                    if suggestions.len() == 1 {
+                        "must be of the form"
+                    } else {
+                        "the following are the possible correct uses"
+                    },
+                    suggestions,
+                    Applicability::HasPlaceholders,
+                )
+                .emit();
+        }
+    }
+
     /// Returns whether there is a parser for an attribute with this name
     pub fn is_parsed_attribute(path: &[Symbol]) -> bool {
         Late::parsers().0.contains_key(path)

compiler/rustc_builtin_macros/src/cfg_accessible.rs

@@ -51,7 +51,6 @@ impl MultiItemModifier for Expander {
             ast::AttrStyle::Outer,
             sym::cfg_accessible,
             template,
-            true,
         );
 
         let Some(path) = validate_input(ecx, meta_item) else {

compiler/rustc_builtin_macros/src/derive.rs

@@ -42,7 +42,6 @@ impl MultiItemModifier for Expander {
                     ast::AttrStyle::Outer,
                     sym::derive,
                     template,
-                    true,
                 );
 
                 let mut resolutions = match &meta_item.kind {

compiler/rustc_builtin_macros/src/util.rs

@@ -22,7 +22,6 @@ pub(crate) fn check_builtin_macro_attribute(ecx: &ExtCtxt<'_>, meta_item: &MetaI
         AttrStyle::Outer,
         name,
         template,
-        true,
     );
 }
 

compiler/rustc_parse/src/validate_attr.rs

@@ -7,7 +7,7 @@ use rustc_ast::{
     Path, Safety,
 };
 use rustc_errors::{Applicability, DiagCtxtHandle, FatalError, PResult};
-use rustc_feature::{AttributeSafety, AttributeTemplate, BUILTIN_ATTRIBUTE_MAP, BuiltinAttribute};
+use rustc_feature::{AttributeSafety, AttributeTemplate, BUILTIN_ATTRIBUTE_MAP};
 use rustc_session::errors::report_lit_error;
 use rustc_session::lint::BuiltinLintDiag;
 use rustc_session::lint::builtin::{ILL_FORMED_ATTRIBUTE_INPUT, UNSAFE_ATTR_OUTSIDE_UNSAFE};
@@ -26,34 +26,6 @@ pub fn check_attr(psess: &ParseSess, attr: &Attribute, id: NodeId) {
 
     let builtin_attr_safety = builtin_attr_info.map(|x| x.safety);
     check_attribute_safety(psess, builtin_attr_safety, attr, id);
-
-    // Check input tokens for built-in and key-value attributes.
-    match builtin_attr_info {
-        // `rustc_dummy` doesn't have any restrictions specific to built-in attributes.
-        Some(BuiltinAttribute { name, template, .. }) if *name != sym::rustc_dummy => {
-            match parse_meta(psess, attr) {
-                // Don't check safety again, we just did that
-                Ok(meta) => {
-                    check_builtin_meta_item(psess, &meta, attr.style, *name, *template, false)
-                }
-                Err(err) => {
-                    err.emit();
-                }
-            }
-        }
-        _ => {
-            let attr_item = attr.get_normal_item();
-            if let AttrArgs::Eq { .. } = attr_item.args {
-                // All key-value attributes are restricted to meta-item syntax.
-                match parse_meta(psess, attr) {
-                    Ok(_) => {}
-                    Err(err) => {
-                        err.emit();
-                    }
-                }
-            }
-        }
-    }
 }
 
 pub fn parse_meta<'a>(psess: &'a ParseSess, attr: &Attribute) -> PResult<'a, MetaItem> {
@@ -141,7 +113,7 @@ pub(super) fn check_cfg_attr_bad_delim(psess: &ParseSess, span: DelimSpan, delim
 }
 
 /// Checks that the given meta-item is compatible with this `AttributeTemplate`.
-fn is_attr_template_compatible(template: &AttributeTemplate, meta: &ast::MetaItemKind) -> bool {
+pub fn is_attr_template_compatible(template: &AttributeTemplate, meta: &ast::MetaItemKind) -> bool {
     let is_one_allowed_subword = |items: &[MetaItemInner]| match items {
         [item] => item.is_word() && template.one_of.iter().any(|&word| item.has_name(word)),
         _ => false,
@@ -262,70 +234,11 @@ pub fn check_builtin_meta_item(
     style: ast::AttrStyle,
     name: Symbol,
     template: AttributeTemplate,
-    deny_unsafety: bool,
 ) {
     if !is_attr_template_compatible(&template, &meta.kind) {
-        // attrs with new parsers are locally validated so excluded here
-        if matches!(
-            name,
-            sym::inline
-                | sym::export_stable
-                | sym::ffi_const
-                | sym::ffi_pure
-                | sym::rustc_std_internal_symbol
-                | sym::may_dangle
-                | sym::rustc_as_ptr
-                | sym::rustc_pub_transparent
-                | sym::rustc_const_stable_indirect
-                | sym::rustc_force_inline
-                | sym::rustc_confusables
-                | sym::rustc_skip_during_method_dispatch
-                | sym::rustc_pass_by_value
-                | sym::rustc_deny_explicit_impl
-                | sym::rustc_do_not_implement_via_object
-                | sym::rustc_coinductive
-                | sym::const_trait
-                | sym::rustc_specialization_trait
-                | sym::rustc_unsafe_specialization_marker
-                | sym::rustc_allow_incoherent_impl
-                | sym::rustc_coherence_is_core
-                | sym::marker
-                | sym::fundamental
-                | sym::rustc_paren_sugar
-                | sym::type_const
-                | sym::repr
-                | sym::align
-                | sym::deprecated
-                | sym::optimize
-                | sym::cold
-                | sym::target_feature
-                | sym::rustc_allow_const_fn_unstable
-                | sym::naked
-                | sym::no_mangle
-                | sym::non_exhaustive
-                | sym::omit_gdb_pretty_printer_section
-                | sym::path
-                | sym::ignore
-                | sym::must_use
-                | sym::track_caller
-                | sym::link_name
-                | sym::link_ordinal
-                | sym::export_name
-                | sym::rustc_macro_transparency
-                | sym::link_section
-                | sym::rustc_layout_scalar_valid_range_start
-                | sym::rustc_layout_scalar_valid_range_end
-                | sym::no_implicit_prelude
-                | sym::automatically_derived
-        ) {
-            return;
-        }
         emit_malformed_attribute(psess, style, meta.span, name, template);
     }
-
-    if deny_unsafety {
-        deny_builtin_meta_unsafety(psess.dcx(), meta.unsafety, &meta.path);
-    }
+    deny_builtin_meta_unsafety(psess.dcx(), meta.unsafety, &meta.path);
 }
 
 fn emit_malformed_attribute(