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

[Vicente Romero]

On Wed, 1 Feb 2023 13:51:21 GMT, Maurizio Cimadamore <mcimadamore 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
>
> I think the fix looks sensible. Browsing the `ArgumentAttr` code it also seems that the same strategy is used for checking the receiver expression of a method reference - so in that case we would also attribute multiple times.
> 
> IIRC, the cache is mostly used to save the speculative attribution results so that we don't need to re-check again when we're outside the speculative mode. E.g. if an explicit lambda is passed to an overloaded method, it is still checked only once (e.g. not one time per overload candidate). But, with this fix, if the lambda is nested deeper in a method call chain, it will be checked multiple times.
> 
> One thing that strikes my in your analysis is that we drop the cached entry for the lambda, but we don't drop the entry for the method call, even though the method call is "nested" inside the lambda. That seems like a potential angle to attack this.

@mcimadamore thanks for all the comments and suggestions. I have uploaded a new iteration, code cleaner now thanks!

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

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