fix: fix false positives on duplicated_attributes

[InvalidPathException]

Fixes #13238

Previously, PR #13355 (partly) addressed similar issues, @kpreid identified the cause, I hope this fix matches the intention outlined there.

changelog: Limit duplicated_attribute to lint-level attributes to reduce false positives and overlaps with rustc

[rustbot]

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

[kpreid]

Since I was pinged, here’s my opinion: I don't think this is the correct fix.

This is still making the assumption that all attributes or fragments of attributes not specifically identified otherwise (lints, doc) should be linted. This is, as I said in #13355, just not correct. The macro (or tool attribute) processor can do anything with the attributes it’s looking for. An example of this is even in the test cases: without inspecting the code of the proc-macro proc_macro_attr::DerivedAttrs, you can’t know whether the #[attr] it looks for is idempotent or not.

The lint should not fire on attributes it does not know the semantics of. It should have a list of the attributes it can check.

View changes since this review

[CommanderStorm]

community review:
could you (inline) comment on the changes why they were done? I find it a bit harder than usual to find the rationale for each of the changes.

View changes since this review

[CommanderStorm]

Can you refactor this to stay with Vec<Symbol>? I think it is a "thighter" type, so modeling the state space sligtly better, or?
Or would Symbol::intern be worse for performance?
Why was this change done?

[InvalidPathException]

Hi, you are right, it's to avoid Symbol::intern, I don't think it meaningfully change performance but did not take the "tighter" aspect into account.

[InvalidPathException]

Since I was pinged, here’s my opinion: I don't think this is the correct fix.

This is still making the assumption that all attributes or fragments of attributes not specifically identified otherwise (lints, doc) should be linted. This is, as I said in #13355, just not correct. The macro (or tool attribute) processor can do anything with the attributes it’s looking for. An example of this is even in the test cases: without inspecting the code of the proc-macro proc_macro_attr::DerivedAttrs, you can’t know whether the #[attr] it looks for is idempotent or not.

The lint should not fire on attributes it does not know the semantics of. It should have a list of the attributes it can check.

View changes since this review

Thanks for the feedback! Seems like the right approach is to limit the linted attributes to lint-level ones.

Another thing I noted: rustc catches most duplication patterns it knows, and clippy catches them again, for example #[inline] #[inline] should probably not be clippy's concern if it will become a hard error eventually.

What rustc gives

warning: unused attribute
 --> example.rs:2:1
  |
2 | #[inline]
  | ^^^^^^^^^ help: remove this attribute
  |
note: attribute also specified here
 --> example.rs:1:1
  = warning: this was previously accepted by the compiler but is being phased out;
    it will become a hard error in a future release!
  = note: `#[warn(unused_attributes)]` (part of `#[warn(unused)]`) on by default

repr and target_feature will no longer be covered if we go that route, but we can easily add them back too.

[kpreid]

This should be inside a code path that knows that the containing attribute is a lint level attribute, not applying everywhere.

Perhaps checking parent would do the trick? Or maybe check_duplicated_attr should separately take an enum of the syntactic contexts it can process, something like:

enum Context {
    /// top level or cfg_attr()
    Attribute,
    /// inside of allow() deny() etc
    LintName,
}

That would do the same thing as checking parent but be more efficient and likely easier to get right, at the price of writing more code.

There’s also a terminology problem that perpetuates the confusion this lint used to have: this attr is in general not an attribute — it’s a meta item that might be part of an attribute. (For example, allow(...) and doc = "..." are attributes, but clippy::pedantic and reason = "..." are not attributes, only meta items that appear in specific attributes.)

I’m not sure whether it’s worth renaming the variable and other terms here, but it might help.

View changes since the review

[InvalidPathException]

Thanks! looks like the enum actually reduces code and complexity.