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

[Maurizio Cimadamore]

On Sat, 22 Jun 2024 18:11:36 GMT, Vicente Romero <vromero 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` th...
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 3893:
> 
>> 3891:     @Override
>> 3892:     public void visitLambda(JCLambda tree) {
>> 3893:         Type expectedRet = types.findDescriptorType(tree.type).getReturnType();
> 
> shouldn't we erase the return type here?

`tree.type` should be erased (from `TransTypes`)

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 4043:
> 
>> 4041:                     // use a let expression so that the receiver expression is evaluated eagerly
>> 4042:                     return make.at(tree.pos).LetExpr(
>> 4043:                             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?

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 4725:
> 
>> 4723:             !types.isDirectSuperInterface(((JCFieldAccess)tree.selected).selected.type.tsym, currentClass);
>> 4724:         tree.selected = translate(tree.selected);
>> 4725:         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).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1650656395
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1650653767
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1650651890