Pmd: [java] UnnecessaryLocalBeforeReturn enhancement is overly restrictive -- method order matters

[epkugelmass]

Rule Set:
UnnecessaryLocalBeforeReturn

Description:
The enhancement in https://github.com/pmd/pmd/issues/240 was ill-advised. The change in behavior should have been formulated as a new rule or an option on the existing rule, not added to an existing rule in a "bug-fix" release.

There are many cases where order does matter even for variables that are declared and then only referenced a single time.

Code Sample demonstrating the issue:

int i = compute(); // might throw
markComputationDone();
return i;

Mutable m = ...;
int i = compute(m);
sideEffect(m);
return i;

[jsotuyod]

Thanks for your report!

Your concerns have been partially raised before on #282, which was fixed on #294 (for next release cycle). These changes cover your second example.

As for the first example, I can't think of a valid example that doesn't point to a design issue (ie: the called methods are nor truly independent and should therefore be merged). If you can think of such scenario, please let us know.

A minor note. PMD doesn't follow semantic versioning, although there are open discussions to start doing so.

[epkugelmass]

Awesome, glad to see the fix for example number two. Example one would be better with a listener pattern (which would wrap the mark...() method up in the first call) but there are reasonable cases where you might not want to do that (at least not immediately). This seems gray enough that it warrants a separate rule or rule option rather than being part of what I see as a very black-and-white rule.

[jsotuyod]

@epkugelmass that's a very reasonable scenario. Let me think this over and come back to you.

[adangel]

@epkugelmass @jsotuyod
I've added for now a new property "statementOrderMatters" for this rule. It is by default enabled and preserves the old behavior. If this property is set to false, it switches to the new improved implementation, that would detect detect again cases like your first example - we do not have a way to know about the side-effects of "markComputationDone".
We might improve the rule with #241, if possible.