[improvement] Ensure Optional#orElse argument is not method invocation #655

gatesn · 2019-06-19T20:00:55Z

Before this PR

Run into Optional#orElse code three times now that was intended to be lazy.

After this PR

Argument to Optional#orElse must not include a method invocation. Otherwise a fix is suggested to convert to Optional#orElseGet(Supplier)

…ake lazy

markelliot · 2019-06-19T20:28:57Z

I think compile time constant is too aggressive. We should just enforce it’s not an expression (a variable further up the function is totally appropriate as an argument to orElse).

…ake lazy

ferozco · 2019-06-19T21:01:46Z

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseConstant.java

+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
+        severity = SeverityLevel.ERROR,
+        summary = "Allow passing only compile-time constants to Optional#orElse.")


can you update this as well?

ferozco · 2019-06-19T21:03:40Z

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseConstant.java

+
+@AutoService(BugChecker.class)
+@BugPattern(
+        name = "OptionalOrElseConstant",


I think this is a little stale, perhaps OptionalOrElseMethodInvocation, OptionalOrElseValue?

ferozco · 2019-06-19T21:07:02Z

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseConstant.java

+                .setMessage("Optional#orElse uses a non-constant expression")
+                .addFix(SuggestedFix.builder()
+                        .postfixWith(tree.getMethodSelect(), "Get")
+                        .prefixWith(orElseArg, "() -> ")


Could we pass a method reference instead of allocating a new lamba? i.e. .orElseGet(this::foo) or orElseGet(foo)

It's not so easy to detect these things, and if we could, they should probably be done as their own check.

carterkozak · 2019-06-19T22:03:05Z

...ror-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocation.java

+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
+        severity = SeverityLevel.ERROR,
+        summary = "Ensure Optional#orElse argument does not invoke a ny methods.")


s/a ny/any

It's fairly obvious in this case, but it may be helpful to spell out exactly why orElseGet is better in the summary

carterkozak · 2019-06-19T22:04:26Z

Very excited about this check. Thanks for implementing the suggested fix, they've been invaluable for a certain large internal project!

gatesn · 2019-06-20T06:26:52Z

Addressed all comments, should be good to go

markelliot

Could we add a test that shows we prevent foo + bar as an argument to orElse?

iamdanfox · 2019-06-20T12:28:03Z

Probably worth mentioning this on the README with the other error-prone checks too pls! https://github.com/palantir/gradle-baseline#baseline-error-prone-checks

uschi2000 · 2019-06-20T13:07:50Z

...rone/src/test/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocationTests.java

+                        "    Optional.of(\"hello\").orElse(\"constant\");",
+                        "    Optional.of(\"hello\").orElse(compileTimeConstant);",
+                        "    String string = f();",
+                        "    Optional.of(\"hello\").orElse(string);",


this is missing test coverage for Optional.of(foo).orElseGet(() -> bar);

Not sure what you mean, you want this check to revert it to orElse(bar)?

I don't see a test verifying that Optional.of(foo).orElseGet(() -> bar) is an acceptable thing to write

sorry, I mean Optional.of(foo).orElseGet(() -> f());

Ah, I've done that now

uschi2000 · 2019-06-20T13:08:57Z

...rone/src/test/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocationTests.java

+                        "import java.util.Optional;",
+                        "class Test {",
+                        "  String f() { return \"hello\"; }",
+                        "  // BUG: Diagnostic contains: non-constant expression",


what does "non-constant" mean? suspect that's not quite the right word. what you mean here is "method invocation"

Well it is, but then as Mark says this doesn't cover operator expressions. Not sure what the right word is?

If I understand the Matchers.contains(ExpressionTree.class, MethodMatchers.anyMethod()) thing correctly, then it errors iff a method is called anywhere inside the orElse(...). A reasonable description would be, for instance, Expression passed to Optional#orElse must not invoke a method, or positively (not sure what the right one is), Expression passed to Optional#orElse invokes a method.

(or contains a method invocation, if you prefer). To double-check: foo.orElse(bar + baz) is allowed, ja?

Well, it is currently but should it be allowed? Are operators ever sufficiently expensive for us to care about? If yes, then we need to think again about our matcher. Perhaps compileTimeConstant OR identifier.

String concatenation using operators should be required to execute in a lambda. I don't mind the lambda allocation for math because the optional is already boxing numerical results with similar (avoidable) overhead to creating a lambda.

.. then you'll likely see

String bar = boom + baz; return foo.orElse(bar); // can't concat in here.

I don't really mind the exact version of the check, as long as you make the text reflect the check. (that's what I was commenting on initially, sorry if this was unclear.)

gatesn · 2019-06-21T08:56:30Z

Is this good to go now?

markelliot

May want to consider in a separate PR the opposite of this — if you’re calling orElseGet when you could be calling orElse it’d be good to warn (IntelliJ has that as a refactor suggestion), but not worth blocking on

iamdanfox · 2019-06-21T11:48:14Z

Didn't update the README 😢

ferozco · 2019-06-21T18:28:40Z

@gatesn I think that this check may be overly aggressive since accessing any field on an immutable is considered a method invocation. In general, I feel like this check has a very large number of false positives so we may want to lower the severity to warn or reconsider the check entirely.

…nvocation (#655)" This reverts commit f1264dd.

gatesn · 2019-06-21T19:03:01Z

@ferozco can you / @carterkozak enumerate the false positives in case we want to pick this back up?

There are a few I disagree with, for instance I'm not convinced there's a difference in accessing a property of an immutable as orElse(foo.bar()) or orElseGet(foo::bar). If its purely style, I'd strongly suggest we prefer the latter.

A viable opposition would be for example Collections.emptyList(). However it would be useful to understand all the cases you hit so we can attempt to refine the check.

markelliot · 2019-06-21T19:12:16Z

It might be that we need an automated refactor of method calls into suppliers in order to make this work. Seems reasonable that you'd orElseGet(Collections::emptyList), too.

gatesn · 2019-06-21T19:22:54Z

If that's the case, then I imagine it shouldn't be too hard to implement the logic as either in the check itself, or as part of a refaster rule

gatesn · 2019-06-24T20:51:38Z

So, we can now auto-apply error-prone fixes: https://github.com/palantir/gradle-baseline/pull/660/files

Is this sufficient? Or do we want to block on having the () -> foo() -> this::foo rewrite

Add a line for an error-prone check added in palantir#655, which is missing from the README.

Add a line for an error-prone check added in #655, which is missing from the README.

Ensure Optional#orElse argument is compile-time constant, otherwise m…

7057c7e

…ake lazy

gatesn requested a review from a team as a code owner June 19, 2019 20:00

Ensure Optional#orElse argument is compile-time constant, otherwise m…

a571b92

…ake lazy

gatesn changed the title ~~[improvement] Ensure Optional#orElse argument is compile-time constant~~ [improvement] Ensure Optional#orElse argument is not method invocation Jun 19, 2019

ferozco reviewed Jun 19, 2019

View reviewed changes

gatesn added 2 commits June 19, 2019 22:22

Rename class

6130ccf

Rename class

63526e0

carterkozak reviewed Jun 19, 2019

View reviewed changes

Update summary

42c80f8

Use static matcher

0f0a6f6

markelliot reviewed Jun 20, 2019

View reviewed changes

uschi2000 reviewed Jun 20, 2019

View reviewed changes

Reword descriptions

b21ba5c

markelliot approved these changes Jun 21, 2019

View reviewed changes

gatesn merged commit f1264dd into palantir:develop Jun 21, 2019

gatesn deleted the ngates/constant-or-else-expr branch June 21, 2019 09:54

ferozco added a commit that referenced this pull request Jun 21, 2019

Revert "[improvement] Ensure Optional#orElse argument is not method i…

f88e383

…nvocation (#655)" This reverts commit f1264dd.

ferozco mentioned this pull request Jun 21, 2019

Revert "[improvement] Ensure Optional#orElse argument is not method invocation" #659

Merged

ferozco mentioned this pull request Jul 2, 2019

add refaster rule to migrate away from orElse(supplier.get()) #679

Merged

David132639 added a commit to David132639/gradle-baseline that referenced this pull request Mar 29, 2023

Add OptionalOrElseMethodInvocation to README.md

17a1410

Add a line for an error-prone check added in palantir#655, which is missing from the README.

David132639 mentioned this pull request Mar 29, 2023

Add OptionalOrElseMethodInvocation to README.md #2534

Merged

bulldozer-bot bot pushed a commit that referenced this pull request Apr 4, 2023

Add OptionalOrElseMethodInvocation to README.md (#2534)

df78651

Add a line for an error-prone check added in #655, which is missing from the README.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improvement] Ensure Optional#orElse argument is not method invocation #655

[improvement] Ensure Optional#orElse argument is not method invocation #655

gatesn commented Jun 19, 2019 •

edited

Loading

markelliot commented Jun 19, 2019

ferozco Jun 19, 2019

ferozco Jun 19, 2019

ferozco Jun 19, 2019

gatesn Jun 19, 2019

carterkozak Aug 22, 2019

carterkozak Jun 19, 2019

carterkozak commented Jun 19, 2019

gatesn commented Jun 20, 2019

markelliot left a comment

iamdanfox commented Jun 20, 2019

uschi2000 Jun 20, 2019

gatesn Jun 20, 2019

uschi2000 Jun 20, 2019

uschi2000 Jun 20, 2019

gatesn Jun 20, 2019

uschi2000 Jun 20, 2019

gatesn Jun 20, 2019

uschi2000 Jun 20, 2019

uschi2000 Jun 20, 2019

gatesn Jun 20, 2019

carterkozak Jun 20, 2019

uschi2000 Jun 20, 2019

uschi2000 Jun 20, 2019

gatesn commented Jun 21, 2019 •

edited

Loading

markelliot left a comment

iamdanfox commented Jun 21, 2019

ferozco commented Jun 21, 2019

gatesn commented Jun 21, 2019

markelliot commented Jun 21, 2019

gatesn commented Jun 21, 2019

gatesn commented Jun 24, 2019

[improvement] Ensure Optional#orElse argument is not method invocation #655

[improvement] Ensure Optional#orElse argument is not method invocation #655

Conversation

gatesn commented Jun 19, 2019 • edited Loading

Before this PR

After this PR

markelliot commented Jun 19, 2019

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

carterkozak commented Jun 19, 2019

gatesn commented Jun 20, 2019

markelliot left a comment

Choose a reason for hiding this comment

iamdanfox commented Jun 20, 2019

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

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

gatesn commented Jun 21, 2019 • edited Loading

markelliot left a comment

Choose a reason for hiding this comment

iamdanfox commented Jun 21, 2019

ferozco commented Jun 21, 2019

gatesn commented Jun 21, 2019

markelliot commented Jun 21, 2019

gatesn commented Jun 21, 2019

gatesn commented Jun 24, 2019

gatesn commented Jun 19, 2019 •

edited

Loading

gatesn commented Jun 21, 2019 •

edited

Loading