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

[B. Blaser]

The suggested fix is intended to reflect the actual successful
behavior without 'return':

http://hg.openjdk.java.net/jdk/jdk/file/8f1d5d706bdd/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransTypes.java#l540

If 'tree.body.type != null', the single expression without 'return' is
translated with 'pt = erasure(tree.body.type)' becoming
'(Character)List.of(a).get(0)'.

Further in 'LambdaToMethod.makeLambdaExpressionBody()' the 'return' is
added (along with another type conversion to the lambda return type
when necessary):

http://hg.openjdk.java.net/jdk/jdk/file/8f1d5d706bdd/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java#l660

... ending with '{ return (Character)List.of(a).get(0); }' which will
later be successfully unboxed.

So, the suggested fix is performing the same thing with 'return':

http://hg.openjdk.java.net/jdk/jdk/file/8f1d5d706bdd/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransTypes.java#l604

If 'currentMethod == null' means we are in a lambda and the fix adds
'pt = types.erasure(tree.expr.type)' if 'tree.expr != null' (an
expression is returned), ending also with '{ return
(Character)List.of(a).get(0); }' which will later be successfully
unboxed.

I guess the current patch is rather safe because it mirrors the actual
behavior without 'return' (it simply keeps the original expression
typing) but I'll take a look at your suggestion too.

What do you think?

Thanks,
Bernard


On 18 June 2018 at 04:29, Vicente Romero <vicente.romero at oracle.com> wrote:
> Hi,
>
> Not sure about this one. I think that the type of the return expression
> inside a lambda should be erased to the erasure of the return type of the
> lambda type. The `currentMethod` field at TransTypes is just a carrier to
> actually access the erasure of the return type of the current method, and
> this access happens only inside visitReturn. So I wonder if we actually
> should have a field in TransTypes that could hold that erasure directly. At
> TransTypes.visitMethodDef we could have:
>
> theField = types.erasure(tree.type.getReturnType());
>
> and at TransTypes.visitLambda it could be:
>
> theField =
> types.erasure(tree.getDescriptorType(types).asMethodType().restype);
>
> of course with a better name, and then at TransTypes.visitReturn, it could
> use the field to erase the returned expression.
>
> Thanks,
> Vicente
>
>
> On 06/16/2018 06:20 PM, B. Blaser wrote:
>>
>> Hi,
>>
>> As noted in the JBS issue, the following example fails at run-time with a
>> CCE:
>>
>>      List.of('x', 'y').stream().max((a, b) -> { return List.of(a).get(0);
>> });
>>
>> The interesting point here is that removing 'return' makes it run
>> successfully:
>>
>>      List.of('x', 'y').stream().max((a, b) -> List.of(a).get(0));
>>
>> The problem seems to be in 'TransType.visitReturn()'. The lambda
>> method doesn't yet exist ('currentMethod == null', see
>> 'TransType.visitLambda()') causing the return expression type to be
>> set to the erasure of 'List.get()' (= Object) which further causes the
>> type conversion problem (see
>> 'LambdaToMethod.makeLambdaStatementBody()').
>>
>> The suggested fix (here under) is to keep the original erased
>> expression type (= Character) if 'currentMethod == null'.
>>
>> Tier1 is OK.
>>
>> Any feedback is welcome,
>> Bernard
>>
>> diff -r ed8de3d0cd28
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransTypes.java
>> ---
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransTypes.java
>>     Sat Jun 16 10:10:54 2018 +0100
>> +++
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransTypes.java
>>     Sat Jun 16 21:16:33 2018 +0200
>> @@ -601,7 +601,8 @@
>>       }
>>
>>       public void visitReturn(JCReturn tree) {
>> -        tree.expr = translate(tree.expr, currentMethod != null ?
>> types.erasure(currentMethod.type).getReturnType() : null);
>> +        tree.expr = translate(tree.expr, currentMethod != null ?
>> types.erasure(currentMethod.type).getReturnType() :
>> +                tree.expr != null ? types.erasure(tree.expr.type) :
>> null);
>>           result = tree;
>>       }
>
>