RFR: JDK-8207224: Javac compiles source code despite illegal use of unchecked conversions

[Maurizio Cimadamore]

The patch and CSR look good - but I'm not sure if it's worth playing 
defensively from the start (sorry if my message was misinterpreted).

That is, if there is a chance that by dropping the subtyping test we 
could consolidate the various code paths in Types for checking for 
return type subst, then we should evaluate the cost of breaking clients 
against the cost of having diverging implementations. Of course if the 
breakage is big, the implementation aspects become secondary, but if the 
breakage is minimal/non-observed in practice, then we should strive for 
fidelity with the spec (and cleaner code) ?

Maurizio


On 03/12/2018 23:32, Vicente Romero wrote:
> Hi,
>
> On 12/3/18 4:13 PM, Maurizio Cimadamore wrote:
>>
>> Looks good - btw, if the compatibility cost proves to be too much we 
>> could consider putting the new behavior behind the source switch (as 
>> we did with override/hide string clash checking for some time).
>>
>
> I have modified the patch adding a guard to execute the new test only 
> if source is >= 12, please take a look at the new iteration [1], I 
> have also modified the CSR [2] accordingly
>
>> Cheers
>> Maurizio
>>
>
> Thanks,
> Vicente
>
> [1] http://cr.openjdk.java.net/~vromero/8207224/webrev.01/
> [2] https://bugs.openjdk.java.net/browse/JDK-8214729
>
>> On 03/12/2018 21:01, Vicente Romero wrote:
>>> Hi,
>>>
>>> Please review the related CSR [1]
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8214729
>>>
>>> On 12/1/18 2:46 PM, Vicente Romero wrote:
>>>> Please review fix for [1] at [2]. Javac is accepting this program 
>>>> as valid:
>>>>
>>>> class ReturnTypeSubstitutableTest {
>>>>     abstract class AbstractDemo<Request extends AbstractResult, 
>>>> Response extends AbstractResult> {
>>>>         protected abstract Response test(Request request);
>>>>     }
>>>>
>>>>     abstract interface AbstractResult {}
>>>>
>>>>     abstract interface SimpleResult extends AbstractResult {}
>>>>
>>>>     class Result1 implements SimpleResult {}
>>>>
>>>>     class OtherResult implements AbstractResult {}
>>>>
>>>>     public class SimpleDemo<Request extends AbstractResult, 
>>>> Response extends AbstractResult> extends AbstractDemo<Request, 
>>>> Response> {
>>>>         @Override
>>>>         protected SimpleResult test(AbstractResult request) {  
>>>> <----------- this method is accepted as a good override for 
>>>> AbstractDemo::test
>>>>             return null;
>>>>         }
>>>>     }
>>>> }
>>>>
>>>> From Dan's evaluation at the bug entry [1]:
>>>>
>>>> Per JLS 8.4.8.3, the first method must be return-type-substitutable 
>>>> for the second. This can be satisfied in 3 ways (JLS 8.4.5):
>>>> - SimpleResult is a subtype of Response (no)
>>>> - SimpleResult can be converted to a subtype of Response by 
>>>> unchecked conversion (no)
>>>> - The methods have different signatures (yep) and SimpleResult = 
>>>> erasure(Response) (no)
>>>>
>>>> No case applies, so an error should occur. This fix syncs javac 
>>>> with the spec.
>>>>
>>>> Thanks,
>>>> Vicente
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8207224
>>>> [2] http://cr.openjdk.java.net/~vromero/8207224/webrev.00/
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20181204/332d163f/attachment-0001.html>