JDK-8179483: Return type inference in if

[Johannes Kuhn]

On 30-Jun-22 16:32, Remi Forax wrote:
> ----- Original Message -----
>> From: "Johannes Kuhn" <info at j-kuhn.de>
>> To: classfile-api-dev at openjdk.org
>> Sent: Thursday, June 30, 2022 3:31:57 PM
>> Subject: JDK-8179483: Return type inference in if
> 
>> So, I tried to extract the jdk.classfile API and compile it using Java
>> 18 with preview features enabled. Using Eclipse.
>>
>> While doing that, I encountered some compilation problems - one is the
>> use of something similar like this:
>>
>>      <T> T optionValue(Classfile.Option.Key key) {...}
>>      ...
>>      if (reader.optionValue(...)) {...}
>>
>> According to https://bugs.openjdk.org/browse/JDK-8179483 ('if'
>> conditions are treated like assignments by inference) this abuses a bug
>> in javac - and is not valid java.
>>
>> The places where I found a use like this are:
>>
>> https://github.com/openjdk/jdk-sandbox/blob/classfile-api-branch/src/java.base/share/classes/jdk/classfile/constantpool/ConstantPoolBuilder.java#L76
>> https://github.com/openjdk/jdk-sandbox/blob/classfile-api-branch/src/java.base/share/classes/jdk/classfile/impl/BoundAttribute.java#L150
>> https://github.com/openjdk/jdk-sandbox/blob/classfile-api-branch/src/java.base/share/classes/jdk/classfile/impl/CodeImpl.java#L153
>> https://github.com/openjdk/jdk-sandbox/blob/classfile-api-branch/src/java.base/share/classes/jdk/classfile/impl/CodeImpl.java#L208
>> https://github.com/openjdk/jdk-sandbox/blob/classfile-api-branch/src/java.base/share/classes/jdk/classfile/impl/DirectCodeBuilder.java#L111
>> https://github.com/openjdk/jdk-sandbox/blob/classfile-api-branch/src/java.base/share/classes/jdk/classfile/impl/DirectCodeBuilder.java#L209
>> https://github.com/openjdk/jdk-sandbox/blob/classfile-api-branch/src/java.base/share/classes/jdk/classfile/impl/DirectCodeBuilder.java#L277
> 
> yes, this is an annoying bug of javac, i've also encounter it in student codes :)
> 
> Here, the real question is why optionValue() has such weird signature, it means there is a @SuppressWarnings("unchecked") which is not safe in the implementation.
> This kind of signature is only valid if T is always null, like in Collections.emptyList().
> 

 From some point of view, it provides a "nice" API - if it would return 
Object - as it should - then a cast is needed at the callsite. Which is 
IMHO not that bad, but preferences differ, and some people prefer to 
write clever code.

I have to admit, the use of such a method is nice - but IMHO such a 
method can only ever fulfill its contract (return something the caller 
wants, but the caller doesn't tell what it wants) if it returns `null`.

>>
>> As far as I am concerned, there are two possible fixes:
>>
>> * Make `if` an assignment context - but then why is `for (String s :
>> EXPR) {}` not an assignment context for `Iterable<String>`?
>> * Change the code in jdk.classfile so it adds type hints in those cases.
>>
>> I am not qualified enough to judge which one is the better fix.
>>
>> - Johannes
>>
>> PS.: FYI: There seems also be a bug in ECJ w.r.t. handling of class
>> hierarchies of sealed classes. I still try to create a small reproducer.
> 
> 
> Rémi

- Johannes