Introduce/extend `RequireNonNullElse{,Get}` Refaster rules #425

benhalasi · 2022-12-27T17:19:27Z

Related issue: #364

Added Rules:

prefer requireNonNullElse(A, B) over Optional.ofNullable(A).orElse(B)
prefer requireNonNullElseGet(A, () -> B) over Optional.ofNullable(A).orElseGet(() -> B)

Wrote test inputs and outputs.

Potential problems:

In the tests, referenced Optional by it's fully qualified name to avoid breaking RefasterRulesTest or leaving unused imports in the code. - this seems hacky but didn't came up with better solution.

Neither the new or already existing rules are not dealing with potential NPE when the second argument can be or produce null. - this seems to be a larger or at least a separated issue.

github-actions · 2022-12-27T18:32:19Z

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 · 2022-12-27T18:32:20Z

Thanks for the PR @benhalasi! I'll have a closer look at the changes later.

In the tests, referenced Optional by it's fully qualified name to avoid breaking RefasterRulesTest or leaving unused imports in the code. - this seems hacky but didn't came up with better solution.

Yes, this a known issue. For this we have a little trick; see how elidedTypesAndStaticImports is used :).

Stephan202

Rebased and added a commit. Thanks for your contribution @benhalasi!

Suggested commit message:

Introduce/extend `RequireNonNullElse{,Get}` Refaster rules (#425)

Fixes #364.

The next release may be a few weeks out, but if you'd like to use these changes sooner, let us know and we'll see what we can do :)

Stephan202 · 2022-12-28T06:59:20Z

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

    @AfterTemplate
    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
    T after(T first, T second) {
      return requireNonNullElse(first, second);
    }
  }

+  /**
+   * Prefer {@link Objects#requireNonNullElseGet(Object, Supplier)} over {@link Optional}{@code
+   * .ofNullable(Object).orElseGet(Supplier)}


Rather than reconstructing the @BeforeTemplate code, in Javadoc we often use more generic phrasing. In other classes we often talk about "more contrived alternatives".

Stephan202 · 2022-12-28T06:59:58Z

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

+    @BeforeTemplate
+    T beforeOptional(T first, T second) {
+      return Optional.ofNullable(first).orElse(second);
+    }


Using Refaster.anyOf we can collapse the two @BeforeTemplates. We generally prefer that approach where feasible.

Stephan202 · 2022-12-28T07:01:35Z

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

+   */
+  // XXX: This rule is not valid in case `supplier` may return `null`: Optional will return `null`,
+  // while the `requireNonNullElseGet` replacement will throw an NPE.
+  static final class RequireNonNullElseGet<T, S extends Supplier<T>> {


This kind of subtype matching is done automatically by Refaster. That said, there's indeed work to be done here: we should introduce S extends T, so that we can match Supplier<S>.

github-actions · 2022-12-28T07:08: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.

rickie

Rebased and have one small remark for @Stephan202.

Aside from that; changes LGTM! Suggested commit message also LGTM!

@benhalasi Thanks for your contribution!

rickie · 2022-12-29T07:49:50Z

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

+   * Prefer {@link Objects#requireNonNullElse(Object, Object)} over non-JDK or more contrived
+   * alternatives.
+   */
+  // XXX: This rule is not valid in case `second` is `@Nullable`: in that case the Guava and


@Stephan202 we looked into a NullableMatcher related to this, maybe we should prioritize that one again sometime. Such master could be useful here.

s/master/matcher/ ;)

Indeed I started work on that here, but I recall hitting some issues. (The AbstractMatcherTestChecker changes are part of that, but IIRC there was more. Would have to check out the branch and run the tests to refresh my memory.)

github-actions · 2022-12-29T08:03: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.

rickie · 2022-12-29T08:03:05Z

Shall we add Fixes #364 to the suggested commit message and create a new ticket for the specific rewrite you mention there @Stephan202 : requireNonNullElse(map.get(k), defaultValue) -> map.getOrDefault(k, defaultValue)?

Stephan202 · 2022-12-29T08:11:01Z

Shall we add Fixes #364 to the suggested commit message and create a new ticket for the specific rewrite you mention there @Stephan202 : requireNonNullElse(map.get(k), defaultValue) -> map.getOrDefault(k, defaultValue)?

SGTM; updated the suggested commit message. (There are also @EnricSala's observations around Optional.of, which are perhaps worth tracking separately? Those may require a BugChecker, because we'll likely want two suggested fixes: (a) changing of to ofNullable and (b) using requireNonNull instead.)

rickie · 2022-12-30T11:30:14Z

Rebased.

Will soon make the earlier mentioned GH issues. Sorry, haven't come around do doing it yet. Will merge right after that.

github-actions · 2022-12-30T11:32:41Z

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.

…ireNonNullElseGet(A, () -> B)` over Optional alternative. Added Rules: (NullRules) - prefer `requireNonNullElse(A, B)` over `Optional.ofNullable(A).orElse(B)` - prefer `requireNonNullElseGet(A, () -> B)` over `Optional.ofNullable(A).orElseGet(() -> B)` Related issue: PicnicSupermarket#364

github-actions · 2023-01-02T09:09:28Z

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-01-02T13:34:27Z

The follow-up issues are #431 and #436.

benhalasi changed the title ~~Add refaster rules for prefering requireNonNullElse(A, B) and `requ…~~ Add refaster rules for prefering requireNonNullElse(A, B) over Optional alternative. Dec 27, 2022

Stephan202 force-pushed the benhalasi/prefer-require_non_null_else-over-optional branch from 1739c77 to 399fee6 Compare December 28, 2022 07:06

Stephan202 approved these changes Dec 28, 2022

View reviewed changes

Stephan202 requested a review from rickie December 28, 2022 07:13

Stephan202 added this to the 0.7.0 milestone Dec 28, 2022

rickie added the new feature label Dec 29, 2022

rickie force-pushed the benhalasi/prefer-require_non_null_else-over-optional branch from 399fee6 to ea7f5c6 Compare December 29, 2022 08:01

rickie approved these changes Dec 29, 2022

View reviewed changes

rickie force-pushed the benhalasi/prefer-require_non_null_else-over-optional branch from ea7f5c6 to 67c1519 Compare December 30, 2022 11:30

benhalasi and others added 2 commits January 2, 2023 10:07

Suggestions

8e36bd0

rickie force-pushed the benhalasi/prefer-require_non_null_else-over-optional branch from 67c1519 to 8e36bd0 Compare January 2, 2023 09:07

rickie changed the title ~~Add refaster rules for prefering requireNonNullElse(A, B) over Optional alternative.~~ Introduce/extend RequireNonNullElse{,Get} Refaster rules Jan 2, 2023

rickie merged commit d456821 into PicnicSupermarket:master Jan 2, 2023

benhalasi deleted the benhalasi/prefer-require_non_null_else-over-optional branch January 3, 2023 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce/extend `RequireNonNullElse{,Get}` Refaster rules #425

Introduce/extend `RequireNonNullElse{,Get}` Refaster rules #425

benhalasi commented Dec 27, 2022

github-actions bot commented Dec 27, 2022

Stephan202 commented Dec 27, 2022

Stephan202 left a comment •

edited

Loading

Stephan202 Dec 28, 2022

Stephan202 Dec 28, 2022

Stephan202 Dec 28, 2022

github-actions bot commented Dec 28, 2022

rickie left a comment

rickie Dec 29, 2022

Stephan202 Dec 29, 2022

github-actions bot commented Dec 29, 2022

rickie commented Dec 29, 2022

Stephan202 commented Dec 29, 2022 •

edited

Loading

rickie commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

github-actions bot commented Jan 2, 2023

Stephan202 commented Jan 2, 2023

Introduce/extend RequireNonNullElse{,Get} Refaster rules #425

Introduce/extend RequireNonNullElse{,Get} Refaster rules #425

Conversation

benhalasi commented Dec 27, 2022

github-actions bot commented Dec 27, 2022

Stephan202 commented Dec 27, 2022

Stephan202 left a comment • edited Loading

Choose a reason for hiding this comment

Stephan202 Dec 28, 2022

Choose a reason for hiding this comment

Stephan202 Dec 28, 2022

Choose a reason for hiding this comment

Stephan202 Dec 28, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 28, 2022

rickie left a comment

Choose a reason for hiding this comment

rickie Dec 29, 2022

Choose a reason for hiding this comment

Stephan202 Dec 29, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2022

rickie commented Dec 29, 2022

Stephan202 commented Dec 29, 2022 • edited Loading

rickie commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

github-actions bot commented Jan 2, 2023

Stephan202 commented Jan 2, 2023

Introduce/extend `RequireNonNullElse{,Get}` Refaster rules #425

Introduce/extend `RequireNonNullElse{,Get}` Refaster rules #425

Stephan202 left a comment •

edited

Loading

Stephan202 commented Dec 29, 2022 •

edited

Loading