From 8fa3ff3702f48d77a23bb12401d1ac4a4993f71a Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 29 Oct 2022 13:42:51 +0200 Subject: [PATCH] By default, prevent `BugChecker`s from introducing new dependencies (#308) --- error-prone-contrib/pom.xml | 5 + .../bugpatterns/CollectorMutability.java | 4 +- .../ScheduledTransactionTrace.java | 4 +- .../bugpatterns/util/ThirdPartyLibrary.java | 105 ++++++++++++++++ .../bugpatterns/CollectorMutabilityTest.java | 17 +++ .../ScheduledTransactionTraceTest.java | 16 +++ .../util/ThirdPartyLibraryTest.java | 115 ++++++++++++++++++ 7 files changed, 264 insertions(+), 2 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index 3e7d97d6a6..a27da56f31 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -173,6 +173,11 @@ slf4j-api test + + org.springframework + spring-context + test + org.springframework spring-test diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java index a715b613a2..e2baa3c10f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java @@ -18,6 +18,7 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import java.util.stream.Collector; +import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** * A {@link BugChecker} that flags {@link Collector Collectors} that don't clearly express @@ -50,7 +51,8 @@ public CollectorMutability() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!COLLECTOR_METHOD.matches(tree, state)) { + if (!ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state) + || !COLLECTOR_METHOD.matches(tree, state)) { return Description.NO_MATCH; } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java index a23a6ceeb6..73a9ec5f50 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java @@ -26,6 +26,7 @@ import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; +import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** * A {@link BugChecker} that flags methods with Spring's {@code @Scheduled} annotation that lack New @@ -51,7 +52,8 @@ public ScheduledTransactionTrace() {} @Override public Description matchMethod(MethodTree tree, VisitorState state) { - if (!IS_SCHEDULED.matches(tree, state)) { + if (!ThirdPartyLibrary.NEW_RELIC_AGENT_API.isIntroductionAllowed(state) + || !IS_SCHEDULED.matches(tree, state)) { return Description.NO_MATCH; } 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 new file mode 100644 index 0000000000..9b8cbd9ebe --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java @@ -0,0 +1,105 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.suppliers.Supplier; +import com.sun.tools.javac.code.ClassFinder; +import com.sun.tools.javac.code.Symbol.CompletionFailure; +import com.sun.tools.javac.util.Name; + +/** + * Utility class that helps decide whether it is appropriate to introduce references to (well-known) + * third-party libraries. + * + *

This class should be used by {@link BugChecker}s that may otherwise suggest the introduction + * of code that depends on possibly-not-present third-party libraries. + */ +// XXX: Consider giving users more fine-grained control. This would be beneficial in cases where a +// dependency is on the classpath, but new usages are undesirable. +public enum ThirdPartyLibrary { + /** + * AssertJ. + * + * @see AssertJ documentation + */ + ASSERTJ("org.assertj.core.api.Assertions"), + /** + * Google's Guava. + * + * @see Guava on GitHub + */ + GUAVA("com.google.common.collect.ImmutableList"), + /** + * New Relic's Java agent API. + * + * @see New Relic + * Java agent API on GitHub + */ + NEW_RELIC_AGENT_API("com.newrelic.api.agent.Agent"), + /** + * VMWare's Project Reactor. + * + * @see Home page + */ + REACTOR("reactor.core.publisher.Flux"); + + private static final String IGNORE_CLASSPATH_COMPAT_FLAG = + "ErrorProneSupport:IgnoreClasspathCompat"; + + @SuppressWarnings("ImmutableEnumChecker" /* Supplier is deterministic. */) + private final Supplier canUse; + + /** + * Instantiates a {@link ThirdPartyLibrary} enum value. + * + * @param witnessFqcn The fully-qualified class name of a type that is expected to be on the + * classpath iff the associated third-party library is on the classpath. + */ + ThirdPartyLibrary(String witnessFqcn) { + this.canUse = VisitorState.memoize(state -> canIntroduceUsage(witnessFqcn, state)); + } + + /** + * Tells whether it is okay to introduce a dependency on this well-known third party library in + * the given context. + * + * @param state The context under consideration. + * @return {@code true} iff it is okay to assume or create a dependency on this library. + */ + public boolean isIntroductionAllowed(VisitorState state) { + return canUse.get(state); + } + + private static boolean canIntroduceUsage(String className, VisitorState state) { + return shouldIgnoreClasspath(state) || isKnownClass(className, state); + } + + /** + * Attempts to determine whether a class with the given FQCN is on the classpath. + * + *

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 canLoadClass(String className, VisitorState state) { + ClassFinder classFinder = ClassFinder.instance(state.context); + Name binaryName = state.binaryNameFromClassname(className); + try { + classFinder.loadClass(state.getSymtab().unnamedModule, binaryName); + return true; + } catch (CompletionFailure e) { + return false; + } + } + + private static boolean shouldIgnoreClasspath(VisitorState state) { + return state + .errorProneOptions() + .getFlags() + .getBoolean(IGNORE_CLASSPATH_COMPAT_FLAG) + .orElse(Boolean.FALSE); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CollectorMutabilityTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CollectorMutabilityTest.java index 48509379e5..1b837e61b5 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CollectorMutabilityTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CollectorMutabilityTest.java @@ -65,6 +65,23 @@ void identification() { .doTest(); } + @Test + void identificationWithoutGuavaOnClasspath() { + compilationTestHelper + .withClasspath() + .addSourceLines( + "A.java", + "import java.util.stream.Collectors;", + "import java.util.stream.Stream;", + "", + "class A {", + " void m() {", + " Stream.empty().collect(Collectors.toList());", + " }", + "}") + .doTest(); + } + @Test void replacementFirstSuggestedFix() { refactoringTestHelper diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceTest.java index 4753f141b7..95478fe712 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceTest.java @@ -4,6 +4,7 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; +import org.springframework.scheduling.annotation.Scheduled; final class ScheduledTransactionTraceTest { private final CompilationTestHelper compilationTestHelper = @@ -43,6 +44,21 @@ void identification() { .doTest(); } + @Test + void identificationWithoutNewRelicAgentApiOnClasspath() { + compilationTestHelper + .withClasspath(Scheduled.class) + .addSourceLines( + "A.java", + "import org.springframework.scheduling.annotation.Scheduled;", + "", + "class A {", + " @Scheduled(fixedDelay = 1)", + " void scheduledButNotTraced() {}", + "}") + .doTest(); + } + @Test void replacement() { refactoringTestHelper 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 new file mode 100644 index 0000000000..8d88885128 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java @@ -0,0 +1,115 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static java.util.stream.Collectors.joining; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.ClassTree; +import java.util.Arrays; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import reactor.core.publisher.Flux; + +final class ThirdPartyLibraryTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(TestChecker.class, getClass()); + + @Test + void isIntroductionAllowed() { + compilationTestHelper + .addSourceLines( + "A.java", + "// BUG: Diagnostic contains: ASSERTJ: true, GUAVA: true, NEW_RELIC_AGENT_API: true, REACTOR: true", + "class A {}") + .doTest(); + } + + @Test + void isIntroductionAllowedWitnessClassesInSymtab() { + compilationTestHelper + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableList;", + "import com.newrelic.api.agent.Agent;", + "import org.assertj.core.api.Assertions;", + "import reactor.core.publisher.Flux;", + "", + "// BUG: Diagnostic contains: ASSERTJ: true, GUAVA: true, NEW_RELIC_AGENT_API: true, REACTOR: true", + "class A {", + " void m(Class clazz) {", + " m(Assertions.class);", + " m(ImmutableList.class);", + " m(Agent.class);", + " m(Flux.class);", + " }", + "}") + .doTest(); + } + + @Test + void isIntroductionAllowedWitnessClassesPartiallyOnClassPath() { + compilationTestHelper + .withClasspath(ImmutableList.class, Flux.class) + .addSourceLines( + "A.java", + "// BUG: Diagnostic contains: ASSERTJ: false, GUAVA: true, NEW_RELIC_AGENT_API: false, REACTOR: true", + "class A {}") + .doTest(); + } + + @Test + void isIntroductionAllowedWitnessClassesNotOnClassPath() { + compilationTestHelper + .withClasspath() + .addSourceLines( + "A.java", + "// BUG: Diagnostic contains: ASSERTJ: false, GUAVA: false, NEW_RELIC_AGENT_API: false, REACTOR:", + "// false", + "class A {}") + .doTest(); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void isIntroductionAllowedIgnoreClasspathCompat(boolean ignoreClassPath) { + compilationTestHelper + .setArgs("-XepOpt:ErrorProneSupport:IgnoreClasspathCompat=" + ignoreClassPath) + .withClasspath(ImmutableList.class, Flux.class) + .addSourceLines( + "A.java", + String.format( + "// BUG: Diagnostic contains: ASSERTJ: %s, GUAVA: true, NEW_RELIC_AGENT_API: %s, REACTOR: true", + ignoreClassPath, ignoreClassPath), + "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 { + private static final long serialVersionUID = 1L; + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + return buildDescription(tree) + .setMessage( + Arrays.stream(ThirdPartyLibrary.values()) + .map( + lib -> + String.join( + ": ", lib.name(), String.valueOf(lib.isIntroductionAllowed(state)))) + .collect(joining(", "))) + .build(); + } + } +}