RFR: JDK-8203338: Unboxing in return from lambda miscompiled to throw ClassCastException

[Maurizio Cimadamore]

Looks good to me - thanks

Maurizio


On 20/06/18 15:15, B. Blaser wrote:
> I've attached the patch along with the following test cases:
> * lambda without 'return' (coercion + unboxing + widening primitive)
> * lambda with 'return' (idem)
> * method reference converted to lambda (coercion)
> * method with 'return' (idem)
>
> Any comment (tier1 is OK)?
> Bernard
>
> On 19 June 2018 at 22:37, Vicente Romero <vicente.romero at oracle.com> wrote:
>> Hi,
>>
>> I agree with Maurizio that another test covering method references would be
>> nice. Apart from that I like the patch, thanks for taking the time to test
>> this other approach.
>>
>> Vicente
>>
>>
>> On 06/19/2018 04:02 PM, Maurizio Cimadamore wrote:
>>> I was not 100% sure that removing all coerce logic from LambdaToMethod is
>>> gonna work - LambdaToMethod is actively rewriting method reference
>>> expression as lambdas - and these synthetic lambdas are not seen by
>>> TransTypes - see here:
>>>
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java#l903
>>>
>>> But luckily, it seems L2M already handles these from inside the
>>> MemberReferenceToLambda class:
>>>
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java#l1051
>>>
>>> So that should be ok? But I'd be more comfortable if the tests included a
>>> case that stressed the method reference path - e.g. an example of a method
>>> reference that requires a synthetic cast to be compliant with the target
>>> type - just to make sure.
>>>
>>> Maurizio
>>>
>>>
>>>
>>> On 19/06/18 19:31, B. Blaser wrote:
>>>> On 18 June 2018 at 23:09, B. Blaser <bsrbnd at gmail.com> wrote:
>>>>> On 18 June 2018 at 22:12, Maurizio Cimadamore
>>>>> <maurizio.cimadamore at oracle.com> wrote:
>>>>>> I think that we should use the (erased) return type of the functional
>>>>>> descriptor as the target of the translation. In both the
>>>>>> expression/statement case.
>>>>>>
>>>>>> For statement, why not assigning currentMethod to the full method
>>>>>> descriptor
>>>>>> type? Or, if we really use currentMethod just for visitReturn, maybe
>>>>>> just
>>>>>> storing the expected return (as Vicente suggests) is ok?
>>>>>>
>>>>>> Maurizio
>>>>> I think this should probably work too but it might have more
>>>>> side-effects than the fix I suggested.
>>>>> I'll take a look and provide another patch with this variant if all goes
>>>>> well...
>>>>>
>>>>> Bernard
>>>> I've attached the variant you both suggested.
>>>> Any comment is welcome (langtools:tier1 is OK)!
>>>>
>>>> Bernard
>>>