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

Enable error-prone static analysis checks #961

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c66f692
Enable error-prone static analysis checks
jbduncan Jul 18, 2017
cc48c98
Fix some compilation errors and add a missing comment to build.gradle
jbduncan Sep 29, 2017
a9efe7e
Upgrade errorprone to latest version
jbduncan Sep 29, 2017
34573c2
Update EnumSource to follow the advice given in http://errorprone.inf…
jbduncan Sep 29, 2017
387a662
Comment fixup
jbduncan Sep 30, 2017
0e770db
Comment fixup
jbduncan Sep 30, 2017
adbeeaa
Update to latest error-prone plugin version
jbduncan Oct 12, 2017
ed3f5ce
Externalise error-prone version
jbduncan Oct 13, 2017
630719b
Attempt to get error-prone working on JDK 9
jbduncan Oct 13, 2017
fd74fea
Move 'propdeps-maven' back to its original location (as per review)
jbduncan Oct 13, 2017
c66367d
Update to error-prone plugin v0.0.13
jbduncan Oct 13, 2017
3e75ef9
Just disable "literal name check" checker, as it will be removed in a
jbduncan Oct 14, 2017
95e235a
Remove custom error-prone tasks and use default error-prone plugin be…
jbduncan Oct 14, 2017
c08c189
Create custom error-prone tasks again (unfortunately they're rife wit…
jbduncan Oct 14, 2017
038e789
Attempt 1 to fix problems with `compile[Test]JavaWithErrorProne`
jbduncan Oct 14, 2017
8bca358
Push newest changes regarding `compile[Test]JavaWithErrorProne`.
jbduncan Oct 15, 2017
31add43
Make compileJavaWithErrorProne depend on compileKotlin & fix errors
jbduncan Dec 11, 2017
2beb69b
Apply Spotless (oops!)
jbduncan Dec 11, 2017
fe8c123
Attempt to fix problem with junit-platform-commons-java-9
jbduncan Dec 11, 2017
20bac2e
WIP
jbduncan Dec 11, 2017
bdec69c
WIP - update version, minor cleanup in some areas, and more mess in o…
jbduncan Jan 15, 2018
82767e8
Merge remote-tracking branch 'upstream/master' into error-prone
jbduncan Jan 15, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 154 additions & 39 deletions build.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import java.time.OffsetDateTime
import java.time.format.DateTimeFormatter
import net.ltgt.gradle.errorprone.ErrorProneToolChain

buildscript {
repositories {
Expand All @@ -10,6 +11,7 @@ buildscript {
dependencies {
classpath('com.diffplug.spotless:spotless-plugin-gradle:3.8.0')
classpath('com.github.ben-manes:gradle-versions-plugin:0.17.0')
classpath('net.ltgt.gradle:gradle-errorprone-plugin:0.0.13')
classpath('org.ajoberstar:gradle-git-publish:0.3.2')
classpath('org.ajoberstar:gradle-git:1.7.2')
classpath('org.junit.platform:junit-platform-gradle-plugin:1.0.1')
Expand Down Expand Up @@ -94,46 +96,9 @@ ext {
'junit-platform-surefire-provider',
'platform-tests'
]
}

allprojects { subproj ->

apply plugin: 'java-library'
if (Runtime.version().major() < 10) apply plugin: 'kotlin'
apply plugin: 'eclipse'
apply plugin: 'idea'
apply plugin: 'com.diffplug.gradle.spotless'
apply plugin: 'checkstyle'
apply plugin: 'com.github.ben-manes.versions' // gradle dependencyUpdates
apply from: "$rootDir/gradle/degraph.gradle"

repositories {
// mavenLocal()
mavenCentral()
maven { url 'https://oss.sonatype.org/content/repositories/snapshots' }
}

dependencies {
api("org.apiguardian:apiguardian-api:${apiGuardianVersion}")
}

tasks.withType(Test) { task ->
task.testLogging.exceptionFormat = 'full'
}

// Declare "javacRelease" as a global property to make it overridable by
// a sub-project.
ext.javacRelease = rootProject.javacRelease

tasks.withType(JavaCompile) {
sourceCompatibility = javacRelease // needed by clover
targetCompatibility = javacRelease // needed by clover/asm
options.compilerArgs += ['--release', javacRelease]
options.encoding = 'UTF-8'
}

// See: https://docs.oracle.com/javase/9/tools/javac.htm#JSWOR627
compileJava.options*.compilerArgs = [
compileJavaBaseArgs = [
'-Xlint:cast',
'-Xlint:classfile',
'-Xlint:deprecation',
Expand All @@ -156,7 +121,7 @@ allprojects { subproj ->
]

// See: https://docs.oracle.com/javase/9/tools/javac.htm#JSWOR627
compileTestJava.options*.compilerArgs = [
compileTestJavaBaseArgs = [
'-Xlint:cast',
'-Xlint:classfile',
'-Xlint:deprecation',
Expand All @@ -176,6 +141,51 @@ allprojects { subproj ->
'-Xlint:-options',
'-Xlint:-overrides'
]
}

allprojects { subproj ->

apply plugin: 'java-library'
if (Runtime.version().major() < 10) apply plugin: 'kotlin'
apply plugin: 'eclipse'
apply plugin: 'idea'
apply plugin: 'com.diffplug.gradle.spotless'
apply plugin: 'checkstyle'
apply plugin: 'com.github.ben-manes.versions' // gradle dependencyUpdates
apply from: "$rootDir/gradle/degraph.gradle"

repositories {
// mavenLocal()
mavenCentral()
maven { url 'https://oss.sonatype.org/content/repositories/snapshots' }
}

dependencies {
api("org.apiguardian:apiguardian-api:${apiGuardianVersion}")
}

tasks.withType(Test) { task ->
task.testLogging.exceptionFormat = 'full'
}

// Declare "javacRelease" as a global property to make it overridable by
// a sub-project.
ext.javacRelease = rootProject.javacRelease

tasks.withType(JavaCompile) {
sourceCompatibility = javacRelease // needed by clover
targetCompatibility = javacRelease // needed by clover/asm
options.compilerArgs += ['--release', javacRelease]
options.encoding = 'UTF-8'
}

// See: https://docs.oracle.com/javase/9/tools/javac.htm#JSWOR627
compileJava.options*.compilerArgs =
compileJavaBaseArgs + ['--release', rootProject.javacRelease]

// See: https://docs.oracle.com/javase/9/tools/javac.htm#JSWOR627
compileTestJava.options*.compilerArgs =
compileTestJavaBaseArgs + ['--release', rootProject.javacRelease]

compileTestJava {
options.encoding = 'UTF-8'
Expand Down Expand Up @@ -504,6 +514,111 @@ subprojects { subproj ->

}

if (
// error-prone doesn't work with Java 9 sources yet:
// https://github.com/google/error-prone/issues/448
// TODO: Try removing this check, since errorprone may support Java 9 now?
!subproj.name.endsWith('-java-9')
&& subproj.name != 'documentation') {

println(String.format("Currently setting up error-prone for subproject: %s", subproj.name))

apply plugin: 'net.ltgt.errorprone-base'

dependencies {
errorprone("com.google.errorprone:error_prone_core:${errorproneVersion}")
// TODO: JDK 9 fails to find any of the annotations from the package
// com.google.errorprone.annotations, even if they're applied to the JUnit 5 code base.
// Apply a select subset of them once they work properly with JDK 9.
compileOnly("com.google.errorprone:error_prone_annotations:${errorproneVersion}")
}

def errorproneBaseFlags = [
// enable experimental checks
'-XepAllDisabledChecksAsWarnings',

// generates too many false positives on constructors that call other constructors.
// TODO: see if constructors can be refactored more easily to not call each other.
'-Xep:ConstructorLeaksThis:OFF',

// we don't use @Nullable
'-Xep:FieldMissingNullable:OFF',

// adds a lot of churn for not a lot of gain
'-Xep:InconsistentOverloads:OFF',

// TODO: turning this check on and applying @com.google.errorprone.annotations.Immutable
// to immutable classes causes JDK 9 to fail to compile; investigate and consider
// turning it on when error-prone supports JDK 9
'-Xep:Immutable:OFF',

// adds a lot of churn for not a lot of gain
'-Xep:MethodCanBeStatic:OFF',

// we declare redundant `throws <general-exception>, <more-specific-exception>` for
// maintainability
'-Xep:RedundantThrows:OFF',

// we don't use @Nullable
'-Xep:ReturnMissingNullable:OFF',

// we do not use Guava, so we cannot import com.google.common.base.Splitter
'-Xep:StringSplitter:OFF',

// we target Java 8, not Android
'-Xep:StaticOrDefaultInterfaceMethod:OFF',

// we declare `throws <unchecked-exception>` for maintainability
'-Xep:ThrowsUncheckedException:OFF',

// we do not follow the Google Java Style Guide
'-Xep:UngroupedOverloads:OFF',

// TODO: turning this check on and applying @com.google.errorprone.annotations.Var to
// non-final variables causes JDK 9 to fail to compile; investigate and consider turning
// it on when error-prone supports JDK 9
'-Xep:Var:OFF'
]

def errorproneTestFlags = errorproneBaseFlags + [
'-Xep:ClassCanBeStatic:OFF',
'-Xep:ConstantField:OFF',
'-Xep:ConstructorInvokesOverridable:OFF',
'-Xep:EqualsHashCode:OFF',
'-Xep:MultipleTopLevelClasses:OFF',
'-Xep:PrivateConstructorForUtilityClass:OFF',
'-Xep:ReturnValueIgnored:OFF'
]

task compileJavaWithErrorProne(type: JavaCompile/*, dependsOn: compileKotlin*/) {
group = 'verification'
toolChain = ErrorProneToolChain.create(project)
classpath = compileJava.classpath + compileKotlin.classpath
//println("classpath = " + classpath.files)
source = compileJava.source + compileKotlin.source
//println("source = " + source.files)
options.compilerArgs =
compileJava.options.compilerArgs + compileJavaBaseArgs + errorproneBaseFlags
// println("compileJavaWithErrorProne.options.compilerArgs = $options.compilerArgs")
destinationDir = file("$buildDir/classes/error-prone-java/main")
}
check.dependsOn(compileJavaWithErrorProne)

task compileTestJavaWithErrorProne(type: JavaCompile/*, dependsOn: [compileKotlin, compileTestKotlin]*/) {
group = 'verification'
toolChain = ErrorProneToolChain.create(project)
classpath = compileTestJava.classpath + compileTestKotlin.classpath
//println("classpath = " + classpath.files)
source = compileTestJava.source + compileTestKotlin.source
//println("source = " + source.files)
options.compilerArgs +=
compileTestJava.options.compilerArgs + compileTestJavaBaseArgs + errorproneTestFlags
// println("compileTestJavaWithErrorProne.options.compilerArgs = $options.compilerArgs")
destinationDir = file("$buildDir/classes/error-prone-java/test")
}
check.dependsOn(compileTestJavaWithErrorProne)
}

spotless {
def headerFile = rootProject.file('src/spotless/' + licenseOf(project)['headerFile'])
def importOrderConfigFile = rootProject.file('src/eclipse/junit-eclipse.importorder')
Expand Down
1 change: 1 addition & 0 deletions gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ javacRelease = 8
apiGuardianVersion = 1.0.0
assertJVersion = 3.9.0
degraphVersion = 0.1.4
errorproneVersion = 2.2.0
junit4Version = 4.12
kotlinVersion = 1.2.10
log4jVersion = 2.10.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ static boolean floatsAreEqual(float value1, float value2, float delta) {
return floatsAreEqual(value1, value2) || Math.abs(value1 - value2) <= delta;
}

static boolean floatsAreEqual(float value1, float value2) {
return Float.floatToIntBits(value1) == Float.floatToIntBits(value2);
}

Copy link
Member

Choose a reason for hiding this comment

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

Why did you move this method?

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 moved it because, before I added "-Xep:UngroupedOverloads:OFF" to the build.gradle file, error-prone reported that methods like this one were not 'grouped' together with methods of the same name, potentially causing confusion for future maintainers.

I ultimately added "-Xep:UngroupedOverloads:OFF" to the build.gradle because the corresponding rule produced a lot of churn for little gain, but as I was reverting the changes I made initially to satisfy the rule, I thought that moving all the floatsAreEqual() methods together made a great deal of sense, so I decided to keep it.

If you want me to move it back to where it was, or to put this change in a new PR, please let me know!

Copy link
Member

Choose a reason for hiding this comment

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

You can leave it "as is" for the time being.

static void assertValidDelta(float delta) {
if (Float.isNaN(delta) || delta <= 0.0) {
failIllegalDelta(String.valueOf(delta));
Expand All @@ -131,10 +135,6 @@ static void assertValidDelta(double delta) {
}
}

static boolean floatsAreEqual(float value1, float value2) {
return Float.floatToIntBits(value1) == Float.floatToIntBits(value2);
}

static boolean doublesAreEqual(double value1, double value2, double delta) {
assertValidDelta(delta);
return doublesAreEqual(value1, value2) || Math.abs(value1 - value2) <= delta;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ private Assertions() {
* <p>See Javadoc for {@link #fail(String, Throwable)} for an explanation of
* this method's generic return type {@code V}.
*/
// we purposely return a V that has nothing to do with the parameters of this method; see
// the javadoc for details.
@SuppressWarnings("TypeParameterUnusedInFormals")
Copy link
Member

Choose a reason for hiding this comment

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

Where does this warning come from?

It looks non-standard to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed non-standard. It's an error-prone warning.

I added it here because I thought that there weren't enough false positives to justify disabling the corresponding error-prone rule "TypeParameterUnusedInFormals" globally. If you disagree, I'd be happy to disable "TypeParameterUnusedInFormals" by adding ""-Xep:TypeParameterUnusedInFormals:OFF" to the build.gradle file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm simply hesitant to introduce all of these @SuppressWarnings statements for non-standard warning labels since that will result in warnings about unknown warning labels in IDEs... which will annoy the hell out of me.

Is there not a more elegant way to achieve this without introducing @SuppressWarnings statements that use proprietary warning labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I don't think there is a more elegant way.

AFAICT, it's the only solution that the docs for that rule provides.

(The docs for all the other rules I've looked at in the past suggest the exact same workaround, IIRC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbrannen We have two options AFAICT:

  1. Accept the @SuppressWarnings("TypeParameterUnusedInFormals"), at the expense of your sanity. 😉
  2. Globally disable the rule.

I'm personally not a fan of disabling the rule, because according the bug patterns docs (type Ctrl+F and search for "TypeParameterUnusedInFormals"), it's a default non-experimental rule, which suggests to me that it's worth keeping on.

But on the other hand, I empathise with you on annoying warnings in IDEs, so if it really does drive you round the bend, then I'd be happy to disable it.

Copy link
Member

Choose a reason for hiding this comment

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

MMmmm.... ok.

Well, my sanity is not paramount here. 😇

So let's see how it goes once all of this is in place.

public static <V> V fail(String message) {
AssertionUtils.fail(message);
return null; // appeasing the compiler: this line will never be executed.
Expand All @@ -78,6 +81,9 @@ public static <V> V fail(String message) {
* Stream.of().map(entry -> fail("should not be called"));
* }</pre>
*/
// we purposely return a V that has nothing to do with the parameters of this method; see
// the javadoc for details.
@SuppressWarnings("TypeParameterUnusedInFormals")
public static <V> V fail(String message, Throwable cause) {
AssertionUtils.fail(message, cause);
return null; // appeasing the compiler: this line will never be executed.
Expand All @@ -89,6 +95,9 @@ public static <V> V fail(String message, Throwable cause) {
* <p>See Javadoc for {@link #fail(String, Throwable)} for an explanation of
* this method's generic return type {@code V}.
*/
// we purposely return a V that has nothing to do with the parameters of this method; see
// the javadoc for details.
@SuppressWarnings("TypeParameterUnusedInFormals")
public static <V> V fail(Throwable cause) {
AssertionUtils.fail(cause);
return null; // appeasing the compiler: this line will never be executed.
Expand All @@ -101,6 +110,9 @@ public static <V> V fail(Throwable cause) {
* <p>See Javadoc for {@link #fail(String, Throwable)} for an explanation of
* this method's generic return type {@code V}.
*/
// we purposely return a V that has nothing to do with the parameters of this method; see
// the javadoc for details.
@SuppressWarnings("TypeParameterUnusedInFormals")
public static <V> V fail(Supplier<String> messageSupplier) {
AssertionUtils.fail(messageSupplier);
return null; // appeasing the compiler: this line will never be executed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ void testAndRepeatedTest(LogRecordListener listener) {

// @formatter:off
assertThat(listener.stream()
.filter(logRecord -> logRecord.getLevel() == Level.WARNING)
.filter(logRecord -> logRecord.getLevel().equals(Level.WARNING))
.map(LogRecord::getMessage)
.filter(m -> m.matches("Possible configuration error: method .+ resulted in multiple TestDescriptors .+"))
.count()
Expand Down
Loading