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
10.77k stars 1.43k forks source link

duplicated_attributes false positive: cfg expressions #12537

Closed dtolnay closed 5 days ago

dtolnay commented 1 month ago

Lint Name

duplicated_attributes

Reproducer

#[cfg(any(
    all(
        target_os = "linux",
        any(
            target_arch = "aarch64",
            target_arch = "arm",
            target_arch = "hexagon",
            target_arch = "powerpc",
            target_arch = "powerpc64",
            target_arch = "s390x",
            target_arch = "riscv64",
            target_arch = "riscv32"
        )
    ),
    all(
        target_os = "android",
        any(target_arch = "aarch64", target_arch = "arm")
    ),
))]
pub type c_char = u8;
$ cargo clippy
warning: duplicated attribute
  --> src/main.rs:17:13
   |
17 |         any(target_arch = "aarch64", target_arch = "arm")
   |             ^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first defined here
  --> src/main.rs:5:13
   |
5  |             target_arch = "aarch64",
   |             ^^^^^^^^^^^^^^^^^^^^^^^
help: remove this attribute
  --> src/main.rs:17:13
   |
17 |         any(target_arch = "aarch64", target_arch = "arm")
   |             ^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes
   = note: `#[warn(clippy::duplicated_attributes)]` on by default

warning: duplicated attribute
  --> src/main.rs:17:38
   |
17 |         any(target_arch = "aarch64", target_arch = "arm")
   |                                      ^^^^^^^^^^^^^^^^^^^
   |
note: first defined here
  --> src/main.rs:6:13
   |
6  |             target_arch = "arm",
   |             ^^^^^^^^^^^^^^^^^^^
help: remove this attribute
  --> src/main.rs:17:38
   |
17 |         any(target_arch = "aarch64", target_arch = "arm")
   |                                      ^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes

Clippy just says "remove this attribute" but doing so would violate the intended meaning of the program.

The case shown as an example of the intended purpose of this lint in https://rust-lang.github.io/rust-clippy/master/index.html#/duplicated_attributes (#[allow(dead_code)] #[allow(dead_code)]) seems like a quite different thing than this one.

Version

rustc 1.79.0-nightly (85e449a32 2024-03-22)
binary: rustc
commit-hash: 85e449a3237e82c9ade8936a82bd4fc64cfe1057
commit-date: 2024-03-22
host: x86_64-unknown-linux-gnu
release: 1.79.0-nightly
LLVM version: 18.1.2

Additional Labels

No response

dtolnay commented 1 month ago

@GuillaumeGomez @blyxyas (the lint is new from https://github.com/rust-lang/rust-clippy/pull/12378)

GuillaumeGomez commented 1 month ago

You're right, it should not check cfg like this.

SteveLauC commented 1 month ago

Here is another false positive case found in Nix:

https://github.com/nix-rust/nix/blob/01cd697b82087730b00bafa60e25fd53b025cfdd/src/sys/statfs.rs#L77-L79

#[cfg(any(
    target_os = "freebsd",
    target_os = "android",
    all(target_os = "linux", target_arch = "s390x"),
    all(target_os = "linux", target_env = "musl"),
    all(target_os = "linux", target_env = "ohos"),
    all(
        target_os = "linux",
        not(any(target_arch = "s390x", target_env = "musl"))
    ),
))]
#[derive(Eq, Copy, Clone, PartialEq, Debug)]
pub struct FsType(pub fs_type_t);

Clippy suggests me to remove these "duplicate" linux attributes even though that they are actually not the same thing.

Full log here: https://github.com/nix-rust/nix/blob/01cd697b82087730b00bafa60e25fd53b025cfdd/src/sys/statfs.rs#L77-L79

tamird commented 1 month ago

There's also a false positive when using https://crates.io/crates/test-case.

#[test_case("pointer", false, None, 42)]
#[test_case("pointer", true, None, 21)]
#[test_case("struct_flavors", false, None, 1)]
#[test_case("struct_flavors", true, None, 2)]
fn relocation_tests(
    program: &str,
    with_relocations: bool,
    required_kernel_version: Option<(KernelVersion, &str)>,
    expected: u64,
) {
  ...
}
help: remove this attribute
  --> test/integration-test/src/tests/btf_relocations.rs:23:30
   |
23 | #[test_case("pointer", true, None, 21)]
   |                              ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes

warning: duplicated attribute
  --> test/integration-test/src/tests/btf_relocations.rs:24:38
   |
24 | #[test_case("struct_flavors", false, None, 1)]
   |                                      ^^^^
   |
note: first defined here
  --> test/integration-test/src/tests/btf_relocations.rs:12:40
   |
12 | #[test_case("enum_unsigned_32", false, None, 0xAAAAAAAA)]
   |                                        ^^^^
help: remove this attribute
  --> test/integration-test/src/tests/btf_relocations.rs:24:38
   |
24 | #[test_case("struct_flavors", false, None, 1)]
   |                                      ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes

warning: duplicated attribute
  --> test/integration-test/src/tests/btf_relocations.rs:25:37
   |
25 | #[test_case("struct_flavors", true, None, 2)]
   |                                     ^^^^
   |
note: first defined here
  --> test/integration-test/src/tests/btf_relocations.rs:12:40
   |
12 | #[test_case("enum_unsigned_32", false, None, 0xAAAAAAAA)]
   |                                        ^^^^
help: remove this attribute
  --> test/integration-test/src/tests/btf_relocations.rs:25:37
   |
25 | #[test_case("struct_flavors", true, None, 2)]
   |                                     ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes

warning: `integration-test` (lib test) generated 9 warnings
GuillaumeGomez commented 1 month ago

Normally #12555 should fix both your cases since it's a late pass expansion, but please double-check @tamird once it's merged and the new clippy version published.

tamird commented 1 month ago

@GuillaumeGomez it'd be helpful if you pinged this issue when a new nightly is out with this fix.

GuillaumeGomez commented 1 month ago

I can ask @flip1995 for that when he does it. :)

flip1995 commented 1 month ago

2024-04-05

tamird commented 3 weeks ago

Looks like #12555 did not fix this issue. Tested with rustc 1.79.0-nightly (aa1c45908 2024-04-06) which includes #12555 as https://github.com/rust-lang/rust/pull/123465/commits/e3f3a4b7dc0eb64b92497045db475fa8f0bbe5d0.

GuillaumeGomez commented 2 weeks ago

Just tested locally and can't reproduce it. Just in case, I opened https://github.com/rust-lang/rust-clippy/pull/12646 to ensure that attributes generated by proc-macros don't emit a warning.

tamird commented 2 weeks ago

I'm able to reproduce with yesterday's (2024-04-07) nightly as well. If you want, you can revert https://github.com/aya-rs/aya/commit/b552c8330077c39e21c4f3d3f667078c00124560 locally and then run cargo +nightly clippy --all-targets --workspace --target x86_64-unknown-linux-gnu -- --deny warnings from the root of the repository.

GuillaumeGomez commented 2 weeks ago

I'm starting to wonder if proc-macros don't need special treatment here...

blyxyas commented 2 weeks ago

I'll do some testing =^w^=

blyxyas commented 2 weeks ago

I am also having these issues. It seems like the current master still lints on it. You can try it by cloning the aya repo, pulling master and doing the following command:

cargo dev lint ../aya -- --all-targets --workspace --target x86_64-unknown-linux-gnu
GuillaumeGomez commented 2 weeks ago

Ok so that confirms that some tokens are not marked as "from expansion" in proc-macros. Gonna investigate a bit more.

la10736 commented 2 weeks ago

Also on rstest we have the same issue : https://github.com/la10736/rstest/issues/238

warning: duplicated attribute
  --> src/main.rs:13:14
   |
13 | #[case(None, None)] // warns
   |              ^^^^
   |
note: first defined here
  --> src/main.rs:13:8
   |
13 | #[case(None, None)] // warns
   |        ^^^^
help: remove this attribute
  --> src/main.rs:13:14
   |
13 | #[case(None, None)] // warns
   |              ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes
   = note: `#[warn(clippy::duplicated_attributes)]` on by default

warning: duplicated attribute
  --> src/main.rs:14:17
   |
14 | #[case(Some(1), None)] // warns
   |                 ^^^^
   |
note: first defined here
  --> src/main.rs:13:8
   |
13 | #[case(None, None)] // warns
   |        ^^^^
help: remove this attribute
  --> src/main.rs:14:17
   |
14 | #[case(Some(1), None)] // warns
   |                 ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes

warning: duplicated attribute
  --> src/main.rs:15:8
   |
15 | #[case(None, Some(1.0))] // warns
   |        ^^^^
   |
note: first defined here
  --> src/main.rs:13:8
   |
13 | #[case(None, None)] // warns
   |        ^^^^
help: remove this attribute
  --> src/main.rs:15:8
   |
15 | #[case(None, Some(1.0))] // warns
   |        ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes

warning: `rstest_238` (bin "rstest_238") generated 3 warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.38s
mdamico@miklap:~/dev_random/rstest_238$ cargo +nightly clippy --version
clippy 0.1.79 (ab5bda1 2024-04-08)
GuillaumeGomez commented 2 weeks ago

Yes got it. Still trying to write a minimal code to reproduce the bug.

blyxyas commented 2 weeks ago

I tested #12646, and this should be fixed when that hits nightly :)

la10736 commented 2 weeks ago

@GuillaumeGomez Still have the issue also on the last nightly

mdamico@miklap:~/dev_random/rstest_238$ cargo +nightly clippy --version
clippy 0.1.79 (0bf471f 2024-04-13)

Clippy behaviour is still the same of https://github.com/rust-lang/rust-clippy/issues/12537#issuecomment-2044757156

la10736 commented 2 weeks ago

@tamird I guess that also for test_case the issue is still unresolved with clippy 0.1.79 (0bf471f 2024-04-13).

GuillaumeGomez commented 2 weeks ago

Then please provide a minimal code I can use to reproduce the bug and fix it (and also use as regression test).

y21 commented 2 weeks ago

Worth noting that the last change here (#12646) is, again, not on nightly yet, since the clippy subtree is only updated every 2 weeks. So it's entirely possible that it is fixed, just not on nightly yet and so we can't do anything but wait. It'd be useful to also run master clippy on the crate (like here) and see what happens there.

blyxyas commented 1 week ago

I second y21's comment. If you're open to some testing, clone the repo (or pull master) and test it with a similar command. If that doesn't work, please report back! :heart:

la10736 commented 5 days ago

@GuillaumeGomez I tested with clippy 0.1.79 (7f2fc33 2024-04-22) and now it's works!!!

Thanks!!!

GuillaumeGomez commented 5 days ago

Perfect. :)

Closing then!