-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8262891: Compiler implementation for Pattern Matching for switch (Preview) #3863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into |
@lahodaj The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work. There's a lot to take in here. I think overall, it holds up well - I like how case labels have been extended to accommodate for patterns. In the front-end, I think there are some questions over the split between Attr and Flow - maybe it is unavoidable, but I'm not sure why some control-flow analysis happens in Attr instead of Flow and I left some questions to make sure I understand what's happening.
In the backend it's mostly good work - but overall the structure of the code, both in Lower and in TransPattern is getting very difficult to follow, given there are many different kind of switches each with its own little twist attached to it. It would be nice, I think (maybe in a separate cleanup?) if the code paths serving the different switch kinds could be made more separate, so that, when reading the code we can worry only about one possible code shape. That would make the code easier to document as well.
R visitGuardedPattern(GuardedPatternTree node, P p); | ||
|
||
/** | ||
* Visits an AndPatternTree node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment correct?
@@ -290,6 +302,26 @@ | |||
*/ | |||
R visitNewArray(NewArrayTree node, P p); | |||
|
|||
/** | |||
* Visits an GuardPatternTree node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Visits an GuardPatternTree node. | |
* Visits a GuardPatternTree node. |
if (!allowPatternSwitch) { | ||
log.error(DiagnosticFlag.SOURCE_LEVEL, selector.pos(), | ||
Feature.PATTERN_SWITCH.error(this.sourceName)); | ||
allowPatternSwitch = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this logic is to show only one error in case we're compiling multiple methods with pattern switches and preview features are not enabled? Is this consistent with what happens with other preview features though?
if (!allowCaseNull) { | ||
log.error(DiagnosticFlag.SOURCE_LEVEL, expr.pos(), | ||
Feature.CASE_NULL.error(this.sourceName)); | ||
allowCaseNull = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here about error recovery and minimize messages - if this is what we'd like to do, perhaps centralizing the check in Preview would be better (so that e.g. we'd return that a given preview feature is not supported only once).
@@ -1758,6 +1858,26 @@ private Symbol enumConstant(JCTree tree, Type enumType) { | |||
} | |||
return null; | |||
} | |||
private Pair<Type, Boolean> primaryType(JCPattern pat) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a record here would lead for better code (no boxing of Boolean, but, more importantly, better field names for fst/snd). More generally, now that we have records I think we should think twice before using Pairs :-)
c.stats = translate(c.stats); | ||
JCContinue continueSwitch = make.Continue(null); | ||
continueSwitch.target = tree; | ||
c.stats = c.stats.prepend(make.If(makeUnary(Tag.NOT, test).setType(syms.booleanType), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is code which "resumes" to the following case
if the pattern doesn't match. I guess in most cases the bootstrap method would do the check anyway - but I suppose that with guards, the bootstrap method, alone, cannot guarantee the match. Also, it seems like this requires backend support for continue
in switch. Again, all this needs to be better documented, it's pretty hard to infer all this by looking at the code.
* used with {@code invokedynamic}, this is provided by | ||
* the {@code NameAndType} of the {@code InvokeDynamic} | ||
* structure and is stacked automatically by the VM. | ||
* @param labels non-null case labels - {@code String} and {@code Integer} constants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these types be mixed? E.g. can I pass, as static args: 42
, Runnable.class
, "hello"
? If not, should be document, and throw?
* structure and is stacked automatically by the VM. | ||
* @param labels non-null case labels - {@code String} and {@code Integer} constants | ||
* and {@code Class} instances | ||
* @return the index into {@code labels} of the target value, if the target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't return an index. Returns a CallSite which, when invoked with an argument of type E (where E is the type of the target expression), returns the index into...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also mention that the handle returned accepts a start index (which is used by the continue logic)
@@ -494,9 +497,28 @@ compiler.err.same.binary.name=\ | |||
compiler.err.duplicate.case.label=\ | |||
duplicate case label | |||
|
|||
compiler.err.pattern.dominated=\ | |||
this pattern is dominated by a preceding pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether the concept of "dominance" is the one that will work best here. As I said elsewhere, this is, morally, unreachable code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification uses "dominance", so it seemed appropriate to use the same term that is used by the specification. We can say "unreachable code", of course, but it will not be consistent with what the specification says, and also with what we do for duplicate constant labels. Also considering code like:
switch (o) {
case A a -> {}
case B b -> {} //error on this line
}
It may not be obvious why the code is "unreachable", while saying "dominated" might be more helpful/clear.
illegal fall-through to a pattern | ||
|
||
compiler.err.multiple.patterns=\ | ||
multiple pattern declarations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text is very terse and doesn't really say what's wrong with the code. I think the point here is that we don't want code like this:
case String s, Integer i: ...
because this is morally:
case String s:
case Integer i: ...
which is, again, fall-through to a pattern. Maybe, just reusing the same "fall-through" message would be ok here?
@mcimadamore, thanks a lot for the comments! I tried to reflect most of them in 1a5a424 - please let me know how that looks. Thanks! |
Webrevs
|
make/CompileInterimLangtools.gmk
Outdated
@@ -49,7 +49,7 @@ TARGETS += $(patsubst %, $(BUILDTOOLS_OUTPUTDIR)/gensrc/%/module-info.java, \ | |||
$(INTERIM_LANGTOOLS_MODULES)) | |||
|
|||
$(eval $(call SetupCopyFiles, COPY_PREVIEW_FEATURES, \ | |||
FILES := $(TOPDIR)/src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java, \ | |||
FILES := $(TOPDIR)/src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java $(TOPDIR)/src/java.base/share/classes/jdk/internal/javac/NoPreview.java, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please break this line (adding 4 additional space indent from the original line). Otherwise build changes ok.
} | ||
} | ||
|
||
private static<T extends CallSite> MethodHandle typeInitHook(T receiver) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no point to have a type parameter here,
private static MethodHandle typeInitHook(CallSite receiver) {
will work the same
} | ||
|
||
private static void verifyLabel(Object label) { | ||
if (Objects.isNull(label)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (label == true) {
is more readable as said in the javadoc of Objects.isNull
if (Objects.isNull(label)) { | ||
throw new IllegalArgumentException("null label found"); | ||
} | ||
if (label.getClass() != Class.class && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
store label.getClass
in a local variable,
it's too bad that you can no use pattern matching here :)
} | ||
} | ||
|
||
static class TypeSwitchCallSite extends ConstantCallSite { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, having a callsite extending MutableCallSite is expected but having a callsite extending constant callsite is useless because you can not change it after being constructed.
As an interesting trivia, the VM does not store the CallSite returned by the BSM, but only the target inside it.
So there is no point of keeping a ConstantCallSite around.
So doSwitch()
should be static and takes the array of Object[] as parameter, will array will be injected with an insertArguments().
labels = labels.clone(); | ||
Stream.of(labels).forEach(SwitchBootstraps::verifyLabel); | ||
|
||
return new TypeSwitchCallSite(invocationType, labels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See why below
MethodHandle target = MethodHandles.insertArguments(DO_SWITCH, 2, labels);
return new ConstantCallsite(target);
// Dumbest possible strategy | ||
Class<?> targetClass = target.getClass(); | ||
for (int i = startIndex; i < labels.length; i++) { | ||
if (labels[i] instanceof Class<?>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label[i] should be stored is a local variable and
using instanceof Class<?> c
(like the other instanceof below) will make the code more readable
@mcimadamore, @forax, thanks a lot for comments! I tried to apply the requested changes in recent commits (3fc2502 aeddb85 ). I've also tried to update the implementation for the latest spec changes here: The spec changes are: total patterns are nullable; pattern matching ("new") statement switches must be complete (as switch expressions). Any further feedback is welcome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me if it's the first change and several will follow or not.
The code looks good but the metaprotocol is not the right one IMO,
i would prefer the one we discuss in April
https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-April/002992.html
But this can be tackle in another PR
…changing total patterns to default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approving to keep the bots happy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* If the {@code target} is {@code null}, then the method of the call site | ||
* returns {@literal -1}. | ||
* <p> | ||
* the {@code target} is not {@code null}, then the method of the call site |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* the {@code target} is not {@code null}, then the method of the call site | |
* If the {@code target} is not {@code null}, then the method of the call site |
* <li>the element is of type {@code Class} and the target value | ||
* is a subtype of this {@code Class}; or</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* <li>the element is of type {@code Class} and the target value | |
* is a subtype of this {@code Class}; or</li> | |
* <li>the element is of type {@code Class} that is assignable | |
* from the target's class; or</li> |
if (c.isAssignableFrom(targetClass)) | ||
return i; | ||
} else { | ||
if (label instanceof Integer constant) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion: use else if
rather than nest
if (target instanceof Number input && constant.intValue() == input.intValue()) { | ||
return i; | ||
} | ||
if (target instanceof Character input && constant.intValue() == input.charValue()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use else if
/**A marker interface for {@code Tree}s that may be used as {@link CaseTree} labels. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**A marker interface for {@code Tree}s that may be used as {@link CaseTree} labels. | |
* | |
/** | |
* A marker interface for {@code Tree}s that may be used as {@link CaseTree} labels. | |
* |
/** A case label that marks {@code default} in {@code case null, default}. | ||
* @since 17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** A case label that marks {@code default} in {@code case null, default}. | |
* @since 17 | |
/** | |
* A case label that marks {@code default} in {@code case null, default}. | |
* | |
* @since 17 |
catch (NoSuchMethodException | IllegalAccessException e) { | ||
throw new RuntimeException(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch (NoSuchMethodException | IllegalAccessException e) { | |
throw new RuntimeException(e); | |
} | |
catch (ReflectiveOperationException e) { | |
throw new AssertionError(e, "Should not happen"); | |
} |
C; | ||
} | ||
|
||
public void testTypes() throws Throwable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow up issue consider adding tests for null
values
MethodTree method = (MethodTree) clazz.getMembers().get(0); | ||
SwitchTree st = (SwitchTree) method.getBody().getStatements().get(0); | ||
CaseLabelTree label = st.getCases().get(0).getLabels().get(0); | ||
ExpressionType actualType = switch (label) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the test be careful of using a pattern match switch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think using the new feature in the tests is problematic (esp. javac tests related to the feature). It helps to ensure the feature really works on real code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the java.base
change namely, SwitchBootstraps.java. Looks good.
* take additional static arguments corresponding to the {@code case} labels | ||
* of the {@code switch}, implicitly numbered sequentially from {@code [0..N)}. | ||
* | ||
* <p>The bootstrap call site accepts a single parameter of the type of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It takes 2 parameters (not single parameter). Perhaps you can take out this paragraph since it's specified in the typeSwitch
method.
* and {@code Class} instances, in any combination | ||
* @return a {@code CallSite} returning the first matching element as described above | ||
* | ||
* @throws NullPointerException if any argument is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @throws NullPointerException if any argument is null | |
* @throws NullPointerException if any argument is {@code null} |
same formatting nit for other occurrenace of "null"
final static class A implements SealedTypeChangesIntf {} | ||
} | ||
|
||
sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for completeness shouldn't we have a test with sealed, non-abstract classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that for sealed non-abstract classes, the permits is not checked (as an instance of the non-abstract class may be created and passed to the switch, the switch needs to contain a case that will cover the class anyway). I've added tests that check the behavior for abstract class, and non-abstract classes (error is produced in the latter case).
|
||
void statement(SealedTypeChangesIntf obj) { | ||
switch (obj) { | ||
case A a -> System.err.println(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about having tests with a case that matches the sealed class?
Thanks a lot for all the feedback. I've tried to do the requested changes in the recent commits. |
@lahodaj I also noticed that https://bugs.openjdk.java.net/browse/JDK-8213076 as well as https://openjdk.java.net/jeps/406 state
and wondering if this should be changed on
or maybe even removed? |
Dynamic constants are needed when de-structuring classes that are not record at top-level, to make the type that will carry the bindings, from invokedynamic to where they are accessed, opaque. So dynamic constants are not needed yet ! |
/contributor briangoetz |
@lahodaj Syntax:
|
@lahodaj Syntax:
|
@lahodaj Syntax:
|
/contributor add briangoetz |
@lahodaj |
@lahodaj |
@lahodaj |
/integrate |
@lahodaj Since your change was applied there have been 53 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 908aca2. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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 fromCaseTree.getLabels()
.-to support
case default
, there is an implementation ofCaseLabelTree
that represents it (DefaultCaseLabelTree
). It is used also to represent the conventionaldefault
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 representcase 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 inAttr
,Flow
andTransPatterns
.The specdiff for the change is here (to be updated):
http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
Progress
Issue
Reviewers
Contributors
<briangoetz@openjdk.org>
<mchung@openjdk.org>
<jlahoda@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863
$ git checkout pull/3863
Update a local copy of the PR:
$ git checkout pull/3863
$ git pull https://git.openjdk.java.net/jdk pull/3863/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3863
View PR using the GUI difftool:
$ git pr show -t 3863
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3863.diff