RFR: 8303374: Compiler Implementation for Primitive types in patterns, instanceof, and switch (Preview) [v3]

[Aggelos Biboudis]

On Tue, 3 Oct 2023 09:58:11 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Aggelos Biboudis has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - Merge branch 'master' into primitive-patterns
>>  - Implement type pairs to exactnessMethod name
>>  - Apply suggestions from code review
>>    
>>    Co-authored-by: Raffaello Giulietti <raffaello.giulietti at oracle.com>
>>  - 8303374: Compiler Implementation for Primitive types in patterns, instanceof, and switch (Preview)
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 228:
> 
>> 226:                         currentTest = trueDef;
>> 227:                     } else if (currentLabelClass.isPrimitive()) {
>> 228:                          if (selectorType.isInstance(Object.class)) {
> 
> As discussed offline, I believe this is wrong, and it should be replaced by `Class::equals`. That said, I think that "accidentally" you get the desired behavior here, as demonstrated by jshell:
> 
> 
> jshell> Byte.class.isInstance(Object.class)
> $6 ==> false
> 
> 
> jshell> Object.class.isInstance(Object.class)
> $8 ==> true
> 
> 
> So, this effectively acts as a test to check if the selector type is `Object`. Of course, since `isInstance` is used, spurious stuff is picked up as well:
> 
> 
> jshell> Class.class.isInstance(Object.class)
> $7 ==> true
> 
> But this situation turns out to be non problematic, given that a primitive pattern is not applicable to a selector type Class.

Fixed as well.

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 5052:
> 
>> 5050: 
>> 5051:         return (source.isPrimitive() && target.isPrimitive()) &&
>> 5052:                 ((source.hasTag(BYTE) && !target.hasTag(CHAR) ||
> 
> Does this mean that `byte` -> `char` is not exact? Aren't both integral types, thus invalidating `widening from one integral type to another` ?

Indeed it is not unconditionally exact. byte to char combines both widening and narrowing primitive conversions 

https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.4

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4154:
> 
>> 4152:     public void visitTypeTest(JCInstanceOf tree) {
>> 4153:         Type exprtype = attribExpr(tree.expr, env);
>> 4154:         if(!allowPrimitivePatterns) {
> 
> nit: space missing

Fixed!

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 735:
> 
>> 733:                 }
>> 734:             }
>> 735:             if (tree.selector.type.hasTag(TypeTag.BOOLEAN)) {
> 
> I would have expected true/false to be treated more or less like enum constants when it came to exhaustiveness? E.g. boolean is similar to an enum type that only has two options (or, if you will, a sealed type with two permitted subtypes). So, IMHO the treatment for booleans should happen in the `exhausts` method - and this method should be left very simple (possibly unchanged?)

Fixed.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344099605
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344095080
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344095209
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344095335