From 5f80fb5370c124e5e94bf6a789e0c1b88681ffbb Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 29 Jan 2024 10:57:45 +0100 Subject: [PATCH] Have `ErrorProneRuntimeClasspath` ignore non-public types (#972) --- .../bugpatterns/util/ThirdPartyLibrary.java | 30 ++++++--- .../ErrorProneRuntimeClasspathTest.java | 1 + .../util/ThirdPartyLibraryTest.java | 62 ++++++++++++++++--- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java index 04c994a915..e5fb961677 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java @@ -6,10 +6,13 @@ import com.google.errorprone.suppliers.Supplier; import com.sun.tools.javac.code.ClassFinder; import com.sun.tools.javac.code.Source; +import com.sun.tools.javac.code.Symbol; 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; +import javax.lang.model.element.Modifier; /** * Utility class that helps decide whether it is appropriate to introduce references to (well-known) @@ -70,12 +73,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,11 +87,16 @@ public static boolean canIntroduceUsage(String className, VisitorState state) { *

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 && isPublic(type.tsym); + } + + 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. @@ -96,10 +104,9 @@ private static boolean canLoadClass(String className, VisitorState state) { 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 isPublic(classFinder.loadClass(module, binaryName)); } catch ( @SuppressWarnings("java:S1166" /* Not exceptional. */) CompletionFailure e) { @@ -107,6 +114,11 @@ private static boolean canLoadClass(String className, VisitorState state) { } } + // XXX: Once we target JDK 14+, drop this method in favour of `Symbol#isPublic()`. + private static boolean isPublic(Symbol symbol) { + return symbol.getModifiers().contains(Modifier.PUBLIC); + } + private static boolean shouldIgnoreClasspath(VisitorState state) { return state .errorProneOptions() diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java index 7aff4887d0..f4bcfa8735 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java +++ b/error-prone-contrib/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/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java index 4338f9af35..a21a4f46cd 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java +++ b/error-prone-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,12 +87,24 @@ 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 @@ -99,10 +112,39 @@ 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 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(); }