Rollup merge of rust-lang#88681 - ehuss:duplicate-attributes, r=petro…

…chenkov

Check for duplicate attributes.

This adds some checks for duplicate attributes. In many cases, the duplicates were being ignored without error or warning. This adds several kinds of checks (see `AttributeDuplicates` enum).

The motivation here is to issue unused warnings with similar reasoning for any unused lint, and to error for cases where there are conflicts.

This also adds a check for empty attribute lists in a few attributes where this causes the attribute to be ignored.

Closes rust-lang#55112.

compiler/rustc_feature/src/lib.rs

@@ -11,6 +11,7 @@
 //! even if it is stabilized or removed, *do not remove it*. Instead, move the
 //! symbol to the `accepted` or `removed` modules respectively.
 
+#![feature(derive_default_enum)]
 #![feature(once_cell)]
 
 mod accepted;
@@ -146,6 +147,7 @@ pub fn find_feature_issue(feature: Symbol, issue: GateIssue) -> Option<NonZeroU3
 
 pub use accepted::ACCEPTED_FEATURES;
 pub use active::{Features, ACTIVE_FEATURES, INCOMPATIBLE_FEATURES};
+pub use builtin_attrs::AttributeDuplicates;
 pub use builtin_attrs::{
     deprecated_attributes, find_gated_cfg, is_builtin_attr_name, AttributeGate, AttributeTemplate,
     AttributeType, BuiltinAttribute, GatedCfg, BUILTIN_ATTRIBUTES, BUILTIN_ATTRIBUTE_MAP,

compiler/rustc_passes/src/check_attr.rs

@@ -9,9 +9,9 @@ use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::TyCtxt;
 
 use rustc_ast::{ast, AttrStyle, Attribute, Lit, LitKind, NestedMetaItem};
-use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::{pluralize, struct_span_err, Applicability};
-use rustc_feature::{AttributeType, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP};
+use rustc_feature::{AttributeDuplicates, AttributeType, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP};
 use rustc_hir as hir;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
@@ -23,6 +23,7 @@ use rustc_session::lint::builtin::{
 use rustc_session::parse::feature_err;
 use rustc_span::symbol::{sym, Symbol};
 use rustc_span::{MultiSpan, Span, DUMMY_SP};
+use std::collections::hash_map::Entry;
 
 pub(crate) fn target_from_impl_item<'tcx>(
     tcx: TyCtxt<'tcx>,
@@ -69,7 +70,7 @@ impl CheckAttrVisitor<'tcx> {
         let mut doc_aliases = FxHashMap::default();
         let mut is_valid = true;
         let mut specified_inline = None;
-        let mut seen = FxHashSet::default();
+        let mut seen = FxHashMap::default();
         let attrs = self.tcx.hir().attrs(hir_id);
         for attr in attrs {
             let attr_is_valid = match attr.name_or_empty() {
@@ -148,6 +149,8 @@ impl CheckAttrVisitor<'tcx> {
                 _ => {}
             }
 
+            let builtin = attr.ident().and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name));
+
             if hir_id != CRATE_HIR_ID {
                 if let Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) =
                     attr.ident().and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name))
@@ -165,21 +168,37 @@ impl CheckAttrVisitor<'tcx> {
                 }
             }
 
-            // Duplicate attributes
-            match attr.name_or_empty() {
-                name @ sym::macro_use => {
-                    let args = attr.meta_item_list().unwrap_or_else(Vec::new);
-                    let args: Vec<_> = args.iter().map(|arg| arg.name_or_empty()).collect();
-                    if !seen.insert((name, args)) {
-                        self.tcx.struct_span_lint_hir(
-                            UNUSED_ATTRIBUTES,
-                            hir_id,
+            if let Some(BuiltinAttribute { duplicates, .. }) = builtin {
+                check_duplicates(self.tcx, attr, hir_id, *duplicates, &mut seen);
+            }
+
+            // Warn on useless empty attributes.
+            if matches!(
+                attr.name_or_empty(),
+                sym::macro_use
+                    | sym::allow
+                    | sym::warn
+                    | sym::deny
+                    | sym::forbid
+                    | sym::feature
+                    | sym::repr
+                    | sym::target_feature
+            ) && attr.meta_item_list().map_or(false, |list| list.is_empty())
+            {
+                self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
+                    lint.build("unused attribute")
+                        .span_suggestion(
                             attr.span,
-                            |lint| lint.build("unused attribute").emit(),
-                        );
-                    }
-                }
-                _ => {}
+                            "remove this attribute",
+                            String::new(),
+                            Applicability::MachineApplicable,
+                        )
+                        .note(&format!(
+                            "attribute `{}` with an empty list has no effect",
+                            attr.name_or_empty()
+                        ))
+                        .emit();
+                });
             }
         }
 
@@ -1990,3 +2009,77 @@ fn check_mod_attrs(tcx: TyCtxt<'_>, module_def_id: LocalDefId) {
 pub(crate) fn provide(providers: &mut Providers) {
     *providers = Providers { check_mod_attrs, ..*providers };
 }
+
+fn check_duplicates(
+    tcx: TyCtxt<'_>,
+    attr: &Attribute,
+    hir_id: HirId,
+    duplicates: AttributeDuplicates,
+    seen: &mut FxHashMap<Symbol, Span>,
+) {
+    use AttributeDuplicates::*;
+    if matches!(duplicates, WarnFollowingWordOnly) && !attr.is_word() {
+        return;
+    }
+    match duplicates {
+        DuplicatesOk => {}
+        WarnFollowing | FutureWarnFollowing | WarnFollowingWordOnly | FutureWarnPreceding => {
+            match seen.entry(attr.name_or_empty()) {
+                Entry::Occupied(mut entry) => {
+                    let (this, other) = if matches!(duplicates, FutureWarnPreceding) {
+                        let to_remove = entry.insert(attr.span);
+                        (to_remove, attr.span)
+                    } else {
+                        (attr.span, *entry.get())
+                    };
+                    tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, this, |lint| {
+                        let mut db = lint.build("unused attribute");
+                        db.span_note(other, "attribute also specified here").span_suggestion(
+                            this,
+                            "remove this attribute",
+                            String::new(),
+                            Applicability::MachineApplicable,
+                        );
+                        if matches!(duplicates, FutureWarnFollowing | FutureWarnPreceding) {
+                            db.warn(
+                                "this was previously accepted by the compiler but is \
+                                 being phased out; it will become a hard error in \
+                                 a future release!",
+                            );
+                        }
+                        db.emit();
+                    });
+                }
+                Entry::Vacant(entry) => {
+                    entry.insert(attr.span);
+                }
+            }
+        }
+        ErrorFollowing | ErrorPreceding => match seen.entry(attr.name_or_empty()) {
+            Entry::Occupied(mut entry) => {
+                let (this, other) = if matches!(duplicates, ErrorPreceding) {
+                    let to_remove = entry.insert(attr.span);
+                    (to_remove, attr.span)
+                } else {
+                    (attr.span, *entry.get())
+                };
+                tcx.sess
+                    .struct_span_err(
+                        this,
+                        &format!("multiple `{}` attributes", attr.name_or_empty()),
+                    )
+                    .span_note(other, "attribute also specified here")
+                    .span_suggestion(
+                        this,
+                        "remove this attribute",
+                        String::new(),
+                        Applicability::MachineApplicable,
+                    )
+                    .emit();
+            }
+            Entry::Vacant(entry) => {
+                entry.insert(attr.span);
+            }
+        },
+    }
+}

src/test/ui/empty/empty-attributes.rs

@@ -0,0 +1,14 @@
+#![deny(unused_attributes)]
+#![allow()] //~ ERROR unused attribute
+#![warn()] //~ ERROR unused attribute
+#![deny()] //~ ERROR unused attribute
+#![forbid()] //~ ERROR unused attribute
+#![feature()] //~ ERROR unused attribute
+
+#[repr()] //~ ERROR unused attribute
+pub struct S;
+
+#[target_feature()] //~ ERROR unused attribute
+pub unsafe fn foo() {}
+
+fn main() {}

src/test/ui/empty/empty-attributes.stderr

@@ -0,0 +1,63 @@
+error: unused attribute
+  --> $DIR/empty-attributes.rs:8:1
+   |
+LL | #[repr()]
+   | ^^^^^^^^^ help: remove this attribute
+   |
+note: the lint level is defined here
+  --> $DIR/empty-attributes.rs:1:9
+   |
+LL | #![deny(unused_attributes)]
+   |         ^^^^^^^^^^^^^^^^^
+   = note: attribute `repr` with an empty list has no effect
+
+error: unused attribute
+  --> $DIR/empty-attributes.rs:11:1
+   |
+LL | #[target_feature()]
+   | ^^^^^^^^^^^^^^^^^^^ help: remove this attribute
+   |
+   = note: attribute `target_feature` with an empty list has no effect
+
+error: unused attribute
+  --> $DIR/empty-attributes.rs:2:1
+   |
+LL | #![allow()]
+   | ^^^^^^^^^^^ help: remove this attribute
+   |
+   = note: attribute `allow` with an empty list has no effect
+
+error: unused attribute
+  --> $DIR/empty-attributes.rs:3:1
+   |
+LL | #![warn()]
+   | ^^^^^^^^^^ help: remove this attribute
+   |
+   = note: attribute `warn` with an empty list has no effect
+
+error: unused attribute
+  --> $DIR/empty-attributes.rs:4:1
+   |
+LL | #![deny()]
+   | ^^^^^^^^^^ help: remove this attribute
+   |
+   = note: attribute `deny` with an empty list has no effect
+
+error: unused attribute
+  --> $DIR/empty-attributes.rs:5:1
+   |
+LL | #![forbid()]
+   | ^^^^^^^^^^^^ help: remove this attribute
+   |
+   = note: attribute `forbid` with an empty list has no effect
+
+error: unused attribute
+  --> $DIR/empty-attributes.rs:6:1
+   |
+LL | #![feature()]
+   | ^^^^^^^^^^^^^ help: remove this attribute
+   |
+   = note: attribute `feature` with an empty list has no effect
+
+error: aborting due to 7 previous errors
+

src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs-error.rs

@@ -134,4 +134,27 @@ mod start {
     //~^ ERROR: `start` attribute can only be used on functions
 }
 
+#[repr(C)]
+//~^ ERROR: attribute should be applied to a struct, enum, or union
+mod repr {
+//~^ NOTE not a struct, enum, or union
+    mod inner { #![repr(C)] }
+    //~^ ERROR: attribute should be applied to a struct, enum, or union
+    //~| NOTE not a struct, enum, or union
+
+    #[repr(C)] fn f() { }
+    //~^ ERROR: attribute should be applied to a struct, enum, or union
+    //~| NOTE not a struct, enum, or union
+
+    struct S;
+
+    #[repr(C)] type T = S;
+    //~^ ERROR: attribute should be applied to a struct, enum, or union
+    //~| NOTE not a struct, enum, or union
+
+    #[repr(C)] impl S { }
+    //~^ ERROR: attribute should be applied to a struct, enum, or union
+    //~| NOTE not a struct, enum, or union
+}
+
 fn main() {}

src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs-error.stderr

@@ -91,6 +91,21 @@ LL | |     }
 LL | | }
    | |_- not a free function, impl method or static
 
+error[E0517]: attribute should be applied to a struct, enum, or union
+  --> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:137:8
+   |
+LL |   #[repr(C)]
+   |          ^
+LL |
+LL | / mod repr {
+LL | |
+LL | |     mod inner { #![repr(C)] }
+LL | |
+...  |
+LL | |
+LL | | }
+   | |_- not a struct, enum, or union
+
 error: attribute should be applied to an `extern crate` item
   --> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:25:1
    |
@@ -235,7 +250,31 @@ error: attribute should be applied to a free function, impl method or static
 LL |         #[export_name = "2200"] fn bar() {}
    |         ^^^^^^^^^^^^^^^^^^^^^^^ ----------- not a free function, impl method or static
 
-error: aborting due to 34 previous errors
+error[E0517]: attribute should be applied to a struct, enum, or union
+  --> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:141:25
+   |
+LL |     mod inner { #![repr(C)] }
+   |     --------------------^---- not a struct, enum, or union
+
+error[E0517]: attribute should be applied to a struct, enum, or union
+  --> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:145:12
+   |
+LL |     #[repr(C)] fn f() { }
+   |            ^   ---------- not a struct, enum, or union
+
+error[E0517]: attribute should be applied to a struct, enum, or union
+  --> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:151:12
+   |
+LL |     #[repr(C)] type T = S;
+   |            ^   ----------- not a struct, enum, or union
+
+error[E0517]: attribute should be applied to a struct, enum, or union
+  --> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:155:12
+   |
+LL |     #[repr(C)] impl S { }
+   |            ^   ---------- not a struct, enum, or union
+
+error: aborting due to 39 previous errors
 
-Some errors have detailed explanations: E0518, E0658.
-For more information about an error, try `rustc --explain E0518`.
+Some errors have detailed explanations: E0517, E0518, E0658.
+For more information about an error, try `rustc --explain E0517`.

src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs.rs

@@ -245,19 +245,6 @@ mod bench {
     impl S { }
 }
 
-#[repr()]
-mod repr {
-    mod inner { #![repr()] }
-
-    #[repr()] fn f() { }
-
-    struct S;
-
-    #[repr()] type T = S;
-
-    #[repr()] impl S { }
-}
-
 #[path = "3800"]
 mod path {
     mod inner { #![path="3800"] }