Add BugPattern to rewrite `assertThat(...).isEqualTo(null)` to `assertThat(...).isNull()` #133

oxkitsune · 2022-06-15T13:18:11Z

Implement BugPattern to rewrite assertThat(...).isEqualTo(null) to assertThat(...).isNull()

Example:

- assertThat(foo).isEqualTo(null);
+ assertThat(foo).isNull();

Suggested commit message:

Add BugPattern to rewrite `assertThat(...).isEqualTo(null)` to `assertThat(...).isNull()` (#133)

rickie

Most important thing is that we don't flag only AssertJ usages of isEqualTo, which should be the case 😄.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java

oxkitsune · 2022-06-16T12:38:09Z

I'm going to redo the Matcher as I learned some neat tricks to simplify this a lot!

Stephan202 · 2022-06-19T19:23:12Z

I'm going to redo the Matcher as I learned some neat tricks to simplify this a lot!

That's the spirit 💪. I'll let @rickie do a new review round first, but perhaps some of the (functional and/or stylistic) tweaks I applied in #135 are applicable here as well :)

Stephan202

Rebased and added a commit. Suggested commit message:

Prefer AssertJ's `isNull()` assertion over `isEqualTo(null)` (#133)

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java

...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java

Stephan202

Hmm, one open point: we should document why we can't use Refaster for this, e.g. with an example.

(Requesting changes so we don't forget.)

rickie

First of all, there were like 6 comments from when I started reviewing before @Stephan202 made some changes, sorry about that. Can't delete them anymore 🤔.

Made a suggestion w.r.t. the explanation.

Overall, changes LGTM 😄.

rickie · 2022-06-17T08:34:48Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java

+
+/**
+ * A {@link BugChecker} which flags {@code asserThat(someValue).isEqualTo(null)} for further
+ * simplification


Suggested change

* simplification

* simplification.

Javadoc should end with a dot :).

rickie · 2022-06-17T08:36:07Z

...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java

+            "  public void testAssertThat() {",
+            "    String nullValue = null;",
+            "    assertThat(12).isEqualTo(12);",
+            "    // BUG: Diagnostic contains: assertThat(...).isNull()",


Suggested change

" // BUG: Diagnostic contains: assertThat(...).isNull()",

" // BUG: Diagnostic contains:",

rickie · 2022-06-17T08:36:39Z

...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java

+            "    isEqualTo(null);",
+            "  }",
+            "",
+            " private boolean isEqualTo (Object value) {",


Make sure to follow the google-java-format in the tests as well.

rickie · 2022-06-17T08:37:10Z

...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java

+        .addInputLines(
+            "A.java",
+            "import static org.assertj.core.api.Assertions.assertThat;",
+            "import com.google.common.collect.ImmutableSortedSet;",


I don't see any usages of this 👀.

rickie · 2022-06-17T08:38:52Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java

+  private static final long serialVersionUID = 1L;
+
+  private static final Matcher<MethodInvocationTree> ASSERT_IS_EQUAL =
+      Matchers.allOf(


Matchers we generally statically import. That reads a bit nicer :)

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java

rickie · 2022-06-28T13:20:55Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java

+import com.sun.source.tree.MethodInvocationTree;
+
+/**
+ * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. <br>


The first line of the Javadoc is a short summary. After that we have an empty line with optionally extra explanations.

Also <br> is not used in the Javadoc (see the suggestion I'm about to push).
The google-java-format plugin in IntelliJ automatically adds the <p> in front of the text if there was an empty line between it and the summary.

rickie · 2022-06-28T13:21:17Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java

+/**
+ * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. <br>
+ * This cannot be done using a refaster template, as refaster is unable to match the abstraction
+ * layers in AssertJ


Suggested change

* layers in AssertJ

* layers in AssertJ.

Sentence should end with a dot.

rickie · 2022-06-28T13:21:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java

+
+/**
+ * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. <br>
+ * This cannot be done using a refaster template, as refaster is unable to match the abstraction


Suggested change

* This cannot be done using a refaster template, as refaster is unable to match the abstraction

* This cannot be done using a Refaster template, as Refaster is unable to match the abstraction

rickie · 2022-06-28T13:24:06Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java

+ * This cannot be done using a refaster template, as refaster is unable to match the abstraction
+ * layers in AssertJ
+ *
+ * <p>Example: <code>assertThat("foo").isEqualTo(null)</code> will not be matched by refaster.


Although I completely understand the suggestion, some context is missing here. When trying to implement this using Refaster templates, the issue was a bit more specific. From the +- 10 before templates, only 5 or 6 matched. So a few (I don't remember which ones) didn't work for Refaster.

There is a good chance the proposed examples does work. That's why I proposed a slightly different test. Not sure if we should figure out which (sub)types didn't match to provide examples in the Javadoc. Perhaps this is good enough. WDYT?

…tThat(...).isNull()` Run google-java-format Add some XXXs Suggest fix in AsserThatIsNullCheck Add javadoc Make test class final Add typecheck for literal tree fmt Improve method invocation matching

Stephan202 · 2022-06-28T15:21:22Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java

+ * <p>This cannot be done by using only Refaster templates, as Refaster is not able to match *all*
+ * {@code AbstractAssert} subtypes correctly.


This just raises questions for me. Do we have a concrete example that fails, so that I can identify the underlying cause (and then turn that into a concise explanation)? :)

Ah, assertThat("foo").isEqualTo(null) was mentioned in a preceding commit. Will check.

The key point here is that there exist specialized overloads (not overrides) of isEqualTo that we also wish to match. Added a commit.

I'm not sure whether this suggestions_really_ describes problem. Adding all of these overloads to a Refaster template would not work. For some reason, Refaster fails to match some of the overloads. So I recall we had about 8 before templates but some specific overloads didn't do anything because Refaster couldn't correctly match all these types.

Would like to see such an example, then. OTOH, not sure it matters: the current description isn't wrong: even if Refaster would work it'd (a) be a lot of work and (b) not be future-proof.

TBH, I agree with your points. I don't think it is worth the hassle to get to the specific examples. Let's roll with the current state 🚀 .

oxkitsune force-pushed the gdejong/psm-1298 branch from 8741248 to 5ff3bad Compare June 15, 2022 14:23

oxkitsune marked this pull request as ready for review June 16, 2022 07:26

oxkitsune requested a review from rickie June 16, 2022 07:26

rickie requested changes Jun 16, 2022

View reviewed changes

oxkitsune force-pushed the gdejong/psm-1298 branch from 064606c to 0c87b2b Compare June 16, 2022 13:47

oxkitsune requested a review from rickie June 17, 2022 11:56

oxkitsune force-pushed the gdejong/psm-1298 branch from 07cf810 to 804bebd Compare June 20, 2022 08:50

Stephan202 force-pushed the gdejong/psm-1298 branch from 804bebd to 5bec7f6 Compare June 25, 2022 18:32

Stephan202 approved these changes Jun 25, 2022

View reviewed changes

Stephan202 reviewed Jun 25, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java Show resolved Hide resolved

Stephan202 requested changes Jun 25, 2022

View reviewed changes

rickie approved these changes Jun 28, 2022

View reviewed changes

rickie added this to the 0.1.0 milestone Jun 28, 2022

oxkitsune and others added 9 commits June 28, 2022 17:20

Add BugPattern to rewrite assertThat(...).isEqualTo(null) to `asser…

d595a58

…tThat(...).isNull()` Run google-java-format Add some XXXs Suggest fix in AsserThatIsNullCheck Add javadoc Make test class final Add typecheck for literal tree fmt Improve method invocation matching

Remove public modifier

de8480e

Requested changes

766954d

Simplify matcher

6e49242

Remove unused imports

11ab9b9

Stylistic changes

b869e90

Suggestions

3bc7405

Add reasoning for using a BugPattern instead of Refaster template

fa7a8c8

Javadoc suggestions

6263b17

Stephan202 reviewed Jun 28, 2022

View reviewed changes

Suggestion

dabe437

Stephan202 force-pushed the gdejong/psm-1298 branch from 172f4d4 to dabe437 Compare June 29, 2022 07:57

Stephan202 approved these changes Jun 29, 2022

View reviewed changes

Stephan202 merged commit c500516 into master Jun 29, 2022

Stephan202 deleted the gdejong/psm-1298 branch June 29, 2022 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BugPattern to rewrite `assertThat(...).isEqualTo(null)` to `assertThat(...).isNull()` #133

Add BugPattern to rewrite `assertThat(...).isEqualTo(null)` to `assertThat(...).isNull()` #133

oxkitsune commented Jun 15, 2022 •

edited

Loading

rickie left a comment

oxkitsune commented Jun 16, 2022

Stephan202 commented Jun 19, 2022

Stephan202 left a comment

Stephan202 left a comment

rickie left a comment

rickie Jun 17, 2022

rickie Jun 17, 2022

rickie Jun 17, 2022

rickie Jun 17, 2022

rickie Jun 17, 2022

rickie Jun 28, 2022

rickie Jun 28, 2022

rickie Jun 28, 2022

rickie Jun 28, 2022

Stephan202 Jun 28, 2022

Stephan202 Jun 28, 2022

Stephan202 Jun 29, 2022

rickie Jun 29, 2022

Stephan202 Jun 29, 2022

rickie Jun 29, 2022

	" // BUG: Diagnostic contains: assertThat(...).isNull()",
	" // BUG: Diagnostic contains:",

	* This cannot be done using a refaster template, as refaster is unable to match the abstraction
	* This cannot be done using a Refaster template, as Refaster is unable to match the abstraction

		* <p>This cannot be done by using only Refaster templates, as Refaster is not able to match all
		* {@code AbstractAssert} subtypes correctly.

Add BugPattern to rewrite assertThat(...).isEqualTo(null) to assertThat(...).isNull() #133

Add BugPattern to rewrite assertThat(...).isEqualTo(null) to assertThat(...).isNull() #133

Conversation

oxkitsune commented Jun 15, 2022 • edited Loading

rickie left a comment

Choose a reason for hiding this comment

oxkitsune commented Jun 16, 2022

Stephan202 commented Jun 19, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

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

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

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

Add BugPattern to rewrite `assertThat(...).isEqualTo(null)` to `assertThat(...).isNull()` #133

Add BugPattern to rewrite `assertThat(...).isEqualTo(null)` to `assertThat(...).isNull()` #133

oxkitsune commented Jun 15, 2022 •

edited

Loading