Skip to content

8043179: Lambda expression can mutate final field #10381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from
Closed

8043179: Lambda expression can mutate final field #10381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
989627b
8043179: Lambda expression can mutate final field
archiecobbs Sep 24, 2022
5ee88e7
Use /nodynamiccopyright/ for "golden file" test per instructions.
archiecobbs Sep 25, 2022
db7dd0e
Move unit test into a more appropriate subdirectory.
archiecobbs Sep 25, 2022
faac971
Fix @author in test to match github username per instructions.
archiecobbs Sep 28, 2022
9699523
Fix previously incorrect logic and update unit tests.
archiecobbs Oct 14, 2022
06b39a5
Add a unit test verifying a change in reported error for final variab…
archiecobbs Oct 17, 2022
b1d5b3c
Remove @author tags from tests.
archiecobbs Oct 19, 2022
0081d19
Update failed test output line numbers after removing @author tags.
archiecobbs Oct 20, 2022
1e38c30
Merge branch 'master' into JDK-8043179
archiecobbs Nov 17, 2022
86808b2
Merge branch 'master' into JDK-8043179
archiecobbs Dec 19, 2022
e1cd797
Merge branch 'master' into JDK-8043179
archiecobbs Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2856,6 +2856,11 @@ public void visitLambda(JCLambda tree) {
int nextadrPrev = nextadr;
ListBuffer<PendingExit> prevPending = pendingExits;
try {
// JLS 16.1.10: No rule allows V to be definitely unassigned before a lambda
// body. This is by design: a variable that was definitely unassigned before the
// lambda body may end up being assigned to later on, so we cannot conclude that
// the variable will be unassigned when the body is executed.
uninits.excludeFrom(firstadr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will exclude all tracked variables not only fields, shouldn't we exclude fields only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

returnadr = nextadr;
pendingExits = new ListBuffer<>();
for (List<JCVariableDecl> l = tree.params; l.nonEmpty(); l = l.tail) {
Expand Down
13 changes: 13 additions & 0 deletions test/langtools/tools/javac/lambda/8043179/LambdaMutateFinalField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
* @test /nodynamiccopyright/
* @summary Verify lambda expression can't mutate a final field
* @bug 8043179
* @compile/fail/ref=LambdaMutateFinalField.out -XDrawDiagnostics LambdaMutateFinalField.java
*/
class LambdaMutateFinalField {
final String x;
LambdaMutateFinalField() {
Runnable r1 = () -> x = "not ok";
this.x = "ok";
}
}
2 changes: 2 additions & 0 deletions test/langtools/tools/javac/lambda/8043179/LambdaMutateFinalField.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
LambdaMutateFinalField.java:10:29: compiler.err.var.might.already.be.assigned: x
1 error
13 changes: 13 additions & 0 deletions test/langtools/tools/javac/lambda/8043179/LambdaMutateFinalVar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
* @test /nodynamiccopyright/
* @summary Verify lambda expression can't mutate a final variable
* @bug 8043179
* @compile/fail/ref=LambdaMutateFinalVar.out -XDrawDiagnostics LambdaMutateFinalVar.java
*/
class LambdaMutateFinalVar {
LambdaMutateFinalVar() {
final String x;
Runnable r1 = () -> x = "not ok";
x = "ok";
}
}
2 changes: 2 additions & 0 deletions test/langtools/tools/javac/lambda/8043179/LambdaMutateFinalVar.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
LambdaMutateFinalVar.java:10:29: compiler.err.var.might.already.be.assigned: x
1 error