Have ErrorProneRuntimeClasspath ignore non-public types

PicnicSupermarket · Jan 13, 2024 · c7fc8a9 · c7fc8a9
1 parent dff67fe
commit c7fc8a9
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 19 deletions.
diff --git a/...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java b/...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java
@@ -9,6 +9,7 @@
 import com.sun.tools.javac.code.Symbol.CompletionFailure;
 import com.sun.tools.javac.code.Symbol.ModuleSymbol;
 import com.sun.tools.javac.code.Symtab;
+import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.util.Name;
 
 /**
@@ -70,12 +71,12 @@ public boolean isIntroductionAllowed(VisitorState state) {
   /**
    * Tells whether the given fully qualified type is available on the current class path.
    *
-   * @param className The type of interest.
+   * @param typeName The type of interest.
    * @param state The context under consideration.
    * @return {@code true} iff it is okay to assume or create a dependency on this type.
    */
-  public static boolean canIntroduceUsage(String className, VisitorState state) {
-    return shouldIgnoreClasspath(state) || isKnownClass(className, state);
+  public static boolean canIntroduceUsage(String typeName, VisitorState state) {
+    return shouldIgnoreClasspath(state) || isKnownClass(typeName, state);
   }
 
   /**
@@ -84,22 +85,26 @@ public static boolean canIntroduceUsage(String className, VisitorState state) {
    * <p>The {@link VisitorState}'s symbol table is consulted first. If the type has not yet been
    * loaded, then an attempt is made to do so.
    */
-  private static boolean isKnownClass(String className, VisitorState state) {
-    return state.getTypeFromString(className) != null || canLoadClass(className, state);
+  private static boolean isKnownClass(String typeName, VisitorState state) {
+    return isPublicClassInSymbolTable(typeName, state) || canLoadPublicClass(typeName, state);
   }
 
-  private static boolean canLoadClass(String className, VisitorState state) {
+  private static boolean isPublicClassInSymbolTable(String typeName, VisitorState state) {
+    Type type = state.getTypeFromString(typeName);
+    return type != null && type.tsym.isPublic();
+  }
+
+  private static boolean canLoadPublicClass(String typeName, VisitorState state) {
     ClassFinder classFinder = ClassFinder.instance(state.context);
     Symtab symtab = state.getSymtab();
     // XXX: Drop support for targeting Java 8 once the oldest supported JDK drops such support.
     ModuleSymbol module =
         Source.instance(state.context).compareTo(Source.JDK9) < 0
             ? symtab.noModule
             : symtab.unnamedModule;
-    Name binaryName = state.binaryNameFromClassname(className);
+    Name binaryName = state.binaryNameFromClassname(typeName);
     try {
-      classFinder.loadClass(module, binaryName);
-      return true;
+      return classFinder.loadClass(module, binaryName).isPublic();
     } catch (
         @SuppressWarnings("java:S1166" /* Not exceptional. */)
         CompletionFailure e) {

diff --git a/...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java b/...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java
@@ -47,6 +47,7 @@ void identification() {
             "    m(\"com.google.errorprone.NonExistent\");",
             "    m(\"com.google.common.NonExistent.toString\");",
             "    m(\"java.lang.NonExistent\");",
+            "    m(\"com.google.common.collect.ImmutableEnumSet\");",
             "    // BUG: Diagnostic matches: USE_CLASS_REFERENCE",
             "    m(\"com.google.errorprone.BugPattern\");",
             "    // BUG: Diagnostic matches: USE_CLASS_REFERENCE",

diff --git a/...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java b/...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java
@@ -4,6 +4,7 @@
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.CompilationTestHelper;
 import com.google.errorprone.VisitorState;
@@ -20,7 +21,7 @@
 final class ThirdPartyLibraryTest {
   @Test
   void isIntroductionAllowed() {
-    CompilationTestHelper.newInstance(TestChecker.class, getClass())
+    CompilationTestHelper.newInstance(IsIntroductionAllowedTestChecker.class, getClass())
         .addSourceLines(
             "A.java",
             "// BUG: Diagnostic contains: ASSERTJ: true, GUAVA: true, REACTOR: true",
@@ -29,8 +30,8 @@ void isIntroductionAllowed() {
   }
 
   @Test
-  void isIntroductionAllowedWitnessClassesInSymtab() {
-    CompilationTestHelper.newInstance(TestChecker.class, getClass())
+  void isIntroductionAllowedWitnessClassesInSymbolTable() {
+    CompilationTestHelper.newInstance(IsIntroductionAllowedTestChecker.class, getClass())
         .addSourceLines(
             "A.java",
             "import com.google.common.collect.ImmutableList;",
@@ -50,7 +51,7 @@ void isIntroductionAllowedWitnessClassesInSymtab() {
 
   @Test
   void isIntroductionAllowedWitnessClassesPartiallyOnClassPath() {
-    CompilationTestHelper.newInstance(TestChecker.class, getClass())
+    CompilationTestHelper.newInstance(IsIntroductionAllowedTestChecker.class, getClass())
         .withClasspath(ImmutableList.class, Flux.class)
         .addSourceLines(
             "A.java",
@@ -61,7 +62,7 @@ void isIntroductionAllowedWitnessClassesPartiallyOnClassPath() {
 
   @Test
   void isIntroductionAllowedWitnessClassesNotOnClassPath() {
-    CompilationTestHelper.newInstance(TestChecker.class, getClass())
+    CompilationTestHelper.newInstance(IsIntroductionAllowedTestChecker.class, getClass())
         .withClasspath()
         .addSourceLines(
             "A.java",
@@ -74,7 +75,7 @@ void isIntroductionAllowedWitnessClassesNotOnClassPath() {
   @ParameterizedTest
   @ValueSource(booleans = {true, false})
   void isIntroductionAllowedIgnoreClasspathCompat(boolean ignoreClassPath) {
-    CompilationTestHelper.newInstance(TestChecker.class, getClass())
+    CompilationTestHelper.newInstance(IsIntroductionAllowedTestChecker.class, getClass())
         .setArgs("-XepOpt:ErrorProneSupport:IgnoreClasspathCompat=" + ignoreClassPath)
         .withClasspath(ImmutableList.class, Flux.class)
         .addSourceLines(
@@ -86,23 +87,64 @@ void isIntroductionAllowedIgnoreClasspathCompat(boolean ignoreClassPath) {
         .doTest();
   }
 
+  @Test
+  void canIntroduceUsage() {
+    CompilationTestHelper.newInstance(CanIntroduceUsageTestChecker.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "// BUG: Diagnostic contains: GUAVA_PUBLIC: true, GUAVA_PRIVATE: false, ERROR_PRONE_PUBLIC_NESTED:",
+            "// true",
+            "class A {}")
+        .doTest();
+  }
+
   /**
    * Flags classes with a diagnostics message that indicates, for each {@link ThirdPartyLibrary}
    * element, whether they can be used.
    */
   @BugPattern(severity = ERROR, summary = "Interacts with `ThirdPartyLibrary` for testing purposes")
-  public static final class TestChecker extends BugChecker implements ClassTreeMatcher {
+  public static final class IsIntroductionAllowedTestChecker extends BugChecker
+      implements ClassTreeMatcher {
     private static final long serialVersionUID = 1L;
 
     @Override
     public Description matchClass(ClassTree tree, VisitorState state) {
       return buildDescription(tree)
           .setMessage(
               Arrays.stream(ThirdPartyLibrary.values())
+                  .map(lib -> lib.name() + ": " + lib.isIntroductionAllowed(state))
+                  .collect(joining(", ")))
+          .build();
+    }
+  }
+
+  /**
+   * Flags classes with a diagnostics message that indicates, for selected types, the result of
+   * {@link ThirdPartyLibrary#canIntroduceUsage(String, VisitorState)}.
+   */
+  @BugPattern(severity = ERROR, summary = "Interacts with `ThirdPartyLibrary` for testing purposes")
+  public static final class CanIntroduceUsageTestChecker extends BugChecker
+      implements ClassTreeMatcher {
+    private static final long serialVersionUID = 1L;
+    private static final ImmutableMap<String, String> TYPES =
+        ImmutableMap.of(
+            "GUAVA_PUBLIC",
+            ImmutableList.class.getCanonicalName(),
+            "GUAVA_PRIVATE",
+            "com.google.common.collect.ImmutableEnumSet",
+            "ERROR_PRONE_PUBLIC_NESTED",
+            "com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput");
+
+    @Override
+    public Description matchClass(ClassTree tree, VisitorState state) {
+      return buildDescription(tree)
+          .setMessage(
+              TYPES.entrySet().stream()
                   .map(
-                      lib ->
-                          String.join(
-                              ": ", lib.name(), String.valueOf(lib.isIntroductionAllowed(state))))
+                      e ->
+                          e.getKey()
+                              + ": "
+                              + ThirdPartyLibrary.canIntroduceUsage(e.getValue(), state))
                   .collect(joining(", ")))
           .build();
     }