Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce PatternRules Refaster rule collection #771

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Introduce PatternRules Refaster rule collection #771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21ba662
Introduce `PatternRules` Refaster rule collection
Stephan202 May 11, 2023
45dafa8
Test it!
Stephan202 Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java
View file
Open in desktop
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that adding contains and empty to STATIC_IMPORT_EXEMPTED_IDENTIFIERS would maybe be even better :).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting suggestion. Perhaps yes, though (hypothetical?) static contains/empty methods that deal with "regular" collection membership would not necessarily be problematic when imported statically. (The currently exempted methods instead return predicates with custom semantics.)

So... let's revisit if/when we add more such exclusions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deal, let's do that :).

.put("com.mongodb.client.model.Filters", "empty")
.putAll(
"java.util.Collections",
Expand Down
46 changes: 46 additions & 0 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java
View file
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that only this rule is not in RefasterRulesTest#RULE_COLLECTIONS, shouldn't it be included?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means that this one is not actually being tested, so we definitely should add it.
Now that I think of it, we forgot that part for the #811 PR 🤯, will open a PR right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch; will fix! (And indeed, cue the "we should automate this" message 😅)

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here I'd personally argue that the Guava variant is more descriptive (nor do they recommend against using it)

I'm also not sure it's 100% equivalent as JDK uses String vs. Guava's CharSequence

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here I'd personally argue that the Guava variant is more descriptive (nor do they recommend against using it)

On descriptiveness I guess we'll just have to disagree 😅. W.r.t. recommendations: while contains itself doesn't call it out, the returned com.google.common.base.Predicate is described as a legacy type that should be avoided whenever possible.

I'm also not sure it's 100% equivalent as JDK uses String vs. Guava's CharSequence

Indeed it isn't; this is a Refaster limitation that unfortunately more of our rules suffer from. (Longer-term I plan to automatically flag and annotate such rules such that they can e.g. be skipped.)

Copy link
Contributor

@Venorcis Venorcis Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the returned com.google.common.base.Predicate is described as a legacy type that should be avoided whenever possible.

good point, should that be 'attacked' even more generically? essentially the entire Predicates class shouldn't be used then 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally yes! git grep com.google.common.base.Predicate -- '*.java' shows that internally not too many usages remain, so it's doable. But in some cases the result may be passed to a com.google.common.base.Predicate-expecting method, in which case a Refaster approach isn't great. That's the same issue as we have here, but I expect that the impact will be larger when replacing or and not. So propose to defer that to a separate PR.

}
}

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

@AfterTemplate
Predicate<String> after(String pattern) {
return Pattern.compile(pattern).asPredicate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the Guava static builder is a nice short alternative; not sure I see the advantage in standardizing here (they also do not recommend themselves to not use it like with some other utilities we don't rewrite like Lists.transform)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how this suggestion is slightly controversial, but my thinking is that, besides being a JDK alternative:

  1. This code makes explicit that the operation involves pattern compilation; something one will generally want to move to a static field whenever possible.
    • Or, as in the cleaned-up test code, avoid altogether by simply calling String#contains instead.
  2. This code better conveys (through the explicit Pattern reference) that the predicate performs regex matching, rather than any other kind of pattern matching.

W.r.t. List#transform: indeed we don't rewrite that variant, but that's primarily because realistically it can only be rewritten when considering the surrounding context. That method's documentation does suggest that Java 8+ users should prefer Stream#map.

}
}
}
4 changes: 1 addition & 3 deletions ...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorTest.java
View file
Open in desktop
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
11 changes: 3 additions & 8 deletions ...prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java
View file
Open in desktop
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))
Comment on lines -23 to +21
Copy link
Contributor

@benhalasi benhalasi Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking aloud, not in this PR, but I was wondering whether utility functions that return non pattern based predicates would be useful. In this case it wouldn't hide any potentially expensive underlying operation.

In my very personal opinion contains("foo") is much more nicer to read than str -> str.contains("foo").

Seeing that we don't have Predicates#contains in Picnic codebase, I myself questioning the usefulness of these utilities, but maybe only for eps? Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I would say that such a utility method has an insufficient utility times ubiquity (or power-to-weight) ratio. Separately from that, when I see str -> str.contains("foo") I unambiguously know what that code will do, while with contains("foo") I'm like "that's funny, a helper method for such a simple operation; let me check the implementation to see whether it does anything special". So all-in-all I favour the current approach.

.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
6 changes: 2 additions & 4 deletions error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringJoinTest.java
View file
Open in desktop
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
1 change: 1 addition & 0 deletions ...r-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java
View file
Open in desktop
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
22 changes: 22 additions & 0 deletions ...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestInput.java
View file
Open in desktop
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");
}
}
22 changes: 22 additions & 0 deletions ...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/PatternRulesTestOutput.java
View file
Open in desktop
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();
}
}
72 changes: 37 additions & 35 deletions refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java
View file
Open in desktop
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 {
Comment on lines +168 to +170
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this wasn't necessary, because the expectErrorMessage declarations would trigger a "I didn't see anything" AssertionError, which would then cause the satisfies clause to succeed. But that's obscure.

assertThatThrownBy(compilationTestHelper::doTest)
.isInstanceOf(AssertionError.class)
.message()
.satisfies(
message ->
assertThat(extractRefasterSeverities("A.java", message))
.containsExactlyElementsOf(expectedSeverities));
}
}

private static ImmutableList<SeverityLevel> extractRefasterSeverities(
Expand Down
Loading