Add new `duplicated_attributes` lint #12378

GuillaumeGomez · 2024-02-28T16:43:25Z

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

rustbot · 2024-02-28T16:43:31Z

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

GuillaumeGomez · 2024-02-28T16:45:25Z

Manish said they're busy so let's pick someone else.

r? @blyxyas

bors · 2024-02-29T12:02:39Z

☔ The latest upstream changes (presumably #12354) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2024-02-29T13:43:03Z

Rebased.

blyxyas

I've done some tinkering with that FIXME and cfg_attr, I think it wouldn't be too hard to support it.

Really good first iteration! :3

clippy_dev/src/update_lints.rs

blyxyas · 2024-02-29T18:29:13Z

clippy_lints/src/attrs.rs

@@ -512,6 +515,31 @@ declare_clippy_lint! {
    "item has both inner and outer attributes"
 }

+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks if an item has duplicated attributes.


As this also covers check_crate.

Suggested change

/// Checks if an item has duplicated attributes.

/// Checks for attributes that appear two or more times.

blyxyas · 2024-02-29T18:30:16Z

clippy_lints/src/attrs.rs

+    /// Checks if an item has duplicated attributes.
+    ///
+    /// ### Why is this bad?
+    /// More code for no reason.


A little more developed? Maybe something like this:

Suggested change

/// More code for no reason.

/// Repeating an attribute on the same item (or globally on the same crate) is unnecessary and doesn't have an effect.

Much better indeed. ^^'

blyxyas · 2024-02-29T19:08:09Z

clippy_lints/src/attrs.rs

+    /// fn foo() {}
+    /// ```
+    #[clippy::version = "1.78.0"]
+    pub DUPLICATED_ATTR,


I'm not sure about the lint. It should be definitely plural to fit in with the naming convention ("Allow duplicated attributes"). But I'm not sure if it should be duplicated_attrs or duplicated_attributes.

What do you think?

I'd prefer the full version.

GuillaumeGomez · 2024-02-29T23:51:58Z

Applied suggestions.

bors · 2024-03-02T14:49:23Z

☔ The latest upstream changes (presumably #12394) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2024-03-02T19:17:13Z

Renamed the lint as suggested, fixed merge conflict and double checked: still doesn't work without the rename.rs generation update.

GuillaumeGomez · 2024-03-02T19:31:22Z

And even fixed CI. :D

blyxyas

Sorry for the delay, I was prioritizing newcomer's reviews, and then got lost in playing a little bit of Helldivers 2, I liked it.

This is good as it is, but please double-check that the update_lints.rs change is necessary, because I removed it + the rename.rs attribute and there was no change.

Maybe an artifact of previous versions? If there's black magic and on some platforms that forced attribute is necessary, we'll just keep it there, no big deal :)

GuillaumeGomez · 2024-03-07T13:32:42Z

Just checked, still not working. Giving the steps I'm doing to reproduce:

$ git rebase -i HEAD~4 # I remove the commit making the change
Successfully rebased and updated detached HEAD.
$ cargo dev update_lints
   Compiling clippy_dev v0.0.1 (/home/imperio/rust/clippy/clippy_dev)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.68s
     Running `target/debug/clippy_dev update_lints`
$ TESTNAME=rename.rs cargo uitest
   Compiling clippy_config v0.1.78 (/home/imperio/rust/clippy/clippy_config)
   Compiling clippy_utils v0.1.78 (/home/imperio/rust/clippy/clippy_utils)
   Compiling clippy_lints v0.1.78 (/home/imperio/rust/clippy/clippy_lints)
   Compiling clippy v0.1.78 (/home/imperio/rust/clippy)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 35.57s
     Running tests/compile-test.rs (target/debug/deps/compile_test-b0bc7a5dfdc2e878)
Building dependencies ... ok
tests/ui/rename.rs ... FAILED                                                                           

FAILED TEST: tests/ui/rename.rs
command: CLIPPY_CONF_DIR="tests" "/home/imperio/rust/clippy/target/debug/clippy-driver" "--error-format=json" "--emit=metadata" "-Aunused" "-Ainternal_features" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Dwarnings" "-Ldependency=/home/imperio/rust/clippy/target/debug/deps" "--extern=clippy_config=/home/imperio/rust/clippy/target/debug/deps/libclippy_config-5d14e48a50025dbd.rlib" "--extern=clippy_lints=/home/imperio/rust/clippy/target/debug/deps/libclippy_lints-992d72a6c56735a3.rlib" "--extern=clippy_utils=/home/imperio/rust/clippy/target/debug/deps/libclippy_utils-da6e3f676bd0803e.rlib" "--extern=futures=/home/imperio/rust/clippy/target/debug/deps/libfutures-b2e68350a4947d59.rlib" "--extern=if_chain=/home/imperio/rust/clippy/target/debug/deps/libif_chain-0d767436d7e18eee.rlib" "--extern=itertools=/home/imperio/rust/clippy/target/debug/deps/libitertools-dcd2b845627dc871.rlib" "--extern=parking_lot=/home/imperio/rust/clippy/target/debug/deps/libparking_lot-7b8f849c7fda2d42.rlib" "--extern=quote=/home/imperio/rust/clippy/target/debug/deps/libquote-28a9e8d5e247e24e.rlib" "--extern=regex=/home/imperio/rust/clippy/target/debug/deps/libregex-66a489862c42a796.rlib" "--extern=serde=/home/imperio/rust/clippy/target/debug/deps/libserde-30375b3bb2f254b9.rlib" "--extern=serde_derive=/home/imperio/rust/clippy/target/debug/deps/libserde_derive-68eaa659e149f4ae.so" "--extern=syn=/home/imperio/rust/clippy/target/debug/deps/libsyn-251cd79fb57cdb56.rlib" "--extern=tokio=/home/imperio/rust/clippy/target/debug/deps/libtokio-78db72a98dbeacfc.rlib" "--out-dir" "target/ui_test/tests/ui" "tests/ui/rename.fixed" "--edition" "2021" "--crate-name" "rename"

error: rustfix failed with exit status: 1

full stderr:
error: duplicated attribute
  --> tests/ui/rename.fixed:96:9
   |
LL | #![warn(for_loops_over_fallibles)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first defined here
  --> tests/ui/rename.fixed:95:9
   |
LL | #![warn(for_loops_over_fallibles)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
help: remove this attribute
  --> tests/ui/rename.fixed:96:9
   |
LL | #![warn(for_loops_over_fallibles)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
   = note: `-D clippy::duplicated-attributes` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::duplicated_attributes)]`

error: duplicated attribute
  --> tests/ui/rename.fixed:97:9
   |
LL | #![warn(for_loops_over_fallibles)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first defined here
  --> tests/ui/rename.fixed:95:9
   |
LL | #![warn(for_loops_over_fallibles)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
help: remove this attribute
  --> tests/ui/rename.fixed:97:9
   |
LL | #![warn(for_loops_over_fallibles)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors


full stdout:


FAILURES:
    tests/ui/rename.rs

test result: FAIL. 1 failed; 966 filtered out;

thread 'main' panicked at tests/compile-test.rs:176:6:
called `Result::unwrap()` on an `Err` value: tests failed

Location:
    /home/imperio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ui_test-0.22.2/src/lib.rs:395:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: test failed, to rerun pass `--test compile-test`

As you can see, it fails. So no, I confirm the change is needed.

GuillaumeGomez · 2024-03-07T13:34:15Z

and then got lost in playing a little bit of Helldivers 2, I liked it.

Having same issue with baldur's gate 3. :D

Maybe an artifact of previous versions? If there's black magic and on some platforms that forced attribute is necessary, we'll just keep it there, no big deal :)

I rebased on the latest clippy version (locally) and ran the commands from my previous message, still not working as you can see.

… `duplicated_attributes`

blyxyas · 2024-03-11T18:35:13Z

Meow meow meow
@bors r+

bors · 2024-03-11T18:35:15Z

📌 Commit ae52a9d has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-03-11T18:36:23Z

⌛ Testing commit ae52a9d with merge f685a4b...

bors · 2024-03-11T18:44:01Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing f685a4b to master...

rustbot assigned Manishearth Feb 28, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 28, 2024

rustbot assigned blyxyas and unassigned Manishearth Feb 28, 2024

GuillaumeGomez force-pushed the duplicated_attr branch 2 times, most recently from 9b52084 to 3f4cbaf Compare February 29, 2024 13:37

blyxyas requested changes Feb 29, 2024

View reviewed changes

GuillaumeGomez force-pushed the duplicated_attr branch from 3f4cbaf to 852311f Compare February 29, 2024 23:51

GuillaumeGomez force-pushed the duplicated_attr branch from 852311f to 0c167d5 Compare March 2, 2024 19:16

GuillaumeGomez changed the title ~~Add new duplicated_attr lint~~ Add new duplicated_attributes lint Mar 2, 2024

blyxyas approved these changes Mar 7, 2024

View reviewed changes

blyxyas force-pushed the duplicated_attr branch from 0c167d5 to 40507fa Compare March 9, 2024 00:35

GuillaumeGomez added 4 commits March 9, 2024 12:43

Add new duplicated_attributes lint

f34804d

Add ui test for new duplicated_attributes lint

749e225

Update cargo dev update_lints command to fix new warning emitted by…

d57d001

… `duplicated_attributes`

Update ui tests

ae52a9d

GuillaumeGomez force-pushed the duplicated_attr branch from 40507fa to ae52a9d Compare March 9, 2024 11:43

bors merged commit f685a4b into rust-lang:master Mar 11, 2024
8 checks passed

GuillaumeGomez deleted the duplicated_attr branch March 11, 2024 18:48

This was referenced Mar 23, 2024

duplicated_attributes false positive: cfg expressions #12537

Closed

allow(clippy::duplicated_attributes) does not take effect across out-of-line modules #12538

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `duplicated_attributes` lint #12378

Add new `duplicated_attributes` lint #12378

GuillaumeGomez commented Feb 28, 2024 •

edited

Loading

rustbot commented Feb 28, 2024

GuillaumeGomez commented Feb 28, 2024

bors commented Feb 29, 2024

GuillaumeGomez commented Feb 29, 2024

blyxyas left a comment

blyxyas Feb 29, 2024

GuillaumeGomez Feb 29, 2024

blyxyas Feb 29, 2024

GuillaumeGomez Feb 29, 2024

blyxyas Feb 29, 2024

GuillaumeGomez Feb 29, 2024

GuillaumeGomez commented Feb 29, 2024

bors commented Mar 2, 2024

GuillaumeGomez commented Mar 2, 2024

GuillaumeGomez commented Mar 2, 2024

blyxyas left a comment

GuillaumeGomez commented Mar 7, 2024

GuillaumeGomez commented Mar 7, 2024

blyxyas commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

	/// Checks if an item has duplicated attributes.
	/// Checks for attributes that appear two or more times.

	/// More code for no reason.
	/// Repeating an attribute on the same item (or globally on the same crate) is unnecessary and doesn't have an effect.

Add new duplicated_attributes lint #12378

Add new duplicated_attributes lint #12378

Conversation

GuillaumeGomez commented Feb 28, 2024 • edited Loading

rustbot commented Feb 28, 2024

GuillaumeGomez commented Feb 28, 2024

bors commented Feb 29, 2024

GuillaumeGomez commented Feb 29, 2024

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas Feb 29, 2024

Choose a reason for hiding this comment

GuillaumeGomez Feb 29, 2024

Choose a reason for hiding this comment

blyxyas Feb 29, 2024

Choose a reason for hiding this comment

GuillaumeGomez Feb 29, 2024

Choose a reason for hiding this comment

blyxyas Feb 29, 2024

Choose a reason for hiding this comment

GuillaumeGomez Feb 29, 2024

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 29, 2024

bors commented Mar 2, 2024

GuillaumeGomez commented Mar 2, 2024

GuillaumeGomez commented Mar 2, 2024

blyxyas left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 7, 2024

GuillaumeGomez commented Mar 7, 2024

blyxyas commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

Add new `duplicated_attributes` lint #12378

Add new `duplicated_attributes` lint #12378

GuillaumeGomez commented Feb 28, 2024 •

edited

Loading