Introduce `PatternRules` Refaster rule collection #771

Stephan202 · 2023-08-30T08:25:07Z

Suggested commit message:

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.

github-actions · 2023-08-30T08:28:10Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-08-30T09:43:04Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java

+    if (expectedSeverities.isEmpty()) {
+      compilationTestHelper.doTest();
+    } else {


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.

rickie

Only one suggestion. Will approve nonetheless. Feel free when the second approval is in 😉.

rickie · 2023-08-31T11:36:47Z

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<String, String> STATIC_IMPORT_EXEMPTED_MEMBERS =
      ImmutableSetMultimap.<String, String>builder()
+          .put("com.google.common.base.Predicates", "contains")


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

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?

Deal, let's do that :).

Stephan202

Tnx for the review!

Stephan202 · 2023-08-31T19:05:29Z

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<String, String> STATIC_IMPORT_EXEMPTED_MEMBERS =
      ImmutableSetMultimap.<String, String>builder()
+          .put("com.google.common.base.Predicates", "contains")


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?

rickie · 2023-09-01T09:27:42Z

Rebased.

sonarqubecloud · 2023-09-01T09:30:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

github-actions · 2023-09-01T09:30:31Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Venorcis · 2023-10-06T13:48:37Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

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


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)

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

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.

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.

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

Venorcis · 2023-10-06T13:52:17Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

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


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

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.)

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 😅

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.

Stephan202

Partially the discussion may be moot; a git grep 'Predicates.contains' against the Picnic code base yields only the Error Prone Support code modified in this PR 😄

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

Stephan202 · 2023-10-07T14:19:33Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

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


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

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.

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.

Stephan202 · 2023-10-07T14:31:58Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

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


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.)

benhalasi

Cool stuff!

Left just one question about the testing.

benhalasi · 2023-10-08T16:02:23Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

+
+/** Refaster rules related to code dealing with regular expressions. */
+@OnlineDocumentation
+final class PatternRules {


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

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.

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

benhalasi · 2023-10-08T16:59:35Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java

-            and(
-                containsPattern("SuppressWarnings"),
-                containsPattern("toImmutableList"),
-                containsPattern("toList")))
+            m -> Stream.of("SuppressWarnings", "toImmutableList", "toList").allMatch(m::contains))


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?

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.

Stephan202

Tnx for all the careful reviews! ❤️

Stephan202 · 2023-10-09T11:16:43Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

+
+/** Refaster rules related to code dealing with regular expressions. */
+@OnlineDocumentation
+final class PatternRules {


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

Stephan202 · 2023-10-09T11:21:10Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PatternRules.java

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


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.

Stephan202 · 2023-10-09T11:25:11Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java

-            and(
-                containsPattern("SuppressWarnings"),
-                containsPattern("toImmutableList"),
-                containsPattern("toList")))
+            m -> Stream.of("SuppressWarnings", "toImmutableList", "toList").allMatch(m::contains))


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.

github-actions · 2023-10-09T11:30:00Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-10-10T06:40:13Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-10-10T11:19:41Z

Rebased, will merge once 📗 , nice discussions 🚀 !

github-actions · 2023-10-10T11:21:37Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

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.

sonarqubecloud · 2023-10-11T06:21:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

github-actions · 2023-10-11T06:21:48Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 added this to the 0.14.0 milestone Aug 30, 2023

Stephan202 requested review from rickie and benhalasi August 30, 2023 08:25

Stephan202 commented Aug 30, 2023

View reviewed changes

rickie added the new feature label Aug 31, 2023

rickie approved these changes Aug 31, 2023

View reviewed changes

Stephan202 commented Aug 31, 2023

View reviewed changes

rickie force-pushed the sschroevers/introduce-pattern-rules branch from 8583a5e to 55c299a Compare September 1, 2023 09:27

rickie modified the milestones: 0.14.0, 0.15.0 Oct 4, 2023

Venorcis reviewed Oct 6, 2023

View reviewed changes

Stephan202 commented Oct 7, 2023

View reviewed changes

benhalasi reviewed Oct 9, 2023

View reviewed changes

rickie mentioned this pull request Oct 9, 2023

Enable test for ClassRules rule collection #826

Merged

Stephan202 commented Oct 9, 2023

View reviewed changes

Stephan202 force-pushed the sschroevers/introduce-pattern-rules branch from 55c299a to a16548c Compare October 9, 2023 11:27

Venorcis approved these changes Oct 9, 2023

View reviewed changes

benhalasi self-requested a review October 9, 2023 15:13

benhalasi approved these changes Oct 9, 2023

View reviewed changes

rickie force-pushed the sschroevers/introduce-pattern-rules branch from a16548c to b33c1d9 Compare October 10, 2023 06:38

rickie force-pushed the sschroevers/introduce-pattern-rules branch from b33c1d9 to ec6a790 Compare October 10, 2023 11:19

Introduce PatternRules Refaster rule collection

21ba662

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.

Test it!

45dafa8

rickie force-pushed the sschroevers/introduce-pattern-rules branch from ec6a790 to 45dafa8 Compare October 11, 2023 06:19

rickie merged commit 7567939 into master Oct 11, 2023
17 checks passed

rickie deleted the sschroevers/introduce-pattern-rules branch October 11, 2023 06:36

Introduce PatternRules Refaster rule collection #771

Introduce PatternRules Refaster rule collection #771

Conversation

Stephan202 commented Aug 30, 2023 • edited Loading

github-actions bot commented Aug 30, 2023

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Sep 1, 2023

sonarqubecloud bot commented Sep 1, 2023

github-actions bot commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Venorcis Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhalasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhalasi Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 9, 2023

github-actions bot commented Oct 10, 2023

rickie commented Oct 10, 2023

github-actions bot commented Oct 10, 2023

sonarqubecloud bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

Introduce `PatternRules` Refaster rule collection #771

Introduce `PatternRules` Refaster rule collection #771

Stephan202 commented Aug 30, 2023 •

edited

Loading

Venorcis Oct 8, 2023 •

edited

Loading

benhalasi Oct 8, 2023 •

edited

Loading