RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]

[Jan Lahoda]

On Tue, 18 Apr 2023 20:24:33 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Jan Lahoda has updated the pull request incrementally with six additional commits since the last revision:
>> 
>>  - Fixing infinite loop where a binding pattern is replaced with a binding pattern for the same type.
>>  - Reflecting review comments.
>>  - Fixing exhaustiveness for unsealed supertype pattern.
>>  - No need to enable features after error reported.
>>  - SwitchBootstraps.typeSwitch should not initialize enum classes.
>>  - A prototype of avoiding enum initialization.
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 94:
> 
>> 92:      *   <li>the element is of type {@code String} or {@code Integer} and
>> 93:      *       equals to the target.</li>
>> 94:      *   <li>the element is of type {@code EnumDesc}, that describes a constant that is
> 
> I think that at ~line 76, up in this same javadoc you need to add a reference to EnumDesc in the general description. Where it says:
> 
> 
> The static arguments are an array of case labels which must be non-null and of type
> {@code String} or {@code Integer} or {@code Class}.
> 
> also around line 107, where it says:
> 
> @param labels case labels - {@code String} and {@code Integer} constants
>                and {@code Class} instances, in any combination

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 263:
> 
>> 261:     private static <E extends Enum<E>> void validateEnumLabel(Class<?> enumClassTemplate, Object label) {
>> 262:         if (label == null) {
>> 263:             throw new IllegalArgumentException("null label found");
> 
> I think that this one is not hit by any test

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 271:
> 
>> 269:                                                    ", expected the provided enum class: " + enumClassTemplate);
>> 270:             }
>> 271:         } else if (labelClass == String.class) {
> 
> I think that if the condition is changed to `labelClass != String.class` then `throw` we can drop an `else` branch

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 274:
> 
>> 272:             //OK
>> 273:         } else {
>> 274:             throw new IllegalArgumentException("label with illegal type found: " + labelClass +
> 
> I think that this code is not being tested by any test

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 201:
> 
>> 199:     /** Are unconditional patterns in instanceof allowed
>> 200:      */
>> 201:     private boolean allowUnconditionalPatternsInstanceOf;
> 
> so I guess this field can be made final now

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 909:
> 
>> 907: 
>> 908:                             Set<Symbol> permitted = allPermittedSubTypes(clazz, csym -> {
>> 909:                                 Type instantiated = infer.instantiatePatternType(selectorType, csym);
> 
> for some cases, when there are no type parameters, this invocation is a no-op, do we really need inference at this point?

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877205
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877482
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877294
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877401
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172878133
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172878329