8294461: wrong effectively final determination by javac

[archiecobbs]

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.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8294461: wrong effectively final determination by javac

Reviewers

• Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to 449a5efb

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10856/head:pull/10856
$ git checkout pull/10856

Update a local copy of the PR:
$ git checkout pull/10856
$ git pull https://git.openjdk.org/jdk pull/10856/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10856

View PR using the GUI difftool:
$ git pr show -t 10856

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10856.diff

[bridgekeeper]

👋 Welcome back archiecobbs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@archiecobbs The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (014d67d3)
• 00: Full (449a5efb)

[vicente-romero-oracle]

looks sensible

[openjdk]

@archiecobbs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8294461: wrong effectively final determination by javac

Reviewed-by: vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

• 628820f: 8283093: JMX connections should default to using an ObjectInputFilter
• 521e712: 8286431: Do not use resource array in posix mmap_attach_shared()
• 4d9a1cd: 8292159: TYPE_USE annotations on generic type arguments of record components discarded
• 210fe49: 6244831: JFileChooser does not have tooltip for Desktop, Recent etc ToggleButton on Windows Look and feel
• c2d7a35: 8269235: serviceability/sa/ClhsdbJstackXcompStress.java timed out
• 58a7141: 8295066: Folding of loads is broken in C2 after JDK-8242115

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vicente-romero-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[archiecobbs]

Thanks for the review.

[archiecobbs]

/integrate

[openjdk]

@archiecobbs
Your change (at version 014d67d) is now ready to be sponsored by a Committer.

[vicente-romero-oracle]

/sponsor

[openjdk]

Going to push as commit b8ad6cd.
Since your change was applied there have been 7 commits pushed to the master branch:

• d667895: 8294399: (ch) Refactor some methods out of sun.nio.ch.UnixFileDispatcherImpl
• 628820f: 8283093: JMX connections should default to using an ObjectInputFilter
• 521e712: 8286431: Do not use resource array in posix mmap_attach_shared()
• 4d9a1cd: 8292159: TYPE_USE annotations on generic type arguments of record components discarded
• 210fe49: 6244831: JFileChooser does not have tooltip for Desktop, Recent etc ToggleButton on Windows Look and feel
• c2d7a35: 8269235: serviceability/sa/ClhsdbJstackXcompStress.java timed out
• 58a7141: 8295066: Folding of loads is broken in C2 after JDK-8242115

Your commit was automatically rebased without conflicts.

[openjdk]

@vicente-romero-oracle @archiecobbs Pushed as commit b8ad6cd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[jddarcy]

Should this change have a (retroactive) CSR and/or release note?

[archiecobbs]

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.

[vicente-romero-oracle]

Should this change have a (retroactive) CSR and/or release note?

well on similar cases we have done a CSR, I didn't realize during the review, sorry. I guess a CSR should probably be enough. I can help reviewing it. The reason @jddarcy is proposing the CSR is, IMO, because even though the spec didn't change, the universe of acceptable Java programs have changed, more in this case when that universe have been reduced. This is one of the situations for which a CSR should be created. @jddarcy not sure about the release notes but I if you think so I have no issues. I will file the CSR

[jddarcy]

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.

Adding some additional context, there CSR reviews both specification changes and behavioral changes considered to have sufficiently high compatibility impact.

This change renders code that previously (if erroneously) compiled into a compilation error, which is a source compatibility concern. (Changing code that compiles into code that doesn't compile is considered a more serious change than the reverse.)

Offhand, I don't know how often such coding patterns may appear in practice; we should some internal tooling which can help gauge that. A modification that may be suggested is only doing the updated check for source level 20 and higher.

HTH

[archiecobbs]

Thanks for the clarifications, now I understand better the rationale.

Seems this example might be a good one to add to the spec in a non-normative/informational blurb, to demonstrate (a) how "JLS unreachable" doesn't always equal "flow analysis unreachable", and/or (b) how "effectively final" can fail because of statements that are "flow analysis unreachable".