Migrate baseline error-prone checks to use jdk13 compatible qualifiers (

#1110) Migrate baseline error-prone checks to use jdk13 compatible qualifiers
palantir · Dec 10, 2019 · 690a9ec · 690a9ec
1 parent b35adda
commit 690a9ec
Show file tree

Hide file tree

Showing 16 changed files with 73 additions and 29 deletions.
diff --git a/...rror-prone/src/main/java/com/palantir/baseline/errorprone/AssertjPrimitiveComparison.java b/...rror-prone/src/main/java/com/palantir/baseline/errorprone/AssertjPrimitiveComparison.java
@@ -21,7 +21,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.Matchers;
@@ -119,7 +118,7 @@ private static String getExpressionSource(
         if (strict ? types.isSameType(resultType, targetType) : types.isAssignable(resultType, targetType)) {
             return source;
         }
-        String cast = '(' + SuggestedFixes.prettyType(state, null, targetType) + ") ";
+        String cast = '(' + MoreSuggestedFixes.prettyType(state, null, targetType) + ") ";
         if (ASTHelpers.requiresParentheses(expression, state)) {
             return cast + '(' + source + ')';
         }

diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchSpecificity.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchSpecificity.java
@@ -23,7 +23,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.Matchers;
@@ -131,7 +130,7 @@ public Description matchTry(TryTree tree, VisitorState state) {
                         catchTree.accept(new ImpossibleConditionScanner(
                                 fix, replacements, parameterName), state);
                         fix.replace(catchTypeTree, replacements.stream()
-                                .map(type -> SuggestedFixes.prettyType(state, fix, type))
+                                .map(type -> MoreSuggestedFixes.prettyType(state, fix, type))
                                 .collect(Collectors.joining(" | ")));
                     }
                     state.reportMatch(buildDescription(catchTree)

diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreASTHelpers.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreASTHelpers.java
@@ -19,7 +19,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.VisitorState;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.CatchTree;
 import com.sun.source.tree.ClassTree;
@@ -78,7 +77,7 @@ static ImmutableList<Type> flattenTypesForAssignment(ImmutableList<Type> input,
                         type != item
                                 && state.getTypes().isSubtype(type, item)))
                 // Sort by pretty name
-                .sorted(Comparator.comparing(type -> SuggestedFixes.prettyType(state, null, type)))
+                .sorted(Comparator.comparing(type -> MoreSuggestedFixes.prettyType(state, null, type)))
                 .collect(ImmutableList.toImmutableList());
     }
 

diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreSuggestedFixes.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreSuggestedFixes.java
@@ -18,12 +18,15 @@
 
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.Tree;
+import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.tree.JCTree;
 import java.util.stream.Collectors;
+import javax.annotation.Nullable;
 
 /**
  * Additional utility functionality for {@link SuggestedFix} objects.
@@ -64,5 +67,50 @@ static SuggestedFix renameInvocationRetainingTypeArguments(
         return fix.replace(startPos, endPos, extra + newMethodName).build();
     }
 
+    /**
+     * Identical to {@link SuggestedFixes#qualifyType(VisitorState, SuggestedFix.Builder, String)} unless the
+     * compiling JVM is not supported by error-prone (JDK13) in which case a fallback is attempted.
+     */
+    static String qualifyType(VisitorState state, SuggestedFix.Builder fix, String typeName) {
+        try {
+            return SuggestedFixes.qualifyType(state, fix, typeName);
+        } catch (LinkageError e) {
+            // Work around https://github.com/google/error-prone/issues/1432
+            // by avoiding the findIdent function. It's possible this may result
+            // in colliding imports when classes have the same simple name, but
+            // the output is correct in most cases, in the failures are easy for
+            // humans to fix.
+            for (int startOfClass = typeName.indexOf('.');
+                    startOfClass > 0;
+                    startOfClass = typeName.indexOf('.', startOfClass + 1)) {
+                int endOfClass = typeName.indexOf('.', startOfClass + 1);
+                if (endOfClass < 0) {
+                    endOfClass = typeName.length();
+                }
+                if (!Character.isUpperCase(typeName.charAt(startOfClass + 1))) {
+                    continue;
+                }
+                String className = typeName.substring(startOfClass + 1);
+                fix.addImport(typeName.substring(0, endOfClass));
+                return className;
+            }
+            return typeName;
+        }
+    }
+
+    /**
+     * Identical to {@link SuggestedFixes#prettyType(VisitorState, SuggestedFix.Builder, Type)} unless the
+     * compiling JVM is not supported by error-prone (JDK13) in which case a fallback is attempted.
+     */
+    static String prettyType(@Nullable VisitorState state, @Nullable SuggestedFix.Builder fix, Type type) {
+        try {
+            return SuggestedFixes.prettyType(state, fix, type);
+        } catch (LinkageError e) {
+            // Work around https://github.com/google/error-prone/issues/1432
+            // by using a path which cannot add imports, this does not throw on jdk13.
+            return SuggestedFixes.prettyType(null, null, type);
+        }
+    }
+
     private MoreSuggestedFixes() {}
 }
diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java
@@ -28,7 +28,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.Matchers;
@@ -352,7 +351,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
                     fix.replace(tree, assertThat
                             + replacement.orElseGet(() ->
                             ".is(new "
-                            + SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
+                            + MoreSuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
                             + "<>("
                             + argSource(tree, state, 1) + "))")));
         }
@@ -363,7 +362,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
                     fix.replace(tree, assertThat
                             + ".describedAs(" + argSource(tree, state, 0) + ")"
                             + replacement.orElseGet(() -> ".is(new "
-                            + SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
+                            + MoreSuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
                             + "<>(" + argSource(tree, state, 2) + "))")));
         }
         if (ASSERT_EQUALS_CATCHALL.matches(tree, state)) {
@@ -465,7 +464,7 @@ private Description withAssertThat(
         String actualArgumentString = argSource(tree, state, actualIndex);
         ExpressionTree actualArgument = tree.getArguments().get(actualIndex);
         if (isIterableMap(actualArgument, state)) {
-            String qualifiedMap = SuggestedFixes.prettyType(
+            String qualifiedMap = MoreSuggestedFixes.prettyType(
                     state,
                     fix,
                     state.getTypes().asSuper(
@@ -486,7 +485,7 @@ private static String qualifyAssertThat(SuggestedFix.Builder fix, VisitorState s
                     .addStaticImport("org.assertj.core.api.Assertions.assertThat");
             return "assertThat";
         } else {
-            return SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.Assertions.assertThat");
+            return MoreSuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.Assertions.assertThat");
         }
     }
 

diff --git a/...r-prone/src/main/java/com/palantir/baseline/errorprone/PreferBuiltInConcurrentKeySet.java b/...r-prone/src/main/java/com/palantir/baseline/errorprone/PreferBuiltInConcurrentKeySet.java
@@ -21,7 +21,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.method.MethodMatchers;
@@ -53,7 +52,7 @@ public final class PreferBuiltInConcurrentKeySet extends BugChecker implements B
     public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
         if (MATCHER.matches(tree, state)) {
             SuggestedFix.Builder fix = SuggestedFix.builder();
-            String qualifiedType = SuggestedFixes.qualifyType(state, fix, "java.util.concurrent.ConcurrentHashMap");
+            String qualifiedType = MoreSuggestedFixes.qualifyType(state, fix, "java.util.concurrent.ConcurrentHashMap");
             return buildDescription(tree)
                     .setMessage(ERROR_MESSAGE)
                     .addFix(fix.replace(tree.getMethodSelect(), qualifiedType + ".newKeySet").build())

diff --git a/...or-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/...or-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java
@@ -23,7 +23,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.method.MethodMatchers;
@@ -280,7 +279,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
         }
 
         SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
-        String collectionType = SuggestedFixes.qualifyType(state, fixBuilder, collectionClass.getName());
+        String collectionType = MoreSuggestedFixes.qualifyType(state, fixBuilder, collectionClass.getName());
         String typeArgs = tree.getTypeArguments()
                 .stream()
                 .map(state::getSourceForNode)

diff --git a/...error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java b/...error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java
@@ -22,7 +22,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.method.MethodMatchers;
@@ -64,11 +63,12 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
                 SuggestedFix.Builder fix = SuggestedFix.builder();
                 if (LIST_MATCHER.matches(args.get(0), state)) {
                     // Fail on any 'Iterables.transform(List, Function) invocation
-                    qualifiedType = SuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Lists");
+                    qualifiedType = MoreSuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Lists");
                     errorMessage = "Prefer Lists.transform";
                 } else {
                     // Fail on any 'Iterables.transform(Collection, Function) invocation
-                    qualifiedType = SuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Collections2");
+                    qualifiedType = MoreSuggestedFixes.qualifyType(
+                            state, fix, "com.google.common.collect.Collections2");
                     errorMessage = "Prefer Collections2.transform";
                 }
                 String method = qualifiedType + ".transform";

diff --git a/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferListsPartition.java b/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferListsPartition.java
@@ -22,7 +22,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.method.MethodMatchers;
@@ -67,7 +66,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
             if (LIST_MATCHER.matches(args.get(0), state)) {
                 // Fail on any 'Iterables.partition(List, int) invocation
                 SuggestedFix.Builder fix = SuggestedFix.builder();
-                String qualifiedType = SuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Lists");
+                String qualifiedType = MoreSuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Lists");
                 String method = qualifiedType + ".partition";
                 return buildDescription(tree)
                         .setMessage(ERROR_MESSAGE)

diff --git a/...-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLoggingPreconditions.java b/...-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLoggingPreconditions.java
@@ -22,7 +22,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
@@ -96,7 +95,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
         }
 
         SuggestedFix.Builder fix = SuggestedFix.builder();
-        String logSafeQualifiedClassName = SuggestedFixes.qualifyType(state, fix, "com.palantir.logsafe.Preconditions");
+        String logSafeQualifiedClassName = MoreSuggestedFixes.qualifyType(
+                state, fix, "com.palantir.logsafe.Preconditions");
         String logSafeMethodName = getLogSafeMethodName(ASTHelpers.getSymbol(tree));
         String replacement = String.format("%s.%s", logSafeQualifiedClassName, logSafeMethodName);
 

diff --git a/...ne-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java b/...ne-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java
@@ -145,7 +145,7 @@ private static Optional<SuggestedFix> replaceWithStatic(
         }
         MemberSelectTree memberSelectTree = (MemberSelectTree) methodSelect;
         SuggestedFix.Builder fix = SuggestedFix.builder();
-        String qualifiedReference = SuggestedFixes.qualifyType(state, fix, fullyQualifiedReplacement);
+        String qualifiedReference = MoreSuggestedFixes.qualifyType(state, fix, fullyQualifiedReplacement);
         CharSequence args = sourceCode.subSequence(
                 state.getEndPosition(methodSelect) + 1,
                 state.getEndPosition(lastItem(tree.getArguments())));

diff --git a/...rone/src/main/java/com/palantir/baseline/errorprone/StrictCollectionIncompatibleType.java b/...rone/src/main/java/com/palantir/baseline/errorprone/StrictCollectionIncompatibleType.java
@@ -299,7 +299,7 @@ private static boolean isSupportedFunctionalInterface(Type type, VisitorState st
      * state and builder, it doesn't add relevant imports.
      */
     private static String prettyType(Type type) {
-        return SuggestedFixes.prettyType(null, null, type);
+        return MoreSuggestedFixes.prettyType(null, null, type);
     }
 
     private interface IncompatibleTypeMatcher {

diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java
@@ -21,7 +21,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
@@ -86,7 +85,7 @@ private static Optional<SuggestedFix> generateFix(NewClassTree newClassTree, Vis
         List<? extends ExpressionTree> arguments = newClassTree.getArguments();
         if (arguments.isEmpty()) {
             SuggestedFix.Builder fix = SuggestedFix.builder();
-            String qualifiedName = SuggestedFixes.qualifyType(state, fix, IllegalStateException.class.getName());
+            String qualifiedName = MoreSuggestedFixes.qualifyType(state, fix, IllegalStateException.class.getName());
             return Optional.of(fix.replace(newClassTree.getIdentifier(), qualifiedName).build());
         }
         ExpressionTree firstArgument = arguments.get(0);
@@ -95,7 +94,7 @@ private static Optional<SuggestedFix> generateFix(NewClassTree newClassTree, Vis
                 state.getTypeFromString(String.class.getName()),
                 state)) {
             SuggestedFix.Builder fix = SuggestedFix.builder();
-            String qualifiedName = SuggestedFixes.qualifyType(state, fix,
+            String qualifiedName = MoreSuggestedFixes.qualifyType(state, fix,
                     compileTimeConstExpressionMatcher.matches(firstArgument, state)
                     ? "com.palantir.logsafe.exceptions.SafeIllegalStateException"
                     : IllegalStateException.class.getName());

diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowSpecificity.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowSpecificity.java
@@ -90,7 +90,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
         SuggestedFix.Builder fix = SuggestedFix.builder();
         return buildDescription(throwsExpression)
                 .addFix(fix.replace(throwsExpression, checkedExceptions.stream()
-                        .map(checkedException -> SuggestedFixes.prettyType(state, fix, checkedException))
+                        .map(checkedException -> MoreSuggestedFixes.prettyType(state, fix, checkedException))
                         .collect(Collectors.joining(", ")))
                         .build())
                 .build();

diff --git a/changelog/@unreleased/pr-1110.v2.yml b/changelog/@unreleased/pr-1110.v2.yml
@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: Migrate baseline error-prone checks to use jdk13 compatible qualifiers
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/1110
diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java
@@ -212,7 +212,6 @@ private static void configureErrorProneOptions(
             // Errorprone 2.3.4 isn't officially compatible with Java13 either
             // https://github.com/google/error-prone/issues/1106
             errorProneOptions.check("TypeParameterUnusedInFormals", CheckSeverity.OFF);
-            errorProneOptions.check("PreferCollectionConstructors", CheckSeverity.OFF);
         }
 
         if (javaCompile.equals(compileRefaster)) {