rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.43k stars 1.54k forks source link

duplicated_attributes erroneously reports when two different attributes have the same reason text #13500

Open ShakenCodes opened 1 month ago

ShakenCodes commented 1 month ago

Summary

The duplicated_attributes lint is being tripped in cases where there are two different allow attributes that share the same text in their reason field. I suspect this is a requirements bug where two dissimilar conditions have been erroneously conflated into a single lint.

  1. The lint duplicated_attributes reports if two duplicated attributes are applied to the same bit of code. This is the desired behavior for the lint, and is a lint my team wants enabled:

    #[allow(dead_code)]
    #[allow(dead_code)]
    fn foo() {}
  2. The lint duplicated_attributes also reports if two different lint allow attributes have the same reason. Since the attributes are different, I believe this is erroneous behavior. (It may be desirable behavior, if it were performed as a separate lint.) This is not desired by our team and we would like to NOT lint for this in our application

    #[allow(dead_code, reason = "Inherited code. TODO fix at future date.")]
    #[allow(clippy::empty_loop, reason = "Inherited code. TODO fix at future date.")]
    fn bar() {
    loop {
        // an empty loop. eek!
    }
    }

I suggest these two conditions be split into two separate lints:

  1. duplicated_attributes
  2. duplicated_allow_reason

Reproducer

I tried this code:

#![deny(dead_code)]
#![deny(clippy::empty_loop)]
#![deny(clippy::duplicated_attributes)]
#![deny(clippy::allow_attributes_without_reason)]

#[allow(dead_code, reason = "dead_code 1")]
#[allow(dead_code, reason = "dead_code 2")]
fn foo() {}

#[allow(dead_code, reason = "TODO fix at future date.")]
#[allow(clippy::empty_loop, reason = "TODO fix at future date.")]
fn bar() {
    loop {
        // an empty loop. eek!
    }
}

I expected to see this happen: No error reported on "#[allow(clippy::empty_loop, reason = "TODO fix at future date.")]", as this is not a duplicated attribute. Ideally, the error reported on this line would be a different clip lint (suggest name: duplicated_allow_reason)

Instead, this happened:

error: duplicated attribute
  --> src/lib.rs:11:29
   |
11 | #[allow(clippy::empty_loop, reason = "TODO fix at future date.")]
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first defined here
  --> src/lib.rs:10:20
   |
10 | #[allow(dead_code, reason = "TODO fix at future date.")]
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: remove this attribute
  --> src/lib.rs:11:29
   |
11 | #[allow(clippy::empty_loop, reason = "TODO fix at future date.")]
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes

Version

rustc 1.81.0 (eeb90cda1 2024-09-04) binary: rustc commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c commit-date: 2024-09-04 host: aarch64-apple-darwin release: 1.81.0 LLVM version: 18.1.7

Additional Labels

@rustbot label +l-false-positive

samueltardieu commented 1 month ago

Thanks for the report. This appears to have been fixed in commit 7097830a already. The fix will be part of the 1.83 release.

samueltardieu commented 1 month ago

Duplicate of #13355