JDK-8179483: Return type inference in if

[forax at univ-mlv.fr]

----- Original Message -----
> From: "Johannes Kuhn" <info at j-kuhn.de>
> To: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "classfile-api-dev" <classfile-api-dev at openjdk.org>
> Sent: Thursday, June 30, 2022 5:32:17 PM
> Subject: Re: JDK-8179483: Return type inference in if

> On 30-Jun-22 17:12, forax at univ-mlv.fr wrote:
>> ----- Original Message -----
>>> From: "Johannes Kuhn" <info at j-kuhn.de>
>>> To: "Remi Forax" <forax at univ-mlv.fr>
>>> Cc: "classfile-api-dev" <classfile-api-dev at openjdk.org>
>>> Sent: Thursday, June 30, 2022 4:44:37 PM
>>> Subject: Re: JDK-8179483: Return type inference in if
>> 
>>> 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.
>> 
>> It's clever until it's not, by example
>>    var list = new ArrayList<SpaceGiant>();
>>    list.add(optionValue(...));
>> 
>> the compiler will be happy to compile that snippet and *not* insert a cast
>> because optionValue() says it returns a space giant and both E and T erases to
>> Object.
> 
> That... sounds wrong. It should. Javac 17 does.
> https://gist.github.com/DasBrain/212f36f88978b7ddd4fb6ea3a97a392e (fails
> with CCE)
> Might be a different story if it's an ArrayList<T> (T is a type variable).

It even more fun, it depends on the compiler, javac 1.8 is happy to not insert the cast while javac 11 insert it.

Here is the bytecode generated by javac 1.8.

  public static void main(java.lang.String[]);
    Code:
       0: new           #2                  // class java/util/ArrayList
       3: dup
       4: invokespecial #3                  // Method java/util/ArrayList."<init>":()V
       7: astore_1
       8: aload_1
       9: ldc           #4                  // String not an Integer
      11: invokestatic  #5                  // Method uncheckedCast:(Ljava/lang/Object;)Ljava/lang/Object;
      14: invokevirtual #6                  // Method java/util/ArrayList.add:(Ljava/lang/Object;)Z
      17: pop
      18: return

and the one generated by javac 11

 public static void main(java.lang.String[]);
    Code:
       0: new           #2                  // class java/util/ArrayList
       3: dup
       4: invokespecial #3                  // Method java/util/ArrayList."<init>":()V
       7: astore_1
       8: aload_1
       9: ldc           #4                  // String not an Integer
      11: invokestatic  #5                  // Method uncheckedCast:(Ljava/lang/Object;)Ljava/lang/Object;
      14: checkcast     #6                  // class java/lang/Integer      <-----------
      17: invokevirtual #7                  // Method java/util/ArrayList.add:(Ljava/lang/Object;)Z
      20: pop
      21: return


Rémi