upgrade error-prone to 2.7.1 and support checks with Java 11+ #11363
Conversation
- upgrade error-prone to 2.7.1
- support running error-prone with Java 11 and above using -Xplugin
instead of custom compiler
- add compiler arguments to ignore warnings/errors in Java 15/16
- introduce strictCompile property to enable strict profiles since we
now need multiple strict profiles for Java 8
- properly exclude all generated source files from error-prone
- fix druid-processing overriding annotation processors from parent pom
- fix druid-core disabling most non-default checks
- align plugin and annotation errorprone versions
- fix / suppress additional issues found by error-prone:
* fix bug in SeekableStreamSupervisor initializing ArrayList size with
the taskGroupdId
* fix missing @OverRide annotations
- remove outdated compiler plugin in benchmarks
- remove deleted ParameterPackage error-prone rule
- re-enable checks on benchmark module as well
|
@asdf2014 I see you gave a thumbs up, do you mind reviewing :) ? |
| @@ -113,6 +113,7 @@ public CryptoService( | |||
|
|
|||
| SecretKey tmp = getKeyFromPassword(passPhrase, salt); | |||
| SecretKey secret = new SecretKeySpec(tmp.getEncoded(), cipherAlgName); | |||
| @SuppressWarnings("InsecureCryptoUsage") | |||
There was a problem hiding this comment.
What's insecure about it? It is bad?
If it's not bad, it'd be good to have a comment here about why it's not bad.
There was a problem hiding this comment.
error-prone warns if the ciphers are not compile-time constants that it can inspect for insecure defaults.
In our case our defaults are secure, but I can add a comment if that helps.
There was a problem hiding this comment.
I think it'd be helpful, since the suppression looks scary.
| @@ -27,7 +27,7 @@ | |||
| import java.util.Objects; | |||
|
|
|||
| // logical operators live here | |||
|
|
|||
| @SuppressWarnings("ClassName") | |||
There was a problem hiding this comment.
I guess the issue is there's no public class BinaryLogicalOperatorExpr?
I suppose that's strange, but fine.
There was a problem hiding this comment.
right, and it also expects each class to be in its own file with the same name
| </compilerArgs> | ||
| <arg>-XDcompilePolicy=simple</arg> | ||
| <!-- disable LongFloatConversion until https://github.com/google/error-prone/issues/2396 is fixed --> | ||
| <arg>-Xplugin:ErrorProne -XepExcludedPaths:.*/target/generated-(test-)?sources/.* -XepDisableWarningsInGeneratedCode -Xep:ClassCanBeStatic:ERROR -Xep:PreconditionsInvalidPlaceholder:ERROR -Xep:MissingOverride:ERROR -Xep:DefaultCharset:ERROR -Xep:QualifierOrScopeOnInjectMethod:ERROR -Xep:AssistedInjectAndInjectOnSameConstructor -Xep:AutoFactoryAtInject -Xep:ClassName -Xep:ComparisonContractViolated -Xep:DepAnn -Xep:DivZero -Xep:EmptyIf -Xep:InjectInvalidTargetingOnScopingAnnotation -Xep:InjectMoreThanOneQualifier -Xep:InjectScopeAnnotationOnInterfaceOrAbstractClass -Xep:InjectScopeOrQualifierAnnotationRetention -Xep:InjectedConstructorAnnotations -Xep:InsecureCryptoUsage -Xep:JMockTestWithoutRunWithOrRuleAnnotation -Xep:JavaxInjectOnFinalField -Xep:LockMethodChecker -Xep:LongLiteralLowerCaseSuffix -Xep:NoAllocation -Xep:NonRuntimeAnnotation -Xep:NumericEquality -Xep:ProtoStringFieldReferenceEquality -Xep:UnlockMethod -Xep:LongFloatConversion:OFF</arg> |
There was a problem hiding this comment.
If this still works when broken onto multiple lines, I think it'd be nice to do that, for readability's sake. The line is super long.
There was a problem hiding this comment.
unfortunately we cannot wrap arguments in JDK 8, it's only possible for JDK 9 and beyond see https://errorprone.info/docs/flags#maven
| @@ -526,6 +526,7 @@ public int nextInt() | |||
| }; | |||
| } | |||
|
|
|||
| @SuppressWarnings("ReturnValueIgnored") | |||
There was a problem hiding this comment.
Will this suppression work if it's moved down to the single line values.contains(null)?
If not, consider adding a comment here about what line needs the ReturnValueIgnored suppression. It might not be obvious to people.
There was a problem hiding this comment.
the annotation can only be put on a method or an assignment
| analysis.getBaseTableDataSource() | ||
| .filter(ds -> dataSource.equals(ds.getName())) | ||
| .orElseThrow(() -> new ISE("Cannot handle datasource: %s", analysis.getDataSource())); | ||
| if (!analysis.getBaseTableDataSource().filter(ds -> dataSource.equals(ds.getName())).isPresent()) { |
There was a problem hiding this comment.
it was a fight between error-prone complaining about unused return values, and spotbugs complaining about DLS_DEAD_LOCAL_STORE
|
Looks good to me, btw, other than the nits about comments and formatting and such. |
instead of custom compiler
now need multiple strict profiles for Java 8
the taskGroupdId
@Overrideannotations