Introduce `IsCharacter` matcher and assorted test improvements #237

rickie · 2022-09-13T09:18:18Z

We extracted some commits from #225 to this PR and introduced the IsCharacter matcher.

Didn't list the exact test improvements, as it feels too specific to mention in the suggested commit message.

I think this qualifies as new feature instead of improvement right, as the Matcher is now available for use.

rickie · 2022-09-13T09:21:11Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsCharacter.java

+import com.sun.source.tree.ExpressionTree;
+
+/** A matcher of {@link Character}-typed expressions. */
+public final class IsCharacter implements Matcher<ExpressionTree> {


Was doubting a bit about the name. We flag both char and Character 🤔. Not sure how to reflect it in the name, especially considering Refaster does auto-boxing by default. Therefore I went for this one.

CoolTomatos

Thanks.

CoolTomatos · 2022-09-13T09:54:55Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsCharacter.java

+import com.sun.source.tree.ExpressionTree;
+
+/** A matcher of {@link Character}-typed expressions. */
+public final class IsCharacter implements Matcher<ExpressionTree> {


Shall we "generify" this?

The idea of a Matcher is to be specific in cases like this actually. We have to use this in a @Matches(value) or @NotMatches(value) annotation where value is:

Class<? extends Matcher<? super ExpressionTree>> value();

This setup limits the expressiveness of specifying what Matchers we want to use.

Good question though 😉.

To be more explicit about the limitation we're facing here: we can't pass constructor parameters.

Stephan202

Rebased, since #232 was merged

Stephan202 · 2022-09-17T17:31:11Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsCharacter.java

+import com.sun.source.tree.ExpressionTree;
+
+/** A matcher of {@link Character}-typed expressions. */
+public final class IsCharacter implements Matcher<ExpressionTree> {


To be more explicit about the limitation we're facing here: we can't pass constructor parameters.

Stephan202

Added a commit.

Suggested commit message:

Introduce `IsCharacter` matcher for use by Refaster templates (#237)

This new matcher is used to improve the `AssertThatIsOdd` and
`AssertThatIsEven` Refaster templates.

While there, apply assorted semi-related test improvements.

Stephan202 · 2022-09-17T19:44:11Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java

+  /**
+   * Prefer {@link AbstractLongAssert#isOdd()} over more contrived alternatives.
+   *
+   * <p>Note that for {@link Character}s this rewrite would lead to non-compilable code.


It's not about being non-compilable, but that the alternative is not available.

That said, I still feel like this can be left out: it's implied, and people doubting can just try to see what happens if @NotMatches(IsCharacter.class) is omitted.

Ah correct, should've been more specific here.

Stephan202 · 2022-09-17T19:44:47Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java

@@ -226,9 +228,14 @@ AbstractBigDecimalAssert<?> before(AbstractBigDecimalAssert<?> numberAssert) {
    }
  }

+  /**
+   * Prefer {@link AbstractLongAssert#isOdd()} over more contrived alternatives.


It's not just AbstractLongAssert#isOdd(). Would also argue that the original code isn't contrived, but rather that it yields a less informative error message.

Stephan202 · 2022-09-18T10:09:39Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsCharacter.java

+public final class IsCharacter implements Matcher<ExpressionTree> {
+  private static final long serialVersionUID = 1L;
+  private static final Matcher<ExpressionTree> DELEGATE =
+      anyOf(isSameType(Character.class), isSameType(s -> s.getSymtab().charType));


I suspect that the second case is more common. I.c.w. a predefined supplier we can do:

Suggested change

anyOf(isSameType(Character.class), isSameType(s -> s.getSymtab().charType));

anyOf(isSameType(CHAR_TYPE), isSameType(Character.class));

Stephan202 · 2022-09-18T10:55:18Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsCharacterTest.java

+            "  Character positive1() {",
+            "    // BUG: Diagnostic contains:",
+            "    return 'a';",
+            "  }",
+            "",
+            "  Character positive2() {",
+            "    // BUG: Diagnostic contains:",
+            "    return Character.valueOf('a');",
+            "  }",
+            "",
+            "  char positive3() {",
+            "    // BUG: Diagnostic contains:",
+            "    return \"a\".charAt(1);",
+            "  }",
+            "",
+            "  char positive4() {",
+            "    // BUG: Diagnostic contains:",
+            "    return Character.valueOf((char) 1);",
+            "  }",
+            "",
+            "  char positive5() {",
+            "    // BUG: Diagnostic contains:",
+            "    return (char) 1;",
+            "  }",


All these test cases also pass if we match only against isSameType(s -> s.getSymtab().charType) (because they all contain a char-typed (sub)expression).

Ai, I thought I checked this 😬.

Stephan202 · 2022-09-18T11:00:43Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsCharacterTest.java

+import com.google.errorprone.bugpatterns.BugChecker;
+import org.junit.jupiter.api.Test;
+
+final class IsCharacterTest {


Suggest we also verify that non-char primitives aren't flagged. (Not super relevant given the current IsCharacter, but one could be tempted to perform a refactor that would then match too much.)

rickie · 2022-09-18T12:04:48Z

Changes LGTM and the suggested commit message as well 👍🏻.

rickie added the new feature label Sep 13, 2022

rickie requested a review from Stephan202 September 13, 2022 09:18

rickie added this to the 0.2.1 milestone Sep 13, 2022

rickie commented Sep 13, 2022

View reviewed changes

rickie force-pushed the rossendrijver/char_matcher branch from 79c9139 to 774f8b7 Compare September 13, 2022 09:40

rickie requested a review from CoolTomatos September 13, 2022 09:43

rickie mentioned this pull request Sep 13, 2022

Introduce Refaster templates for AbstractComparableAsserts #225

Merged

CoolTomatos approved these changes Sep 13, 2022

View reviewed changes

Base automatically changed from sschroevers/prefer-mono-from-supplier to master September 15, 2022 07:42

Introduce IsCharacter matcher and improve test coverage

cb9fc83

Stephan202 force-pushed the rossendrijver/char_matcher branch from 774f8b7 to cb9fc83 Compare September 17, 2022 17:43

Stephan202 reviewed Sep 17, 2022

View reviewed changes

Suggestions

59e11b4

Stephan202 approved these changes Sep 18, 2022

View reviewed changes

Stephan202 merged commit bfc951b into master Sep 18, 2022

Stephan202 deleted the rossendrijver/char_matcher branch September 18, 2022 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `IsCharacter` matcher and assorted test improvements #237

Introduce `IsCharacter` matcher and assorted test improvements #237

rickie commented Sep 13, 2022

rickie Sep 13, 2022

CoolTomatos left a comment

CoolTomatos Sep 13, 2022

rickie Sep 13, 2022

Stephan202 Sep 17, 2022 •

edited

Loading

Stephan202 left a comment

Stephan202 Sep 17, 2022 •

edited

Loading

Stephan202 left a comment

Stephan202 Sep 17, 2022

rickie Sep 18, 2022

Stephan202 Sep 17, 2022

Stephan202 Sep 18, 2022

Stephan202 Sep 18, 2022

rickie Sep 18, 2022

Stephan202 Sep 18, 2022

rickie commented Sep 18, 2022

	anyOf(isSameType(Character.class), isSameType(s -> s.getSymtab().charType));
	anyOf(isSameType(CHAR_TYPE), isSameType(Character.class));

Introduce IsCharacter matcher and assorted test improvements #237

Introduce IsCharacter matcher and assorted test improvements #237

Conversation

rickie commented Sep 13, 2022

Choose a reason for hiding this comment

CoolTomatos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Sep 17, 2022 • edited Loading

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

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

rickie commented Sep 18, 2022

Introduce `IsCharacter` matcher and assorted test improvements #237

Introduce `IsCharacter` matcher and assorted test improvements #237

Stephan202 Sep 17, 2022 •

edited

Loading

Stephan202 Sep 17, 2022 •

edited

Loading