You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/11/30 14:58:22 UTC
[GitHub] [solr] risdenk opened a new pull request, #1200: Invert error-prone configuration to be allow-list vs deny-list
risdenk opened a new pull request, #1200:
URL: https://github.com/apache/solr/pull/1200
This ports https://github.com/apache/lucene/pull/11970 to Solr. Copied commit message from Lucene below:
This does not change the semantics or performance of our setup.
Instead, it explicitly enables checks that we want vs disabling checks that we don't want.
Also reordered checks to match the error-prone website list of checks for easier maintenance.
It is now clear that many useless checks are enabled, we can disable some of them and try to get the performance reasonable.
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1200: Invert error-prone configuration to be allow-list vs deny-list
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1200:
URL: https://github.com/apache/solr/pull/1200#discussion_r1036103805
##########
gradle/validation/error-prone.gradle:
##########
@@ -62,53 +62,421 @@ allprojects { prj ->
options.errorprone.disableWarningsInGeneratedCode = true
options.errorprone.errorproneArgs = [
- // bug patterns related to our tests
- '-Xep:ExtendingJUnitAssert:OFF', // we inherit from LuceneTestCase which extends Assert
- '-Xep:UseCorrectAssertInTests:OFF', // and this is a consequence of the above
- '-Xep:CatchFail:OFF', // our code is generally descriptive enough, fix case by case if tests fail
- '-Xep:JUnit4TestNotRun:OFF', // RandomizedRunner finds unannotated test methods no problem
- '-Xep:StaticAssignmentInConstructor:OFF', // we assign SolrTestCaseJ4.configString in many tests, difficult to untangle
+ '-XepDisableAllChecks', // only enable specific checks
+ '-XepAllErrorsAsWarnings', // warnings still fail build by default, but allows usage of -Pjavac.failOnWarnings=false
+
+ // List of enabled/disabled checks
+ // Please keep this synced with https://errorprone.info/bugpatterns when upgrading!
+
+ // On by Default : ERROR
+
+ '-Xep:AlwaysThrows:ERROR',
+ '-Xep:AndroidInjectionBeforeSuper:ERROR',
+ '-Xep:ArrayEquals:ERROR',
+ '-Xep:ArrayFillIncompatibleType:ERROR',
+ '-Xep:ArrayHashCode:ERROR',
+ '-Xep:ArrayToString:ERROR',
+ '-Xep:ArraysAsListPrimitiveArray:ERROR',
+ '-Xep:AsyncCallableReturnsNull:ERROR',
+ '-Xep:AsyncFunctionReturnsNull:ERROR',
+ '-Xep:AutoValueBuilderDefaultsInConstructor:ERROR',
+ '-Xep:AutoValueConstructorOrderChecker:ERROR',
+ '-Xep:BadAnnotationImplementation:ERROR',
+ '-Xep:BadShiftAmount:ERROR',
+ '-Xep:BanJNDI:ERROR',
+ '-Xep:BoxedPrimitiveEquality:ERROR',
+ '-Xep:BundleDeserializationCast:ERROR',
+ '-Xep:ChainingConstructorIgnoresParameter:ERROR',
+ '-Xep:CheckNotNullMultipleTimes:ERROR',
+ '-Xep:CheckReturnValue:ERROR',
+ '-Xep:CollectionToArraySafeParameter:ERROR',
'-Xep:ComparableType:OFF', // SolrTestCaseJ4.Doc and Fld are messy
Review Comment:
I personally liked that we were excluding these explicitly. There is a comment here https://github.com/apache/lucene/pull/11971#issuecomment-1326008327 that `XepDisableAllChecks` doesn't actually disable all the checks. So I figured it was safer to explicitly disable the checks we don't want?
--
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
[GitHub] [solr] risdenk commented on pull request #1200: Invert error-prone configuration to be allow-list vs deny-list
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1200:
URL: https://github.com/apache/solr/pull/1200#issuecomment-1333769408
Going to further clean this up here - https://github.com/apache/solr/pull/1201
--
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
[GitHub] [solr] rmuir commented on a diff in pull request #1200: Invert error-prone configuration to be allow-list vs deny-list
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #1200:
URL: https://github.com/apache/solr/pull/1200#discussion_r1036127682
##########
gradle/validation/error-prone.gradle:
##########
@@ -62,53 +62,421 @@ allprojects { prj ->
options.errorprone.disableWarningsInGeneratedCode = true
options.errorprone.errorproneArgs = [
- // bug patterns related to our tests
- '-Xep:ExtendingJUnitAssert:OFF', // we inherit from LuceneTestCase which extends Assert
- '-Xep:UseCorrectAssertInTests:OFF', // and this is a consequence of the above
- '-Xep:CatchFail:OFF', // our code is generally descriptive enough, fix case by case if tests fail
- '-Xep:JUnit4TestNotRun:OFF', // RandomizedRunner finds unannotated test methods no problem
- '-Xep:StaticAssignmentInConstructor:OFF', // we assign SolrTestCaseJ4.configString in many tests, difficult to untangle
+ '-XepDisableAllChecks', // only enable specific checks
+ '-XepAllErrorsAsWarnings', // warnings still fail build by default, but allows usage of -Pjavac.failOnWarnings=false
+
+ // List of enabled/disabled checks
+ // Please keep this synced with https://errorprone.info/bugpatterns when upgrading!
+
+ // On by Default : ERROR
+
+ '-Xep:AlwaysThrows:ERROR',
+ '-Xep:AndroidInjectionBeforeSuper:ERROR',
+ '-Xep:ArrayEquals:ERROR',
+ '-Xep:ArrayFillIncompatibleType:ERROR',
+ '-Xep:ArrayHashCode:ERROR',
+ '-Xep:ArrayToString:ERROR',
+ '-Xep:ArraysAsListPrimitiveArray:ERROR',
+ '-Xep:AsyncCallableReturnsNull:ERROR',
+ '-Xep:AsyncFunctionReturnsNull:ERROR',
+ '-Xep:AutoValueBuilderDefaultsInConstructor:ERROR',
+ '-Xep:AutoValueConstructorOrderChecker:ERROR',
+ '-Xep:BadAnnotationImplementation:ERROR',
+ '-Xep:BadShiftAmount:ERROR',
+ '-Xep:BanJNDI:ERROR',
+ '-Xep:BoxedPrimitiveEquality:ERROR',
+ '-Xep:BundleDeserializationCast:ERROR',
+ '-Xep:ChainingConstructorIgnoresParameter:ERROR',
+ '-Xep:CheckNotNullMultipleTimes:ERROR',
+ '-Xep:CheckReturnValue:ERROR',
+ '-Xep:CollectionToArraySafeParameter:ERROR',
'-Xep:ComparableType:OFF', // SolrTestCaseJ4.Doc and Fld are messy
Review Comment:
i think i ended up commenting them out in the followup PR. It does make it easier for my editor to see the disabled ones.
I'm not concerned about disabling-all-checks not actually disabling all checks. I think it only enables a few critical ones, such as unicode directional checker. These checks override `disableable = false` in their annotation in the source code, e.g. https://github.com/google/error-prone/blob/4c7dff57339bba6b4cf41fc776a51de6c6bd5e00/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeDirectionalityCharacters.java#L34
And if you explicitly disable them, error-prone fails:
```
Execution failed for task ':lucene:core:compileJava'.
> UnicodeDirectionalityCharacters may not be disabled
```
So I think it is best to make life easier on ourselves by using the comments? In this example, yeah we check for these unicode characters elsewhere in our build, so it is redundant, but it is not possible to disable with error-prone so best to just have it configured as `ERROR` and move on.
--
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
[GitHub] [solr] risdenk commented on pull request #1200: Invert error-prone configuration to be allow-list vs deny-list
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1200:
URL: https://github.com/apache/solr/pull/1200#issuecomment-1332299825
I'm planning to follow up with disabling useless checks - like https://github.com/apache/lucene/pull/11971 is doing for Lucene.
--
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
[GitHub] [solr] madrob commented on a diff in pull request #1200: Invert error-prone configuration to be allow-list vs deny-list
Posted by GitBox <gi...@apache.org>.
madrob commented on code in PR #1200:
URL: https://github.com/apache/solr/pull/1200#discussion_r1036087253
##########
gradle/validation/error-prone.gradle:
##########
@@ -62,53 +62,421 @@ allprojects { prj ->
options.errorprone.disableWarningsInGeneratedCode = true
options.errorprone.errorproneArgs = [
- // bug patterns related to our tests
- '-Xep:ExtendingJUnitAssert:OFF', // we inherit from LuceneTestCase which extends Assert
- '-Xep:UseCorrectAssertInTests:OFF', // and this is a consequence of the above
- '-Xep:CatchFail:OFF', // our code is generally descriptive enough, fix case by case if tests fail
- '-Xep:JUnit4TestNotRun:OFF', // RandomizedRunner finds unannotated test methods no problem
- '-Xep:StaticAssignmentInConstructor:OFF', // we assign SolrTestCaseJ4.configString in many tests, difficult to untangle
+ '-XepDisableAllChecks', // only enable specific checks
+ '-XepAllErrorsAsWarnings', // warnings still fail build by default, but allows usage of -Pjavac.failOnWarnings=false
+
+ // List of enabled/disabled checks
+ // Please keep this synced with https://errorprone.info/bugpatterns when upgrading!
+
+ // On by Default : ERROR
+
+ '-Xep:AlwaysThrows:ERROR',
+ '-Xep:AndroidInjectionBeforeSuper:ERROR',
+ '-Xep:ArrayEquals:ERROR',
+ '-Xep:ArrayFillIncompatibleType:ERROR',
+ '-Xep:ArrayHashCode:ERROR',
+ '-Xep:ArrayToString:ERROR',
+ '-Xep:ArraysAsListPrimitiveArray:ERROR',
+ '-Xep:AsyncCallableReturnsNull:ERROR',
+ '-Xep:AsyncFunctionReturnsNull:ERROR',
+ '-Xep:AutoValueBuilderDefaultsInConstructor:ERROR',
+ '-Xep:AutoValueConstructorOrderChecker:ERROR',
+ '-Xep:BadAnnotationImplementation:ERROR',
+ '-Xep:BadShiftAmount:ERROR',
+ '-Xep:BanJNDI:ERROR',
+ '-Xep:BoxedPrimitiveEquality:ERROR',
+ '-Xep:BundleDeserializationCast:ERROR',
+ '-Xep:ChainingConstructorIgnoresParameter:ERROR',
+ '-Xep:CheckNotNullMultipleTimes:ERROR',
+ '-Xep:CheckReturnValue:ERROR',
+ '-Xep:CollectionToArraySafeParameter:ERROR',
'-Xep:ComparableType:OFF', // SolrTestCaseJ4.Doc and Fld are messy
Review Comment:
Can you comment out all the off checks so they are visually distinct?
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1200: Invert error-prone configuration to be allow-list vs deny-list
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1200:
URL: https://github.com/apache/solr/pull/1200#discussion_r1036137861
##########
gradle/validation/error-prone.gradle:
##########
@@ -62,53 +62,421 @@ allprojects { prj ->
options.errorprone.disableWarningsInGeneratedCode = true
options.errorprone.errorproneArgs = [
- // bug patterns related to our tests
- '-Xep:ExtendingJUnitAssert:OFF', // we inherit from LuceneTestCase which extends Assert
- '-Xep:UseCorrectAssertInTests:OFF', // and this is a consequence of the above
- '-Xep:CatchFail:OFF', // our code is generally descriptive enough, fix case by case if tests fail
- '-Xep:JUnit4TestNotRun:OFF', // RandomizedRunner finds unannotated test methods no problem
- '-Xep:StaticAssignmentInConstructor:OFF', // we assign SolrTestCaseJ4.configString in many tests, difficult to untangle
+ '-XepDisableAllChecks', // only enable specific checks
+ '-XepAllErrorsAsWarnings', // warnings still fail build by default, but allows usage of -Pjavac.failOnWarnings=false
+
+ // List of enabled/disabled checks
+ // Please keep this synced with https://errorprone.info/bugpatterns when upgrading!
+
+ // On by Default : ERROR
+
+ '-Xep:AlwaysThrows:ERROR',
+ '-Xep:AndroidInjectionBeforeSuper:ERROR',
+ '-Xep:ArrayEquals:ERROR',
+ '-Xep:ArrayFillIncompatibleType:ERROR',
+ '-Xep:ArrayHashCode:ERROR',
+ '-Xep:ArrayToString:ERROR',
+ '-Xep:ArraysAsListPrimitiveArray:ERROR',
+ '-Xep:AsyncCallableReturnsNull:ERROR',
+ '-Xep:AsyncFunctionReturnsNull:ERROR',
+ '-Xep:AutoValueBuilderDefaultsInConstructor:ERROR',
+ '-Xep:AutoValueConstructorOrderChecker:ERROR',
+ '-Xep:BadAnnotationImplementation:ERROR',
+ '-Xep:BadShiftAmount:ERROR',
+ '-Xep:BanJNDI:ERROR',
+ '-Xep:BoxedPrimitiveEquality:ERROR',
+ '-Xep:BundleDeserializationCast:ERROR',
+ '-Xep:ChainingConstructorIgnoresParameter:ERROR',
+ '-Xep:CheckNotNullMultipleTimes:ERROR',
+ '-Xep:CheckReturnValue:ERROR',
+ '-Xep:CollectionToArraySafeParameter:ERROR',
'-Xep:ComparableType:OFF', // SolrTestCaseJ4.Doc and Fld are messy
Review Comment:
addressed in 9245fec36031360bb46b5c43a2d48412ad2de096
--
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
[GitHub] [solr] risdenk merged pull request #1200: Invert error-prone configuration to be allow-list vs deny-list
Posted by GitBox <gi...@apache.org>.
risdenk merged PR #1200:
URL: https://github.com/apache/solr/pull/1200
--
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