Extend `null` check Refaster rules #523

Ptijohn · 2023-03-07T08:41:20Z

Follow up of this issue.

Suggested commit message:

Extend `null` check Refaster rules (#523)

Summary of changes:
- Replace `CheckNotNull` with `RequireNonNull{,WithMessage}{,Statement}`.
- Extend `Is{,Not}Null`.

Fixes #437.

github-actions · 2023-03-07T08:43:30Z

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-03-07T08:48:49Z

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-03-07T09:14:15Z

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-03-07T09:24:24Z

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-03-10T12:32:55Z

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-03-21T08:23:56Z

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

Added a commit.

Changes LGTM 😄. Added an extra template to make our lives easier. We can make more assumptions about the code and we need to account for less cases.

rickie · 2023-03-21T08:18:47Z

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

@@ -78,7 +79,7 @@ void after(int index, int size, String message) {
  static final class RequireNonNull<T> {
    @BeforeTemplate
    void before(T object) {
-      if (object == null) {
+      if (Refaster.anyOf(object == null, null == object)) {


Instead of having to add the inverse check, we can add another Refaster rule that rewrites null == object to object == null. That allows us to simplify the code in many other places.

rickie · 2023-03-21T08:27:52Z

Rebased and tweaked suggested commit message.

github-actions · 2023-03-21T08:29:32Z

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

Rebased and added a commit. Open point is about the IllegalArgumentException changes; IMHO those should be omitted.

Stephan202 · 2023-03-26T10:20:11Z

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

-    checkArgument(clazz != null, "Visited node is not enclosed by a class");
+    requireNonNull(clazz, "Visited node is not enclosed by a class");


The fact that we're validating the input though a null check is a function of the findEnclosing API; it could also have returned an Optional<ClassTree>. In this case IllegalArgumentException seems like a more appropriate exception to throw. (IllegalStateException could perhaps also work.)

I don't have strong options for this particular bit of code, but IMHO it shows that we should not apply the checkArgument rewrite below 👇

Hmm this is a difficult one. A GH code search shows there are quite some good improvements here. However, indeed some of them are not entirely correct.

There is only one heuristic that comes to mind, but that is probably a bit weird and odd. If the message contains "null" than it's a good check in all our cases. However, I see why this wouldn't fly for the OSS world 😋.

In that case, we should indeed drop it 🤔.

Dropping is the safe way forward 👍

Stephan202 · 2023-03-26T10:20:32Z

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

@@ -25,6 +25,11 @@ private NullRules() {}
  static final class IsNull {
    @BeforeTemplate
    boolean before(@Nullable Object object) {
+      return null == object;


Nice one. Let's also update the documentation.

Stephan202 · 2023-03-26T10:22:39Z

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

+    @BeforeTemplate
+    void before3(T object) {
+      checkArgument(object != null);
+    }


As mentioned above, IMHO this change is too invasive/opinionated for this check. Note that impacted code may be part of a method with a documented contract of throwing IllegalArgumentException rather than NullArgumentException.

Stephan202 · 2023-03-26T10:23:00Z

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

@@ -72,35 +74,55 @@ void after(int index, int size, String message) {
    }
  }

-  /** Prefer {@link Preconditions#checkNotNull(Object)} over more verbose alternatives. */
-  static final class CheckNotNull<T> {
+  /** Prefer {@link Objects#requireNonNull(Object)} over more verbose alternatives. */


checkNotNull is not more verbose 👀

Stephan202 · 2023-03-26T17:52:42Z

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

    @BeforeTemplate
    void before(T object) {
      if (object == null) {
        throw new NullPointerException();
      }
    }

+    @BeforeTemplate
+    void before2(T object) {
+      checkNotNull(object);


This rule won't match expressions of this type. The way Renovate works, we'll need a separate rule for that. (And then drop the rule here, because expression rules match statements, but not vice versa.)

Haha Renovate 😉. But good one, this is where I should improve with reviewing 🤔.

Stephan202 · 2023-03-26T18:15:10Z

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

+    @BeforeTemplate
+    boolean before2(@Nullable Object object) {
      return Objects.isNull(object);
    }


Here and below: we can use Refaster.anyOf.

You are right (I hear you say it hehe) we should automate this :).

Indeed we should ;)

github-actions · 2023-03-26T18:21:35Z

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

Added a commit and answered the open points :).

rickie · 2023-03-27T06:26:24Z

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

-    checkArgument(clazz != null, "Visited node is not enclosed by a class");
+    requireNonNull(clazz, "Visited node is not enclosed by a class");


Hmm this is a difficult one. A GH code search shows there are quite some good improvements here. However, indeed some of them are not entirely correct.

There is only one heuristic that comes to mind, but that is probably a bit weird and odd. If the message contains "null" than it's a good check in all our cases. However, I see why this wouldn't fly for the OSS world 😋.

In that case, we should indeed drop it 🤔.

rickie · 2023-03-27T06:35:49Z

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

+    @BeforeTemplate
+    boolean before2(@Nullable Object object) {
      return Objects.isNull(object);
    }


You are right (I hear you say it hehe) we should automate this :).

rickie · 2023-03-27T06:46:39Z

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

    @BeforeTemplate
    void before(T object) {
      if (object == null) {
        throw new NullPointerException();
      }
    }

+    @BeforeTemplate
+    void before2(T object) {
+      checkNotNull(object);


Haha Renovate 😉. But good one, this is where I should improve with reviewing 🤔.

rickie · 2023-03-27T06:50:19Z

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

    @BeforeTemplate
    void before(T object) {
      if (object == null) {
        throw new NullPointerException();
      }
    }

+    @BeforeTemplate
+    void before3(T object) {


Suggested change

void before3(T object) {

void before2(T object) {

rickie · 2023-03-27T06:50:25Z

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

+  /** Prefer {@link Objects#requireNonNull(Object, String)} over non-JDK alternatives. */
+  static final class RequireNonNullWithMessage<T> {
+    @BeforeTemplate
+    T before2(T object, String message) {


Suggested change

T before2(T object, String message) {

T before(T object, String message) {

github-actions · 2023-03-27T07:17:24Z

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-03-27T08:46:55Z

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

+    }
+  }
+
+  /** Prefer {@link Objects#requireNonNull(Object)} over non-JDK alternatives. */


Suggested change

/** Prefer {@link Objects#requireNonNull(Object)} over non-JDK alternatives. */

/** Prefer {@link Objects#requireNonNull(Object)} over more verbose or non-JDK alternatives. */

But, assuming we drop the checkArgument check:

Suggested change

/** Prefer {@link Objects#requireNonNull(Object)} over non-JDK alternatives. */

/** Prefer {@link Objects#requireNonNull(Object)} over more verbose alternatives. */

Stephan202 · 2023-03-27T08:47:39Z

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

-    checkArgument(clazz != null, "Visited node is not enclosed by a class");
+    requireNonNull(clazz, "Visited node is not enclosed by a class");


Dropping is the safe way forward 👍

rickie

Added a commit and rebased.

rickie · 2023-03-27T19:44:04Z

One open question is how to address the "Statement" part of the Refaster rules in the suggested commit message. I'm not sure if it adds value to put it there. Any suggestions @Stephan202 ?

github-actions · 2023-03-27T19:44:18Z

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-03-27T19:45:17Z

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

Not sure either, but proposed something ;).

Code changes LGTM!

Ptijohn requested a review from rickie March 7, 2023 08:41

Ptijohn force-pushed the bdiederichs/require-not-null branch from 0181a9a to 942c604 Compare March 10, 2023 12:31

rickie approved these changes Mar 21, 2023

View reviewed changes

rickie force-pushed the bdiederichs/require-not-null branch from 791ca72 to 1f2a953 Compare March 21, 2023 08:27

rickie requested a review from Stephan202 March 21, 2023 10:43

rickie added this to the 0.9.0 milestone Mar 21, 2023

rickie added the new feature label Mar 21, 2023

Stephan202 reviewed Mar 26, 2023

View reviewed changes

Stephan202 force-pushed the bdiederichs/require-not-null branch from 1f2a953 to 26c323f Compare March 26, 2023 18:19

rickie reviewed Mar 27, 2023

View reviewed changes

Stephan202 reviewed Mar 27, 2023

View reviewed changes

rickie reviewed Mar 27, 2023

View reviewed changes

Ptijohn and others added 8 commits March 27, 2023 21:43

Rewrite checkNotNull variants to requireNonNull

fbc27a6

Cover checkArgument and reversed null checks

bd12a28

Use different strings in test cases

4465ad6

Self apply

0b6bef1

🤦

52f63a8

Add extra case to IsNullRules and simplify

38de444

Suggestions

8a2fbfd

Tweaks

af95bfc

Suggestions

94900a4

rickie force-pushed the bdiederichs/require-not-null branch from 9d8141e to 94900a4 Compare March 27, 2023 19:43

Stephan202 approved these changes Mar 27, 2023

View reviewed changes

rickie changed the title ~~Rewrite null check variants to requireNonNull~~ Extend null check Refaster rules Mar 27, 2023

rickie merged commit 334c374 into master Mar 27, 2023

rickie deleted the bdiederichs/require-not-null branch March 27, 2023 20:08

		checkArgument(clazz != null, "Visited node is not enclosed by a class");
		requireNonNull(clazz, "Visited node is not enclosed by a class");

	T before2(T object, String message) {
	T before(T object, String message) {

	/** Prefer {@link Objects#requireNonNull(Object)} over non-JDK alternatives. */
	/** Prefer {@link Objects#requireNonNull(Object)} over more verbose or non-JDK alternatives. */

Extend null check Refaster rules #523

Extend null check Refaster rules #523

Conversation

Ptijohn commented Mar 7, 2023 • edited by Stephan202

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 21, 2023

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Mar 21, 2023

github-actions bot commented Mar 21, 2023

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

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 Mar 26, 2023

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

github-actions bot commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie commented Mar 27, 2023

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 27, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Extend `null` check Refaster rules #523

Extend `null` check Refaster rules #523

Ptijohn commented Mar 7, 2023 •

edited by Stephan202