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

[Vicente Romero]

On Tue, 21 Mar 2023 13:57:46 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> 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?

yep I agree that the error message is better with the patch. My concern was not that the patch was wrong but about the fact that we were applying the change for all variables and fields, but yes it probably makes sense to go for this and have the side effect of better error messages

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

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