8334037: Local class creation in lambda in pre-construction context crashes javac #19836

mcimadamore · 2024-06-21T16:47:06Z

A long, long time ago, we used to have two competing translation strategies for lambda expressions. One strategy was to translate lambdas to anonymous inner classes. The other was to translate them as invokedynamic, pointing to a synthetic method containing the lambda body.

The simpler inner class traslation strategy predates the more complex indy-based one. Since that translation strategy was based on inner classes, it made a lot of sense to do that translation before Lower, so that any anon class generated by javac could then be translated correctly (and take into account issues such as local variable capture).

When we added LambdaToMethod, it seemed natural to add it in the same step (e.g. before Lower) - after all, users could toggle between the two translation strategies with a switch, so it made sense to keep them close together.

But, as we got rid of the inner class translation strategy, and doubled down on LambdaToMethod we have started to see some artifacts. For instance, if a lambda expression contains a local class declaration, LambdaToMethod has to guess which captured variables might get captured later in Lower, and re-captured them in the lambda. Now, guessing the full set of local captures is not that easy, as the set depends on the state in which Lower is in: more specifically, sometimes Lower might decide to also capture the enclosing this constructor parameter, when in early-construction context. Of course LambdaToMethod didn't know any of this, and over the years several workarounds have been introduced.

Unfortunately, JEP 482 is in collision course with the current compiler pipeline: Lower is the only place in the compiler which knows how to correctly translate references to enclosing instances. Trying to mimic what Lower is doing in LambdaToMethod, doesn't scale, as the many bugs that were discovered in this area show.

For this reason, instead of keep playing whack-a-mole, I think the time has come to finally move LambdaToMethod after Lower. This move makes LambdaToMethod the last compiler step (before Gen).

The move is relatively straightforward, but there are some caveats:

the new LambdaToMethod deals with separate classes, not a single toplevel one (as it now occurs after lowering);
Lower needs a to override visitLambda, so that it can properly lower all the lambda return expressions;
the code to desugars complex method references into lambdas has been moved from LambdaToMethod to Lower (so that we can fixup references to inner classes etc.) - this means that LambdaToMethod now only has to worry about lambdas and simple method references;
the lambda deserialization method generated by LambdaToMethod contains string in switch, and implicitly requires boxing, so we need to explictly lower that (given Lower has already ran, we need to trigger the lowering of the method manually)
some code generated by Lower is not stable (this is a pre-existing issue), so lambda deduplication fails in new ways

This PR addresses all the issues above and removes all the workaround we have accumulated over the years to make up for the fact that LambdaToMethod runs at the wrong time. In doing so, we fix many issues related to JEP 482 (thanks to @archiecobbs for the tests which I've included here).

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

Issues

JDK-8334037: Local class creation in lambda in pre-construction context crashes javac (Bug - P4)
JDK-8333313: NullPointerException in lambda instantiating an inner local class in prologue (Bug - P3)
JDK-8333766: Stack overflow with anonymous class in super() parameter (Bug - P4)
JDK-8334679: Wrong bug number in regression test for JDK-8334252 (Bug - P5)

Reviewers

Jan Lahoda (@lahodaj - Reviewer)
Vicente Romero (@vicente-romero-oracle - Reviewer)

Contributors

Archie Cobbs <acobbs@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19836

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

Using diff file

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

Webrev

Link to Webrev Comment

(as lambdas are translated last, XDprintSource shows them untouched)

Lower unnecessarily rewrites symbols for Foo.class where Foo is a reference class. We still have an issue with Lower generating "random" tmp local names.

Remove more dead code in LambdaToMethod

Simplify code not to stash method reference receiver in desugared lambda.

bridgekeeper · 2024-06-21T16:47:46Z

👋 Welcome back mcimadamore! 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 · 2024-06-21T16:47:53Z

@mcimadamore 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:

8334037: Local class creation in lambda in pre-construction context crashes javac
8333313: NullPointerException in lambda instantiating an inner local class in prologue
8333766: Stack overflow with anonymous class in super() parameter
8334679: Wrong bug number in regression test for JDK-8334252

Co-authored-by: Archie Cobbs <acobbs@openjdk.org>
Reviewed-by: jlahoda, 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 83 new commits pushed to the master branch:

7f6804c: 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960
e139005: 8333994: NMT: call stacks should show source information
b88af94: 8269870: PS: Membar in PSPromotionManager::copy_unmarked_to_survivor_space could be relaxed
a5f401f: 8334650: Add debug information about whether an Assertion Predicate is for the init or last value
25c3845: 8333133: Simplify QuickSort::sort
c66f785: 8334505: RISC-V: Several tests fail when MaxVectorSize does not match VM_Version::_initial_vector_length
f101e15: 8333583: Crypto-XDH.generateSecret regression after JDK-8329538
b3bf31a: 8333542: Breakpoint in parallel code does not work
86b0cf2: 8334653: ISO 4217 Amendment 177 Update
861aefc: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
... and 73 more: https://git.openjdk.org/jdk/compare/7b3a96d57023e8a7cf495e2d7c551976f0e5656b...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-06-21T16:48:31Z

@mcimadamore 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.

mcimadamore · 2024-06-21T16:49:36Z

/solves JDK-8333313
/solves JDK-8333766

openjdk · 2024-06-21T16:50:16Z

@mcimadamore
Adding additional issue to solves list: 8333313: NullPointerException in lambda instantiating an inner local class in prologue.

openjdk · 2024-06-21T16:51:14Z

@mcimadamore
Adding additional issue to solves list: 8334252: Verifier error for lambda declared in early construction context.

openjdk · 2024-06-21T16:52:24Z

@mcimadamore
Adding additional issue to solves list: 8333766: Stack overflow with anonymous class in super() parameter.

mcimadamore · 2024-06-21T17:12:39Z

/contributor add acobbs

openjdk · 2024-06-21T17:12:57Z

@mcimadamore
Contributor Archie Cobbs <acobbs@openjdk.org> successfully added.

mcimadamore · 2024-06-21T17:14:10Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java

-    /**
-     * Converts a method reference which cannot be used directly into a lambda
-     */
-    private class MemberReferenceToLambda {


This code is now in Lower

mcimadamore · 2024-06-21T17:14:51Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java

-        }
-
-        @Override
-        public void visitNewClass(JCNewClass tree) {


most of the removed visitor methods had to do with the typesUnderConstruction workaround, which has now been removed

mcimadamore · 2024-06-21T17:15:12Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java

-                translatedSymbols.put(LOCAL_VAR, new LinkedHashMap<Symbol, Symbol>());
-                translatedSymbols.put(CAPTURED_VAR, new LinkedHashMap<Symbol, Symbol>());
-                translatedSymbols.put(CAPTURED_THIS, new LinkedHashMap<Symbol, Symbol>());
-                translatedSymbols.put(CAPTURED_OUTER_THIS, new LinkedHashMap<Symbol, Symbol>());


note that CAPTURED_OUTER_THIS no longer exists

mcimadamore · 2024-06-21T17:17:24Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java

@@ -725,7 +657,7 @@ private JCMethodDecl makeDeserializeMethod(Symbol kSym) {
        deser.sym = kInfo.deserMethodSym;
        deser.type = kInfo.deserMethodSym.type;
        //System.err.printf("DESER: '%s'\n", deser);
-        return deser;
+        return lower.translateMethod(attrEnv, deser, make);


explicit call to Lower is needed here

mcimadamore · 2024-06-21T17:20:57Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

-     * This is required when a capturing local class is created from a lambda (in which
-     * case the captured symbols should be replaced with the translated lambda symbols).
-     */
-    Map<Symbol, Symbol> lambdaTranslationMap = null;


This is now no longer needed

mcimadamore · 2024-06-21T17:34:36Z

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java

@@ -344,11 +344,13 @@ public void visitExec(JCExpressionStatement stat) {
        @Override
        public void visitClassDef(JCClassDecl tree) {
            // don't descend any further
+            result = tree;


This was an issue in the current code: this is a tree translator (not just a tree scanner) and a tree translator has to side-effect the result variable, otherwise clients can see null popping up in strange places (which is what started happening when I moved LambdaToMethod around).

mlbridge · 2024-06-21T17:40:35Z

Webrevs

mcimadamore · 2024-06-21T17:45:19Z

test/langtools/tools/javac/SuperInit/LambdaOuterCapture.java

@@ -22,7 +22,7 @@
 */
 /*
 * @test
- * @bug 8194743
+ * @bug 8334252


just a fix for the wrong bug ID

In which case this also fixes JDK-8334679. If you want, add /solves 8334679 here and I'll close PR #19816. Thanks.

mcimadamore · 2024-06-21T17:59:36Z

/solves 8334679

openjdk · 2024-06-21T18:01:06Z

@mcimadamore
Adding additional issue to solves list: 8334679: Wrong bug number in regression test for JDK-8334252.

mcimadamore · 2024-06-21T21:14:05Z

/issue remove JDK-8334252

openjdk · 2024-06-21T21:14:39Z

@mcimadamore
Removing additional issue from issue list: 8334252.

vicente-romero-oracle · 2024-06-22T18:11:36Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

        result = tree;
    }

+    @Override
+    public void visitLambda(JCLambda tree) {
+        Type expectedRet = types.findDescriptorType(tree.type).getReturnType();


shouldn't we erase the return type here?

tree.type should be erased (from TransTypes)

After following up offline, we realized that this does indeed require erasure of the getReturnType, given findDescriptorType calls findDescriptor on the type's symbol (and symbols are not erased).

vicente-romero-oracle · 2024-06-22T18:24:53Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

+        }
+
+        boolean receiverAccessible(JCMemberReference tree) {
+            //hack needed to workaround 292 bug (7087658)


side: 7087658 has been fixed but if we comment the invocation to this method some tests fail, so I think this comment is stale, and we could probably remove it, and if we remove the comment then this predicate probably doesn't deserve to be a method anymore. Also not related to your patch so this could be tackle in another PR but is seems like the ownerAccessible field in method references reflects incomplete information. As we are adding here several predicates all related to accessibility to determine if we should generate a lambda for a particular method reference or not. See isProtectedInSuperClassOfEnclosingClassInOtherPackage, isPrivateInOtherClass, etc. So it seems like the degree of accessibility we want to determine for the expr in a method reference is stricter than the one needed in general for other types.

I agree there's a lot of cruft in here. We started adding predicates simply because the 292 machinery could not support all invocation modes. But then we also added other stuff: all the accessibility checks are there because if javac decides to generate an accessibility bridge for the method, then we can't use a MH directly (as the target method would be inaccessible - the bridge should be called instead). In these cases, it was preferred to just let Lower do its job, and translate the method call correctly, using bridges etc.

But, with respect to this specific issue, I agree, ownerAccessible is probably no longer needed, as the underlying MH deficiency was addressed long ago.

vicente-romero-oracle · 2024-06-22T18:49:28Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

+                slam.target = tree.target;
+                slam.type = tree.type;
+                slam.pos = tree.pos;
+                slam.wasMethodReference = true;


yep I agree that we can remove this field, probably in another PR, if really needed we can add a separate DS to track those lambdas that were method references

vicente-romero-oracle · 2024-06-22T18:50:39Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

+                if (receiverExpression != null) {
+                    // use a let expression so that the receiver expression is evaluated eagerly
+                    return make.at(tree.pos).LetExpr(
+                            make.VarDef(rcvr, translate(receiverExpression)), slam).setType(tree.type);


should we erase this type?

Should be erased already, since we're past TransTypes and that visitor had a chance to translate the method reference expression?

vicente-romero-oracle · 2024-06-22T19:11:08Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

@@ -4381,7 +4722,7 @@ public void visitSelect(JCFieldAccess tree) {
            TreeInfo.name(tree.selected) == names._super &&
            !types.isDirectSuperInterface(((JCFieldAccess)tree.selected).selected.type.tsym, currentClass);
        tree.selected = translate(tree.selected);
-        if (tree.name == names._class) {
+        if (tree.name == names._class && tree.selected.type.isPrimitiveOrVoid()) {


not sure why this change was needed

This is again lambda deduplication. The current code will take String.class and rewrite it from scratch, with a new symbol in it (which is what makes deduplication fail).

vicente-romero-oracle · 2024-06-22T19:21:01Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java

+                translatedSymbols.put(PARAM, new LinkedHashMap<>());
+                translatedSymbols.put(LOCAL_VAR, new LinkedHashMap<>());
+                translatedSymbols.put(CAPTURED_VAR, new LinkedHashMap<>());
+                translatedSymbols.put(CAPTURED_THIS, new LinkedHashMap<>());


we don't seem to actually be doing anything fancy for captured_this, the symbol is self represented, I wonder if this category could be removed in a future PR

CAPTURED_THIS is used to test whether a lambda needs to be instance or static (but it's true we don't really use any of the symbols in here, just using a boolean would be ok)

vicente-romero-oracle · 2024-06-22T19:23:21Z

awesome job, this change will make our lives a lot easier in the future

Erase lambda functional return and simplifify lambda visitor in `Lower`

mcimadamore · 2024-06-24T14:47:22Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

-                tree.body = translate((JCExpression) tree.body, expectedRet);
-            }
-            result = tree;
+            currentRestype = types.findDescriptorType(tree.type).getReturnType();


We no longer need to mess with variableIndex in the new code: if the lambda is a statement lambda, this will call visitBlock which will update variableIndex accordingly

I probably miss something, but where is the return type erased?

mcimadamore · 2024-06-24T17:50:27Z

For the records, I've also put together a patch which moves the code to lower method references into TransTypes. This allows us to simplify code generation of the desugared lambda, w/o worrying about things such as intersection types:

https://github.com/mcimadamore/jdk/compare/cleanup_capture...mcimadamore:jdk:cleanup_capture+mref_to_lambda_in_trans_types?expand=1

I will pursue this as a separate PR.

lahodaj

Overall, looks OK to me. Some comments inline. I also was thinking if the reference to lambda conversion should be a separate phase (in order to not put more work into Lower), but that's probably overkill.

lahodaj · 2024-06-24T15:18:07Z

src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java

@@ -1647,6 +1639,16 @@ public void visitSwitchExpression(JCSwitchExpression tree) {
            if (shouldStop(CompileState.LOWER))
                return;

+            if (scanner.hasLambdas) {


IIRC, this was added because the new phase slowed down performance running of very small, simple, testcase(s) without any lambda (https://bugs.openjdk.org/browse/JDK-8026936).

We also do the same for patterns/TransPatterns.

To what degree this is needed currently is not obvious.

lahodaj · 2024-06-24T18:39:18Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

-                tree.body = translate((JCExpression) tree.body, expectedRet);
-            }
-            result = tree;
+            currentRestype = types.findDescriptorType(tree.type).getReturnType();


I probably miss something, but where is the return type erased?

mcimadamore · 2024-06-24T21:10:53Z

Overall, looks OK to me. Some comments inline. I also was thinking if the reference to lambda conversion should be a separate phase (in order to not put more work into Lower), but that's probably overkill.

See my comment. I think it makes sense to move this code in a separate phase as this code really wants to be before TransTypes, or in TransTypes itself (which is what I did in the patch I shared there). I'd prefer to address that separately though, as that requires other small changes too (e.g. to undo erasure etc.)

Add missing erasure

vicente-romero-oracle

looks great!

lahodaj

Looks good to me.

mcimadamore · 2024-06-26T09:11:34Z

/integrate

openjdk · 2024-06-26T09:12:05Z

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

7f6804c: 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960
e139005: 8333994: NMT: call stacks should show source information
b88af94: 8269870: PS: Membar in PSPromotionManager::copy_unmarked_to_survivor_space could be relaxed
a5f401f: 8334650: Add debug information about whether an Assertion Predicate is for the init or last value
25c3845: 8333133: Simplify QuickSort::sort
c66f785: 8334505: RISC-V: Several tests fail when MaxVectorSize does not match VM_Version::_initial_vector_length
f101e15: 8333583: Crypto-XDH.generateSecret regression after JDK-8329538
b3bf31a: 8333542: Breakpoint in parallel code does not work
86b0cf2: 8334653: ISO 4217 Amendment 177 Update
861aefc: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
... and 73 more: https://git.openjdk.org/jdk/compare/7b3a96d57023e8a7cf495e2d7c551976f0e5656b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-26T09:12:10Z

@mcimadamore Pushed as commit 4ce8822.

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

mcimadamore added 15 commits June 14, 2024 16:21

Make logic of FreeVarCollector::addSymbol a little more explicit

005912f

Move LambdaToMethod at the end of the compiler pipeline

dccb30d

Cleanup more code

714120e

Readd L2M stats for method references

9ce4376

Fix source to source test

9e9df03

(as lambdas are translated last, XDprintSource shows them untouched)

Lower lambda deser method for good

627882a

Cleanup

125d318

Restore desugaring of method reference when using -source 14

b6574e4

Fix dedup - part 1

ac908bf

Lower unnecessarily rewrites symbols for Foo.class where Foo is a reference class. We still have an issue with Lower generating "random" tmp local names.

Fix remaining dedup issues

4b668e5

Remove more dead code in LambdaToMethod

More unused code cleanup

2ecabb3

Use LetExpr to force eager computaton of method reference receiver.

c6a4193

Simplify code not to stash method reference receiver in desugared lambda.

Add comment

83843d4

Merge branch 'master' into cleanup_capture

de1413c

Add tests

0ac44f5

openjdk bot added the compiler compiler-dev@openjdk.org label Jun 21, 2024

mcimadamore commented Jun 21, 2024

View reviewed changes

Remove spurious changes in Gen

75a52ff

mcimadamore marked this pull request as ready for review June 21, 2024 17:35

openjdk bot added the rfr Pull request is ready for review label Jun 21, 2024

mcimadamore commented Jun 21, 2024

View reviewed changes

archiecobbs mentioned this pull request Jun 21, 2024

8334679: Wrong bug number in regression test for JDK-8334252 #19816

Closed

3 tasks

vicente-romero-oracle reviewed Jun 22, 2024

View reviewed changes

Address review comments

488fb48

Erase lambda functional return and simplifify lambda visitor in `Lower`

mcimadamore commented Jun 24, 2024

View reviewed changes

lahodaj reviewed Jun 24, 2024

View reviewed changes

Address review comment

8ecc153

Add missing erasure

vicente-romero-oracle approved these changes Jun 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 26, 2024

lahodaj approved these changes Jun 26, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 26, 2024

openjdk bot closed this Jun 26, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 26, 2024

mcimadamore mentioned this pull request Jun 26, 2024

8335159: Move method reference to lambda desugaring before Lower #19903

Closed

3 tasks

8334037: Local class creation in lambda in pre-construction context crashes javac #19836

8334037: Local class creation in lambda in pre-construction context crashes javac #19836

Uh oh!

Conversation

mcimadamore commented Jun 21, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Contributors

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 21, 2024

Uh oh!

openjdk bot commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 21, 2024

Uh oh!

mcimadamore commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 21, 2024

Uh oh!

openjdk bot commented Jun 21, 2024

Uh oh!

openjdk bot commented Jun 21, 2024

Uh oh!

mcimadamore commented Jun 21, 2024

Uh oh!

openjdk bot commented Jun 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented Jun 21, 2024

Uh oh!

openjdk bot commented Jun 21, 2024

Uh oh!

mcimadamore commented Jun 21, 2024

Uh oh!

openjdk bot commented Jun 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mcimadamore commented Jun 21, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 21, 2024 •

edited

Loading

mcimadamore commented Jun 21, 2024 •

edited

Loading

mlbridge bot commented Jun 21, 2024 •

edited

Loading