Auto merge of #138164 - jdonszelmann:attr-parsing-lint-infra, r=oli-obk

Infrastructure for lints during attribute parsing, specifically duplicate usages of attributes

r? `@oli-obk`

This PR adds a new field to OwnerInfo to buffer lints which are generated during attribute parsing and ast lowering in general. They can't be emitted at this stage because at that point there's no HIR yet, and early lints are already emitted.

This also adds the generic `S: Stage` to attribute parsers. Currently we don't emit any lints during early attribute parsing, but if we ever want to that logic will be different. That's because there we don't have hir ids yet, while at the same time still having access to node ids and early lints. Even though that logic isn't completely there in this PR (no worries, we don't use it), that's why the parameter is there.

With this PR, we also add 2 associated consts  to `SingleAttributeParser`. Those determine what logic should be applied when finding a duplicate attribute.

This PR was getting pretty large, so the first code using this logic is in #138165. This code is all new things that weren't possible before so it also doesn't break any behaviour. However, some of it will be dead code right now. I recommend reviewing both before merging, though in some sense that doubles the size of the review again, and the other PR might be more controversial. Let me know how you want to do this `@oli-obk`

Author: bors

Cargo.lock

@@ -3793,6 +3793,7 @@ dependencies = [
  "rustc_arena",
  "rustc_ast",
  "rustc_attr_data_structures",
+ "rustc_attr_parsing",
  "rustc_data_structures",
  "rustc_errors",
  "rustc_feature",

compiler/rustc_ast_lowering/src/expr.rs

@@ -73,16 +73,15 @@ impl<'hir> LoweringContext<'_, 'hir> {
                     // Merge attributes into the inner expression.
                     if !e.attrs.is_empty() {
                         let old_attrs = self.attrs.get(&ex.hir_id.local_id).copied().unwrap_or(&[]);
-                        let attrs = &*self.arena.alloc_from_iter(
-                            self.lower_attrs_vec(&e.attrs, e.span)
-                                .into_iter()
-                                .chain(old_attrs.iter().cloned()),
-                        );
-                        if attrs.is_empty() {
+                        let new_attrs = self
+                            .lower_attrs_vec(&e.attrs, e.span, ex.hir_id)
+                            .into_iter()
+                            .chain(old_attrs.iter().cloned());
+                        let new_attrs = &*self.arena.alloc_from_iter(new_attrs);
+                        if new_attrs.is_empty() {
                             return ex;
                         }
-
-                        self.attrs.insert(ex.hir_id.local_id, attrs);
+                        self.attrs.insert(ex.hir_id.local_id, new_attrs);
                     }
                     return ex;
                 }
@@ -2035,7 +2034,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 let ret_expr = self.checked_return(Some(from_residual_expr));
                 self.arena.alloc(self.expr(try_span, ret_expr))
             };
-            self.lower_attrs(ret_expr.hir_id, &attrs, ret_expr.span);
+            self.lower_attrs(ret_expr.hir_id, &attrs, span);
 
             let break_pat = self.pat_cf_break(try_span, residual_local);
             self.arm(break_pat, ret_expr)

compiler/rustc_ast_lowering/src/lib.rs

@@ -51,6 +51,7 @@ use rustc_data_structures::tagged_ptr::TaggedRef;
 use rustc_errors::{DiagArgFromDisplay, DiagCtxtHandle, StashKey};
 use rustc_hir::def::{DefKind, LifetimeRes, Namespace, PartialRes, PerNS, Res};
 use rustc_hir::def_id::{CRATE_DEF_ID, LOCAL_CRATE, LocalDefId};
+use rustc_hir::lints::DelayedLint;
 use rustc_hir::{
     self as hir, AngleBrackets, ConstArg, GenericArg, HirId, ItemLocalMap, LangItem,
     LifetimeSource, LifetimeSyntax, ParamName, TraitCandidate,
@@ -141,6 +142,8 @@ struct LoweringContext<'a, 'hir> {
     allow_for_await: Arc<[Symbol]>,
     allow_async_fn_traits: Arc<[Symbol]>,
 
+    delayed_lints: Vec<DelayedLint>,
+
     attribute_parser: AttributeParser<'hir>,
 }
 
@@ -190,6 +193,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             allow_async_iterator: [sym::gen_future, sym::async_iterator].into(),
 
             attribute_parser: AttributeParser::new(tcx.sess, tcx.features(), registered_tools),
+            delayed_lints: Vec::new(),
         }
     }
 
@@ -198,6 +202,22 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
     }
 }
 
+struct SpanLowerer {
+    is_incremental: bool,
+    def_id: LocalDefId,
+}
+
+impl SpanLowerer {
+    fn lower(&self, span: Span) -> Span {
+        if self.is_incremental {
+            span.with_parent(Some(self.def_id))
+        } else {
+            // Do not make spans relative when not using incremental compilation.
+            span
+        }
+    }
+}
+
 #[extension(trait ResolverAstLoweringExt)]
 impl ResolverAstLowering {
     fn legacy_const_generic_args(&self, expr: &Expr) -> Option<Vec<usize>> {
@@ -573,6 +593,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             std::mem::replace(&mut self.item_local_id_counter, hir::ItemLocalId::new(1));
         let current_impl_trait_defs = std::mem::take(&mut self.impl_trait_defs);
         let current_impl_trait_bounds = std::mem::take(&mut self.impl_trait_bounds);
+        let current_delayed_lints = std::mem::take(&mut self.delayed_lints);
 
         // Do not reset `next_node_id` and `node_id_to_def_id`:
         // we want `f` to be able to refer to the `LocalDefId`s that the caller created.
@@ -606,6 +627,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         self.item_local_id_counter = current_local_counter;
         self.impl_trait_defs = current_impl_trait_defs;
         self.impl_trait_bounds = current_impl_trait_bounds;
+        self.delayed_lints = current_delayed_lints;
 
         debug_assert!(!self.children.iter().any(|(id, _)| id == &owner_id.def_id));
         self.children.push((owner_id.def_id, hir::MaybeOwner::Owner(info)));
@@ -616,6 +638,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         let mut bodies = std::mem::take(&mut self.bodies);
         let define_opaque = std::mem::take(&mut self.define_opaque);
         let trait_map = std::mem::take(&mut self.trait_map);
+        let delayed_lints = std::mem::take(&mut self.delayed_lints).into_boxed_slice();
 
         #[cfg(debug_assertions)]
         for (id, attrs) in attrs.iter() {
@@ -629,14 +652,16 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         let bodies = SortedMap::from_presorted_elements(bodies);
 
         // Don't hash unless necessary, because it's expensive.
-        let (opt_hash_including_bodies, attrs_hash) =
-            self.tcx.hash_owner_nodes(node, &bodies, &attrs, define_opaque);
+        let (opt_hash_including_bodies, attrs_hash, delayed_lints_hash) =
+            self.tcx.hash_owner_nodes(node, &bodies, &attrs, &delayed_lints, define_opaque);
         let num_nodes = self.item_local_id_counter.as_usize();
         let (nodes, parenting) = index::index_hir(self.tcx, node, &bodies, num_nodes);
         let nodes = hir::OwnerNodes { opt_hash_including_bodies, nodes, bodies };
         let attrs = hir::AttributeMap { map: attrs, opt_hash: attrs_hash, define_opaque };
+        let delayed_lints =
+            hir::lints::DelayedLints { lints: delayed_lints, opt_hash: delayed_lints_hash };
 
-        self.arena.alloc(hir::OwnerInfo { nodes, parenting, attrs, trait_map })
+        self.arena.alloc(hir::OwnerInfo { nodes, parenting, attrs, trait_map, delayed_lints })
     }
 
     /// This method allocates a new `HirId` for the given `NodeId`.
@@ -759,15 +784,17 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         })
     }
 
+    fn span_lowerer(&self) -> SpanLowerer {
+        SpanLowerer {
+            is_incremental: self.tcx.sess.opts.incremental.is_some(),
+            def_id: self.current_hir_id_owner.def_id,
+        }
+    }
+
     /// Intercept all spans entering HIR.
     /// Mark a span as relative to the current owning item.
     fn lower_span(&self, span: Span) -> Span {
-        if self.tcx.sess.opts.incremental.is_some() {
-            span.with_parent(Some(self.current_hir_id_owner.def_id))
-        } else {
-            // Do not make spans relative when not using incremental compilation.
-            span
-        }
+        self.span_lowerer().lower(span)
     }
 
     fn lower_ident(&self, ident: Ident) -> Ident {
@@ -889,7 +916,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         if attrs.is_empty() {
             &[]
         } else {
-            let lowered_attrs = self.lower_attrs_vec(attrs, self.lower_span(target_span));
+            let lowered_attrs = self.lower_attrs_vec(attrs, self.lower_span(target_span), id);
 
             debug_assert_eq!(id.owner, self.current_hir_id_owner);
             let ret = self.arena.alloc_from_iter(lowered_attrs);
@@ -909,9 +936,23 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         }
     }
 
-    fn lower_attrs_vec(&self, attrs: &[Attribute], target_span: Span) -> Vec<hir::Attribute> {
-        self.attribute_parser
-            .parse_attribute_list(attrs, target_span, OmitDoc::Lower, |s| self.lower_span(s))
+    fn lower_attrs_vec(
+        &mut self,
+        attrs: &[Attribute],
+        target_span: Span,
+        target_hir_id: HirId,
+    ) -> Vec<hir::Attribute> {
+        let l = self.span_lowerer();
+        self.attribute_parser.parse_attribute_list(
+            attrs,
+            target_span,
+            target_hir_id,
+            OmitDoc::Lower,
+            |s| l.lower(s),
+            |l| {
+                self.delayed_lints.push(DelayedLint::AttributeParsing(l));
+            },
+        )
     }
 
     fn alias_attrs(&mut self, id: HirId, target_id: HirId) {

compiler/rustc_attr_data_structures/src/lib.rs

@@ -8,6 +8,8 @@ mod attributes;
 mod stability;
 mod version;
 
+pub mod lints;
+
 use std::num::NonZero;
 
 pub use attributes::*;

compiler/rustc_attr_data_structures/src/lints.rs

@@ -0,0 +1,14 @@
+use rustc_macros::HashStable_Generic;
+use rustc_span::Span;
+
+#[derive(Clone, Debug, HashStable_Generic)]
+pub struct AttributeLint<Id> {
+    pub id: Id,
+    pub span: Span,
+    pub kind: AttributeLintKind,
+}
+
+#[derive(Clone, Debug, HashStable_Generic)]
+pub enum AttributeLintKind {
+    UnusedDuplicate { this: Span, other: Span, warning: bool },
+}

compiler/rustc_attr_parsing/messages.ftl

@@ -131,7 +131,15 @@ attr_parsing_unsupported_literal_generic =
 attr_parsing_unsupported_literal_suggestion =
     consider removing the prefix
 
+attr_parsing_unused_duplicate =
+    unused attribute
+    .suggestion = remove this attribute
+    .note = attribute also specified here
+    .warn = {-passes_previously_accepted}
 attr_parsing_unused_multiple =
     multiple `{$name}` attributes
     .suggestion = remove this attribute
     .note = attribute also specified here
+
+-attr_parsing_perviously_accepted =
+    this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

compiler/rustc_attr_parsing/src/attributes/allow_unstable.rs

@@ -4,41 +4,43 @@ use rustc_attr_data_structures::AttributeKind;
 use rustc_span::{Span, Symbol, sym};
 
 use super::{CombineAttributeParser, ConvertFn};
-use crate::context::AcceptContext;
+use crate::context::{AcceptContext, Stage};
 use crate::parser::ArgParser;
 use crate::session_diagnostics;
 
 pub(crate) struct AllowInternalUnstableParser;
-impl CombineAttributeParser for AllowInternalUnstableParser {
-    const PATH: &'static [Symbol] = &[sym::allow_internal_unstable];
+impl<S: Stage> CombineAttributeParser<S> for AllowInternalUnstableParser {
+    const PATH: &[Symbol] = &[sym::allow_internal_unstable];
     type Item = (Symbol, Span);
     const CONVERT: ConvertFn<Self::Item> = AttributeKind::AllowInternalUnstable;
 
-    fn extend<'a>(
-        cx: &'a AcceptContext<'a>,
-        args: &'a ArgParser<'a>,
-    ) -> impl IntoIterator<Item = Self::Item> + 'a {
-        parse_unstable(cx, args, Self::PATH[0]).into_iter().zip(iter::repeat(cx.attr_span))
+    fn extend<'c>(
+        cx: &'c mut AcceptContext<'_, '_, S>,
+        args: &'c ArgParser<'_>,
+    ) -> impl IntoIterator<Item = Self::Item> {
+        parse_unstable(cx, args, <Self as CombineAttributeParser<S>>::PATH[0])
+            .into_iter()
+            .zip(iter::repeat(cx.attr_span))
     }
 }
 
 pub(crate) struct AllowConstFnUnstableParser;
-impl CombineAttributeParser for AllowConstFnUnstableParser {
-    const PATH: &'static [Symbol] = &[sym::rustc_allow_const_fn_unstable];
+impl<S: Stage> CombineAttributeParser<S> for AllowConstFnUnstableParser {
+    const PATH: &[Symbol] = &[sym::rustc_allow_const_fn_unstable];
     type Item = Symbol;
     const CONVERT: ConvertFn<Self::Item> = AttributeKind::AllowConstFnUnstable;
 
-    fn extend<'a>(
-        cx: &'a AcceptContext<'a>,
-        args: &'a ArgParser<'a>,
-    ) -> impl IntoIterator<Item = Self::Item> + 'a {
-        parse_unstable(cx, args, Self::PATH[0])
+    fn extend<'c>(
+        cx: &'c mut AcceptContext<'_, '_, S>,
+        args: &'c ArgParser<'_>,
+    ) -> impl IntoIterator<Item = Self::Item> + 'c {
+        parse_unstable(cx, args, <Self as CombineAttributeParser<S>>::PATH[0])
     }
 }
 
-fn parse_unstable<'a>(
-    cx: &AcceptContext<'_>,
-    args: &'a ArgParser<'a>,
+fn parse_unstable<S: Stage>(
+    cx: &AcceptContext<'_, '_, S>,
+    args: &ArgParser<'_>,
     symbol: Symbol,
 ) -> impl IntoIterator<Item = Symbol> {
     let mut res = Vec::new();

compiler/rustc_attr_parsing/src/attributes/confusables.rs

@@ -3,7 +3,7 @@ use rustc_span::{Span, Symbol, sym};
 use thin_vec::ThinVec;
 
 use super::{AcceptMapping, AttributeParser};
-use crate::context::FinalizeContext;
+use crate::context::{FinalizeContext, Stage};
 use crate::session_diagnostics;
 
 #[derive(Default)]
@@ -12,8 +12,8 @@ pub(crate) struct ConfusablesParser {
     first_span: Option<Span>,
 }
 
-impl AttributeParser for ConfusablesParser {
-    const ATTRIBUTES: AcceptMapping<Self> = &[(&[sym::rustc_confusables], |this, cx, args| {
+impl<S: Stage> AttributeParser<S> for ConfusablesParser {
+    const ATTRIBUTES: AcceptMapping<Self, S> = &[(&[sym::rustc_confusables], |this, cx, args| {
         let Some(list) = args.list() else {
             // FIXME(jdonszelmann): error when not a list? Bring validation code here.
             //       NOTE: currently subsequent attributes are silently ignored using
@@ -45,7 +45,7 @@ impl AttributeParser for ConfusablesParser {
         this.first_span.get_or_insert(cx.attr_span);
     })];
 
-    fn finalize(self, _cx: &FinalizeContext<'_>) -> Option<AttributeKind> {
+    fn finalize(self, _cx: &FinalizeContext<'_, '_, S>) -> Option<AttributeKind> {
         if self.confusables.is_empty() {
             return None;
         }

compiler/rustc_attr_parsing/src/attributes/deprecation.rs

@@ -1,17 +1,17 @@
 use rustc_attr_data_structures::{AttributeKind, DeprecatedSince, Deprecation};
 use rustc_span::{Span, Symbol, sym};
 
-use super::SingleAttributeParser;
 use super::util::parse_version;
-use crate::context::AcceptContext;
+use super::{AttributeOrder, OnDuplicate, SingleAttributeParser};
+use crate::context::{AcceptContext, Stage};
 use crate::parser::ArgParser;
 use crate::session_diagnostics;
 use crate::session_diagnostics::UnsupportedLiteralReason;
 
 pub(crate) struct DeprecationParser;
 
-fn get(
-    cx: &AcceptContext<'_>,
+fn get<S: Stage>(
+    cx: &AcceptContext<'_, '_, S>,
     name: Symbol,
     param_span: Span,
     arg: &ArgParser<'_>,
@@ -41,19 +41,12 @@ fn get(
     }
 }
 
-impl SingleAttributeParser for DeprecationParser {
-    const PATH: &'static [Symbol] = &[sym::deprecated];
+impl<S: Stage> SingleAttributeParser<S> for DeprecationParser {
+    const PATH: &[Symbol] = &[sym::deprecated];
+    const ATTRIBUTE_ORDER: AttributeOrder = AttributeOrder::KeepFirst;
+    const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::Error;
 
-    fn on_duplicate(cx: &AcceptContext<'_>, first_span: Span) {
-        // FIXME(jdonszelmann): merge with errors from check_attrs.rs
-        cx.emit_err(session_diagnostics::UnusedMultiple {
-            this: cx.attr_span,
-            other: first_span,
-            name: sym::deprecated,
-        });
-    }
-
-    fn convert(cx: &AcceptContext<'_>, args: &ArgParser<'_>) -> Option<AttributeKind> {
+    fn convert(cx: &mut AcceptContext<'_, '_, S>, args: &ArgParser<'_>) -> Option<AttributeKind> {
         let features = cx.features();
 
         let mut since = None;