Upgrade error-prone to 2.16 (#2432)

Upgrade error-prone to 2.16
palantir · Oct 20, 2022 · 84f9f3e · 84f9f3e
1 parent f38f713
commit 84f9f3e
Show file tree

Hide file tree

Showing 9 changed files with 40 additions and 93 deletions.
diff --git a/...ror-prone/src/main/java/com/palantir/baseline/errorprone/ClassInitializationDeadlock.java b/...ror-prone/src/main/java/com/palantir/baseline/errorprone/ClassInitializationDeadlock.java
@@ -171,7 +171,7 @@ public Void visitCompoundAssignment(CompoundAssignmentTree node, VisitorState st
         private boolean handleAssignment(ExpressionTree variable, VisitorState state) {
             // Matches if a subtype static field assignment occurs
             Symbol symbol = ASTHelpers.getSymbol(variable);
-            if (symbol != null && symbol.isStatic() && symbol instanceof VarSymbol) {
+            if (symbol != null && ASTHelpers.isStatic(symbol) && symbol instanceof VarSymbol) {
                 VarSymbol varSymbol = (VarSymbol) symbol;
                 if (isSubtype(varSymbol.owner.type, baseType, state)) {
                     state.reportMatch(describeMatch(variable));
@@ -184,7 +184,7 @@ private boolean handleAssignment(ExpressionTree variable, VisitorState state) {
         @Override
         public Void visitMemberSelect(MemberSelectTree node, VisitorState state) {
             Symbol symbol = ASTHelpers.getSymbol(node);
-            if (symbol != null && symbol.isStatic() && symbol instanceof VarSymbol) {
+            if (symbol != null && ASTHelpers.isStatic(symbol) && symbol instanceof VarSymbol) {
                 VarSymbol varSymbol = (VarSymbol) symbol;
                 // Constant values may be accessed without forcing initialization
                 if (varSymbol.getConstValue() == null
@@ -231,7 +231,7 @@ private static boolean canBeExternallyInitialized(Symbol symbol) {
             case FIELD:
                 // fall through
             case METHOD:
-                if (symbol.isStatic() && !symbol.isPrivate()) {
+                if (ASTHelpers.isStatic(symbol) && !symbol.isPrivate()) {
                     return true;
                 }
                 break;

diff --git a/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqResultStreamLeak.java b/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqResultStreamLeak.java
@@ -23,11 +23,12 @@
 import com.google.errorprone.bugpatterns.StreamResourceLeak;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.Matchers;
 import com.google.errorprone.matchers.method.MethodMatchers;
 import com.google.errorprone.suppliers.Supplier;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ExpressionTree;
-import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
 import com.sun.tools.javac.code.Type;
 
 @AutoService(BugChecker.class)
@@ -39,32 +40,28 @@
                 + " try-with-resources. Not doing so can result in leaked database resources (such as connections"
                 + " or cursors) in code paths that throw an exception or fail to call #close().")
 public final class JooqResultStreamLeak extends StreamResourceLeak {
-    private static final Matcher<ExpressionTree> MATCHER = MethodMatchers.instanceMethod()
-            .onDescendantOf("org.jooq.ResultQuery")
-            .withAnyName();
+    private static final Matcher<ExpressionTree> MATCHER = Matchers.allOf(
+            MethodMatchers.instanceMethod()
+                    .onDescendantOf("org.jooq.ResultQuery")
+                    .withAnyName(),
+            JooqResultStreamLeak::shouldBeAutoClosed);
 
     private static final Supplier<Type> JOOQ_QUERY_PART =
             VisitorState.memoize(state -> state.getTypeFromString("org.jooq.QueryPart"));
 
-    @Override
-    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
-        if (!MATCHER.matches(tree, state)) {
-            return Description.NO_MATCH;
-        }
-
-        if (!shouldBeAutoClosed(tree, state)) {
-            return Description.NO_MATCH;
-        }
+    private static final Supplier<Type> AUTO_CLOSEABLE =
+            VisitorState.memoize(state -> state.getTypeFromString(AutoCloseable.class.getName()));
 
-        return matchNewClassOrMethodInvocation(tree, state, findingPerSite());
+    @Override
+    public Description matchMethod(MethodTree tree, VisitorState state) {
+        return scanEntireMethodFor(MATCHER, tree, state);
     }
 
-    private static boolean shouldBeAutoClosed(MethodInvocationTree tree, VisitorState state) {
+    private static boolean shouldBeAutoClosed(ExpressionTree tree, VisitorState state) {
         Type returnType = ASTHelpers.getReturnType(tree);
 
         // Most auto-closeable returns should be auto-closed.
-        boolean isAutoCloseable =
-                ASTHelpers.isSubtype(returnType, state.getTypeFromString(AutoCloseable.class.getName()), state);
+        boolean isAutoCloseable = ASTHelpers.isSubtype(returnType, AUTO_CLOSEABLE.get(state), state);
 
         // QueryParts can hold resources but usually don't, so auto-tripping and trying to fix on things
         // like SelectConditionStep is unnecessary.

diff --git a/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java b/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java
@@ -41,6 +41,7 @@
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.isStatic;
 import static com.google.errorprone.util.ASTHelpers.isSubtype;
 import static com.google.errorprone.util.SideEffectAnalysis.hasSideEffect;
 import static com.sun.source.tree.Tree.Kind.POSTFIX_DECREMENT;
@@ -449,8 +450,7 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
                 if (hasSideEffect(initializer) && TOP_LEVEL_EXPRESSIONS.contains(initializer.getKind())) {
                     if (varKind == ElementKind.FIELD) {
                         String newContent = String.format(
-                                "%s{ %s; }",
-                                varSymbol.isStatic() ? "static " : "", state.getSourceForNode(initializer));
+                                "%s{ %s; }", isStatic(varSymbol) ? "static " : "", state.getSourceForNode(initializer));
                         fix.merge(SuggestedFixes.replaceIncludingComments(usagePath, newContent, state));
                     } else {
                         fix.replace(statement, String.format("%s;", state.getSourceForNode(initializer)));

diff --git a/...ine-error-prone/src/main/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiter.java b/...ine-error-prone/src/main/java/com/palantir/baseline/errorprone/ZeroWarmupRateLimiter.java
@@ -81,7 +81,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
      */
     private static boolean isDurationZero(ExpressionTree expressionTree, VisitorState state) {
         Symbol symbol = ASTHelpers.getSymbol(expressionTree);
-        if (symbol != null && symbol.isStatic() && symbol instanceof VarSymbol) {
+        if (symbol != null && ASTHelpers.isStatic(symbol) && symbol instanceof VarSymbol) {
             VarSymbol varSymbol = (VarSymbol) symbol;
             return varSymbol.name.contentEquals("ZERO")
                     && state.getTypes()

diff --git a/changelog/@unreleased/pr-2432.v2.yml b/changelog/@unreleased/pr-2432.v2.yml
@@ -0,0 +1,6 @@
+type: improvement
+improvement:
+  description: Upgrade error-prone to 2.16, removing support for compilation with
+    a jdk-15 target
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2432
diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java
@@ -16,12 +16,10 @@
 
 package com.palantir.baseline.plugins;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import java.util.Optional;
 import net.ltgt.gradle.errorprone.ErrorProneOptions;
 import org.gradle.api.Action;
-import org.gradle.api.JavaVersion;
 import org.gradle.api.Plugin;
 import org.gradle.api.Project;
 import org.gradle.api.artifacts.Configuration;
@@ -38,14 +36,6 @@ public final class BaselineNullAway implements Plugin<Project> {
     /** We may add a gradle extension in a future release allowing custom additional packages. */
     private static final ImmutableSet<String> DEFAULT_ANNOTATED_PACKAGES = ImmutableSet.of("com.palantir");
 
-    private static final ImmutableList<String> NON_15_DEPS = ImmutableList.of(
-            "com.uber.nullaway:nullaway:0.10.1",
-            // align 'org.checkerframework' dependency versions on
-            // the current latest version.
-            "org.checkerframework:dataflow-errorprone:3.25.0",
-            "org.checkerframework:dataflow-nullaway:3.25.0",
-            "org.checkerframework:checker-qual:3.25.0");
-
     @Override
     public void apply(Project project) {
         project.getPluginManager().withPlugin("com.palantir.baseline-error-prone", _unused0 -> {
@@ -88,7 +78,6 @@ public void execute(ErrorProneOptions options) {
                 }
             }
         });
-        newerNullAwayInNonJdk15Projects(project);
     }
 
     private static void configureErrorProneOptions(Project proj, Action<ErrorProneOptions> action) {
@@ -106,38 +95,4 @@ public void execute(JavaCompile javaCompile) {
             }
         });
     }
-
-    // Workaround for nullaway bugs in the last release to support jdk-15. Projects that don't use jdk-15
-    // resolve a newer nullaway version with bug-fixes. This may be deleted after we've rolled everything
-    // off jdk-15 MTS.
-    private static void newerNullAwayInNonJdk15Projects(Project project) {
-        project.getConfigurations()
-                .matching(new Spec<Configuration>() {
-                    @Override
-                    public boolean isSatisfiedBy(Configuration config) {
-                        return "errorprone".equals(config.getName());
-                    }
-                })
-                .configureEach(new Action<Configuration>() {
-                    @Override
-                    public void execute(Configuration _files) {
-                        for (String dep : NON_15_DEPS) {
-                            project.getDependencies().addProvider("errorprone", project.provider(() -> {
-                                // Cannot upgrade dependencies in this case because newer nullaway/checkerframework
-                                // are not compatible with java 15, but fully support jdk11 and jdk17.
-                                return anyProjectUsesJava15(project) ? null : dep;
-                            }));
-                        }
-                    }
-                });
-    }
-
-    private static boolean anyProjectUsesJava15(Project proj) {
-        return proj.getAllprojects().stream()
-                .anyMatch(project -> ImmutableList.copyOf(project.getTasks().withType(JavaCompile.class)).stream()
-                        .anyMatch(comp -> {
-                            JavaVersion javaVersion = JavaVersion.toVersion(comp.getTargetCompatibility());
-                            return javaVersion == JavaVersion.VERSION_15;
-                        }));
-    }
 }
diff --git a/...aseline-java/src/test/groovy/com/palantir/baseline/BaselineNullAwayIntegrationTest.groovy b/...aseline-java/src/test/groovy/com/palantir/baseline/BaselineNullAwayIntegrationTest.groovy
@@ -115,15 +115,6 @@ class BaselineNullAwayIntegrationTest extends IntegrationSpec {
         runTasksSuccessfully('compileJava')
     }
 
-    def 'compileJava succeeds when null-away finds no errors on jdk15'() {
-        when:
-        buildFile << standardBuildFile.replace('libraryTarget = 17', 'libraryTarget = 15')
-        writeJavaSourceFile(validJavaFile)
-
-        then:
-        runTasksSuccessfully('compileJava')
-    }
-
     def 'compileJava succeeds when null-away finds no errors on jdk11'() {
         when:
         buildFile << standardBuildFile.replace('libraryTarget = 17', 'libraryTarget = 11')

diff --git a/versions.lock b/versions.lock
@@ -14,13 +14,13 @@ com.google.auto.service:auto-service-annotations:1.0.1 (3 constraints: 2a3379a0)
 com.google.auto.value:auto-value:1.7.4 (1 constraints: 1f1221fb)
 com.google.auto.value:auto-value-annotations:1.9 (3 constraints: 802d5ac8)
 com.google.code.findbugs:jsr305:3.0.2 (6 constraints: 66626968)
-com.google.errorprone:error_prone_annotation:2.15.0 (3 constraints: c43898ed)
-com.google.errorprone:error_prone_annotations:2.15.0 (12 constraints: 85bda891)
-com.google.errorprone:error_prone_check_api:2.15.0 (2 constraints: b22516ff)
-com.google.errorprone:error_prone_core:2.15.0 (2 constraints: 1118d166)
-com.google.errorprone:error_prone_refaster:2.15.0 (1 constraints: 3a053e3b)
-com.google.errorprone:error_prone_test_helpers:2.15.0 (1 constraints: 3a053e3b)
-com.google.errorprone:error_prone_type_annotations:2.15.0 (1 constraints: 251151c9)
+com.google.errorprone:error_prone_annotation:2.16 (3 constraints: ad370ed6)
+com.google.errorprone:error_prone_annotations:2.16 (12 constraints: 6ebc6da5)
+com.google.errorprone:error_prone_check_api:2.16 (2 constraints: f8242c7e)
+com.google.errorprone:error_prone_core:2.16 (2 constraints: 5717731a)
+com.google.errorprone:error_prone_refaster:2.16 (1 constraints: dd04fb30)
+com.google.errorprone:error_prone_test_helpers:2.16 (1 constraints: dd04fb30)
+com.google.errorprone:error_prone_type_annotations:2.16 (1 constraints: c81038a7)
 com.google.googlejavaformat:google-java-format:1.13.0 (1 constraints: 8b149d75)
 com.google.guava:failureaccess:1.0.1 (1 constraints: 140ae1b4)
 com.google.guava:guava:31.1-jre (14 constraints: d6e9749b)
@@ -40,7 +40,7 @@ com.palantir.javaformat:palantir-java-format-spi:1.1.0 (1 constraints: 711560be)
 com.palantir.safe-logging:preconditions:3.1.0 (6 constraints: 445723f0)
 com.palantir.safe-logging:safe-logging:3.1.0 (8 constraints: 587745d3)
 com.palantir.tritium:tritium-registry:0.57.0 (1 constraints: 3e05483b)
-com.uber.nullaway:nullaway:0.9.9 (1 constraints: 14050f36)
+com.uber.nullaway:nullaway:0.10.2 (1 constraints: 3505253b)
 commons-io:commons-io:2.11.0 (1 constraints: 57119bcb)
 commons-lang:commons-lang:2.6 (1 constraints: ac04232c)
 io.dropwizard.metrics:metrics-core:4.1.1 (1 constraints: 901088a5)
@@ -69,9 +69,9 @@ org.apache.maven.resolver:maven-resolver-util:1.6.3 (3 constraints: 5930fd65)
 org.apache.maven.shared:maven-dependency-analyzer:1.13.0 (1 constraints: 3705323b)
 org.apache.maven.shared:maven-shared-utils:3.3.4 (1 constraints: e60b61f3)
 org.assertj:assertj-core:3.23.1 (3 constraints: e42af49e)
-org.checkerframework:checker-qual:3.23.0 (3 constraints: 08256088)
-org.checkerframework:dataflow-errorprone:3.23.0 (4 constraints: fa3d685c)
-org.checkerframework:dataflow-nullaway:3.23.0 (2 constraints: 671166f1)
+org.checkerframework:checker-qual:3.25.0 (3 constraints: 08256088)
+org.checkerframework:dataflow-errorprone:3.25.0 (4 constraints: 023e005f)
+org.checkerframework:dataflow-nullaway:3.25.0 (2 constraints: 6911b8f1)
 org.codehaus.groovy:groovy:3.0.10 (3 constraints: e32879d6)
 org.codehaus.groovy:groovy-xml:3.0.10 (1 constraints: 791161da)
 org.codehaus.plexus:plexus-cipher:2.0 (1 constraints: 641174c7)

diff --git a/versions.props b/versions.props
@@ -5,14 +5,17 @@ com.palantir.safe-logging:* = 3.1.0
 commons-lang:commons-lang = 2.6
 org.apache.maven.shared:maven-dependency-analyzer = 1.13.0
 org.apache.maven:maven-core = 3.8.5
+org.checkerframework:* = 3.25.0
 org.inferred:freebuilder = 1.14.6
 org.jooq:jooq = 3.17.4
 org.slf4j:* = 1.7.36
 org.immutables:* = 2.8.8
 org.ow2.asm:asm = 9.4
+com.google.errorprone:error_prone_* = 2.16
 com.googlecode.java-diff-utils:diffutils = 1.3.0
 com.puppycrawl.tools:checkstyle = 10.3.4
 com.palantir.gradle.utils:* = 0.1.0
+com.uber.nullaway:nullaway = 0.10.2
 
 # test deps
 com.fasterxml.jackson.*:* = 2.13.4
@@ -42,11 +45,6 @@ org.mockito:* = 4.8.0
 com.palantir.javaformat:gradle-palantir-java-format = 1.1.0
 # Newer spotless versions have issues resolving dependencies at configuration time
 com.diffplug.spotless:spotless-plugin-gradle = 6.6.0
-# Newer releases of checkerframework do not support jdk15. These dependencies may
-# be moved back to hte upgradable block once we've removed uses of jdk-15 MTS
-com.google.errorprone:error_prone_* = 2.15.0
-com.uber.nullaway:nullaway = 0.9.9
-org.checkerframework:* = 3.23.0
 # Groovy versions must be compatible with gradle
 org.codehaus.groovy:* = 3.0.10
 # dependency-upgrader:ON