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

[Maurizio Cimadamore]

On Thu, 2 Feb 2023 19:30:06 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:
> 
>   a more general approach to cover every expression

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

> 440:         try {
> 441:             localEnv.info.returnResult = resultInfo;
> 442:             /* we should be on the safe side for lambdas that declare a local type as it could be that the cache

I'd suggest a more succinct comment - e.g.:


When performing speculative attribution on an argument expression, we should make sure that argument type cache does not get polluted with local types, as that leads to spurious type errors (see JDK-8295019)

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

> 447:              */
> 448:             JCBlock speculativeTree = hasTypeDeclaration(that) ?
> 449:                     (JCBlock)attribSpeculative(lambdaBlock, localEnv, resultInfo, argumentAttr.withLocalCacheContext()) :

Given `attribSpeculative` also checks for `hasTypeDeclaration` do we need to also check here?

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

> 466:     }
> 467: 
> 468:     boolean hasTypeDeclaration(JCTree tree) {

Nice and simple!

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

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