RFR: 8238735 NPE compiling lambda expression within conditional expression

[Maurizio Cimadamore]

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/a8694f44/attachment-0001.htm>