Better argument diagnostics in attributes

Cargo.lock

@@ -3402,6 +3402,7 @@ dependencies = [
  "rustc_errors",
  "rustc_feature",
  "rustc_fluent_macro",
+ "rustc_hir",
  "rustc_macros",
  "rustc_session",
  "rustc_span",

compiler/rustc_ast_passes/Cargo.toml

@@ -14,6 +14,7 @@ rustc_data_structures = { path = "../rustc_data_structures" }
 rustc_errors = { path = "../rustc_errors" }
 rustc_feature = { path = "../rustc_feature" }
 rustc_fluent_macro = { path = "../rustc_fluent_macro" }
+rustc_hir = { path = "../rustc_hir" }
 rustc_macros = { path = "../rustc_macros" }
 rustc_session = { path = "../rustc_session" }
 rustc_span = { path = "../rustc_span" }

compiler/rustc_ast_passes/src/feature_gate.rs

@@ -1,11 +1,14 @@
 use rustc_ast as ast;
 use rustc_ast::visit::{self, AssocCtxt, FnCtxt, FnKind, Visitor};
 use rustc_ast::{NodeId, PatKind, attr, token};
+use rustc_attr_parsing::AttributeParser;
 use rustc_feature::{AttributeGate, BUILTIN_ATTRIBUTE_MAP, BuiltinAttribute, Features};
+use rustc_hir::Attribute;
+use rustc_hir::attrs::AttributeKind;
 use rustc_session::Session;
 use rustc_session::parse::{feature_err, feature_warn};
 use rustc_span::source_map::Spanned;
-use rustc_span::{Span, Symbol, sym};
+use rustc_span::{DUMMY_SP, Span, Symbol, sym};
 use thin_vec::ThinVec;
 
 use crate::errors;
@@ -587,17 +590,27 @@ fn maybe_stage_features(sess: &Session, features: &Features, krate: &ast::Crate)
         return;
     }
     let mut errored = false;
-    for attr in krate.attrs.iter().filter(|attr| attr.has_name(sym::feature)) {
+
+    if let Some(Attribute::Parsed(AttributeKind::Feature(feature_idents, first_span))) =
+        AttributeParser::parse_limited(
+            sess,
+            &krate.attrs,
+            sym::feature,
+            DUMMY_SP,
+            krate.id,
+            Some(&features),
+        )
+    {
         // `feature(...)` used on non-nightly. This is definitely an error.
         let mut err = errors::FeatureOnNonNightly {
-            span: attr.span,
+            span: first_span,
             channel: option_env!("CFG_RELEASE_CHANNEL").unwrap_or("(unknown)"),
             stable_features: vec![],
             sugg: None,
         };
 
         let mut all_stable = true;
-        for ident in attr.meta_item_list().into_iter().flatten().flat_map(|nested| nested.ident()) {
+        for ident in feature_idents {
             let name = ident.name;
             let stable_since = features
                 .enabled_lang_features()
@@ -612,7 +625,7 @@ fn maybe_stage_features(sess: &Session, features: &Features, krate: &ast::Crate)
             }
         }
         if all_stable {
-            err.sugg = Some(attr.span);
+            err.sugg = Some(first_span);
         }
         sess.dcx().emit_err(err);
         errored = true;

compiler/rustc_attr_parsing/messages.ftl

@@ -259,3 +259,7 @@ attr_parsing_whole_archive_needs_static =
 attr_parsing_limit_invalid =
     `limit` must be a non-negative integer
     .label = {$error_str}
+
+attr_parsing_feature_single_word =
+    rust features are always a single identifier, not paths with multiple segments
+    .help = did you maybe mean `{$first_segment}`?

compiler/rustc_attr_parsing/src/attributes/codegen_attrs.rs

@@ -226,7 +226,7 @@ impl<S: Stage> AttributeParser<S> for NakedParser {
     const ATTRIBUTES: AcceptMapping<Self, S> =
         &[(&[sym::naked], template!(Word), |this, cx, args| {
             if let Err(span) = args.no_args() {
-                cx.expected_no_args(span);
+                cx.expected_no_args(&cx.attr_path, span);
                 return;
             }
 

compiler/rustc_attr_parsing/src/attributes/crate_level.rs

@@ -3,7 +3,7 @@ use std::num::IntErrorKind;
 use rustc_hir::limit::Limit;
 
 use super::prelude::*;
-use crate::session_diagnostics::LimitInvalid;
+use crate::session_diagnostics::{FeatureExpectedSingleWord, LimitInvalid};
 
 impl<S: Stage> AcceptContext<'_, '_, S> {
     fn parse_limit_int(&self, nv: &NameValueParser) -> Option<Limit> {
@@ -183,3 +183,55 @@ impl<S: Stage> NoArgsAttributeParser<S> for RustcCoherenceIsCoreParser {
     const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::CrateLevel;
     const CREATE: fn(Span) -> AttributeKind = AttributeKind::RustcCoherenceIsCore;
 }
+
+pub(crate) struct FeatureParser;
+
+impl<S: Stage> CombineAttributeParser<S> for FeatureParser {
+    const PATH: &[Symbol] = &[sym::feature];
+    type Item = Ident;
+    const CONVERT: ConvertFn<Self::Item> = AttributeKind::Feature;
+    const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::CrateLevel;
+    const TEMPLATE: AttributeTemplate = template!(List: &["feature1, feature2, ..."]);
+
+    fn extend<'c>(
+        cx: &'c mut AcceptContext<'_, '_, S>,
+        args: &'c ArgParser<'_>,
+    ) -> impl IntoIterator<Item = Self::Item> + 'c {
+        let ArgParser::List(list) = args else {
+            cx.expected_list(cx.attr_span);
+            return Vec::new();
+        };
+
+        if list.is_empty() {
+            cx.warn_empty_attribute(cx.attr_span);
+        }
+
+        let mut res = Vec::new();
+
+        for elem in list.mixed() {
+            let Some(elem) = elem.meta_item() else {
+                cx.expected_identifier(elem.span());
+                continue;
+            };
+            if let Err(arg_span) = elem.args().no_args() {
+                cx.expected_no_args(&elem.path().get_attribute_path(), arg_span);
+                continue;
+            }
+
+            let path = elem.path();
+            let Some(ident) = path.word() else {
+                let first_segment = elem.path().segments().next().expect("at least one segment");
+                cx.emit_err(FeatureExpectedSingleWord {
+                    span: path.span(),
+                    first_segment_span: first_segment.span,
+                    first_segment: first_segment.name,
+                });
+                continue;
+            };
+
+            res.push(ident);
+        }
+
+        res
+    }
+}

compiler/rustc_attr_parsing/src/attributes/macro_attrs.rs

@@ -88,7 +88,10 @@ impl<S: Stage> AttributeParser<S> for MacroUseParser {
                                     continue;
                                 };
                                 if let Err(err_span) = item.args().no_args() {
-                                    cx.expected_no_args(err_span);
+                                    cx.expected_no_args(
+                                        &item.path().get_attribute_path(),
+                                        err_span,
+                                    );
                                     continue;
                                 }
                                 let Some(item) = item.path().word() else {

compiler/rustc_attr_parsing/src/attributes/mod.rs

@@ -280,7 +280,7 @@ impl<T: NoArgsAttributeParser<S>, S: Stage> SingleAttributeParser<S> for Without
 
     fn convert(cx: &mut AcceptContext<'_, '_, S>, args: &ArgParser<'_>) -> Option<AttributeKind> {
         if let Err(span) = args.no_args() {
-            cx.expected_no_args(span);
+            cx.expected_no_args(&cx.attr_path, span);
         }
         Some(T::CREATE(cx.attr_span))
     }

compiler/rustc_attr_parsing/src/attributes/proc_macro_attrs.rs

@@ -88,13 +88,19 @@ fn parse_derive_like<S: Stage>(
         return None;
     }
     if let Err(e) = trait_attr.args().no_args() {
-        cx.expected_no_args(e);
+        cx.expected_no_args(&trait_attr.path().get_attribute_path(), e);
         return None;
     };
 
+    // updated if we see `attributes(...)` to keep track of the last
+    // argument we did accept for the final diagnostic
+    let mut last = trait_ident.span;
+
     // Parse optional attributes
     let mut attributes = ThinVec::new();
     if let Some(attrs) = items.next() {
+        last = attrs.span();
+
         let Some(attr_list) = attrs.meta_item() else {
             cx.expected_list(attrs.span());
             return None;
@@ -115,7 +121,7 @@ fn parse_derive_like<S: Stage>(
                 return None;
             };
             if let Err(e) = attr.args().no_args() {
-                cx.expected_no_args(e);
+                cx.expected_no_args(&attr.path().get_attribute_path(), e);
                 return None;
             };
             let Some(ident) = attr.path().word() else {
@@ -132,7 +138,7 @@ fn parse_derive_like<S: Stage>(
 
     // If anything else is specified, we should reject it
     if let Some(next) = items.next() {
-        cx.expected_no_args(next.span());
+        cx.expected_end_of_list(last, next.span());
     }
 
     Some((Some(trait_ident.name), attributes))

compiler/rustc_attr_parsing/src/attributes/rustc_internal.rs

@@ -42,7 +42,7 @@ impl<S: Stage> SingleAttributeParser<S> for RustcObjectLifetimeDefaultParser {
 
     fn convert(cx: &mut AcceptContext<'_, '_, S>, args: &ArgParser<'_>) -> Option<AttributeKind> {
         if let Err(span) = args.no_args() {
-            cx.expected_no_args(span);
+            cx.expected_no_args(&cx.attr_path, span);
             return None;
         }
 

compiler/rustc_attr_parsing/src/attributes/traits.rs

@@ -35,7 +35,7 @@ impl<S: Stage> SingleAttributeParser<S> for SkipDuringMethodDispatchParser {
                 continue;
             };
             if let Err(span) = arg.args().no_args() {
-                cx.expected_no_args(span);
+                cx.expected_no_args(&arg.path().get_attribute_path(), span);
             }
             let path = arg.path();
             let (key, skip): (Symbol, &mut bool) = match path.word_sym() {

compiler/rustc_attr_parsing/src/context.rs

@@ -25,8 +25,9 @@ use crate::attributes::codegen_attrs::{
 };
 use crate::attributes::confusables::ConfusablesParser;
 use crate::attributes::crate_level::{
-    CrateNameParser, MoveSizeLimitParser, NoCoreParser, NoStdParser, PatternComplexityLimitParser,
-    RecursionLimitParser, RustcCoherenceIsCoreParser, TypeLengthLimitParser,
+    CrateNameParser, FeatureParser, MoveSizeLimitParser, NoCoreParser, NoStdParser,
+    PatternComplexityLimitParser, RecursionLimitParser, RustcCoherenceIsCoreParser,
+    TypeLengthLimitParser,
 };
 use crate::attributes::deprecation::DeprecationParser;
 use crate::attributes::dummy::DummyParser;
@@ -163,6 +164,7 @@ attribute_parsers!(
         // tidy-alphabetical-start
         Combine<AllowConstFnUnstableParser>,
         Combine<AllowInternalUnstableParser>,
+        Combine<FeatureParser>,
         Combine<ForceTargetFeatureParser>,
         Combine<LinkParser>,
         Combine<ReprParser>,
@@ -448,13 +450,13 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
         })
     }
 
-    pub(crate) fn expected_no_args(&self, args_span: Span) -> ErrorGuaranteed {
+    pub(crate) fn expected_no_args(&self, path: &AttrPath, args_span: Span) -> ErrorGuaranteed {
         self.emit_err(AttributeParseError {
             span: args_span,
             attr_span: self.attr_span,
             template: self.template.clone(),
             attribute: self.attr_path.clone(),
-            reason: AttributeParseErrorReason::ExpectedNoArgs,
+            reason: AttributeParseErrorReason::ExpectedNoArgs { path },
             attr_style: self.attr_style,
         })
     }
@@ -509,6 +511,21 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
         })
     }
 
+    /// Expected the end of an argument list.
+    ///
+    /// Note: only useful when arguments in an attribute are ordered and we've seen the last one we expected.
+    /// Most attributes shouldn't care about their argument order.
+    pub(crate) fn expected_end_of_list(&self, last_item_span: Span, span: Span) -> ErrorGuaranteed {
+        self.emit_err(AttributeParseError {
+            span,
+            attr_span: self.attr_span,
+            template: self.template.clone(),
+            attribute: self.attr_path.clone(),
+            reason: AttributeParseErrorReason::ExpectedEnd { last: last_item_span },
+            attr_style: self.attr_style,
+        })
+    }
+
     pub(crate) fn expected_single_argument(&self, span: Span) -> ErrorGuaranteed {
         self.emit_err(AttributeParseError {
             span,