Improve `Optional#orElse{,Get}` support #1283

Stephan202 · 2024-08-11T21:00:58Z

Suggested commit message:

Improve `Optional#orElse{,Get}` support (#1283)

Summary of changes:
- Consolidate the `OptionalOrElseGet` Refaster rule and the 
  `OptionalOrElse` bug checker into the latter, keeping the best of
  both.
- Rename the `OptionalOrElse` bug checker to `OptionalOrElseGet` to
  avoid confusion.
- Replace the `IsLikelyTrivialComputation` matcher with the inverse
  `RequiresComputation` variant.
- Introduce an `OptionalOrElse` Refaster rule that simplifies
  expressions in the "opposite direction".

github-actions · 2024-08-11T21:03:44Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.RequiresComputation`	0	12
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElseGet`	0	6

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

Stephan202

Some context.

Stephan202 · 2024-08-11T21:02:12Z

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

-  /**
-   * Tells whether the given expression contains anything other than a literal or a (possibly
-   * dereferenced) variable or constant.
-   */
-  private static boolean requiresComputation(ExpressionTree tree) {
-    return !(tree instanceof IdentifierTree
-        || tree instanceof LiteralTree
-        || (tree instanceof MemberSelectTree memberSelect
-            && !requiresComputation(memberSelect.getExpression()))
-        || ASTHelpers.constValue(tree) != null);
-  }


Instead of two not-quite-complimentary implementations, all logic now lives in the RequiresComputation matcher.

Stephan202 · 2024-08-11T21:02:51Z

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

+// XXX: This rule may introduce a compilation error: the `value` expression may reference a
+// non-effectively final variable, which is not allowed in the replacement lambda expression.
+// Review whether a `@Matcher` can be used to avoid this.


This observation was only moved from the Refaster rule to here. Fixing it is out of scope.

Stephan202 · 2024-08-11T21:03:45Z

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

  /**
-   * Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the
-   * fallback value is not the result of a trivial computation.
+   * Prefer {@link Optional#orElse(Object)} over {@link Optional#orElseGet(Supplier)} if the
+   * fallback value does not require non-trivial computation.
   */
-  // XXX: This rule may introduce a compilation error: the `value` expression may reference a
-  // non-effectively final variable, which is not allowed in the replacement lambda expression.
-  // Review whether a `@Matcher` can be used to avoid this.
-  // XXX: Once `MethodReferenceUsage` is "production ready", replace
-  // `@NotMatches(IsLikelyTrivialComputation.class)` with `@Matches(RequiresComputation.class)` (and
-  // reimplement the matcher accordingly).
-  static final class OptionalOrElseGet<T> {
+  // XXX: This rule is the counterpart to the `OptionalOrElseGet` bug checker. Once the
+  // `MethodReferenceUsage` bug checker is "production ready", that bug checker may similarly be
+  // replaced with a Refaster rule.
+  static final class OptionalOrElse<T> {
    @BeforeTemplate
-    T before(Optional<T> optional, @NotMatches(IsLikelyTrivialComputation.class) T value) {
-      return optional.orElse(value);
+    T before(Optional<T> optional, @NotMatches(RequiresComputation.class) T value) {
+      return optional.orElseGet(() -> value);
    }

    @AfterTemplate
    T after(Optional<T> optional, T value) {
-      return optional.orElseGet(() -> value);
+      return optional.orElse(value);
    }
  }


Rule replaced: the old definition is absorbed into the bug checker, while the new rule implements complementary logic.

Stephan202 · 2024-08-11T21:04:29Z

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

+    @SuppressWarnings({
+      "LexicographicalAnnotationAttributeListing" /* `key-*` entry must remain last. */,
+      "NestedOptionals" /* This violation will be rewritten. */,
+      "OptionalOrElse" /* Here `optional2` is a stand-in for expressions that may require computation. */,
+      "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict"
+    })


Yes, this is getting messy. Maybe those key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict entries should start with zzz- or something.

Stephan202 · 2024-08-11T21:06:53Z

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

@@ -27,6 +28,7 @@ void identification() {
            "    optional.orElse(string);",
            "    optional.orElse(this.string);",
            "    optional.orElse(Refaster.anyOf(\"constant\", \"another\"));",
+            "    Optional.<Supplier<String>>empty().orElse(() -> \"constant\");",


New test for the case that triggered this PR, observed with Picnic-internal code.

To be clearer: previously the bug checker would not see the lambda expression as a trivial computation, while the Refaster rule would. In this context the latter was right.

github-actions · 2024-08-11T21:10:50Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.RequiresComputation`	0	12
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElseGet`	0	6

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

github-actions · 2024-08-11T21:18:53Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.RequiresComputation`	0	12
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElseGet`	0	6

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

Stephan202

More context.

Stephan202 · 2024-08-11T21:23:02Z

...-support/src/test/java/tech/picnic/errorprone/refaster/matchers/RequiresComputationTest.java

+            "  OutputStream negative9() {",
+            "    return System.out;",


This new test is to cover existing code that was previously untested. ~~I filed hcoles/pitest#1341~~ Doh.

Stephan202 · 2024-08-11T21:23:58Z

...-support/src/test/java/tech/picnic/errorprone/refaster/matchers/RequiresComputationTest.java

            "  }",
            "",
-            "  int negative6() {",
-            "    return 1 * 2;",


Previously the bug checker would see this as a trivial computation, but not the Refaster rule. The former was right.

Stephan202 · 2024-08-11T21:25:17Z

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

@@ -27,6 +28,7 @@ void identification() {
            "    optional.orElse(string);",
            "    optional.orElse(this.string);",
            "    optional.orElse(Refaster.anyOf(\"constant\", \"another\"));",
+            "    Optional.<Supplier<String>>empty().orElse(() -> \"constant\");",


To be clearer: previously the bug checker would not see the lambda expression as a trivial computation, while the Refaster rule would. In this context the latter was right.

github-actions · 2024-08-11T22:04:24Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.RequiresComputation`	0	12
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElseGet`	0	6

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

github-actions · 2024-08-12T05:51:55Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.RequiresComputation`	0	10
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElseGet`	0	6

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

mohamedsamehsalah

🚀 🚀

rickie

LGTM! Nice improvements.

Summary of changes: - Consolidate the `OptionalOrElseGet` Refaster rule and the `OptionalOrElse` bug checker into the latter, keeping the best of both. - Rename the `OptionalOrElse` bug checker to `OptionalOrElseGet` to avoid confusion. - Replace the `IsLikelyTrivialComputation` matcher with the inverse `RequiresComputation` variant. - Introduce an `OptionalOrElse` Refaster rule that simplifies expressions in the "opposite direction".

sonarqubecloud · 2024-09-03T13:37:53Z

Quality Gate passed

Issues
1 New issue
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-09-03T13:38:21Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.RequiresComputation`	0	10
🎉`tech.picnic.errorprone.bugpatterns.OptionalOrElseGet`	0	6

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

pzygielo · 2024-11-01T11:48:26Z

After upgrading 0.18.0 -> 0.19.0, with error-prone 2.35.1, Java Temurin-21.0.5+11, Apache Maven 3.9.9, I'm getting:

[ERROR] error: An unhandled exception was thrown by the Error Prone static analysis plugin.

which seems to be related to this change.

Stephan202 · 2024-11-01T12:07:59Z

@pzygielo interesting. Do you have a stack trace/example code/reproducer? Happy to take a look!

pzygielo · 2024-11-01T12:13:37Z

Unfortunately stack trace dumped by maven contains only maven calls, I can't see anything from javac there.

I'm in the middle of preparation of minimal reproducer from the bigger project I see this in. Will share of course.

Stephan202 · 2024-11-01T12:32:23Z

Sometimes passing -e and/or -X to Maven will help. Good luck! 🙏

pzygielo · 2024-11-01T13:25:29Z

https://github.com/pzrep/picnic-error-prone-support-1283 - Bump tech.picnic.error-prone-support:error-prone-contrib from 0.18.0 to 0.19.0 pzrep/picnic-error-prone-support-1283#1

And the run for the same code with 0.18.0: https://github.com/pzrep/picnic-error-prone-support-1283/actions/runs/11630035676/job/32388147249

pzygielo · 2024-11-01T14:01:10Z

error: An unhandled exception was thrown by the Error Prone static analysis plugin.
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.35.1
     BugPattern: (see stack trace)
     Stack Trace:
     java.lang.NoClassDefFoundError: tech/picnic/errorprone/refaster/matchers/RequiresComputation
  	at tech.picnic.errorprone.bugpatterns.OptionalOrElseGet.<clinit>(OptionalOrElseGet.java:54)
  	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
...

pzygielo · 2024-11-01T14:15:08Z

Perhaps tech.picnic.error-prone-support:refaster-support dependency shall be promoted from provided scope.

Stephan202 · 2024-11-01T20:01:18Z

Perhaps tech.picnic.error-prone-support:refaster-support dependency shall be promoted from provided scope.

Exactly right! I filed #1387 with a fix. Thanks for the super clear reproducer @pzygielo!

Stephan202 added the improvement label Aug 11, 2024

Stephan202 added this to the 0.19.0 milestone Aug 11, 2024

Stephan202 requested review from rickie and mohamedsamehsalah August 11, 2024 21:00

Stephan202 commented Aug 11, 2024

View reviewed changes

Stephan202 mentioned this pull request Aug 11, 2024

No mutation for if-statement with instanceof pattern match hcoles/pitest#1341

Closed

Stephan202 commented Aug 11, 2024

View reviewed changes

mohamedsamehsalah approved these changes Aug 12, 2024

View reviewed changes

rickie approved these changes Sep 3, 2024

View reviewed changes

Stephan202 added 5 commits September 3, 2024 15:35

Fix search-and-replace error

2a6fd00

Increase coverage (surprised that Pitest didn't catch this one)

26e8788

Yet more coverage

8b22398

Resolve some SonarCloud complaints

69d9d62

rickie force-pushed the sschroevers/optional-or-else-get-improvements branch from af624dc to 69d9d62 Compare September 3, 2024 13:35

rickie merged commit c679a3f into master Sep 3, 2024
16 checks passed

rickie deleted the sschroevers/optional-or-else-get-improvements branch September 3, 2024 14:00

Stephan202 mentioned this pull request Nov 1, 2024

Fix error-prone-contrib runtime classpath #1387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Optional#orElse{,Get}` support #1283

Improve `Optional#orElse{,Get}` support #1283

Stephan202 commented Aug 11, 2024 •

edited

Loading

github-actions bot commented Aug 11, 2024

Stephan202 left a comment

Stephan202 Aug 11, 2024

Stephan202 Aug 11, 2024

Stephan202 Aug 11, 2024

Stephan202 Aug 11, 2024

Stephan202 Aug 11, 2024

Stephan202 Aug 11, 2024

github-actions bot commented Aug 11, 2024

github-actions bot commented Aug 11, 2024

Stephan202 left a comment

Stephan202 Aug 11, 2024 •

edited

Loading

Stephan202 Aug 11, 2024

Stephan202 Aug 11, 2024

github-actions bot commented Aug 11, 2024

github-actions bot commented Aug 12, 2024

mohamedsamehsalah left a comment

rickie left a comment

sonarqubecloud bot commented Sep 3, 2024

github-actions bot commented Sep 3, 2024

pzygielo commented Nov 1, 2024

Stephan202 commented Nov 1, 2024

pzygielo commented Nov 1, 2024

Stephan202 commented Nov 1, 2024

pzygielo commented Nov 1, 2024

pzygielo commented Nov 1, 2024

pzygielo commented Nov 1, 2024

Stephan202 commented Nov 1, 2024

Improve Optional#orElse{,Get} support #1283

Improve Optional#orElse{,Get} support #1283

Conversation

Stephan202 commented Aug 11, 2024 • edited Loading

github-actions bot commented Aug 11, 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

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 Aug 11, 2024

github-actions bot commented Aug 11, 2024

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Aug 11, 2024 • edited Loading

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 Aug 11, 2024

github-actions bot commented Aug 12, 2024

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 3, 2024

Quality Gate passed

github-actions bot commented Sep 3, 2024

pzygielo commented Nov 1, 2024

Stephan202 commented Nov 1, 2024

pzygielo commented Nov 1, 2024

Stephan202 commented Nov 1, 2024

pzygielo commented Nov 1, 2024

pzygielo commented Nov 1, 2024

pzygielo commented Nov 1, 2024

Stephan202 commented Nov 1, 2024

Improve `Optional#orElse{,Get}` support #1283

Improve `Optional#orElse{,Get}` support #1283

Stephan202 commented Aug 11, 2024 •

edited

Loading

Stephan202 Aug 11, 2024 •

edited

Loading