RFR: 8294461: wrong effectively final determination by javac [v2]

[Vicente Romero]

On Wed, 26 Oct 2022 19:45:05 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> This bug involves DA/DU analysis and the concept of "effectively final".
>> 
>> Here's the test case, which the compiler currently but incorrectly accepts:
>> 
>>     for (int i = 0; i < 10; i++) {
>>         Runnable r = () -> System.out.println(i);   // variable i is NOT effectively final
>>         break;                                      // even though "i++" is never reached
>>     }
>> 
>> 
>> For the purposes of "effectively final", it doesn't matter whether an assignment statement that makes a variable no longer effectively final is actually reachable or not. In cases like the `i++` above, a statement can be actually unreachable yet not "unreachable" according to the JLS, and so it does not generate an "unreachable statement" compiler error (which would otherwise hide this bug).
>> 
>> JLS §4.12.4 states:
>> 
>>> A local variable declared by a statement and whose declarator has an initializer ... is effectively final if all of the following are true:
>>> ...
>>> • It never occurs as the operand of a prefix or postfix increment or decrement operator (§15.14, §15.15).
>> 
>> So clearly `i` is not effectively final.
>> 
>> However, the way we calculate effective finality does not involve actually looking for increment/decrement operators. Instead, it's determined during DA/DU analysis: non-final variables have an `EFFECTIVELY_FINAL` flag which is initialized to `true` and then cleared if/when we encounter contrary evidence - e.g., an assignment when not DU.
>> 
>> For simplicity, the DA/DU analysis works like this:
>> * Variables with initializers are treated like variables without initializers followed by an assignment
>> * Increment/decrement operators are treated as a normal read followed by a normal assignment.
>> 
>> These are reasonable. However, it means this clause of JLS §4.12.4 effectively applies:
>> 
>>> A local variable declared by a statement and whose declarator lacks an initializer is effectively final if all of the following are true:
>>> ...
>>> • Whenever it occurs as the left hand side in an assignment expression, it is definitely unassigned and not definitely assigned before the assignment...
>> 
>> The bug with the current code is that when we see an assignment to a variable with the `EFFECTIVELY_FINAL` flag still set, we clear the flag if the variable is not DU, but we are _not_ clearing the flag if the variable is DA, as required above. This happens with the `i++` statement because, by virtue of it being actually unreachable, `i` is both DA and DU before that statement.
>> 
>> This patch corrects that omission.
>
> Archie L. Cobbs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8294461
>  - A for loop variable is not effectively final even if the loop never increments.

looks sensible

-------------

Marked as reviewed by vromero (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10856