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

Fri Jun 21 17:40:37 UTC 2024

On Fri, 21 Jun 2024 16:47:06 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> A long, long time ago, we used to have [two competing translation strategies](https://hg.openjdk.org/lambda/lambda/langtools/rev/a7e6203a332d?revcount=240) 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](https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java#L1480) 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](https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java#L1244) 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](https://bugs.openjdk.org/issues/?jql=labels%20%3D%20javac-pre-capture) 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...

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

> 658:         deser.type = kInfo.deserMethodSym.type;
> 659:         //System.err.printf("DESER: '%s'\n", deser);
> 660:         return lower.translateMethod(attrEnv, deser, make);

explicit call to Lower is needed here

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

> 893:      * Converts a method reference which cannot be used directly into a lambda
> 894:      */
> 895:     private class MemberReferenceToLambda {

This code is now in `Lower`

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

> 1449: 
> 1450:         @Override
> 1451:         public void visitNewClass(JCNewClass tree) {

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

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

> 1965:                 translatedSymbols.put(CAPTURED_VAR, new LinkedHashMap<Symbol, Symbol>());
> 1966:                 translatedSymbols.put(CAPTURED_THIS, new LinkedHashMap<Symbol, Symbol>());
> 1967:                 translatedSymbols.put(CAPTURED_OUTER_THIS, new LinkedHashMap<Symbol, Symbol>());

note that CAPTURED_OUTER_THIS no longer exists

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

> 191:      * case the captured symbols should be replaced with the translated lambda symbols).
> 192:      */
> 193:     Map<Symbol, Symbol> lambdaTranslationMap = null;

This is now no longer needed

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

> 2779:         JCMethodDecl prevMethodDef = currentMethodDef;
> 2780:         MethodSymbol prevMethodSym = currentMethodSym;
> 2781:         int prevVariableIndex = variableIndex;

This machinery is needed to generate "stable" local variable names, so that lambda deduplication will work as expected. This code is borrowed from `TransPatterns`.

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

> 3915: 
> 3916:     @Override
> 3917:     public void visitReference(JCMemberReference tree) {

All this code comes from LambdaToMethod

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

> 1640:                 return;
> 1641: 
> 1642:             if (scanner.hasLambdas) {

This is another bit I'm not sure about - why do we need a scanner to check if there are lambdas? I think we added this because there were concerns over running "too many visitors", but today we have many more, and we don't have scanners for them, so... (again, this is something for another PR)

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 2016:

> 2014:         public boolean canCompleteNormally = true;
> 2015:         public ParameterKind paramKind;
> 2016:         public boolean wasMethodReference;

This is only needed to retain compatibility with some "statistics" option that was added a long time ago. Not sure how much is relevant today, but perhaps better to cleanup in a separate PR if we decide that's no longer needed.

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

> 345:         public void visitClassDef(JCClassDecl tree) {
> 346:             // don't descend any further
> 347:             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).

test/langtools/tools/javac/lambda/T8129740/Universe.java.out line 72:

> 70:                 
> 71:                 Planet(String name, int moonsCount) {
> 72:                     this(name, moonsCount, ()->{

This test depends on the output of `printSource`, and since that output is what is left in the AST after lower, now it shows lambda expressions as well.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649239384
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649236477
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649237040
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649237415
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649242348
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649243096
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649243965
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649256384
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649250566
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649257386
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649246098