RFR: 8043179: Lambda expression can mutate final field [v12]

[Archie L. Cobbs]

On Tue, 21 Mar 2023 04:17:02 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> 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 10 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8043179
>>  - Merge branch 'master' into JDK-8043179
>>  - Update failed test output line numbers after removing @author tags.
>>  - Remove @author tags from tests.
>>  - Add a unit test verifying a change in reported error for final variables.
>>    
>>    Before this change, compiling this class:
>>    
>>        class LambdaMutateFinalVar {
>>            LambdaMutateFinalVar() {
>>                final String x;
>>                Runnable r1 = () -> x = "not ok";
>>                x = "ok";
>>            }
>>        }
>>    
>>    would report this error:
>>    
>>      local variables referenced from a lambda expression must be final or effectively final
>>    
>>    That error is not really appropriate; after all, the variable IS final. The
>>    real problem is that it can't be assigned from within the lambda because it
>>    can't be assumed to be DU.
>>    
>>    After this change, this error is reported instead:
>>    
>>      variable x might already have been assigned
>>  - Fix previously incorrect logic and update unit tests.
>>  - Fix @author in test to match github username per instructions.
>>  - Move unit test into a more appropriate subdirectory.
>>  - Use /nodynamiccopyright/ for "golden file" test per instructions.
>>  - 8043179: Lambda expression can mutate final field
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 2860:
> 
>> 2858:                 // lambda body may end up being assigned to later on, so we cannot conclude that
>> 2859:                 // the variable will be unassigned when the body is executed.
>> 2860:                 uninits.excludeFrom(firstadr);
> 
> I think this will exclude all tracked variables not only fields, shouldn't we exclude fields only?

This bug refers only to final fields, but JLS section 16 applies to both fields and variables, so I think this code is following the spec as written.

Here's an example of where this change would catch an illegal final variable assignment:

class LambdaMutateFinalVariable {
    void foo() {
        final String x;
        Runnable r1 = () -> x = "not ok";
        x = "ok";
    }
}

So I don't think the code in the PR is wrong. However, there is a subtlety: currently, the compiler already correctly handles the above example, but that's only by coincidence and thanks to the separate logic for "effectively final":

LambdaMutateFinalVariable.java:4: error: local variables referenced from a lambda expression must be final or effectively final
        Runnable r1 = () -> x = "not ok";
                            ^

With the change in this PR, the DA/DU analysis will instead get there first, and so you get a different error:

LambdaMutateFinalVariable.java:4: error: variable x might already have been assigned
        Runnable r1 = () -> x = "not ok";
                            ^

It seems to me either error is appropriate, because the code violates both rules. But I think the change in the error message is actually an improvement, because the current error is complaining about the variable not being final even though it actually *is* final, which is confusing.

Your thoughts?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/10381#discussion_r1143424072