RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]

[Paul Sandoz]

On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> This is a preview of a patch implementing JEP 406: Pattern Matching for switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>> 
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>> 
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common superinterface for expressions and patterns, `CaseLabelTree`, which expressions and patterns implement, and a list of case labels is returned from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree` that represents it (`DefaultCaseLabelTree`). It is used also to represent the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, String, enum) switches. This is a bit tricky for boxed primitives, as there is no value that is not part of the input domain so that could be used to represent `case null`. Requires a bit shuffling with values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It produces code that delegates to a new bootstrap method, that will classify the input value to the switch and return the case number, to which the switch then jumps. To support guards, the switches (and the bootstrap method) are restartable. The bootstrap method as such is written very simply so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String s`/`case null: case String s:`/`case String s: case null:`, handling of these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>> 
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixing typo.

A really nice feature, and a significant amount of work in javac. I mostly focused on the bootstrap and API aspects, and left some minor comments (most of which you can choose to apply or not as you see fit).

I suspect the bootstrap might evolve as we get feedback and switch is enhanced with further forms of matching. But, at the moment it looks good.

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 87:

> 85:      * returns {@literal -1}.
> 86:      * <p>
> 87:      * the {@code target} is not {@code null}, then the method of the call site

Suggestion:

     * If the {@code target} is not {@code null}, then the method of the call site

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 92:

> 90:      * <ul>
> 91:      *   <li>the element is of type {@code Class} and the target value
> 92:      *       is a subtype of this {@code Class}; or</li>

Suggestion:

     *   <li>the element is of type {@code Class} that is assignable
     *       from the target's class; or</li>

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 162:

> 160:                     return i;
> 161:             } else {
> 162:                 if (label instanceof Integer constant) {

Minor suggestion: use `else if` rather than nest

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 166:

> 164:                         return i;
> 165:                     }
> 166:                     if (target instanceof Character input && constant.intValue() == input.charValue()) {

Use `else if`

src/jdk.compiler/share/classes/com/sun/source/tree/CaseLabelTree.java line 31:

> 29: 
> 30: /**A marker interface for {@code Tree}s that may be used as {@link CaseTree} labels.
> 31:  *

Suggestion:

/**
 * A marker interface for {@code Tree}s that may be used as {@link CaseTree} labels.
 *

src/jdk.compiler/share/classes/com/sun/source/tree/DefaultCaseLabelTree.java line 30:

> 28: 
> 29: /** A case label that marks {@code default} in {@code case null, default}.
> 30:  * @since 17

Suggestion:

/** 
 * A case label that marks {@code default} in {@code case null, default}.
 *
 * @since 17

test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 55:

> 53:         catch (NoSuchMethodException | IllegalAccessException e) {
> 54:             throw new RuntimeException(e);
> 55:         }

Suggestion:

        catch (ReflectiveOperationException e) {
            throw new AssertionError(e, "Should not happen");
        }

test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 73:

> 71:     }
> 72: 
> 73:     public void testTypes() throws Throwable {

As a follow up issue consider adding tests for `null` values

test/langtools/tools/javac/patterns/DisambiguateParenthesizedPattern.java line 72:

> 70:         SwitchTree st = (SwitchTree) method.getBody().getStatements().get(0);
> 71:         CaseLabelTree label = st.getCases().get(0).getLabels().get(0);
> 72:         ExpressionType actualType = switch (label) {

Should the test be careful of using a pattern match switch?

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

Marked as reviewed by psandoz (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3863