From 75679396df1113566bd87b6f4be006add4bf09b5 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 11 Oct 2023 08:36:26 +0200 Subject: [PATCH] Introduce `PatternRules` Refaster rule collection (#771) While there, configure `StaticImport` to not require static importing of `com.google.common.base.Predicates.contains`. The new rules triggered cleanup of some `CompilationTestHelper` usages. See google/guava#6483. --- .../errorprone/bugpatterns/StaticImport.java | 1 + .../refasterrules/PatternRules.java | 46 ++++++++++++ .../bugpatterns/AmbiguousJsonCreatorTest.java | 4 +- .../bugpatterns/FluxImplicitBlockTest.java | 11 +-- .../bugpatterns/StringJoinTest.java | 6 +- .../refasterrules/RefasterRulesTest.java | 1 + .../refasterrules/PatternRulesTestInput.java | 22 ++++++ .../refasterrules/PatternRulesTestOutput.java | 22 ++++++ .../refaster/runner/RefasterTest.java | 72 ++++++++++--------- 9 files changed, 135 insertions(+), 50 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java create mode 100644 error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestInput.java create mode 100644 error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestOutput.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java index 6063562002..c5b3e52427 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java @@ -155,6 +155,7 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa @VisibleForTesting static final ImmutableSetMultimap STATIC_IMPORT_EXEMPTED_MEMBERS = ImmutableSetMultimap.builder() + .put("com.google.common.base.Predicates", "contains") .put("com.mongodb.client.model.Filters", "empty") .putAll( "java.util.Collections", diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java new file mode 100644 index 0000000000..39ad88262b --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java @@ -0,0 +1,46 @@ +package tech.picnic.errorprone.refasterrules; + +import static com.google.common.base.Predicates.containsPattern; + +import com.google.common.base.Predicates; +import com.google.errorprone.refaster.annotation.AfterTemplate; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import java.util.function.Predicate; +import java.util.regex.Pattern; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; + +/** Refaster rules related to code dealing with regular expressions. */ +@OnlineDocumentation +final class PatternRules { + private PatternRules() {} + + /** Prefer {@link Pattern#asPredicate()} over non-JDK alternatives. */ + // XXX: This rule could also replace `s -> pattern.matcher(s).find()`, though the lambda + // expression may match functional interfaces other than `Predicate`. If we do add such a rule, we + // should also add a rule that replaces `s -> pattern.matcher(s).matches()` with + // `pattern.asMatchPredicate()`. + static final class PatternAsPredicate { + @BeforeTemplate + Predicate before(Pattern pattern) { + return Predicates.contains(pattern); + } + + @AfterTemplate + Predicate after(Pattern pattern) { + return pattern.asPredicate(); + } + } + + /** Prefer {@link Pattern#asPredicate()} over non-JDK alternatives. */ + static final class PatternCompileAsPredicate { + @BeforeTemplate + Predicate before(String pattern) { + return containsPattern(pattern); + } + + @AfterTemplate + Predicate after(String pattern) { + return Pattern.compile(pattern).asPredicate(); + } + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorTest.java index 785642de02..82976c5336 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorTest.java @@ -1,7 +1,5 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.common.base.Predicates.containsPattern; - import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; @@ -12,7 +10,7 @@ final class AmbiguousJsonCreatorTest { void identification() { CompilationTestHelper.newInstance(AmbiguousJsonCreator.class, getClass()) .expectErrorMessage( - "X", containsPattern("`JsonCreator.Mode` should be set for single-argument creators")) + "X", m -> m.contains("`JsonCreator.Mode` should be set for single-argument creators")) .addSourceLines( "Container.java", "import com.fasterxml.jackson.annotation.JsonCreator;", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java index 6390b2d041..91cecd3439 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java @@ -1,14 +1,12 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.common.base.Predicates.and; -import static com.google.common.base.Predicates.containsPattern; -import static com.google.common.base.Predicates.not; import static com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers.SECOND; import static com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers.THIRD; import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.reactivestreams.Publisher; import reactor.core.CorePublisher; @@ -20,10 +18,7 @@ void identification() { CompilationTestHelper.newInstance(FluxImplicitBlock.class, getClass()) .expectErrorMessage( "X", - and( - containsPattern("SuppressWarnings"), - containsPattern("toImmutableList"), - containsPattern("toList"))) + m -> Stream.of("SuppressWarnings", "toImmutableList", "toList").allMatch(m::contains)) .addSourceLines( "A.java", "import com.google.common.collect.ImmutableList;", @@ -63,7 +58,7 @@ void identification() { void identificationWithoutGuavaOnClasspath() { CompilationTestHelper.newInstance(FluxImplicitBlock.class, getClass()) .withClasspath(CorePublisher.class, Flux.class, Publisher.class) - .expectErrorMessage("X", not(containsPattern("toImmutableList"))) + .expectErrorMessage("X", m -> !m.contains("toImmutableList")) .addSourceLines( "A.java", "import reactor.core.publisher.Flux;", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringJoinTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringJoinTest.java index 379fc830ed..3bc5eb9547 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringJoinTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringJoinTest.java @@ -1,7 +1,5 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.common.base.Predicates.containsPattern; - import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; @@ -12,8 +10,8 @@ final class StringJoinTest { void identification() { CompilationTestHelper.newInstance(StringJoin.class, getClass()) .expectErrorMessage( - "valueOf", containsPattern("Prefer `String#valueOf` over `String#format`")) - .expectErrorMessage("join", containsPattern("Prefer `String#join` over `String#format`")) + "valueOf", m -> m.contains("Prefer `String#valueOf` over `String#format`")) + .expectErrorMessage("join", m -> m.contains("Prefer `String#join` over `String#format`")) .addSourceLines( "A.java", "import java.util.Formattable;", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java index 0fe39dec04..b7a7a017a6 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java @@ -61,6 +61,7 @@ final class RefasterRulesTest { MultimapRules.class, NullRules.class, OptionalRules.class, + PatternRules.class, PreconditionsRules.class, PrimitiveRules.class, ReactorRules.class, diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestInput.java new file mode 100644 index 0000000000..2b19d9ea88 --- /dev/null +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestInput.java @@ -0,0 +1,22 @@ +package tech.picnic.errorprone.refasterrules; + +import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableSet; +import java.util.function.Predicate; +import java.util.regex.Pattern; +import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; + +final class PatternRulesTest implements RefasterRuleCollectionTestCase { + @Override + public ImmutableSet elidedTypesAndStaticImports() { + return ImmutableSet.of(Predicates.class); + } + + Predicate testPatternAsPredicate() { + return Predicates.contains(Pattern.compile("foo")); + } + + Predicate testPatternCompileAsPredicate() { + return Predicates.containsPattern("foo"); + } +} diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestOutput.java new file mode 100644 index 0000000000..9e774fd474 --- /dev/null +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestOutput.java @@ -0,0 +1,22 @@ +package tech.picnic.errorprone.refasterrules; + +import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableSet; +import java.util.function.Predicate; +import java.util.regex.Pattern; +import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; + +final class PatternRulesTest implements RefasterRuleCollectionTestCase { + @Override + public ImmutableSet elidedTypesAndStaticImports() { + return ImmutableSet.of(Predicates.class); + } + + Predicate testPatternAsPredicate() { + return Pattern.compile("foo").asPredicate(); + } + + Predicate testPatternCompileAsPredicate() { + return Pattern.compile("foo").asPredicate(); + } +} diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 90b551b713..8dd3bc2aff 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -1,6 +1,5 @@ package tech.picnic.errorprone.refaster.runner; -import static com.google.common.base.Predicates.containsPattern; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; @@ -24,35 +23,33 @@ import org.junit.jupiter.params.provider.MethodSource; final class RefasterTest { - private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(Refaster.class, getClass()) - .matchAllDiagnostics() - .expectErrorMessage( - "StringOfSizeZeroRule", - containsPattern( - "\\[Refaster Rule\\] FooRules\\.StringOfSizeZeroRule: Refactoring opportunity\\s+.+\\s+")) - .expectErrorMessage( - "StringOfSizeOneRule", - containsPattern( - "\\[Refaster Rule\\] FooRules\\.StringOfSizeOneRule: " - + "A custom description about matching single-char strings\\s+.+\\s+" - + "\\(see https://error-prone.picnic.tech/refasterrules/FooRules#StringOfSizeOneRule\\)")) - .expectErrorMessage( - "StringOfSizeTwoRule", - containsPattern( - "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeTwoRule: " - + "A custom subgroup description\\s+.+\\s+" - + "\\(see https://example.com/rule/FooRules#ExtraGrouping.StringOfSizeTwoRule\\)")) - .expectErrorMessage( - "StringOfSizeThreeRule", - containsPattern( - "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeThreeRule: " - + "A custom description about matching three-char strings\\s+.+\\s+" - + "\\(see https://example.com/custom\\)")); + private static final Pattern DIAGNOSTIC_STRING_OF_SIZE_ZERO = + Pattern.compile( + "\\[Refaster Rule\\] FooRules\\.StringOfSizeZeroRule: Refactoring opportunity\\s+.+\\s+"); + private static final Pattern DIAGNOSTIC_STRING_OF_SIZE_ONE = + Pattern.compile( + "\\[Refaster Rule\\] FooRules\\.StringOfSizeOneRule: " + + "A custom description about matching single-char strings\\s+.+\\s+" + + "\\(see https://error-prone.picnic.tech/refasterrules/FooRules#StringOfSizeOneRule\\)"); + private static final Pattern DIAGNOSTIC_STRING_OF_SIZE_TWO = + Pattern.compile( + "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeTwoRule: " + + "A custom subgroup description\\s+.+\\s+" + + "\\(see https://example.com/rule/FooRules#ExtraGrouping.StringOfSizeTwoRule\\)"); + private static final Pattern DIAGNOSTIC_STRING_OF_SIZE_THREE = + Pattern.compile( + "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeThreeRule: " + + "A custom description about matching three-char strings\\s+.+\\s+" + + "\\(see https://example.com/custom\\)"); @Test void identification() { - compilationHelper + CompilationTestHelper.newInstance(Refaster.class, getClass()) + .matchAllDiagnostics() + .expectErrorMessage("StringOfSizeZeroRule", DIAGNOSTIC_STRING_OF_SIZE_ZERO.asPredicate()) + .expectErrorMessage("StringOfSizeOneRule", DIAGNOSTIC_STRING_OF_SIZE_ONE.asPredicate()) + .expectErrorMessage("StringOfSizeTwoRule", DIAGNOSTIC_STRING_OF_SIZE_TWO.asPredicate()) + .expectErrorMessage("StringOfSizeThreeRule", DIAGNOSTIC_STRING_OF_SIZE_THREE.asPredicate()) .addSourceLines( "A.java", "class A {", @@ -153,7 +150,7 @@ private static Stream severityAssignmentTestCases() { void severityAssignment( ImmutableList arguments, ImmutableList expectedSeverities) { CompilationTestHelper compilationTestHelper = - compilationHelper + CompilationTestHelper.newInstance(Refaster.class, getClass()) .setArgs(arguments) .addSourceLines( "A.java", @@ -167,13 +164,18 @@ void severityAssignment( " };", " }", "}"); - assertThatThrownBy(compilationTestHelper::doTest) - .isInstanceOf(AssertionError.class) - .message() - .satisfies( - message -> - assertThat(extractRefasterSeverities("A.java", message)) - .containsExactlyElementsOf(expectedSeverities)); + + if (expectedSeverities.isEmpty()) { + compilationTestHelper.doTest(); + } else { + assertThatThrownBy(compilationTestHelper::doTest) + .isInstanceOf(AssertionError.class) + .message() + .satisfies( + message -> + assertThat(extractRefasterSeverities("A.java", message)) + .containsExactlyElementsOf(expectedSeverities)); + } } private static ImmutableList extractRefasterSeverities(