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

[Archie L. Cobbs]

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.

There shouldn't be any spec change implied by this change. From my understanding, the compiler was not following the spec, which says that the variable `i` in the example is not effectively final. This change makes the compiler follow the spec and generate a "local variables referenced from a lambda expression must be final or effectively final" error.

Regarding release notes, I'll let someone else who has more familiarity with the release process comment on that.

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

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