Introduce `OptionalOrElse` check #1024

Stephan202 · 2024-02-10T15:01:28Z

I found a branch from back in 2021; decided to dust it off :)

Suggested commit message:

Introduce `OptionalOrElse` check (#1024)

While there, extend the `OptionalIdentity` Refaster rule to
automatically resolve one class of `NestedOptionals` violations.

github-actions · 2024-02-10T15:04:05Z

Looks good. All 35 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.runner.Refaster`	0	1
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElse`	0	34

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-02-11T15:19:57Z

Looks good. All 35 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.runner.Refaster`	0	1
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElse`	0	34

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2024-02-11T17:11:12Z

Rebased and added a commit to utilize JDK 17 functionality.

sonarqubecloud · 2024-02-11T17:13:18Z

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-02-11T17:13:43Z

Looks good. All 37 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.runner.Refaster`	0	1
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElse`	0	36

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah

Sorry for slow review 🐢

I have some questions 🙌

mohamedsamehsalah · 2024-02-16T15:36:31Z

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

@@ -442,9 +446,7 @@ Optional<? extends T> after(Optional<S> optional, Function<? super S, ? extends
  static final class OptionalStream<T> {
    @BeforeTemplate
    Stream<T> before(Optional<T> optional) {
-      return Refaster.anyOf(
-          optional.map(Stream::of).orElse(Stream.empty()),


Just to align my thoughts on this.

We are making an assumption that (the new) BugChecker will flag this, so no need for refaster rule, which makes sense 👍

(This is the part where I need help in)
However, doesn't this conflict with the assumption here that we don't rely on the existence of NestedOptional BugChecker ?

Good question @mohamedsamehsalah. This is explained by a crucial difference between NestedOptionals and OptionalOrElse: the latter has a SuggestedFix (which then obviates the need for a corresponding Refaster rule), while the former only flags issues, but doesn't provide a fix (which means that there's still value in providing a Refaster rule for a subset of violations).

mohamedsamehsalah · 2024-02-16T16:03:56Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java

+            "    optional.orElse(new int[0].length);",
+            "  }",
+            "",
+            "  private <T> T foo() {",
+            "    return null;",
+            "  }",
+            "",
+            "  private <S, T> T bar() {",
+            "    return null;",
+            "  }",
+            "}")


If I do this

Suggested change

" optional.orElse(new int[0].length);",

" }",

"",

" private <T> T foo() {",

" return null;",

" }",

"",

" private <S, T> T bar() {",

" return null;",

" }",

"}")

" optional.orElse(new int[0].length);",

" optional.orElse(B.NUMBER);",

" optional.orElse(B.getNumber());",

" }",

"",

" private <T> T foo() {",

" return null;",

" }",

"",

" private <S, T> T bar() {",

" return null;",

" }",

"",

" class B {",

" static final int NUMBER = 1;",

"",

" static String getNumber() {",

" return \"number\";",

" }",

" }",

"}")

Shouldn't we expect optional.orElse(B.NUMBER) to be optional.orElseGet(() -> B.NUMBER) ?

This is what I get instead

This is intended :). The idea here is that field dereferencing (foo.bar.baz etc.) is very efficient (conceptually: just some pointer dereferencing), so there's no point in deferring that work.

Stephan202

Tnx for reviewing @mohamedsamehsalah! 🙏

Stephan202 · 2024-02-17T10:51:41Z

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

@@ -442,9 +446,7 @@ Optional<? extends T> after(Optional<S> optional, Function<? super S, ? extends
  static final class OptionalStream<T> {
    @BeforeTemplate
    Stream<T> before(Optional<T> optional) {
-      return Refaster.anyOf(
-          optional.map(Stream::of).orElse(Stream.empty()),


Good question @mohamedsamehsalah. This is explained by a crucial difference between NestedOptionals and OptionalOrElse: the latter has a SuggestedFix (which then obviates the need for a corresponding Refaster rule), while the former only flags issues, but doesn't provide a fix (which means that there's still value in providing a Refaster rule for a subset of violations).

Stephan202 · 2024-02-17T11:51:22Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java

+            "    optional.orElse(new int[0].length);",
+            "  }",
+            "",
+            "  private <T> T foo() {",
+            "    return null;",
+            "  }",
+            "",
+            "  private <S, T> T bar() {",
+            "    return null;",
+            "  }",
+            "}")


This is intended :). The idea here is that field dereferencing (foo.bar.baz etc.) is very efficient (conceptually: just some pointer dereferencing), so there's no point in deferring that work.

mohamedsamehsalah

Thanks for answering my questions @Stephan202 🚀

rickie · 2024-03-23T19:17:38Z

/integration-test

rickie

Slightly tweaked suggested commit message.

Nice PR 🚀 !

Curious to see if it has an impact on the Checkstyle integration test, so added a comment.

rickie · 2024-03-23T19:16:56Z

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

@@ -386,9 +386,13 @@ Optional<T> after(Optional<T> optional1, Optional<T> optional2) {
   */
  static final class OptionalIdentity<T> {
    @BeforeTemplate
+    @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)


Suggested change

@SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)

@SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` violation. */)

Not wrong, but this is an alternative.

Hmm, I'm not feeling this suggestion, TBH. Elsewhere we use /* This violation will be rewritten. */, perhaps that would work? (If so, we should do the same for OptionalOrOtherOptional.)

^ So far /* This violation will be rewritten. */ is used only for SonarCloud violations. In e.g ReactorRules and TimeRules I see that NestedPublishers and TimeZoneUsage violations are suppressed with out a comment. Likely we should add some text in each case, but that's for another PR, then. Will drop it here.

Stephan202

Tnx for the typo fix in the suggested commit message!

Stephan202 · 2024-03-23T19:52:05Z

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

@@ -386,9 +386,13 @@ Optional<T> after(Optional<T> optional1, Optional<T> optional2) {
   */
  static final class OptionalIdentity<T> {
    @BeforeTemplate
+    @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)


Hmm, I'm not feeling this suggestion, TBH. Elsewhere we use /* This violation will be rewritten. */, perhaps that would work? (If so, we should do the same for OptionalOrOtherOptional.)

Stephan202 · 2024-03-24T07:54:45Z

/integration-test

Stephan202

Added a commit.

Stephan202 · 2024-03-24T08:16:35Z

integration-tests/checkstyle-expected-changes.patch

+@@ -1081,7 +1083,7 @@ public class XdocsPagesTest {
+         Optional.ofNullable(field)
+             .map(nonNullField -> nonNullField.getAnnotation(XdocsPropertyType.class))
+             .map(propertyType -> propertyType.value().getDescription())
+-            .orElse(fieldClass.getSimpleName());
+            .orElseGet(fieldClass::getSimpleName);
+     final String expectedValue =
+         getModulePropertyExpectedValue(sectionName, propertyName, field, fieldClass, instance);
+


With this change the integration test passes.

Stephan202 · 2024-03-24T08:23:52Z

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

@@ -386,9 +386,13 @@ Optional<T> after(Optional<T> optional1, Optional<T> optional2) {
   */
  static final class OptionalIdentity<T> {
    @BeforeTemplate
+    @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)


^ So far /* This violation will be rewritten. */ is used only for SonarCloud violations. In e.g ReactorRules and TimeRules I see that NestedPublishers and TimeZoneUsage violations are suppressed with out a comment. Likely we should add some text in each case, but that's for another PR, then. Will drop it here.

github-actions · 2024-03-24T08:28:52Z

Looks good. All 37 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.runner.Refaster`	0	1
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElse`	0	36

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

While there, extend the `OptionalIdentity` Refaster rule to automatically resolve once class of `NestedOptionals` violations.

rickie · 2024-03-25T08:16:43Z

Thanks for fixing the test and answering my questions :). Code LGTM, let's merge!

sonarqubecloud · 2024-03-25T08:17:22Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-03-25T08:18:01Z

Looks good. All 37 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.runner.Refaster`	0	1
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElse`	0	36

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 added this to the 0.16.0 milestone Feb 10, 2024

Stephan202 requested review from rickie and mohamedsamehsalah February 10, 2024 15:01

Stephan202 force-pushed the sschroevers/optional-improvements branch from cdced30 to 41e4d30 Compare February 11, 2024 15:17

Stephan202 force-pushed the sschroevers/optional-improvements branch from 41e4d30 to 2cc1e26 Compare February 11, 2024 17:11

mohamedsamehsalah reviewed Feb 16, 2024

View reviewed changes

Stephan202 commented Feb 17, 2024

View reviewed changes

mohamedsamehsalah approved these changes Feb 18, 2024

View reviewed changes

rickie added the new feature label Feb 21, 2024

rickie modified the milestones: 0.16.0, 0.17.0 Mar 8, 2024

Stephan202 force-pushed the sschroevers/optional-improvements branch from 2cc1e26 to 4ada3b2 Compare March 17, 2024 10:00

rickie approved these changes Mar 23, 2024

View reviewed changes

Stephan202 commented Mar 23, 2024

View reviewed changes

Stephan202 force-pushed the sschroevers/optional-improvements branch from 4ada3b2 to 8326173 Compare March 23, 2024 22:04

Stephan202 commented Mar 24, 2024

View reviewed changes

Stephan202 added 4 commits March 25, 2024 09:15

Introduce OptionalOrElse check

7b86718

While there, extend the `OptionalIdentity` Refaster rule to automatically resolve once class of `NestedOptionals` violations.

Use JDK 17 features

5195120

Sync integration test

50dfd35

Tweak

c05aa21

rickie force-pushed the sschroevers/optional-improvements branch from b48264f to c05aa21 Compare March 25, 2024 08:15

Stephan202 merged commit 281a003 into master Mar 25, 2024
15 checks passed

Stephan202 deleted the sschroevers/optional-improvements branch March 25, 2024 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `OptionalOrElse` check #1024

Introduce `OptionalOrElse` check #1024

Stephan202 commented Feb 10, 2024 •

edited by rickie

Loading

github-actions bot commented Feb 10, 2024

github-actions bot commented Feb 11, 2024

Stephan202 commented Feb 11, 2024

sonarqubecloud bot commented Feb 11, 2024

github-actions bot commented Feb 11, 2024

mohamedsamehsalah left a comment

mohamedsamehsalah Feb 16, 2024

Stephan202 Feb 17, 2024

mohamedsamehsalah Feb 16, 2024 •

edited

Loading

Stephan202 Feb 17, 2024

Stephan202 left a comment

Stephan202 Feb 17, 2024

Stephan202 Feb 17, 2024

mohamedsamehsalah left a comment

rickie commented Mar 23, 2024

rickie left a comment

rickie Mar 23, 2024

Stephan202 Mar 23, 2024

Stephan202 Mar 24, 2024

Stephan202 left a comment

Stephan202 Mar 23, 2024

Stephan202 commented Mar 24, 2024

Stephan202 left a comment

Stephan202 Mar 24, 2024

Stephan202 Mar 24, 2024

github-actions bot commented Mar 24, 2024

rickie commented Mar 25, 2024 •

edited

Loading

sonarqubecloud bot commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

	@SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)
	@SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` violation. */)

Introduce OptionalOrElse check #1024

Introduce OptionalOrElse check #1024

Conversation

Stephan202 commented Feb 10, 2024 • edited by rickie Loading

github-actions bot commented Feb 10, 2024

github-actions bot commented Feb 11, 2024

Stephan202 commented Feb 11, 2024

sonarqubecloud bot commented Feb 11, 2024

Quality Gate passed

github-actions bot commented Feb 11, 2024

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedsamehsalah Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

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

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

rickie commented Mar 23, 2024

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

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Mar 24, 2024

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

github-actions bot commented Mar 24, 2024

rickie commented Mar 25, 2024 • edited Loading

sonarqubecloud bot commented Mar 25, 2024

Quality Gate passed

github-actions bot commented Mar 25, 2024

Introduce `OptionalOrElse` check #1024

Introduce `OptionalOrElse` check #1024

Stephan202 commented Feb 10, 2024 •

edited by rickie

Loading

mohamedsamehsalah Feb 16, 2024 •

edited

Loading

rickie commented Mar 25, 2024 •

edited

Loading