RFR: 8295019: Cannot call a method with a parameter of a local class declared in a lambda [v2]

[Maurizio Cimadamore]

On Wed, 1 Feb 2023 23:57:59 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Very interesting and, a bit, tricky bug. So the compiler is rejecting code like:
>> 
>> class LocalClassDeclaredInsideLambdaTest {
>>     void run(Runnable r) {}
>> 
>>     void m() {
>>         run(() -> {
>>             class C {
>>                 static void takeC(C c) {}
>>                 static C giveC() {
>>                     return null;
>>                 }
>>             }
>>             C.takeC(C.giveC());
>>         });
>>     }
>> }
>> 
>> with error:
>> 
>> LocalClassInsideLambda.java:12: error: incompatible types: C cannot be converted to C
>>             C.takeC(C.giveC());
>>                            ^
>> Note: Some messages have been simplified; recompile with -Xdiags:verbose to get full output
>> 1 error
>> 
>> which is very misleading as the type seems to be the same. The issue here is that both the lambda and invocation `C.giveC()` are considered poly expressions. `C.giveC()` is not a poly expression but at the moment the compiler is analyzing if an invocation is a poly expression or not, it doesn't have enough information and errs on the safe side.
>> 
>> We have a cache at `com.sun.tools.javac.comp.ArgumentAttr`, see field `argumentTypeCache`, which stores the current deferred type of arguments considered poly expressions. The problem with this particular code is that inside of the lambda there is a class declaration and every time the lambda is attributed a new class type is created.
>> 
>> The lambda, and thus the expression `C.giveC()` are attributed twice. The first time we have an speculative attribution, as a result `argumentTypeCache` will have two entries, one for the lambda and one for `C.giveC()` but the type of `C` is the type created during speculative attribution on a copy of the original lambda tree, let's call it C1. Later on another pass the lambda is attributed `"for real"`, during the check phase. At this point the entry for the lambda in the cache pointed by `argumentTypeCache` has been removed but there is still the entry for `C.giveC()` which still refers to `C1`. So now in the `"for real"` attribution of the lambda which happens on the original tree, not in a copy, a new type for class `C` is created, let's call it `C2`. But when we get to the point where we need to attribute again `C.takeC(C.giveC())` invocation `C.takeC` is expecting `C2` but as `C.giveC()` is again considered a poly expression and an entry is found for it at the cache, the com
 piler reuses that entry which refers to `C1`. And unfortunately `C1 != C2` and thus the error is issued. So the solution I propose is to use a local cache for the speculative attribution of lambda expressions. This is a one liner fix, although an alternative solution could be to scan the lambda body and only use a local cache if a new type is defined inside the lambda, this could be a more optimal solution performance wise as we could save some attributions in some cases. Comments?
>> 
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   some experiments, close to a stable version

Marked as reviewed by mcimadamore (Reviewer).

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/DeferredAttr.java line 462:

> 460:     }
> 461: 
> 462:     boolean hasTypeDeclaration(JCLambda lambda) {

This is a good start.
A couple of comments:
* I believe this problem is more general than just lambdas (e.g. switch expression probably suffers from that as well)
* It would be better to write a single visitor which does everything in a single pass, rather than visit every statement in a block explicitly (after all, that's what the visitor is gonna do anyways). I even believe the visitor should recurse into everything (including nested lambdas and classes - after all, a class _nested_ in a local class might have a type which is then referred by the lambda return type!).

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

PR: https://git.openjdk.org/jdk/pull/12303