Skip to content
This repository was archived by the owner on Sep 2, 2022. It is now read-only.
/ jdk17 Public archive

8268766: Desugaring of pattern matching enum switch should be improved #81

Closed
wants to merge 12 commits into from
+426 −138
Closed

8268766: Desugaring of pattern matching enum switch should be improved #81

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8f19b0d
8268766: Desugaring of pattern matching enum switch should be improved
lahodaj Jun 15, 2021
2cd2223
Merging master.
lahodaj Jun 16, 2021
053c489
Adding and fixing test.
lahodaj Jun 16, 2021
4e1977a
Creating a new bootstrap method for (pattern matching) enum switches,…
lahodaj Jun 17, 2021
06a1beb
Updating javadoc, code and tests as suggested.
lahodaj Jun 18, 2021
196e106
Updating javadoc, as suggested.
lahodaj Jun 22, 2021
0c37136
Improving javadoc.
lahodaj Jun 23, 2021
0a35b79
Merge branch 'master' into JDK-8268766
lahodaj Jun 28, 2021
ca79b2b
Reflecting review comments.
lahodaj Jun 30, 2021
469254a
Apply suggestions from code review
lahodaj Jul 7, 2021
d970402
If the pattern type is a supertype of the selector type, use selector…
lahodaj Jul 7, 2021
fe3de7d
Adding a testcase for an enum that implements an interface.
lahodaj Jul 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
View file
Original file line number Diff line number Diff line change
@@ -110,14 +110,14 @@ private SwitchBootstraps() {}
* second parameter of type {@code int} and with {@code int} as its return type,
* or if {@code labels} contains an element that is not of type {@code String},
* {@code Integer} or {@code Class}.
* @throws Throwable if there is any error linking the call site
* @jvms 4.4.6 The CONSTANT_NameAndType_info Structure
* @jvms 4.4.10 The CONSTANT_Dynamic_info and CONSTANT_InvokeDynamic_info Structures
*/
public static CallSite typeSwitch(MethodHandles.Lookup lookup,
String invocationName,
MethodType invocationType,
Object... labels) throws Throwable {
Object... labels) throws NullPointerException,
IllegalArgumentException {
if (invocationType.parameterCount() != 2
|| (!invocationType.returnType().equals(int.class))
|| invocationType.parameterType(0).isPrimitive()
@@ -173,9 +173,13 @@ private static int doTypeSwitch(Object target, int startIndex, Object[] labels)
* Bootstrap method for linking an {@code invokedynamic} call site that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be made clearer - e.g. the first argument is of type Class and represents the enum we want to switch on. The remaining constants should be of type String, the names of the various constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not quite what the labels represent. The target Enum type is inferred from the bootstrap method's invocation MethodType. (Alternatively, we can add a new Class parameter to the bootstrap method.)

For the labels, please consider this switch:

         E sel = null;
         switch (sel) {
             case A -> {}
             case E e && "B".equals(e.name()) -> {}
             case C -> {}
             case E e -> {}
         }

The patterns and the constants are mixed, and the order needs to be represented somehow in the labels array, so that the switch will classify the input correctly. The method in this proposal will use {"A", E.class, "C", E.class} as the labels array (which is mostly consistent with the typeSwitch method), but we could use different encodings if needed.

* implements a {@code switch} on a target of an enum type. The static
* arguments are used to encode the case labels associated to the switch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String and Class should appear in code blocks perhaps, or link tags? Also, I think this text could be improved by splitting the sentence by using a bullet list:

The static arguments are used to encode the case labels associated to the switch
construct, where each label can be encoded in two ways:
* as a String value, which represents the name of the enum constant associated with the label
* as a Class value, which represents the enum type associated with a type test pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've (tried to) updated the javadoc as suggested.

* construct, where each label can be encoded as a String (to represent
* an enum constant), or, alternatively, as a Class of the target enum type
* (to represent a type test pattern whose type is the enum type).
* construct, where each label can be encoded in two ways:
* <ul>
* <li>as a {@code String} value, which represents the name of
* the enum constant associated with the label</li>
* <li>as a {@code Class} value, which represents the enum type
* associated with a type test pattern</li>
* </ul>
* <p>
* The returned {@code CallSite}'s method handle will have
* a return type of {@code int} and accepts two parameters: the first argument
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By writing the javadoc text above, it came to mind that, perhaps, some validation is in order for the static arguments. For instance:

  • the enum type of a Class parameter doesn't match that of the BSM
  • the static arguments contain more than one Class parameter (I think even if they are all of the same "correct" type, that's bogus, right?)

@@ -214,14 +218,14 @@ private static int doTypeSwitch(Object target, int startIndex, Object[] labels)
* second parameter of type {@code int} and with {@code int} as its return type,
* or if {@code labels} contains an element that is not of type {@code String} or
* {@code Class} of the target enum type.
* @throws Throwable if there is any error linking the call site
* @jvms 4.4.6 The CONSTANT_NameAndType_info Structure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what we do for other bootstraps, but I wonder if the throws clause here should be more specific - for instance the LambdaMetafactory tells when an exception is gonna be triggered. Here it seems like there are a couple of cases which can be called out, where we know that linkage is going to fail.

* @jvms 4.4.10 The CONSTANT_Dynamic_info and CONSTANT_InvokeDynamic_info Structures
*/
public static CallSite enumSwitch(MethodHandles.Lookup lookup,
String invocationName,
MethodType invocationType,
Object... labels) throws Throwable {
Object... labels) throws NullPointerException,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there are any benefits to declaring the runtime exceptions in the method declaration.

Other bootstrap methods declare a checked exception when certain linkage constraints are violated (and the line between whats an IllegalArgumentException or an explicit linkage checked exception can be blurry). In your case, given the simplicity i think what you have is ok. We could refine later with a specific checked exception for switch linkage violations.

IllegalArgumentException {
if (invocationType.parameterCount() != 2
|| (!invocationType.returnType().equals(int.class))
|| invocationType.parameterType(0).isPrimitive()