Skip to content

Commit

Permalink
Introduce PatternRules Refaster rule collection (#771)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Stephan202 authored Oct 11, 2023
1 parent f1882ca commit 7567939
Show file tree
Hide file tree
Showing 9 changed files with 135 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
@VisibleForTesting
static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTED_MEMBERS =
ImmutableSetMultimap.<String, String>builder()
.put("com.google.common.base.Predicates", "contains")
.put("com.mongodb.client.model.Filters", "empty")
.putAll(
"java.util.Collections",
Expand Down
Original file line number Diff line number Diff line change
@@ -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<CharSequence> before(Pattern pattern) {
return Predicates.contains(pattern);
}

@AfterTemplate
Predicate<String> after(Pattern pattern) {
return pattern.asPredicate();
}
}

/** Prefer {@link Pattern#asPredicate()} over non-JDK alternatives. */
static final class PatternCompileAsPredicate {
@BeforeTemplate
Predicate<CharSequence> before(String pattern) {
return containsPattern(pattern);
}

@AfterTemplate
Predicate<String> after(String pattern) {
return Pattern.compile(pattern).asPredicate();
}
}
}
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;",
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;",
Expand Down Expand Up @@ -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;",
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ final class RefasterRulesTest {
MultimapRules.class,
NullRules.class,
OptionalRules.class,
PatternRules.class,
PreconditionsRules.class,
PrimitiveRules.class,
ReactorRules.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Object> elidedTypesAndStaticImports() {
return ImmutableSet.of(Predicates.class);
}

Predicate<?> testPatternAsPredicate() {
return Predicates.contains(Pattern.compile("foo"));
}

Predicate<?> testPatternCompileAsPredicate() {
return Predicates.containsPattern("foo");
}
}
Original file line number Diff line number Diff line change
@@ -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<Object> elidedTypesAndStaticImports() {
return ImmutableSet.of(Predicates.class);
}

Predicate<?> testPatternAsPredicate() {
return Pattern.compile("foo").asPredicate();
}

Predicate<?> testPatternCompileAsPredicate() {
return Pattern.compile("foo").asPredicate();
}
}
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 {",
Expand Down Expand Up @@ -153,7 +150,7 @@ private static Stream<Arguments> severityAssignmentTestCases() {
void severityAssignment(
ImmutableList<String> arguments, ImmutableList<SeverityLevel> expectedSeverities) {
CompilationTestHelper compilationTestHelper =
compilationHelper
CompilationTestHelper.newInstance(Refaster.class, getClass())
.setArgs(arguments)
.addSourceLines(
"A.java",
Expand All @@ -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<SeverityLevel> extractRefasterSeverities(
Expand Down

0 comments on commit 7567939

Please sign in to comment.