Prefer simple `null` reference check over calling `Objects#{isNull,nonNull}` #228

jeandersonbc · 2022-09-08T15:04:58Z

This PR introduces rules to prefer the use of == and != over Objects.isNull and Objects.nonNull for null checks on expressions.

Suggested commit message:

Prefer simple `null` reference check over `Objects#{isNull,nonNull}` (#228)

rickie

Added a commit and some comments.

I'm also fine with applying the suggestions myself, but I figured it could also be the nice part of this PR and playing around with Refaster, so I left most un-applied :).

rickie · 2022-09-08T16:38:18Z

...trib/src/test/resources/tech/picnic/errorprone/refastertemplates/NullTemplatesTestInput.java

@@ -22,4 +24,18 @@ long testIsNullFunction() {
  long testNonNullFunction() {
    return Stream.of("foo").filter(s -> s != null).count();
  }
+
+  boolean testIsNullReference() {
+    var ref1 = "foo";


We do not use var in this codebase :).

We can simplify the tests quite a bit, we only need a simple and singular test case per before template. Besides that, we prefer using ImmutableSets over Sets. I don't think we need a Set here TBH and can have simplified test :).

As a reference, see other tests for the other templates from NullTemplates :).

Sure! My thought originally was to see whether refaster would match and apply the transformation for the 3 cases (including inferred type with var). I'm updating the test input/output.

rickie · 2022-09-08T16:46:20Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/NullTemplates.java

+   */
+  static final class IsNullReference<T> {
+    @BeforeTemplate
+    boolean before(@Nullable T ref) {


Suggested change

boolean before(@Nullable T ref) {

boolean before(@Nullable Object object) {

The parameter for isNull is Object therefore we can be more specific than using T. In such cases we name the parameter of the before template something like object or obj.

This also holds for the other template.

rickie · 2022-09-08T19:34:20Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/NullTemplates.java

+   * Prefer <code>==</code> operator over {@link java.util.Objects#isNull(Object)} on boolean
+   * expressions.
+   */
+  static final class IsNullReference<T> {


We have the convention to name Refaster templates after the content of the after template. In this case there is not a lot of content in the after template, but I'd suggest naming it something like:
ObjectEqualNull or ObjectEqualsNull.

The other template could be something like ObjectNotEqualNull.

rickie

Another thought.

rickie · 2022-09-08T19:50:08Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/NullTemplates.java

@@ -55,4 +56,30 @@ Predicate<T> after() {
      return Objects::nonNull;
    }
  }
+
+  /** Prefer the <code>==</code> operator over {@link java.util.Objects#isNull(Object)}. */


Perhaps better to add for null checks? To the Javadoc, WDYT?

rickie

Added a commit.

Changes look good to me! Thanks for opening the PR @jeandersonbc 🚀 !

Suggested commit message:

Prefer simple `null` reference check over calling `Objects#{isNull,nonNull}` (#228)

rickie · 2022-09-09T08:40:16Z

...trib/src/test/resources/tech/picnic/errorprone/refastertemplates/NullTemplatesTestInput.java

@@ -22,4 +23,14 @@ long testIsNullFunction() {
  long testNonNullFunction() {
    return Stream.of("foo").filter(s -> s != null).count();
  }
+
+  boolean testObjectEqualsNull() {
+    Object obj = null;


I think we can even use a simple String "foo" for this, will push a proposal.

rickie · 2022-09-09T14:53:54Z

Discussed some things offline with @Stephan202 , applied the feedback 😄. W.r.t. the naming suggestion for the template, it's hard but we plan to formalize this :).

Augments NullTemplates to prefer == and != operators over Objects.isNull and Objects.nonNull on boolean expressions. E.g., ref == null is preferable over Objects.isNull(ref)

- Updated Javadoc - Updated naming convention - Updated test input/output

Stephan202

Rebased and added a commit. Moved the templates up in the file, as they seem more "fundamental".

(If/when we sort templates lexicographically this'll change, but that's for another day.)

Slightly tweaked the suggested commit message; @rickie feel free to merge if you're okay with the changes.

Stephan202 · 2022-09-10T11:19:57Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/NullTemplates.java

+  /**
+   * Prefer the {@code ==} operator over {@link java.util.Objects#isNull(Object)} for null checks.
+   */


Suggested change

/**

* Prefer the {@code ==} operator over {@link java.util.Objects#isNull(Object)} for null checks.

*/

/** Prefer the {@code ==} operator over {@link Objects#isNull(Object)}. */

Stephan202 · 2022-09-10T11:20:19Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/NullTemplates.java

+  /**
+   * Prefer the {@code !=} operator over {@link java.util.Objects#isNull(Object)} for null checks.
+   */


Wrong method referenced 👀

rickie · 2022-09-10T12:05:51Z

Rebased and added a commit. Moved the templates up in the file, as they seem more "fundamental".

SGTM.

Nice catch with the Javadoc.

jeandersonbc self-assigned this Sep 8, 2022

jeandersonbc requested review from rickie and Stephan202 September 8, 2022 15:06

jeandersonbc added the improvement label Sep 8, 2022

rickie requested changes Sep 8, 2022

View reviewed changes

rickie added new feature and removed improvement labels Sep 8, 2022

rickie reviewed Sep 8, 2022

View reviewed changes

jeandersonbc requested a review from rickie September 9, 2022 07:21

rickie added this to the 0.2.1 milestone Sep 9, 2022

rickie approved these changes Sep 9, 2022

View reviewed changes

Jeanderson Candido and others added 8 commits September 10, 2022 13:12

Introduce rules for null check on references

608d6fc

Augments NullTemplates to prefer == and != operators over Objects.isNull and Objects.nonNull on boolean expressions. E.g., ref == null is preferable over Objects.isNull(ref)

Change unrelated test

7406872

Second try to fix selfcheck

82bcbab

Suggestions

1d469af

Addressed PR comments

c89e730

- Updated Javadoc - Updated naming convention - Updated test input/output

Suggestions

05001b8

Suggestions 2

3a02aad

Suggestions

478902e

Stephan202 force-pushed the jeandersonbc/null-check branch from ee7a386 to 478902e Compare September 10, 2022 11:24

Stephan202 approved these changes Sep 10, 2022

View reviewed changes

rickie changed the title ~~Introduce rules for null check on references~~ Prefer simple null reference check over calling Objects#{isNull,nonNull} Sep 10, 2022

rickie merged commit e34c2ba into master Sep 10, 2022

rickie deleted the jeandersonbc/null-check branch September 10, 2022 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer simple `null` reference check over calling `Objects#{isNull,nonNull}` #228

Prefer simple `null` reference check over calling `Objects#{isNull,nonNull}` #228

jeandersonbc commented Sep 8, 2022 •

edited

Loading

rickie left a comment

rickie Sep 8, 2022

rickie Sep 8, 2022

jeandersonbc Sep 9, 2022

rickie Sep 8, 2022

rickie Sep 8, 2022

rickie Sep 8, 2022

rickie left a comment

rickie Sep 8, 2022

rickie left a comment •

edited by Stephan202

Loading

rickie Sep 9, 2022

rickie commented Sep 9, 2022

Stephan202 left a comment

Stephan202 Sep 10, 2022

Stephan202 Sep 10, 2022

rickie commented Sep 10, 2022

	boolean before(@Nullable T ref) {
	boolean before(@Nullable Object object) {

Prefer simple null reference check over calling Objects#{isNull,nonNull} #228

Prefer simple null reference check over calling Objects#{isNull,nonNull} #228

Conversation

jeandersonbc commented Sep 8, 2022 • edited Loading

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

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment • edited by Stephan202 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Sep 9, 2022

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

rickie commented Sep 10, 2022

Prefer simple `null` reference check over calling `Objects#{isNull,nonNull}` #228

Prefer simple `null` reference check over calling `Objects#{isNull,nonNull}` #228

jeandersonbc commented Sep 8, 2022 •

edited

Loading

rickie left a comment •

edited by Stephan202

Loading