RFR: 8238735 NPE compiling lambda expression within conditional expression

[Adam Sotona]

Hi Maurizio,
thanks for the review.

May I ask you to push the patch?
Here is the changeset version of the patch in webrev: http://cr.openjdk.java.net/~asotona/8238735/webrev.01b/ <http://cr.openjdk.java.net/~asotona/8238735/webrev.01b/>

Thank you,
Adam

> On 17 Jun 2020, at 11:54, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> The patch looks less invasive/more pragmatic.
> 
> Looks good.
> 
> Maurizio
> 
> On 17/06/2020 08:37, Adam Sotona wrote:
>> Hi,
>> I've spent some time deep debugging of the javac NPE caused by lambda recovery process and here is an alternate patch (in contrast to the previous proposed patch skipping the recovery for the case).
>> webrev: http://cr.openjdk.java.net/~asotona/8238735/webrev.01/ <http://cr.openjdk.java.net/~asotona/8238735/webrev.01/>
>> JBS:  https://bugs.openjdk.java.net/browse/JDK-8238735 <https://bugs.openjdk.java.net/browse/JDK-8238735>
>> 
>> This patch runs the recovery round in case of invalid FunctionDescriptor, however to avoid return of result containing Type.recoveryType (which has no tsym and so causing the NPE and compiler crash) the final returned type is saved error type from the first round of visitLambda. The patch also contains new test for this NPE case.
>> 
>> The patch passes all Tier1, Tier2 and Tier 3 tests.
>> 
>> Thanks for the review,
>> Adam
>> 
>> 
>>> On 20 May 2020, at 17:25, Adam Sotona <adam.sotona at oracle.com <mailto:adam.sotona at oracle.com>> wrote:
>>> 
>>> Hi,
>>> in this situation the second "recovery" round completely messes the results.
>>> In the first run the result is correctly filled: result = that.type = types.createErrorType(pt()); and suppose to be returned,
>>> however then another recovery is triggered and it  re-enters the whole visitLambda with totally messed initial values.
>>> As a result the second evaluation returns Type.recoveryType constant, which I expect should be never returned and with no indication that the evaluation failed.
>>> Type.recoveryType has missing .tsym field and that is causing NPE. 
>>> My original idea was to simply avoid NPE by appropriate check, however I do not see any such similar code across javac.
>>> I see many direct calls of Type.tsymin the code however no NPE protection anywhere. How it is guaranteed that instances of JCNoType (with null tsym) will never reach the code?
>>> 
>>> Then I tried to fix it by correcting the result of the recovery round, however that seems to be very complex as the initial values of the recovery may vary a lot and detect the "messed" situation and find what suppose to be returned in that case is very complex.
>>> 
>>> Then I realized that this is the situation where recovery should not happen as lambda without its FunctionDescriptor can hardly continue in compilation as it does not know anything about the expected content.
>>> 
>>> The two error messages I had to remove from the test after the fix didn't make any sense to me. From all aspect there are two errors in the two lambdas, however second round of recovery listed two more nonsense errors: "compiler.err.cant.resolve.location: kindname.class, NonExistentClass, , , (compiler.misc.location: kindname.class, TargetType43, null)"
>>> 
>>> BTW: Impact of the NPE is not just when used within conditional expression (as described in the 8238735), however it is silently killing javac with NPE anywhere you try to cast lambda expression that fails to lookup its FunctionDescriptor. For example just try to compile: class A {boolean b = (boolean)(() -> false);}
>>> 
>>> Thanks,
>>> Adam
>>> 
>>>> On 20 May 2020, at 10:19, Adam Sotona <adam.sotona at oracle.com <mailto:adam.sotona at oracle.com>> wrote:
>>>> 
>>>> Hi,
>>>> I would like to add results from Corpus compilation regression testing: the patch caused no regression in compilation of all Corpus sources :)
>>>> 
>>>> Thanks for review,
>>>> Adam
>>>> 
>>>>> On 18 May 2020, at 17:47, Adam Sotona <adam.sotona at oracle.com <mailto:adam.sotona at oracle.com>> wrote:
>>>>> 
>>>>> Hi,
>>>>> I would like to ask for review of the patch fixing NPE while compiling lambda expression within conditional expression.
>>>>> The fix removes obsolete recovery cycle in com.sun.tools.javac.comp.Attr.visitLambda method when Types.FunctionDescriptorLookupError happens and it adds new test.
>>>>> 
>>>>> JBS:  https://bugs.openjdk.java.net/browse/JDK-8238735 <https://bugs.openjdk.java.net/browse/JDK-8238735> 
>>>>> webrev: http://cr.openjdk.java.net/~asotona/8238735/webrev.00/ <http://cr.openjdk.java.net/~asotona/8238735/webrev.00/>
>>>>> 
>>>>> Mach 5 build with the patch passes all Tier1, Tier2 and Tier3 tests.
>>>>> 
>>>>> 
>>>>> Thank you,
>>>>> Adam
>>>> 
>>> 
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20200617/7813cdeb/attachment.htm>