Enable checks from Error Prone Support

[rickie]

In PR #14136 I added Error Prone Support to Checkstyle. As @romani suggested in #14129, I'd like to create this issue to discuss which checks from Error Prone Support are useful for the Checkstyle project to enable.

The full list of checks can be found here: https://error-prone.picnic.tech/.

1. AmbiguousJsonCreator
2. AssertJIsNull
3. AutowiredConstructor
4. CanonicalAnnotationSyntax
5. CollectorMutability
6. DirectReturn
7. EmptyMethod
8. ExplicitEnumOrdering
9. FormatStringConcatenation
10. IdentityConversion
11. ImmutablesSortedSetComparator
12. IsInstanceLambdaUsage
13. JUnitClassModifiers
14. JUnitMethodDeclaration
15. JUnitValueSource
16. LexicographicalAnnotationAttributeListing
17. MockitoMockClassReference
18. MockitoStubbing
19. NestedOptionals
20. PrimitiveComparison
21. RedundantStringConversion
22. Slf4jLogStatement
23. StaticImport
24. StringJoin
25. TimeZoneUsage

I omitted some of the checks from the list as they are definitely not applicable to this project (for example they are Spring or Reactor focused).

Let me know if this is the right approach, or if we should change something.

I'm curious to hear your thoughts on the checks!

[rickie]

Here are a few of my comments on some of the checks:

---

2. AssertJIsNull: Only one AssertJ reference in Checkstyle, so not relevant.

---

5. CollectorMutability: In this PR @romani mentioned that we should enable this rule 😄 Pull #12777: emphasize mutability of collectors #12777 (review).

---

14. JUnitMethodDeclaration: @romani made a comment about that here, saying:

We had this violation in Sonarqube error-prone.picnic.tech/bugpatterns/JUnitMethodDeclaration
In team we agreed to have methods to be public even somebody think it is better to be package-private.

So this one is probably not interesting.

---

23. StaticImport: In this comment @romani mentions the following:

error-prone.picnic.tech/bugpatterns/StaticImport
We like more explicit code, as majority of time we do code review from phones, so context matters for us.

Just to clarify, in this check we statically import these members. It is a list of carefully reviewed members that are statically imported as doing so doesn't result in a loss of context. So mostly to remove "duplicate words".

[romani]

@Vyom-Yadav, FYI

[romani]

Please activate:
AmbiguousJsonCreator
AssertJIsNull
AutowiredConstructor
CanonicalAnnotationSyntax
CollectorMutability

---

DirectReturn we can try in separate PR, to see a scope of violations, I remember in some cases it was benefitials to use not direct return.

---

please activate:
EmptyMethod
ExplicitEnumOrdering

---

FormatStringConcatenation we can try in separate PR, to see a scope of violations, I remember in some cases it was benefitials to use concatenation as compact syntax.

---

please activate:
IdentityConversion

---

ImmutablesSortedSetComparator we can try in separate PR. I have concern on extra dependency, we are library, we care about less transitive dependencies.

---

please activate:
IsInstanceLambdaUsage

---

lets suppress:
JUnitClassModifiers
JUnitMethodDeclaration
JUnitValueSource

---

please activate:
LexicographicalAnnotationAttributeListing
MockitoMockClassReference
MockitoStubbing
NestedOptionals
PrimitiveComparison
RedundantStringConversion
Slf4jLogStatement

---

lets suppress:
StaticImport

---

lets try:
StringJoin
TimeZoneUsage

[romani]

@rickie, please send each activation in separate PR, AND isf there is bunch of udpates required, it is completely OK to split fixes( before activation) in separate PRs.
Some updates might be very controversial, so it is reasonable to have smaller PR to agree on style before forcing whole code to follow it.

[nrmancuso]

I agree with prioritization and selection of bug patterns at #14137 (comment), @rickie please feel free to start sending PRs :)

[Vyom-Yadav]

I agree with the discussion above. @romani This can be a good third/fourth issue. Please convert the issue description to a checklist, I too can send a few PRs to have some examples.

[romani]

This is not good for junior contributors as it might be controversial and tricky.
But you are not a junior, we are welcome with PR