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

[Maurizio Cimadamore]

On Sat, 22 Jun 2024 19:21:01 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/LambdaToMethod.java line 1487:
> 
>> 1485:                 translatedSymbols.put(LOCAL_VAR, new LinkedHashMap<>());
>> 1486:                 translatedSymbols.put(CAPTURED_VAR, new LinkedHashMap<>());
>> 1487:                 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)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1650646056