Auto merge of #12378 - GuillaumeGomez:duplicated_attr, r=blyxyas

Add new `duplicated_attributes` lint

It's a lint idea that `@llogiq` gave me while reviewing another PR.

There are some limitations, in particular for the "output". Initially I wanted to make it possible for directly lint against the whole attribute if its parts were all duplicated, but then I realized that the output would be chaotic if the duplicates were coming from different attributes, so I preferred to go to the simplest way and simply emit a warning for each entry. Not the best, but makes the implementation much easier.

Another limitation is that `cfg_attr` would be a bit more tricky to implement because we need to check if two `cfg` sets are exactly the same. I added a FIXME and will likely come back to it later.

And finally, I updated the `cargo dev update_lints` command because the generated `tests/ui/rename.rs` file was emitting the `duplicated_attributes` lint, so I allowed this lint inside it to prevent it from working.

changelog: Add new `duplicated_attributes` lint

Author: bors

CHANGELOG.md

@@ -5157,6 +5157,7 @@ Released 2018-09-13
 [`drop_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_ref
 [`duplicate_mod`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_mod
 [`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument
+[`duplicated_attributes`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes
 [`duration_subsec`]: https://rust-lang.github.io/rust-clippy/master/index.html#duration_subsec
 [`eager_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#eager_transmute
 [`else_if_without_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#else_if_without_else

clippy_dev/src/update_lints.rs

@@ -689,6 +689,8 @@ fn gen_deprecated_lints_test(lints: &[DeprecatedLint]) -> String {
 fn gen_renamed_lints_test(lints: &[RenamedLint]) -> String {
     let mut seen_lints = HashSet::new();
     let mut res: String = GENERATED_FILE_COMMENT.into();
+
+    res.push_str("#![allow(clippy::duplicated_attributes)]\n");
     for lint in lints {
         if seen_lints.insert(&lint.new_name) {
             writeln!(res, "#![allow({})]", lint.new_name).unwrap();

clippy_lints/src/attrs/duplicated_attributes.rs

@@ -0,0 +1,64 @@
+use super::DUPLICATED_ATTRIBUTES;
+use clippy_utils::diagnostics::span_lint_and_then;
+use rustc_ast::{Attribute, MetaItem};
+use rustc_data_structures::fx::FxHashMap;
+use rustc_lint::EarlyContext;
+use rustc_span::{sym, Span};
+use std::collections::hash_map::Entry;
+
+fn emit_if_duplicated(
+    cx: &EarlyContext<'_>,
+    attr: &MetaItem,
+    attr_paths: &mut FxHashMap<String, Span>,
+    complete_path: String,
+) {
+    match attr_paths.entry(complete_path) {
+        Entry::Vacant(v) => {
+            v.insert(attr.span);
+        },
+        Entry::Occupied(o) => {
+            span_lint_and_then(cx, DUPLICATED_ATTRIBUTES, attr.span, "duplicated attribute", |diag| {
+                diag.span_note(*o.get(), "first defined here");
+                diag.span_help(attr.span, "remove this attribute");
+            });
+        },
+    }
+}
+
+fn check_duplicated_attr(
+    cx: &EarlyContext<'_>,
+    attr: &MetaItem,
+    attr_paths: &mut FxHashMap<String, Span>,
+    parent: &mut Vec<String>,
+) {
+    let Some(ident) = attr.ident() else { return };
+    let name = ident.name;
+    if name == sym::doc || name == sym::cfg_attr {
+        // FIXME: Would be nice to handle `cfg_attr` as well. Only problem is to check that cfg
+        // conditions are the same.
+        return;
+    }
+    if let Some(value) = attr.value_str() {
+        emit_if_duplicated(cx, attr, attr_paths, format!("{}:{name}={value}", parent.join(":")));
+    } else if let Some(sub_attrs) = attr.meta_item_list() {
+        parent.push(name.as_str().to_string());
+        for sub_attr in sub_attrs {
+            if let Some(meta) = sub_attr.meta_item() {
+                check_duplicated_attr(cx, meta, attr_paths, parent);
+            }
+        }
+        parent.pop();
+    } else {
+        emit_if_duplicated(cx, attr, attr_paths, format!("{}:{name}", parent.join(":")));
+    }
+}
+
+pub fn check(cx: &EarlyContext<'_>, attrs: &[Attribute]) {
+    let mut attr_paths = FxHashMap::default();
+
+    for attr in attrs {
+        if let Some(meta) = attr.meta() {
+            check_duplicated_attr(cx, &meta, &mut attr_paths, &mut Vec::new());
+        }
+    }
+}

clippy_lints/src/attrs/mod.rs

@@ -4,6 +4,7 @@ mod allow_attributes_without_reason;
 mod blanket_clippy_restriction_lints;
 mod deprecated_cfg_attr;
 mod deprecated_semver;
+mod duplicated_attributes;
 mod empty_line_after;
 mod inline_always;
 mod maybe_misused_cfg;
@@ -16,7 +17,7 @@ mod useless_attribute;
 mod utils;
 
 use clippy_config::msrvs::Msrv;
-use rustc_ast::{Attribute, MetaItemKind, NestedMetaItem};
+use rustc_ast::{Attribute, Crate, MetaItemKind, NestedMetaItem};
 use rustc_hir::{ImplItem, Item, ItemKind, TraitItem};
 use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, impl_lint_pass};
@@ -489,6 +490,32 @@ declare_clippy_lint! {
     "item has both inner and outer attributes"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for attributes that appear two or more times.
+    ///
+    /// ### Why is this bad?
+    /// Repeating an attribute on the same item (or globally on the same crate)
+    /// is unnecessary and doesn't have an effect.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// #[allow(dead_code)]
+    /// #[allow(dead_code)]
+    /// fn foo() {}
+    /// ```
+    ///
+    /// Use instead:
+    /// ```no_run
+    /// #[allow(dead_code)]
+    /// fn foo() {}
+    /// ```
+    #[clippy::version = "1.78.0"]
+    pub DUPLICATED_ATTRIBUTES,
+    suspicious,
+    "duplicated attribute"
+}
+
 declare_lint_pass!(Attributes => [
     ALLOW_ATTRIBUTES_WITHOUT_REASON,
     INLINE_ALWAYS,
@@ -568,12 +595,18 @@ impl_lint_pass!(EarlyAttributes => [
     DEPRECATED_CLIPPY_CFG_ATTR,
     UNNECESSARY_CLIPPY_CFG,
     MIXED_ATTRIBUTES_STYLE,
+    DUPLICATED_ATTRIBUTES,
 ]);
 
 impl EarlyLintPass for EarlyAttributes {
+    fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &Crate) {
+        duplicated_attributes::check(cx, &krate.attrs);
+    }
+
     fn check_item(&mut self, cx: &EarlyContext<'_>, item: &rustc_ast::Item) {
         empty_line_after::check(cx, item);
         mixed_attributes_style::check(cx, item);
+        duplicated_attributes::check(cx, &item.attrs);
     }
 
     fn check_attribute(&mut self, cx: &EarlyContext<'_>, attr: &Attribute) {

clippy_lints/src/declared_lints.rs

@@ -54,6 +54,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::attrs::DEPRECATED_CFG_ATTR_INFO,
     crate::attrs::DEPRECATED_CLIPPY_CFG_ATTR_INFO,
     crate::attrs::DEPRECATED_SEMVER_INFO,
+    crate::attrs::DUPLICATED_ATTRIBUTES_INFO,
     crate::attrs::EMPTY_LINE_AFTER_DOC_COMMENTS_INFO,
     crate::attrs::EMPTY_LINE_AFTER_OUTER_ATTR_INFO,
     crate::attrs::INLINE_ALWAYS_INFO,

tests/ui/allow_attributes_without_reason.rs

@@ -1,7 +1,7 @@
 //@aux-build:proc_macros.rs
 #![feature(lint_reasons)]
 #![deny(clippy::allow_attributes_without_reason)]
-#![allow(unfulfilled_lint_expectations)]
+#![allow(unfulfilled_lint_expectations, clippy::duplicated_attributes)]
 
 extern crate proc_macros;
 use proc_macros::{external, with_span};

tests/ui/allow_attributes_without_reason.stderr

@@ -1,8 +1,8 @@
 error: `allow` attribute without specifying a reason
   --> tests/ui/allow_attributes_without_reason.rs:4:1
    |
-LL | #![allow(unfulfilled_lint_expectations)]
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | #![allow(unfulfilled_lint_expectations, clippy::duplicated_attributes)]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: try adding a reason at the end with `, reason = ".."`
 note: the lint level is defined here

tests/ui/duplicated_attributes.rs

@@ -0,0 +1,17 @@
+#![warn(clippy::duplicated_attributes)]
+#![cfg(any(unix, windows))]
+#![allow(dead_code)]
+#![allow(dead_code)] //~ ERROR: duplicated attribute
+#![cfg(any(unix, windows))]
+//~^ ERROR: duplicated attribute
+//~| ERROR: duplicated attribute
+
+#[cfg(any(unix, windows, target_os = "linux"))]
+#[allow(dead_code)]
+#[allow(dead_code)] //~ ERROR: duplicated attribute
+#[cfg(any(unix, windows, target_os = "linux"))]
+//~^ ERROR: duplicated attribute
+//~| ERROR: duplicated attribute
+fn foo() {}
+
+fn main() {}

tests/ui/duplicated_attributes.stderr

@@ -0,0 +1,123 @@
+error: duplicated attribute
+  --> tests/ui/duplicated_attributes.rs:4:10
+   |
+LL | #![allow(dead_code)]
+   |          ^^^^^^^^^
+   |
+note: first defined here
+  --> tests/ui/duplicated_attributes.rs:3:10
+   |
+LL | #![allow(dead_code)]
+   |          ^^^^^^^^^
+help: remove this attribute
+  --> tests/ui/duplicated_attributes.rs:4:10
+   |
+LL | #![allow(dead_code)]
+   |          ^^^^^^^^^
+   = note: `-D clippy::duplicated-attributes` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::duplicated_attributes)]`
+
+error: duplicated attribute
+  --> tests/ui/duplicated_attributes.rs:5:12
+   |
+LL | #![cfg(any(unix, windows))]
+   |            ^^^^
+   |
+note: first defined here
+  --> tests/ui/duplicated_attributes.rs:2:12
+   |
+LL | #![cfg(any(unix, windows))]
+   |            ^^^^
+help: remove this attribute
+  --> tests/ui/duplicated_attributes.rs:5:12
+   |
+LL | #![cfg(any(unix, windows))]
+   |            ^^^^
+
+error: duplicated attribute
+  --> tests/ui/duplicated_attributes.rs:5:18
+   |
+LL | #![cfg(any(unix, windows))]
+   |                  ^^^^^^^
+   |
+note: first defined here
+  --> tests/ui/duplicated_attributes.rs:2:18
+   |
+LL | #![cfg(any(unix, windows))]
+   |                  ^^^^^^^
+help: remove this attribute
+  --> tests/ui/duplicated_attributes.rs:5:18
+   |
+LL | #![cfg(any(unix, windows))]
+   |                  ^^^^^^^
+
+error: duplicated attribute
+  --> tests/ui/duplicated_attributes.rs:11:9
+   |
+LL | #[allow(dead_code)]
+   |         ^^^^^^^^^
+   |
+note: first defined here
+  --> tests/ui/duplicated_attributes.rs:10:9
+   |
+LL | #[allow(dead_code)]
+   |         ^^^^^^^^^
+help: remove this attribute
+  --> tests/ui/duplicated_attributes.rs:11:9
+   |
+LL | #[allow(dead_code)]
+   |         ^^^^^^^^^
+
+error: duplicated attribute
+  --> tests/ui/duplicated_attributes.rs:12:11
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |           ^^^^
+   |
+note: first defined here
+  --> tests/ui/duplicated_attributes.rs:9:11
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |           ^^^^
+help: remove this attribute
+  --> tests/ui/duplicated_attributes.rs:12:11
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |           ^^^^
+
+error: duplicated attribute
+  --> tests/ui/duplicated_attributes.rs:12:17
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |                 ^^^^^^^
+   |
+note: first defined here
+  --> tests/ui/duplicated_attributes.rs:9:17
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |                 ^^^^^^^
+help: remove this attribute
+  --> tests/ui/duplicated_attributes.rs:12:17
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |                 ^^^^^^^
+
+error: duplicated attribute
+  --> tests/ui/duplicated_attributes.rs:12:26
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |                          ^^^^^^^^^^^^^^^^^^^
+   |
+note: first defined here
+  --> tests/ui/duplicated_attributes.rs:9:26
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |                          ^^^^^^^^^^^^^^^^^^^
+help: remove this attribute
+  --> tests/ui/duplicated_attributes.rs:12:26
+   |
+LL | #[cfg(any(unix, windows, target_os = "linux"))]
+   |                          ^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 7 previous errors
+

tests/ui/empty_line_after_doc_comments.rs

@@ -1,6 +1,6 @@
 //@aux-build:proc_macro_attr.rs
 #![warn(clippy::empty_line_after_doc_comments)]
-#![allow(clippy::assertions_on_constants)]
+#![allow(clippy::assertions_on_constants, clippy::duplicated_attributes)]
 #![feature(custom_inner_attributes)]
 #![rustfmt::skip]
 

tests/ui/empty_line_after_outer_attribute.rs

@@ -1,6 +1,6 @@
 //@aux-build:proc_macro_attr.rs
 #![warn(clippy::empty_line_after_outer_attr)]
-#![allow(clippy::assertions_on_constants)]
+#![allow(clippy::assertions_on_constants, clippy::duplicated_attributes)]
 #![feature(custom_inner_attributes)]
 #![rustfmt::skip]
 

tests/ui/mixed_attributes_style.rs

@@ -1,4 +1,5 @@
 #![warn(clippy::mixed_attributes_style)]
+#![allow(clippy::duplicated_attributes)]
 
 #[allow(unused)] //~ ERROR: item has both inner and outer attributes
 fn foo1() {

tests/ui/mixed_attributes_style.stderr

@@ -1,5 +1,5 @@
 error: item has both inner and outer attributes
-  --> tests/ui/mixed_attributes_style.rs:3:1
+  --> tests/ui/mixed_attributes_style.rs:4:1
    |
 LL | / #[allow(unused)]
 LL | | fn foo1() {
@@ -10,7 +10,7 @@ LL | |     #![allow(unused)]
    = help: to override `-D warnings` add `#[allow(clippy::mixed_attributes_style)]`
 
 error: item has both inner and outer attributes
-  --> tests/ui/mixed_attributes_style.rs:17:1
+  --> tests/ui/mixed_attributes_style.rs:18:1
    |
 LL | / /// linux
 LL | |
@@ -19,7 +19,7 @@ LL | |     //! windows
    | |_______________^
 
 error: item has both inner and outer attributes
-  --> tests/ui/mixed_attributes_style.rs:32:1
+  --> tests/ui/mixed_attributes_style.rs:33:1
    |
 LL | / #[allow(unused)]
 LL | | mod bar {

tests/ui/rename.fixed

@@ -2,6 +2,7 @@
 // Use that command to update this file and do not edit by hand.
 // Manual edits will be overwritten.
 
+#![allow(clippy::duplicated_attributes)]
 #![allow(clippy::almost_complete_range)]
 #![allow(clippy::disallowed_names)]
 #![allow(clippy::blocks_in_conditions)]

tests/ui/rename.rs

@@ -2,6 +2,7 @@
 // Use that command to update this file and do not edit by hand.
 // Manual edits will be overwritten.
 
+#![allow(clippy::duplicated_attributes)]
 #![allow(clippy::almost_complete_range)]
 #![allow(clippy::disallowed_names)]
 #![allow(clippy::blocks_in_conditions)]