error-prone-support icon indicating copy to clipboard operation
error-prone-support copied to clipboard

Introduce `DuplicateAnnotationListing` bug checker

Open mohamedsamehsalah opened this issue 4 months ago • 2 comments

Problem

Duplicate entries inside annotation listing are a common mistake that should be flagged.

Description of the proposed new feature

  • [x] Support a stylistic preference.
  • [x] Avoid a common gotcha, or potential problem.
  • [x] Improve performance.

Introduce a new BugChecker to flag duplicate entries as this is a common mistake. Similar to LexicographicalAnnotationListing, the new BugChecker should identify and handle duplicate entries to improve code quality and reduce potential errors.

I would like to rewrite the following code:

            @A("foo", "bar", "foo")
            A duplicateAnnotations();

to:

            @A("foo", "bar")
            A duplicateAnnotations();

Considerations

👉 To consider: A duplicate entry is a duplicate pointer reference and not just the same name. 👉 Open question: Are there situations where we do not want to flag this ?

Participation

  • [x] I am willing to submit a pull request to implement this improvement.

Additional context

See this comment.


Edit 1: Update requirements based on this.

mohamedsamehsalah avatar Feb 18 '24 14:02 mohamedsamehsalah

Thanks for filing this issue @mohamedsamehsalah! One key point of feedback: I think we're looking for an analog of LexicographicalAnnotationAttributeListing, i.e. a check that replaces @A("foo", "bar", "foo") with @A("foo", "bar").

(Repeated annotations such as @Baz() @Baz() are rejected by the compiler, unless they're declared @Repeatable, in which case the repetition likely is intended.)

Edit: I see that this comment is indeed located on the wrong class. That's my fault!

Stephan202 avatar Feb 19 '24 06:02 Stephan202

W.r.t. your considerations:

  • I'm not sure how pointer references factor in, but since only primitives, enum values and strings can be annotation arguments, the invariant holds that a value is a duplicated if and only if it is equal to some other value inside the same annotation attribute.
  • There are likely cases we'll want to exclude, but none come to mind right now. We might want to run a first version against the Picnic code base to assess what is flagged.

Stephan202 avatar Feb 19 '24 06:02 Stephan202