RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v11]

[Maurizio Cimadamore]

On Thu, 12 May 2022 10:54:16 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> This is a (preliminary) patch for javac implementation for the third preview of pattern matching for switch (type patterns in switches).
>> 
>> Draft JLS:
>> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html
>> 
>> The changes are:
>> -there are no guarded patterns anymore, guards are not bound to the CaseElement (JLS 15.28)
>> -a new contextual keyword `when` is used to add a guard, instead of `&&`
>> -`null` selector value is handled on switch level (if a switch has `case null`, it is used, otherwise a NPE is thrown), rather than on pattern matching level.
>> -total patterns are allowed in `instanceof`
>> -`java.lang.MatchException` is added for the case where a switch is exhaustive (due to sealed types) at compile-time, but not at runtime.
>> 
>> Feedback is welcome!
>> 
>> Thanks!
>
> Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Updating naming to more closely follow the spec: total patterns are renamed to unconditional patterns, unrefined is now unguarded.
>  - Case label elements cannot be unguarded if they have a guard of a type different than boolean.

Looks good - left some minor comments and questions.

src/java.base/share/classes/java/lang/MatchException.java line 58:

> 56:      * @param  message the detail message (which is saved for later retrieval
> 57:      *         by the {@link #getMessage()} method).
> 58:      * @param  cause the cause (which is saved for later retrieval by the

This looks odd - it seems like the sentence is like this:

`the cause ( foo ). (bar)`. E.g. the test in parenthesis exceeds the test outside parenthesis by a wide margin. I suggest both here and in the "message" @param to avoid the parenthesis and split the sentence instead. Examples:


* @param  message the detail message. The message is saved for later retrieval
*  by the {@link #getMessage()} method).


and


* @param cause the cause. The cause is saved for later retrieval by the
*  {@link #getCause()} method). A {@code null} value is
* permitted, and indicates that the cause is nonexistent or
* unknown.


Of course this is just an idea.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 177:

> 175:         allowPatternSwitch = (preview.isEnabled() || !preview.isPreview(Feature.PATTERN_SWITCH)) &&
> 176:                              Feature.PATTERN_SWITCH.allowedInSource(source);
> 177:         allowUnconditionalPatternsInstance = (preview.isEnabled() || !preview.isPreview(Feature.UNCONDITIONAL_PATTERN_IN_INSTANCEOF)) &&

Nit: perhaps adding "Of" to this already long variable name doesn't add much in terms of chars, but makes the meaning of the variable name clearer?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1802:

> 1800:                                 unguarded &&
> 1801:                                 !patternType.isErroneous() &&
> 1802:                                 types.isSubtype(types.boxedTypeOrType(types.erasure(seltype)),

This seems to be a change compared to the previous code - e.g. handling of boxing in the switch target type. Is this code even exercised? The test "NotApplicableTypes" seems to rule the combination `switch (int) ... case Integer` out.

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java line 1325:

> 1323:     }
> 1324: 
> 1325:     public record PatternPrimaryType(Type type) {}

This is no longer needed - seems just a wrapper around a type.

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

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