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

LUCENE-9497: add google error prone checks #1828

Closed
wants to merge 10 commits into from
2 changes: 2 additions & 0 deletions build.gradle
Expand Up @@ -24,6 +24,7 @@ plugins {
id "org.owasp.dependencycheck" version "5.3.0"
id 'de.thetaphi.forbiddenapis' version '3.0.1' apply false
id "de.undercouch.download" version "4.0.2" apply false
id "net.ltgt.errorprone" version "1.2.1" apply false
}

apply from: file('gradle/defaults.gradle')
Expand Down Expand Up @@ -127,6 +128,7 @@ apply from: file('gradle/ide/intellij-idea.gradle')
apply from: file('gradle/ide/eclipse.gradle')

// Validation tasks
apply from: file('gradle/validation/error-prone.gradle')
apply from: file('gradle/validation/precommit.gradle')
apply from: file('gradle/validation/forbidden-apis.gradle')
apply from: file('gradle/validation/jar-checks.gradle')
Expand Down
5 changes: 4 additions & 1 deletion gradle/defaults-java.gradle
Expand Up @@ -52,8 +52,11 @@ allprojects {
"-Xdoclint:-missing",
"-Xdoclint:-accessibility",
"-proc:none", // proc:none was added because of LOG4J2-1925 / JDK-8186647
"-Werror",
]

if (propertyOrDefault("javac.failOnWarnings", true).toBoolean()) {
options.compilerArgs += "-Werror"
}
}
}
}
5 changes: 3 additions & 2 deletions gradle/hacks/findbugs.gradle
Expand Up @@ -16,13 +16,14 @@
*/

// See LUCENE-9411. This hack adds compile-time only dependencies
// on findbugs annotations. Otherwise javac generates odd warnings about missing
// on findbugs and error_prone annotations. Otherwise javac generates odd warnings about missing
// type information.

configure([project(":solr:core"),
project(":solr:solrj"),
project(":solr:contrib:prometheus-exporter"),
project(":solr:test-framework")]) {
project(":solr:test-framework"),
project(":solr:contrib:analytics")]) {
plugins.withType(JavaPlugin) {
dependencies {
// Use versionless variants because these libraries are in versions.lock.
Expand Down
148 changes: 148 additions & 0 deletions gradle/validation/error-prone.gradle
@@ -0,0 +1,148 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

allprojects { prj ->
plugins.withType(JavaPlugin) {
prj.apply plugin: 'net.ltgt.errorprone'

dependencies {
errorprone("com.google.errorprone:error_prone_core")
}

tasks.withType(JavaCompile) { task ->
options.errorprone.errorproneArgs = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like to see options.errorprone.disableWarningsInGeneratedCode = true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggested the same to Varun.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this ? I probably missed it in my PR

// test
'-Xep:ExtendingJUnitAssert:OFF',
Copy link
Contributor

Choose a reason for hiding this comment

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

Personal style, but I think options.errorprone { disable 'ExtendingJUnitAssert' }
is more clear than using -Xep?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am okay with either of the two styles. Ideally we'd want this list to get much shorter soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Varun - feel free to take this branch (or patch) and roll it out on yours. I didn't intend it to be committed, I just wanted to show what's needed for it to compile and work.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good! I'll take the current branch and add two things and then commit the code

  1. options.errorprone.disableWarningsInGeneratedCode = true
  2. CHANGES entry

'-Xep:UseCorrectAssertInTests:OFF',
'-Xep:DefaultPackage:OFF',
'-Xep:FloatingPointLiteralPrecision:OFF',
'-Xep:CatchFail:OFF',
'-Xep:TryFailThrowable:OFF',
'-Xep:MathAbsoluteRandom:OFF',
'-Xep:AssertionFailureIgnored:OFF',
'-Xep:JUnit4TestNotRun:OFF',
'-Xep:FallThrough:OFF',
'-Xep:CatchAndPrintStackTrace:OFF',
'-Xep:ToStringReturnsNull:OFF',
'-Xep:ArrayAsKeyOfSetOrMap:OFF',
'-Xep:StaticAssignmentInConstructor:OFF',
'-Xep:SelfAssignment:OFF',
'-Xep:InvalidPatternSyntax:OFF',
'-Xep:MissingFail:OFF',
'-Xep:LossyPrimitiveCompare:OFF',
'-Xep:ComparableType:OFF',
'-Xep:InfiniteRecursion:OFF',
'-Xep:MisusedDayOfYear:OFF',
'-Xep:FloatingPointAssertionWithinEpsilon:OFF',

'-Xep:ThrowNull:OFF',
'-Xep:StaticGuardedByInstance:OFF',
'-Xep:ArrayHashCode:OFF',
'-Xep:ArrayEquals:OFF',
'-Xep:IdentityBinaryExpression:OFF',
'-Xep:ComplexBooleanConstant:OFF',
'-Xep:ComplexBooleanConstant:OFF',
'-Xep:StreamResourceLeak:OFF',
'-Xep:UnnecessaryLambda:OFF',
'-Xep:ObjectToString:OFF',
'-Xep:URLEqualsHashCode:OFF',
'-Xep:DoubleBraceInitialization:OFF',
'-Xep:ShortCircuitBoolean:OFF',
'-Xep:InputStreamSlowMultibyteRead:OFF',
'-Xep:NonCanonicalType:OFF',
'-Xep:CollectionIncompatibleType:OFF',
'-Xep:TypeParameterShadowing:OFF',
'-Xep:ThreadJoinLoop:OFF',
'-Xep:MutableConstantField:OFF',
'-Xep:ReturnValueIgnored:OFF',
'-Xep:CollectionIncompatibleType:OFF',
'-Xep:SameNameButDifferent:OFF',
'-Xep:InvalidParam:OFF',
'-Xep:CompareToZero:OFF',
'-Xep:DoubleCheckedLocking:OFF',
'-Xep:BadShiftAmount:OFF',
'-Xep:CollectionUndefinedEquality:OFF',
'-Xep:UnescapedEntity:OFF',
'-Xep:BoxedPrimitiveEquality:OFF',
'-Xep:LogicalAssignment:OFF',
'-Xep:DoubleCheckedLocking:OFF',
'-Xep:AmbiguousMethodReference:OFF',
'-Xep:FormatString:OFF',
'-Xep:InstanceOfAndCastMatchWrongType:OFF',
'-Xep:ModifyCollectionInEnhancedForLoop:OFF',
'-Xep:JavaLangClash:OFF',
'-Xep:TypeParameterUnusedInFormals:OFF',
'-Xep:UnusedNestedClass:OFF',
'-Xep:OverrideThrowableToString:OFF',
'-Xep:FutureReturnValueIgnored:OFF',
'-Xep:BadInstanceof:OFF',
'-Xep:UnusedNestedClass:OFF',
'-Xep:OverrideThrowableToString:OFF',
'-Xep:EqualsIncompatibleType:OFF',
'-Xep:ByteBufferBackingArray:OFF',
'-Xep:ByteBufferBackingArray:OFF',
'-Xep:UnusedMethod:OFF',
'-Xep:ObjectsHashCodePrimitive:OFF',
'-Xep:ObjectsHashCodePrimitive:OFF',
'-Xep:UnnecessaryAnonymousClass:OFF',
'-Xep:BoxedPrimitiveConstructor:OFF',
'-Xep:ArgumentSelectionDefectChecker:OFF',
'-Xep:StringSplitter:OFF',
'-Xep:MixedMutabilityReturnType:OFF',
'-Xep:EqualsUnsafeCast:OFF',
'-Xep:OperatorPrecedence:OFF',
'-Xep:HidingField:OFF',
'-Xep:ThreadPriorityCheck:OFF',
'-Xep:InlineFormatString:OFF',
'-Xep:EqualsUnsafeCast:OFF',
'-Xep:UnsynchronizedOverridesSynchronized:OFF',
'-Xep:OperatorPrecedence:OFF',
'-Xep:ArrayToString:OFF',
'-Xep:ClassCanBeStatic:OFF',
'-Xep:InvalidInlineTag:OFF',
'-Xep:EmptyCatch:OFF',
'-Xep:UnnecessaryParentheses:OFF',
'-Xep:AlmostJavadoc:OFF',
'-Xep:Finally:OFF',
'-Xep:ImmutableEnumChecker:OFF',
'-Xep:NonAtomicVolatileUpdate:OFF',
'-Xep:MutablePublicArray:OFF',
'-Xep:LockNotBeforeTry:OFF',
'-Xep:WaitNotInLoop:OFF',
'-Xep:UndefinedEquals:OFF',
'-Xep:JdkObsolete:OFF',
'-Xep:NarrowingCompoundAssignment:OFF',
'-Xep:InconsistentCapitalization:OFF',
'-Xep:IntLongMath:OFF',
'-Xep:SynchronizeOnNonFinalField:OFF',
'-Xep:ThreadLocalUsage:OFF',
'-Xep:ProtectedMembersInFinalClass:OFF',
'-Xep:BadImport:OFF',
'-Xep:InconsistentHashCode:OFF',
'-Xep:MissingOverride:OFF',
'-Xep:EqualsGetClass:OFF',
'-Xep:PublicConstructorForAbstractClass:OFF',
'-Xep:EscapedEntity:OFF',
'-Xep:ModifiedButNotUsed:OFF',
'-Xep:ReferenceEquality:OFF',
'-Xep:InvalidBlockTag:OFF',
'-Xep:MissingSummary:OFF',
'-Xep:UnusedVariable:OFF'
]
}
}
}
Expand Up @@ -523,6 +523,7 @@ public void clear() {
* @throws NullPointerException
* if the given map is <code>null</code>.
*/
@SuppressWarnings("ReferenceEquality")
Copy link
Contributor

Choose a reason for hiding this comment

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

This was the example you wanted to try out on how to suppress legitimate warnings of ReferenceEquality ( or any other warnings ) when we start enabling the checks ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. I wanted the pr to include an example of how this can be done.

public static <V> CharArrayMap<V> unmodifiableMap(CharArrayMap<V> map) {
if (map == null)
throw new NullPointerException("Given map is null");
Expand Down
2 changes: 1 addition & 1 deletion versions.lock
Expand Up @@ -22,7 +22,7 @@ com.github.virtuald:curvesapi:1.06 (1 constraints: db04f530)
com.github.zafarkhaja:java-semver:0.9.0 (1 constraints: 0b050636)
com.google.code.findbugs:annotations:3.0.1 (1 constraints: 0605fb35)
com.google.code.findbugs:jsr305:3.0.2 (2 constraints: cd195721)
com.google.errorprone:error_prone_annotations:2.1.3 (1 constraints: 180aebb4)
com.google.errorprone:error_prone_annotations:2.4.0 (2 constraints: 1f0fd486)
com.google.guava:guava:25.1-jre (1 constraints: 4a06b047)
com.google.j2objc:j2objc-annotations:1.1 (1 constraints: b609eba0)
com.google.protobuf:protobuf-java:3.11.0 (1 constraints: 3705383b)
Expand Down
1 change: 1 addition & 0 deletions versions.props
Expand Up @@ -8,6 +8,7 @@ com.fasterxml.jackson*:*=2.10.1
com.github.ben-manes.caffeine:caffeine=2.8.4
com.github.virtuald:curvesapi=1.06
com.github.zafarkhaja:java-semver=0.9.0
com.google.errorprone:*=2.4.0
com.google.guava:guava=25.1-jre
com.google.protobuf:protobuf-java=3.11.0
com.google.re2j:re2j=1.2
Expand Down