Rewrite `Optional#orElse` to `Optional#orElseGet` for non-constant expressions #527

mlrprananta · 2023-03-10T14:45:46Z

❗~~This is now on top of #599~~ ❗

Following an internal GH thread it felt fitting to write a rule to cover this case.

This PR proposes a Refaster rule to rewrite Optional#orElse to Optional#orElseGet for method invocation expressions, (with 2 or more args), with performance as the main benefit.

Introduce `OptionalOrElseGet` Refaster rule (#527)

While there, introduce `IsLikelyTrivialComputation` Matcher.

github-actions · 2023-03-10T14:47: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.

Stephan202

Clearly we need to do a bit more work here. To complicate matters further, this rule will introduce quite a lot of lambda expressions that we'd like to write as a method reference instead. So I wonder whether we should first finalize the (very hard) task of getting MethodReferenceUsage production ready. (I have a branch for that, but realistically it'll be months (or longer...) before I'll get back to that).

Added a small commit.

Stephan202 · 2023-03-12T10:06:36Z

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

+   * Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the given
+   * value is not a compile-time constant.
+   */
+  abstract static class OrElseToOrElseGet<T> {


Suggested change

abstract static class OrElseToOrElseGet<T> {

static final class OrElseToOrElseGet<T> {

(This one is also suggested when running ./apply-error-prone-suggestions.sh.)

Stephan202 · 2023-03-12T10:09:13Z

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

+   */
+  abstract static class OrElseToOrElseGet<T> {
+    @BeforeTemplate
+    T before(Optional<T> o, @NotMatches(CompileTimeConstantExpressionMatcher.class) T value) {


So it seems that perhaps CompileTimeConstantExpressionMatcher is a bit too strict. When we run apply-error-prone-suggestions.sh this diff is also generated:

diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java index 70282dd4..5b430448 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java @@ -67,7 +67,7 @@ final class ConflictDetectionTest { return ConflictDetection.findMethodRenameBlocker( ASTHelpers.getSymbol(tree), tree.getName() + "t", state) .map(blocker -> buildDescription(tree).setMessage(blocker).build()) - .orElse(Description.NO_MATCH); + .orElseGet(() -> Description.NO_MATCH); } } }

Similarly, as-is the rule triggers the following violation:

[WARN] /home/sschroevers/workspace/picnic/error-prone-support/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java:[124,80] [Refaster Rule] OptionalRules.OrElseToOrElseGet: Refactoring opportunity (see https://error-prone.picnic.tech/refasterrules/OptionalRules#OrElseToOrElseGet) Did you mean 'return getAnnotationValue(Severity.class, Severity::value, delegate).orElseGet(()->SUGGESTION);'?

We might need a separate Matcher that allows any constant and field dereference 🤔

Agreed, after running the script I did notice a lot of changes and some seemed unintended, in the end we're looking to prevent potentially expensive method invocations used with orElse, perhaps it would be a better to match based on method invocations instead?

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

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

+   * Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the given
+   * value is not a compile-time constant.


I'll need to think a bit about phrasing, but likely we should call out that this rule can, in rare circumstances, change application behavior. (This can happen if value matches a non-pure expression.)

github-actions · 2023-03-12T10:19:54Z

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 · 2023-03-23T10:47:14Z

Hi @mlrprananta ,

I discussed this with @Stephan202 offline and we thought of an approach and are curious to hear what you think.

this rule will introduce quite a lot of lambda expressions that we'd like to write as a method reference instead

This is a blocker for enabling this rule internally. The MethodReferenceUsage check is a difficult one that will most likely not be worked on / finished any time soon. Therefore we need to think about our options:

Just merge the rule (with improved Matcher), without enabling it internally. It's still a clear improvement and can be used by people outside of Picnic.
Create a different matcher that matches when there are 2 or more method invocations present. Those cases cannot be rewritten to a method reference and are therefore an improvement that doesn't require additional (manual) effort. This rule can be enabled internally as well. We can add an XXX to say we have to improve when the MethodReferenceUsage check is finished.

WDYT? 😄

mlrprananta · 2023-03-27T07:13:09Z

Hey @rickie, between those 2 options I would prefer the second route for now, as I don't feel comfortable not enabling it internally. Second route would be a easier to cover as well initially and as you mentioned we can improve it later on!

rickie · 2023-03-27T07:14:27Z

@mlrprananta sounds good to me! Let's go for that option 😄.

github-actions · 2023-04-05T07:29:20Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs`	0	7

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

github-actions · 2023-04-05T07:40:58Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs`	0	7

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

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

...t/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java

mlrprananta · 2023-04-05T07:50:05Z

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

-      return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE));
+      return Refaster.anyOf(
+          optional.map(predicate).orElse(false), optional.map(predicate).orElse(Boolean.FALSE));


Had to rewrite this as this was causing a "false" positive, orElse(Refaster.anyOf(false, Boolean.FALSE)) got rewritten to orElseGet(() -> Refaster.anyOf(false, Boolean.FALSE))

Doubting a little between this and adding a suppression 🤔. I'm leaning towards a suppression as this might raise questions, but let's see what @Stephan202 thinks.

Ah, ideally the new matcher ignores Refaster.anyOf calls, since those aren't true method invocations.

... but the effort of doing this and properly testing it may not be worth it, so let's go with a suppression :)

github-actions · 2023-04-07T08:01:35Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs`	0	7

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

github-actions · 2023-04-07T14:00:01Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs`	0	7

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

sonarqubecloud · 2023-04-08T12:42:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

rickie

Rebased and added a commit. There is the generalization of the Matcher left to also account for multiple method invocations :).

Cool stuff 🚀 !

rickie · 2023-04-07T14:07:12Z

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

-      return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE));
+      return Refaster.anyOf(
+          optional.map(predicate).orElse(false), optional.map(predicate).orElse(Boolean.FALSE));


Doubting a little between this and adding a suppression 🤔. I'm leaning towards a suppression as this might raise questions, but let's see what @Stephan202 thinks.

rickie · 2023-04-07T14:08:51Z

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

+  // XXX: Extend rule to all method invocations (with less than 2 arguments).
+  static final class OptionalOrElseGet<T> {
+    @BeforeTemplate
+    T before(Optional<T> o, @Matches(IsMethodInvocationWithTwoOrMoreArgs.class) T value) {


Suggested change

T before(Optional<T> o, @Matches(IsMethodInvocationWithTwoOrMoreArgs.class) T value) {

T before(Optional<T> optional, @Matches(IsMethodInvocationWithTwoOrMoreArgs.class) T value) {

This is used more often in the file, so lets change.

rickie · 2023-04-21T09:27:30Z

...t/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java

+  @Test
+  void matches() {
+    CompilationTestHelper.newInstance(
+            IsMethodInvocationWithTwoOrMoreArgsTest.MatcherTestChecker.class, getClass())


Suggested change

IsMethodInvocationWithTwoOrMoreArgsTest.MatcherTestChecker.class, getClass())

MatcherTestChecker.class, getClass())

We can drop the qualifier.

rickie · 2023-04-21T09:31:27Z

.../main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java

+  private static final long serialVersionUID = 1L;
+
+  /** Instantiates a new {@link IsMethodInvocationWithTwoOrMoreArgs} instance. */
+  public IsMethodInvocationWithTwoOrMoreArgs() {}


Interesting, here we discussed that we want to check for two MethodInvocations instead of one method invocation wit two arguments. This is not incorrect though. I think it is nice to combine the two things into one Matcher and call it something like: IsChainedMethodCallOrHasAtLeastTwoArgs. I tried implementing this but had some troubles with the VisitorState in the matches method.

In the AbstractMatcherTestChecker we only pass the VisitorState from the CompilationUnit which is not the state that we want. So we need to probably change that setup a bit. For now I'll flush the changes that I have.

Interesting, here we discussed that we want to check for two MethodInvocations instead of one method invocation wit two arguments.

Ah, that was a misunderstanding on my part, but indeed both could apply for this situation. I can look into it a bit more to implement a (combined) matcher for chained method calls as well

Hey @rickie, few questions:

Could you elaborate on the VisitorState issue you mentioned? Which state should we be using instead?

Could nested method invocations also apply for this situation? So something like:

m1(m2("param"))

Indeed, the matcher shouldn't require >= 2 arguments: foo().bar() doesn't have any arguments, yet should be matched. One option is to use a TreeScanner (see examples in the code base) to count the number of MethodInvocationTrees.

IIUC the idea is to match non-trivial expressions that can't (or shouldn't) be replaced with a method reference. In this case we know that we're rewriting the expression to a Supplier<T>. So perhaps we're looking for a matcher of expressions which:

Contain more than one method invocation; or

Contain at least one method invocation with one or more arguments.

(Invocations of a single nullary method such as foo(), x.foo() or x.y.foo() should not be matched for now, since in those case a method reference may be preferable.)

That said, expressions of the form 42 * foo() should also be deferred.... So perhaps we should instead introduce an IsLikelyTrivialComputation matcher that does match true, x, foo(), x.foo(), x.y.foo() etc., and then have the Refaster rule use @NotMatches(IsLikelyTrivialComputation.class). Then later once MethodReferenceUsage works properly, we can replace this matcher with a simpler RequiresComputation matcher and then use @Matches(RequiresComputation.class).

In the AbstractMatcherTestChecker we only pass the VisitorState from the CompilationUnit which is not the state that we want. So we need to probably change that setup a bit. For now I'll flush the changes that I have.

I have a fix for this on a branch somewhere; let me try to open a PR for this ~soon.

I filed #599. As stated there, my suggestion would be to rebase this branch on top of that branch.

@Stephan202 Had some time to look at this again, and realized that I did not have a clear look at this before 😅

Indeed we're trying to only convert non-trivial expressions into Supplier<T>, and avoiding expressions that could be converted into a method reference.

I like the idea of the IsLikelyTrivialComputation matcher which would match trivial cases + method reference cases. However, IIUC, any expression that ends with a nullary method could in this case be converted into a method reference (even if it has 2 or more method invocations), e.g:

x() -> this::x x().y() -> x()::y x.y() -> x::y x.y.z() -> x.y::z x.y().z() -> x.y()::z x.y("x").z() -> x.y("x")::z

etc.
This would mean the matcher should match any expressions that ends the chain with a nullary method invocation, right?

However I do see that IntelliJ mentions semantics might be changed when converting these "chain" expressions into method references, and perhaps it was already decided beforehand that we will not cover these cases for the method reference bug checker?

Nice examples @mlrprananta !

I checked in IntelliJ and saw that it mentions this optimization once you press alt+enter on the specific method. However, it doesn't highlight the method invocation to indicate you should rewrite it to a method reference, which for simple cases IntelliJ does. Because for the more difficult ones it doesn't do that, I would say it is fine for now to consider these things as a non trivial computation and we can rewrite them. Probably the last three examples will not occur that often.

So I think the fist three examples are on the "edge" of being a "trivial" computation... I'm not sure about those. The others can definitely be rewritten in the context of this PR.

Discussed this with @Stephan202 offline;

We can stick to our original plan, the IsChainedMethodCallOrHasAtLeastTwoArgs matcher (name could be improved maybe) to flag two or more method invocations or when an invocation has two arguments.
This would flag the occurrences shown in the examples for which it is a little awkward to rewrite them to method references.
A problem is that there are many edge cases and questions that we can raise about the "boundary" of a IsLikelyTrivialComputation Matcher as you can go quite far with the things that you do or don't want to flag. Making it that difficult is probably not worth the time. Additionally it's less worth the effort as we should finish the MethodReferenceUsage check at some point anyways.

Alright, so I had a close look at this, and pushed a proposal 🙃

rickie · 2023-04-21T09:33:18Z

...t/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java

+            "",
+            "  String positive2() {",
+            "    // BUG: Diagnostic contains:",
+            "    return String.format(\"%s\", \"foo\");",


We use the word "or more" for the Matcher, so we should add a test case :).

Will push some tweaks to simplify a little.

rickie · 2023-04-21T09:38:44Z

...t/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java

+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  String foo(String foo, String bar) {",


Let's move these to the bottom :).

rickie · 2023-04-21T09:47:11Z

...-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java

+
+  ImmutableSet<String> testOptionalOrElseGet() {
+    return ImmutableSet.of(
+        Optional.of("foo").orElse("bar"),


We make sure to use distinct values for every statement :).

Does it need to be distinct for mutation testing?

Nope, it's about making sure that Refaster properly moves things around (which is easier to see if all values are distinct). Unfortunately for technical reasons our Refaster tests aren't exercised by Pitest.

github-actions · 2023-04-21T09:52:19Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs`	0	4

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

github-actions · 2023-04-21T09:52:29Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs`	0	4

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

Stephan202

Interesting discussions!

Stephan202 · 2023-04-26T05:07:19Z

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

-      return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE));
+      return Refaster.anyOf(
+          optional.map(predicate).orElse(false), optional.map(predicate).orElse(Boolean.FALSE));


Ah, ideally the new matcher ignores Refaster.anyOf calls, since those aren't true method invocations.

Stephan202 · 2023-04-26T05:29:00Z

.../main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java

+  private static final long serialVersionUID = 1L;
+
+  /** Instantiates a new {@link IsMethodInvocationWithTwoOrMoreArgs} instance. */
+  public IsMethodInvocationWithTwoOrMoreArgs() {}


Indeed, the matcher shouldn't require >= 2 arguments: foo().bar() doesn't have any arguments, yet should be matched. One option is to use a TreeScanner (see examples in the code base) to count the number of MethodInvocationTrees.

IIUC the idea is to match non-trivial expressions that can't (or shouldn't) be replaced with a method reference. In this case we know that we're rewriting the expression to a Supplier<T>. So perhaps we're looking for a matcher of expressions which:

Contain more than one method invocation; or

Contain at least one method invocation with one or more arguments.

(Invocations of a single nullary method such as foo(), x.foo() or x.y.foo() should not be matched for now, since in those case a method reference may be preferable.)

That said, expressions of the form 42 * foo() should also be deferred.... So perhaps we should instead introduce an IsLikelyTrivialComputation matcher that does match true, x, foo(), x.foo(), x.y.foo() etc., and then have the Refaster rule use @NotMatches(IsLikelyTrivialComputation.class). Then later once MethodReferenceUsage works properly, we can replace this matcher with a simpler RequiresComputation matcher and then use @Matches(RequiresComputation.class).

Stephan202 · 2023-04-26T05:30:31Z

.../main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java

+  private static final long serialVersionUID = 1L;
+
+  /** Instantiates a new {@link IsMethodInvocationWithTwoOrMoreArgs} instance. */
+  public IsMethodInvocationWithTwoOrMoreArgs() {}


In the AbstractMatcherTestChecker we only pass the VisitorState from the CompilationUnit which is not the state that we want. So we need to probably change that setup a bit. For now I'll flush the changes that I have.

I have a fix for this on a branch somewhere; let me try to open a PR for this ~soon.

Stephan202 · 2023-04-26T05:31:57Z

...-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java

+
+  ImmutableSet<String> testOptionalOrElseGet() {
+    return ImmutableSet.of(
+        Optional.of("foo").orElse("bar"),


Nope, it's about making sure that Refaster properly moves things around (which is easier to see if all values are distinct). Unfortunately for technical reasons our Refaster tests aren't exercised by Pitest.

rickie · 2023-04-26T06:02:56Z

Nice answers @Stephan202 😄, curious to see the PR 🚀 !

rickie · 2023-04-26T15:29:47Z

I changed the base to #599 already 😄.

Stephan202

Added a commit; PTAL :)

(I'd be nice to get some reviews of the base PR 🙃)

Stephan202 · 2023-05-20T18:58:50Z

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

-      return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE));
+      return Refaster.anyOf(
+          optional.map(predicate).orElse(false), optional.map(predicate).orElse(Boolean.FALSE));


... but the effort of doing this and properly testing it may not be worth it, so let's go with a suppression :)

Stephan202 · 2023-05-20T19:02:04Z

...-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java

+        Optional.of("foo").orElse("bar"),
+        Optional.of("baz").orElse(toString()),
+        Optional.of("qux").orElse(String.valueOf(true)),
+        Optional.of("quux").orElse(String.format("%s", "quuz")),
+        Optional.of("corge").orElse(String.format("%s", "grault", "garply")));


I guess we can do with fewer test cases :)

Stephan202 · 2023-05-20T19:02:43Z

.../main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java

+  private static final long serialVersionUID = 1L;
+
+  /** Instantiates a new {@link IsMethodInvocationWithTwoOrMoreArgs} instance. */
+  public IsMethodInvocationWithTwoOrMoreArgs() {}


Alright, so I had a close look at this, and pushed a proposal 🙃

github-actions · 2023-05-20T19:07:30Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsLikelyTrivialComputation`	0	45

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

rickie

One consideration, we already discussed that we don't want too many edge cases for this Matcher, so maybe it's already fine like this.

Anyway, really nice improvement! Curious to see how this performs on our codebase. 🚀 !

rickie · 2023-05-25T14:21:52Z

...t/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java

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


For the BinaryTree we can check if both left and right are an LiteralTree or IdentifierTree as start? Also fine with leaving this as is.

Yeah, one could argue that this is a trivial computation. But then maybe so is 4 * 60 * 60 ("four hours"), which still wouldn't be flagged. And since the matcher is already quite complex, I'm not sure how far to go for diminishing returns. Even this case I don't expect to be triggered too frequently (at least in our internal code base).

Stephan202

Tnx for the review @rickie. @mlrprananta we need one more review of #599 before we can merge this one. Are you up for that?

Stephan202 · 2023-05-25T16:03:16Z

...t/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java

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


Yeah, one could argue that this is a trivial computation. But then maybe so is 4 * 60 * 60 ("four hours"), which still wouldn't be flagged. And since the matcher is already quite complex, I'm not sure how far to go for diminishing returns. Even this case I don't expect to be triggered too frequently (at least in our internal code base).

…nput

rickie · 2023-05-26T07:02:59Z

Fair point --> Rebased --> Will merge.

github-actions · 2023-05-26T07:04:02Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsLikelyTrivialComputation`	0	45

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

sonarqubecloud · 2023-05-26T07:04:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

rickie · 2023-05-26T07:16:05Z

Updated the suggested commit message.

Stephan202 · 2023-05-26T07:17:32Z

The second like seems now less important than perhaps it is, but I lack creativity for a better proposal. Let's go 🚀

rickie · 2023-05-26T07:19:33Z

Yeah you are right, I tried combining them but the name is too long 😅.

mlrprananta requested a review from rickie March 10, 2023 14:45

Stephan202 reviewed Mar 12, 2023

View reviewed changes

mlrprananta force-pushed the prefer-or-else-get branch from be70407 to 0709a49 Compare April 5, 2023 07:27

mlrprananta commented Apr 5, 2023

View reviewed changes

mlrprananta requested a review from Stephan202 April 5, 2023 07:53

rickie force-pushed the prefer-or-else-get branch from 229a3c0 to 0b73b98 Compare April 21, 2023 09:48

rickie reviewed Apr 21, 2023

View reviewed changes

rickie force-pushed the prefer-or-else-get branch 2 times, most recently from 5084a81 to 86c429c Compare April 21, 2023 09:50

Stephan202 reviewed Apr 26, 2023

View reviewed changes

rickie added the new feature label Apr 26, 2023

Stephan202 mentioned this pull request Apr 26, 2023

Improve AbstractMatcherTestChecker #599

Merged

rickie force-pushed the prefer-or-else-get branch from 86c429c to d61816d Compare April 26, 2023 15:28

rickie changed the base branch from master to sschroevers/improve-AbstractMatcherTestChecker April 26, 2023 15:29

Stephan202 added this to the 0.12.0 milestone May 20, 2023

Stephan202 approved these changes May 20, 2023

View reviewed changes

rickie force-pushed the sschroevers/improve-AbstractMatcherTestChecker branch from 1d630d7 to 386fd74 Compare May 25, 2023 11:37

rickie approved these changes May 25, 2023

View reviewed changes

Stephan202 approved these changes May 25, 2023

View reviewed changes

Base automatically changed from sschroevers/improve-AbstractMatcherTestChecker to master May 26, 2023 06:29

mlrprananta and others added 9 commits May 26, 2023 08:33

Prefer Optional#orElseGet over Optional#orElse for non-compile-time i…

5269957

…nput

Tweak

435f102

Suggestions

d7274f4

Fix

2e7cde9

Suggestions

78a3c79

Suggestions

b68d264

Suggestions and tweaksg

7e224ad

Suggestions

d3f64af

Post-rebase fix

ead55db

rickie force-pushed the prefer-or-else-get branch from a127136 to ead55db Compare May 26, 2023 07:02

rickie merged commit cc2c49e into master May 26, 2023

rickie deleted the prefer-or-else-get branch May 26, 2023 07:19

	abstract static class OrElseToOrElseGet<T> {
	static final class OrElseToOrElseGet<T> {

		* Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the given
		* value is not a compile-time constant.

	T before(Optional<T> o, @Matches(IsMethodInvocationWithTwoOrMoreArgs.class) T value) {
	T before(Optional<T> optional, @Matches(IsMethodInvocationWithTwoOrMoreArgs.class) T value) {

	IsMethodInvocationWithTwoOrMoreArgsTest.MatcherTestChecker.class, getClass())
	MatcherTestChecker.class, getClass())

Rewrite Optional#orElse to Optional#orElseGet for non-constant expressions #527

Rewrite Optional#orElse to Optional#orElseGet for non-constant expressions #527

Conversation

mlrprananta commented Mar 10, 2023 • edited by rickie Loading

github-actions bot commented Mar 10, 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

github-actions bot commented Mar 12, 2023

rickie commented Mar 23, 2023 • edited Loading

mlrprananta commented Mar 27, 2023

rickie commented Mar 27, 2023

github-actions bot commented Apr 5, 2023

github-actions bot commented Apr 5, 2023

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 Apr 7, 2023

github-actions bot commented Apr 7, 2023

sonarqubecloud bot commented Apr 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

mlrprananta May 12, 2023 • edited Loading

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 Apr 21, 2023

github-actions bot commented Apr 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

rickie commented Apr 26, 2023

rickie commented Apr 26, 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

github-actions bot commented May 20, 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

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented May 26, 2023

github-actions bot commented May 26, 2023

sonarqubecloud bot commented May 26, 2023

rickie commented May 26, 2023

Stephan202 commented May 26, 2023

rickie commented May 26, 2023

Rewrite `Optional#orElse` to `Optional#orElseGet` for non-constant expressions #527

Rewrite `Optional#orElse` to `Optional#orElseGet` for non-constant expressions #527

mlrprananta commented Mar 10, 2023 •

edited by rickie

Loading

rickie commented Mar 23, 2023 •

edited

Loading

Stephan202 Apr 26, 2023 •

edited

Loading

mlrprananta May 12, 2023 •

edited

Loading