Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update com.google.errorprone:* to 2.31.0 #2862

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

malliaridis
Copy link
Contributor

@malliaridis malliaridis commented Nov 13, 2024

Description

This PR updates Google error-prone to 2.31.0.

Solution

In order to avoid blocking #2706 I have updated Google error-prone and added all new checks introduced between 2.23.0 and 2.31.0 without enabling them to gradle/validation/error-prone.gradle. Enabling the new rules should be handled in a separate PR as this may require further evaluation and compatibility check and would further delay #2706.

The selected version 2.31.0 is the last version of error-prone that fully supports JDK 11, so this PR should be backwards compatible. Note that some new rules are not applicable to JDK 11 and should carefully be picked upon activation.

Some of the existing rules seem to have had some changes that caused existing checks to fail. For the affected code, I decided individually wether to suppress the warning or fix the code.

See commit details for additional information about individual changes.

Tests

Some tests were affected by this update and were updated. The updates include assertions that were checking primitive data types for nullability and were therefore removed.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

This check is impossible and will always fail with
an IllegalArgumentException if algorithmString is
invalid. The same IllegalArgumentException is also
thrown in the else block, so the output is the same.
This fixes ClassCanBeStatic warnings
qTime is a (primitive) long and is always 0 by default.
This could be also fixed by changing qTime type to Long.

In the case of fromHumanReadableUnits(), the method will
throw an AssertionError or NumberFormatException for invalid
types, and cannot return a (primitive) double that is null.
Add new error-prone rules to the list without enabling them.
gradle/validation/error-prone.gradle Outdated Show resolved Hide resolved
@malliaridis malliaridis merged commit f6c5af9 into apache:main Nov 13, 2024
4 checks passed
Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added notes going through the new rules. Most can be ignored.

@@ -179,6 +179,7 @@ allprojects { prj ->
'-Xep:MathRoundIntLong:ERROR',
// '-Xep:MislabeledAndroidString:OFF', // we don't use android
'-Xep:MisplacedScopeAnnotations:ERROR',
// '-Xep:MissingRuntimeRetention:ERROR', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use guice so this can be ignored.

@@ -218,12 +219,15 @@ allprojects { prj ->
'-Xep:RandomCast:ERROR',
'-Xep:RandomModInteger:ERROR',
// '-Xep:RectIntersectReturnValueIgnored:OFF', // we don't use android
// '-Xep:RedundantSetterCall:ERROR', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use AutoValue

'-Xep:SelfAssignment:ERROR',
'-Xep:SelfComparison:ERROR',
'-Xep:SelfEquals:ERROR',
// '-Xep:SetUnrecognized:ERROR', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use proto so can be ignored

// '-Xep:RequiredModifiers:OFF', // we don't use this annotation
// '-Xep:RestrictedApiChecker:OFF', // we don't use this annotation
// '-Xep:ReturnValueIgnored:OFF', // todo there are problems that should be fixed
// '-Xep:SelfAssertion:ERROR', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use truth so can be ignored

@@ -265,6 +268,7 @@ allprojects { prj ->
'-Xep:AssertionFailureIgnored:WARN',
'-Xep:AssistedInjectAndInjectOnSameConstructor:WARN',
'-Xep:AttemptedNegativeZero:WARN',
// '-Xep:AutoValueBoxedValues:WARN', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably useful

@@ -469,9 +485,12 @@ allprojects { prj ->
'-Xep:StringCharset:WARN',
'-Xep:StringFormatWithLiteral:WARN',
// '-Xep:StringSplitter:OFF', // todo check if useful or comment why not - might be able to use forbidden-apis for this?
// '-Xep:SunApi:WARN', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably useful

@@ -469,9 +485,12 @@ allprojects { prj ->
'-Xep:StringCharset:WARN',
'-Xep:StringFormatWithLiteral:WARN',
// '-Xep:StringSplitter:OFF', // todo check if useful or comment why not - might be able to use forbidden-apis for this?
// '-Xep:SunApi:WARN', // todo check if useful or comment why not
// '-Xep:SuperCallToObjectMethod:WARN', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably useful

'-Xep:SuperEqualsIsObjectEquals:WARN',
// '-Xep:SwigMemoryLeak:OFF', // we don't use swig
// '-Xep:SynchronizeOnNonFinalField:OFF', // todo check if useful or comment why not
// '-Xep:SystemConsoleNull:WARN', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably useful

@@ -493,6 +512,7 @@ allprojects { prj ->
// '-Xep:UnicodeEscape:OFF', // can't enable since Lucene/Solr tests use unicode a bunch
// '-Xep:UnnecessaryAssignment:OFF', // we don't use these annotations
'-Xep:UnnecessaryAsync:WARN',
// '-Xep:UnnecessaryBreakInSwitch:WARN', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is useful or just subjective

@@ -513,6 +533,7 @@ allprojects { prj ->
// '-Xep:UseBinds:OFF', // we don't use this annotation
// '-Xep:UseCorrectAssertInTests:OFF', // we inherit from LuceneTestCase which extends Assert
'-Xep:VariableNameSameAsType:WARN',
// '-Xep:VoidUsed:WARN', // todo check if useful or comment why not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be useful

@malliaridis
Copy link
Contributor Author

Thanks for the notes @risdenk. I will soon create another PR for applying the rules and see what can be backported as well. We can then address the existing rules as well. Is it fine to do it under the context of SOLR-16427?

@risdenk
Copy link
Contributor

risdenk commented Nov 13, 2024

Yes I think so that jira should probably be closed as "mostly done" by now anyway :D

malliaridis added a commit to malliaridis/solr that referenced this pull request Nov 13, 2024
* Update com.google.errorprone:* to 2.31.0
* Remove impossible null comparison
* Suppress wanted method references
* Fix ClassCanBeStatic warnings
* Remove null checks for primitive data types

(cherry picked from commit f6c5af9)
malliaridis added a commit that referenced this pull request Nov 15, 2024
* Update com.google.errorprone:* to 2.31.0 (#2862)

* Update com.google.errorprone:* to 2.31.0
* Remove impossible null comparison
* Suppress wanted method references
* Fix ClassCanBeStatic warnings
* Remove null checks for primitive data types

(cherry picked from commit f6c5af9)

* Remove unsupported static declaration of inner classes from cherry-pick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants