You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "malliaridis (via GitHub)" <gi...@apache.org> on 2024/11/13 12:16:02 UTC
[PR] Update com.google.errorprone:* to 2.31.0 [solr]
malliaridis opened a new pull request, #2862:
URL: https://github.com/apache/solr/pull/2862
# Description
This PR updates Google error-prone to 2.31.0.
# Solution
In order to avoid blocking https://github.com/apache/solr/pull/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 https://github.com/apache/solr/pull/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.
# 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:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Update com.google.errorprone:* to 2.31.0 [solr]
Posted by "malliaridis (via GitHub)" <gi...@apache.org>.
malliaridis commented on PR #2862:
URL: https://github.com/apache/solr/pull/2862#issuecomment-2473699542
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](https://issues.apache.org/jira/browse/SOLR-16427)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Update com.google.errorprone:* to 2.31.0 [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2862:
URL: https://github.com/apache/solr/pull/2862#issuecomment-2473714247
Yes I think so that jira should probably be closed as "mostly done" by now anyway :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Update com.google.errorprone:* to 2.31.0 [solr]
Posted by "malliaridis (via GitHub)" <gi...@apache.org>.
malliaridis commented on code in PR #2862:
URL: https://github.com/apache/solr/pull/2862#discussion_r1840295379
##########
gradle/validation/error-prone.gradle:
##########
@@ -236,7 +240,7 @@ allprojects { prj ->
// '-Xep:ThrowIfUncheckedKnownChecked:OFF', // we don't use this annotation
'-Xep:ThrowNull:ERROR',
'-Xep:TreeToString:ERROR',
- // '-Xep:TruthSelfEquals:OFF', // we don't use truth
+ // '-Xep:SelfAssertion:OFF', // we don't use truth
Review Comment:
Oh, nice catch. I'll remove the `TruthSelfEquals` then and keep the todo for `SelfAssertion` as it seems that it does check more than `TruthSelfEquals` did.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Update com.google.errorprone:* to 2.31.0 [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2862:
URL: https://github.com/apache/solr/pull/2862#discussion_r1840233468
##########
gradle/validation/error-prone.gradle:
##########
@@ -236,7 +240,7 @@ allprojects { prj ->
// '-Xep:ThrowIfUncheckedKnownChecked:OFF', // we don't use this annotation
'-Xep:ThrowNull:ERROR',
'-Xep:TreeToString:ERROR',
- // '-Xep:TruthSelfEquals:OFF', // we don't use truth
+ // '-Xep:SelfAssertion:OFF', // we don't use truth
Review Comment:
SelfAssertion is listed twice now? Line 226
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Update com.google.errorprone:* to 2.31.0 [solr]
Posted by "malliaridis (via GitHub)" <gi...@apache.org>.
malliaridis merged PR #2862:
URL: https://github.com/apache/solr/pull/2862
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Update com.google.errorprone:* to 2.31.0 [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2862:
URL: https://github.com/apache/solr/pull/2862#discussion_r1840296908
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
Review Comment:
We don't use guice so this can be ignored.
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
Review Comment:
We don't use AutoValue
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
Review Comment:
This is probably useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -403,10 +413,12 @@ allprojects { prj ->
// '-Xep:MissingSummary:OFF', // style preference that we don't want to enforce
// '-Xep:MixedMutabilityReturnType:OFF', // todo check if useful or comment why not
'-Xep:MockNotUsedInProduction:WARN',
+ // '-Xep:MockitoDoSetup:WARN', // todo check if useful or comment why not
Review Comment:
This might be useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -330,6 +338,7 @@ allprojects { prj ->
// '-Xep:FragmentNotInstantiable:OFF', // we don't use android
// '-Xep:FutureReturnValueIgnored:OFF', // todo there are problems that should be fixed
'-Xep:GetClassOnEnum:WARN',
+ // '-Xep:GuiceNestedCombine:WARN', // todo check if useful or comment why not
Review Comment:
We don't use guice
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
Review Comment:
Might be useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
'-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
Review Comment:
This is probably useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
// '-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
'-Xep:SelfAssignment:ERROR',
'-Xep:SelfComparison:ERROR',
'-Xep:SelfEquals:ERROR',
+ // '-Xep:SetUnrecognized:ERROR', // todo check if useful or comment why not
Review Comment:
We don't use proto so can be ignored
##########
gradle/validation/error-prone.gradle:
##########
@@ -285,6 +289,7 @@ allprojects { prj ->
'-Xep:ChainedAssertionLosesContext:WARN',
'-Xep:CharacterGetNumericValue:WARN',
'-Xep:ClassCanBeStatic:WARN',
+ // '-Xep:ClassInitializationDeadlock:WARN', // todo check if useful or comment why not
Review Comment:
This is probably useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -309,6 +316,7 @@ allprojects { prj ->
'-Xep:EmptyBlockTag:WARN',
// '-Xep:EmptyCatch:OFF', // todo check if useful or comment why not - might be handled by ECJ?
// '-Xep:EmptySetMultibindingContributions:OFF', // we don't use this annotation
+ // '-Xep:EnumOrdinal:WARN', // todo check if useful or comment why not
Review Comment:
This might be useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -383,6 +392,7 @@ allprojects { prj ->
'-Xep:JodaPlusMinusLong:WARN',
'-Xep:JodaTimeConverterManager:WARN',
'-Xep:JodaWithDurationAddedLong:WARN',
+ // '-Xep:JUnitIncompatibleType:WARN', // todo check if useful or comment why not
Review Comment:
This is probably useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -296,6 +301,8 @@ allprojects { prj ->
'-Xep:DateChecker:WARN',
'-Xep:DateFormatConstant:WARN',
// '-Xep:DefaultCharset:OFF', // we have forbiddenapis for that
+ //'-Xep:DeeplyNested:WARN', // todo check if useful or comment why not
+ //'-Xep:DefaultLocale:WARN', // todo check if useful or comment why not
Review Comment:
This is not needed. its handled by forbidden apis
##########
gradle/validation/error-prone.gradle:
##########
@@ -296,6 +301,8 @@ allprojects { prj ->
'-Xep:DateChecker:WARN',
'-Xep:DateFormatConstant:WARN',
// '-Xep:DefaultCharset:OFF', // we have forbiddenapis for that
+ //'-Xep:DeeplyNested:WARN', // todo check if useful or comment why not
Review Comment:
This might be useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
// '-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
Review Comment:
We don't use truth so can be ignored
##########
gradle/validation/error-prone.gradle:
##########
@@ -428,7 +440,9 @@ allprojects { prj ->
'-Xep:NullableOptional:WARN',
// '-Xep:NullablePrimitive:OFF', // we don't use this annotation
// '-Xep:NullablePrimitiveArray:OFF', // we don't use this annotation
+ // '-Xep:NullableTypeParameter:WARN', // todo check if useful or comment why not
Review Comment:
Might be useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -403,10 +413,12 @@ allprojects { prj ->
// '-Xep:MissingSummary:OFF', // style preference that we don't want to enforce
// '-Xep:MixedMutabilityReturnType:OFF', // todo check if useful or comment why not
'-Xep:MockNotUsedInProduction:WARN',
+ // '-Xep:MockitoDoSetup:WARN', // todo check if useful or comment why not
'-Xep:ModifiedButNotUsed:WARN',
'-Xep:ModifyCollectionInEnhancedForLoop:WARN',
'-Xep:ModifySourceCollectionInStream:WARN',
'-Xep:MultimapKeys:WARN',
+ // '-Xep:MultipleNullnessAnnotations:WARN', // todo check if useful or comment why not
Review Comment:
We don't use nullness annotations but might be useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -428,7 +440,9 @@ allprojects { prj ->
'-Xep:NullableOptional:WARN',
// '-Xep:NullablePrimitive:OFF', // we don't use this annotation
// '-Xep:NullablePrimitiveArray:OFF', // we don't use this annotation
+ // '-Xep:NullableTypeParameter:WARN', // todo check if useful or comment why not
// '-Xep:NullableVoid:OFF', // we don't use this annotation
+ // '-Xep:NullableWildcard:WARN', // todo check if useful or comment why not
Review Comment:
Might be useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
Review Comment:
This is probably useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -442,6 +456,7 @@ allprojects { prj ->
'-Xep:Overrides:WARN',
// '-Xep:OverridesGuiceInjectableMethod:OFF', // we don't use guice
'-Xep:ParameterName:WARN',
+ // '-Xep:PatternMatchingInstanceof:WARN', // todo check if useful or comment why not
Review Comment:
Not sure we need this since its subjective
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
Review Comment:
This is probably useful
##########
gradle/validation/error-prone.gradle:
##########
@@ -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
Review Comment:
Not sure this is useful or just subjective
##########
gradle/validation/error-prone.gradle:
##########
@@ -459,6 +474,7 @@ allprojects { prj ->
// '-Xep:SameNameButDifferent:OFF', // todo check if useful or comment why not
'-Xep:SelfAlwaysReturnsThis:WARN',
// '-Xep:ShortCircuitBoolean:OFF', // todo check if useful or comment why not
+ // '-Xep:StatementSwitchToExpressionSwitch:WARN', // todo check if useful or comment why not
Review Comment:
Not sure this is needed since its subjective
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org