RFR: 8238735 NPE compiling lambda expression within conditional expression

[Maurizio Cimadamore]

On 17/06/2020 12:23, Adam Sotona wrote:
> 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/

I can push - I assume this needs to go straight into jdk/jdk15 and not 
to jdk/jdk, right?

Maurizio

>
> Thank you,
> Adam
>
>> On 17 Jun 2020, at 11:54, Maurizio Cimadamore 
>> <maurizio.cimadamore at oracle.com 
>> <mailto: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/
>>> JBS: 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
>>>>>> webrev: 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/2618a872/attachment.htm>