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

Vicente Romero vromero at openjdk.org
Sat Jun 22 19:26:17 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...

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

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

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?

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

> 3935: 
> 3936:         boolean receiverAccessible(JCMemberReference tree) {
> 3937:             //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.

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

> 4037:                 slam.type = tree.type;
> 4038:                 slam.pos = tree.pos;
> 4039:                 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

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?

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

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

PR Comment: https://git.openjdk.org/jdk/pull/19836#issuecomment-2184162439
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649774997
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649758177
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649760970
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649769194
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649769388
PR Review Comment: https://git.openjdk.org/jdk/pull/19836#discussion_r1649772304


More information about the compiler-dev mailing list