Automated refactoring for Optional.orElse case is too aggressive

[j-baker]

At present, no method calls are really allowed in Optional.orElse.

This means that code like

maybeSet.orElse(emptySet()); // method call is optimized away to a constant

will get refactored to

maybeSet.orElseGet(Collections::emptySet); // less readable, no longer optimizable

Code like:

config.getSubdomain().orElse(Subdomains.getDefaultSubdomain(region)); // called once at start of execution

will get refactored to

config.getSubdomain().orElseGet(() -> Subdomains.getDefaultSubdomain(region)); // less readable, micro-optimization

and

Optional<T> left, right;
return Optional.ofNullable(left.orElse(right.orElse(null));

will get refactored to

T rightT = right.orElse(null);
return Optional.ofNullable(left.orElse(rightT));

automatically.

Basically, we ended up with a repo that gets much less readable. I get that we're here blocking the strategy of optional.orElse(doExpensiveThing()) but there's always a tradeoff - and I think here we're making a lot of cases harder to read or less efficient - I think the automated migration burden should be higher.

[angieh1]

This is also mentioned by Pkoenig as a final comment on this: https://github.com/palantir/gradle-baseline/pull/680

Is there any intention here to relax the rule for things that return cheap constants (empty maps, sets, etc)?