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

[Maurizio Cimadamore]

On Sat, 22 Jun 2024 18:24:53 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 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.

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.

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

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