Skip to content
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

8250625: Compiler implementation of Pattern Matching for instanceof (Final) #559

Closed
wants to merge 20 commits into from

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Oct 8, 2020

This is the current proposed patch for the upcoming JEP 394, for pattern matching for instanceof.

A summary of changes:
-making the feature permanent (non-preview)
-making the binding variables non-final (as per current specification proposal)
-producing a compile-time error for the case where the expression's type is a subtype of the type test pattern's type (as per current specification proposal)
-changing the AST structure so that the binding variable has a VariableTree in the AST. BindingPatternTree is preserved and encloses the VariableTree. The reason is better consistency in the API, with nodes like CatchTree, EnhancedForLoop Tree, etc.

This change will not be integrated until JEP 394 is targetted.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) (2/9 failed) (1/9 failed)

Failed test tasks

Issue

  • JDK-8250625: Compiler implementation of Pattern Matching for instanceof (Final)

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/559/head:pull/559
$ git checkout pull/559

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 8, 2020

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Oct 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 8, 2020

@lahodaj The following labels will be automatically applied to this pull request:

  • build
  • compiler
  • core-libs

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.

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Oct 8, 2020

/csr

@openjdk openjdk bot added the csr label Oct 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 8, 2020

@lahodaj this pull request will not be integrated until the CSR request JDK-8254235 for issue JDK-8250625 has been approved.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 8, 2020

Webrevs

@erikj79
Copy link
Member

@erikj79 erikj79 commented Oct 8, 2020

/label remove build

@openjdk openjdk bot removed the build label Oct 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 8, 2020

@erikj79
The build label was successfully removed.

@@ -25,8 +25,8 @@
* @test
* @bug 8231827
* @summary Basic tests for bindings from instanceof
* @compile --enable-preview -source ${jdk.version} BindingsTest1.java
* @run main/othervm --enable-preview BindingsTest1
* @compile BindingsTest1.java

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 20, 2020
Contributor

the @compile can be removed

This comment has been minimized.

@lahodaj

lahodaj Oct 20, 2020
Author Contributor

I guess I prefer to keep @compile for javac non-programatic tests. The reason is that, for these tests, we can expect we modify javac, but not the test. And if @build (implicit or explicit) is used, the test will not be re-compiled unless their timestamp changes. So we can actually execute an older (already built) version of the test's class file, instead of a newly built class file, which would reflect the changes in javac. So, for tests like this, I prefer to have an explicit @compile, as that ensures the file is compiled every time, regardless how the working directories are set-up, etc.

* @compile --enable-preview -source ${jdk.version} BindingsTest1.java
* @run main/othervm --enable-preview BindingsTest1
* @compile BindingsTest1.java
* @run main BindingsTest1

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 20, 2020
Contributor

and this line too

* @compile ExamplesFromProposal.java
* @run main ExamplesFromProposal
Comment on lines +28 to +29

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 20, 2020
Contributor

these two lines can be removed now, there are other tests in this commit in which they can also be eliminated

* @modules jdk.jdeps/com.sun.tools.classfile
* @compile -g --enable-preview -source ${jdk.version} LocalVariableTable.java
* @run main/othervm --enable-preview LocalVariableTable
* @compile -g LocalVariableTable.java
Comment on lines 28 to +29

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 20, 2020
Contributor

I believe all tests are always compiled with -g option set, not related to your patch but we could probably remove that line as superfluous

This comment has been minimized.

@lahodaj

lahodaj Oct 20, 2020
Author Contributor

I believe plain jtreg invocations may not be always setting "-g". So probably better to be explicitly clear we need -g, as the test itself requires the debugging info/LocalVariableTable?

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 21, 2020
Contributor

sure I understand that the -g option is necessary. I was just wondering if -g is not passed always by jtreg. If the option is passed by default then it is superfluous, although just a comment, I understand that it is safer to pass it

This comment has been minimized.

@lahodaj

lahodaj Oct 21, 2020
Author Contributor

I tried with a plain jtreg invocation, and it didn't pass -g (and the test failed). But it depends on how jtreg is run, of course - some runs may have -g set.

@@ -105,7 +105,7 @@ private CompilationUnitTree parse(String code) throws IOException {

StringWriter out = new StringWriter();
JavacTask ct = (JavacTask) tool.getTask(out, null, noErrors,
List.of("--enable-preview", "-source", Integer.toString(Runtime.version().feature())), null,
List.of(), null,

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 20, 2020
Contributor

nit: what about passing null instead of List.of()?

*/
Tree getType();
@Deprecated

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 21, 2020
Contributor

shouldn't we use @Deprecated(since=versionNumber)

* A tree node used as the base class for the different kinds of
* statements.
*
* @since 14
* @since 16
*/
public interface PatternTree extends Tree {}

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 21, 2020
Contributor

I think that this interface is there for forward compatibility, and I don't like empty interfaces but I found that we already did that with com.sun.source.tree.DirectiveTree

This comment has been minimized.

@lahodaj

lahodaj Oct 21, 2020
Author Contributor

We have multiple such interfaces - like StatementTree and ExpressionTree. It is a forward-looking interface, but I think necessary: InstanceofTree.getPattern() needs to return something, and having a PatternTree is (I think) better than returning generic Tree, or BindingPatternTree (the latter is likely to be insufficient sometime soon).

* feature of the Java language. Preview features
* may be removed in a future release, or upgraded to permanent
* features of the Java language.}
*
* A tree node used as the base class for the different kinds of

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Oct 21, 2020
Contributor

the javadoc seems to need an update, probably:
A tree node used as the base class for the different kinds of pattern matching expressions?

This comment has been minimized.

@lahodaj

lahodaj Oct 22, 2020
Author Contributor

Patterns are not really expressions - they are a new kind of trees. Basically, before we had "statements" and "expressions" (+declarations, which are a bit special, ClassTree and VariableTree are "statements", but MethodTree, ModuleTree and PackageTree subtype Tree directly, so there is no common supertype). Now we have "patterns" in addition to that.

For example, having: o instanceof String s, this is an expression, but String s is not - it is a pattern. It cannot stand on its own as an expression, it is a special part of the expression.

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Oct 22, 2020

Based on an offline feedback, I've removed the old, preview, deprecated methods from BindingPatternTree.

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

looks good!

@openjdk openjdk bot removed the csr label Nov 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@lahodaj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8250625: Compiler implementation of Pattern Matching for instanceof (Final)

Reviewed-by: vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 15 new commits pushed to the master branch:

  • 60e4aca: 8255890: Zero: remove unused methods from BytecodeInterpreter
  • 397bae2: 8255860: Clean up CDS logging related to lambda
  • 97a81ce: 8253385: annotation processors remove varargs information from record components
  • 166c728: 8255858: Add debug agent support for storing thread names
  • a0ade22: 8255900: x86: Reduce impact when VerifyOops is disabled
  • 29db1dc: 8255886: Shenandoah: Resolve cset address truncation and register clash in interpreter LRB
  • 26e7ef7: 8252870: Finalize (remove "incubator" from) jpackage
  • 804bd72: 8255128: linux x86 build failure with libJNIPoint.c
  • 160759c: 8255838: Use 32-bit immediate movslq in macro assembler if 64-bit value fits in 32 bits on x86_64
  • 94ace03: 8255863: Clean up test/jdk/java/lang/invoke/defineHiddenClass/BasicTest.java
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/7f4d873dd9492c128e98737eb4651a29387269e8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 3, 2020
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Nov 5, 2020

/integrate

@openjdk openjdk bot closed this Nov 5, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 5, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 5, 2020

@lahodaj Since your change was applied there have been 15 commits pushed to the master branch:

  • 60e4aca: 8255890: Zero: remove unused methods from BytecodeInterpreter
  • 397bae2: 8255860: Clean up CDS logging related to lambda
  • 97a81ce: 8253385: annotation processors remove varargs information from record components
  • 166c728: 8255858: Add debug agent support for storing thread names
  • a0ade22: 8255900: x86: Reduce impact when VerifyOops is disabled
  • 29db1dc: 8255886: Shenandoah: Resolve cset address truncation and register clash in interpreter LRB
  • 26e7ef7: 8252870: Finalize (remove "incubator" from) jpackage
  • 804bd72: 8255128: linux x86 build failure with libJNIPoint.c
  • 160759c: 8255838: Use 32-bit immediate movslq in macro assembler if 64-bit value fits in 32 bits on x86_64
  • 94ace03: 8255863: Clean up test/jdk/java/lang/invoke/defineHiddenClass/BasicTest.java
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/7f4d873dd9492c128e98737eb4651a29387269e8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 18bc95b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants