8324651: Compiler Implementation for Derived Record Creation (Preview) #18509

lahodaj · 2024-03-27T10:24:51Z

This is a patch for javac, that adds the Derived Record Creation expressions. The current draft specification for the feature is:
https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html

The current CSR is here:
https://bugs.openjdk.org/browse/JDK-8328637

The patch is mostly straightforward, with two notable changes:

there is a new ElementKind.COMPONENT_LOCAL_VARIABLE, as the specification introduces this term, and it seems consistent with ElementKind.BINDING_VARIABLE that was introduced some time ago.
there are a bit broader changes in Flow, to facilitate the introduction of variables without an explicit declaration for definite assignment and effectively final computation.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8329645 to be approved

Issues

JDK-8324651: Compiler Implementation for Derived Record Creation (Preview) (Sub-task - P4)
JDK-8329556: javax.lang.model support for Derived Record Creation (Preview) (Sub-task - P4)(⚠️ The fixVersion in this issue is [24] but the fixVersion in .jcheck/conf is 23, a new backport will be created when this pr is integrated.)
JDK-8329645: javax.lang.model support for Derived Record Creation (Preview) (CSR)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to a05480cd
Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to a05480cd

Contributors

Joe Darcy <darcy@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509
$ git checkout pull/18509

Update a local copy of the PR:
$ git checkout pull/18509
$ git pull https://git.openjdk.org/jdk.git pull/18509/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18509

View PR using the GUI difftool:
$ git pr show -t 18509

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18509.diff

Webrev

Link to Webrev Comment

…of a record type.

…sion; optimizing the resulting bytecode.

bridgekeeper · 2024-03-27T10:25:21Z

👋 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 · 2024-03-27T10:26:21Z

@lahodaj This change is no longer ready for integration - check the PR body for details.

openjdk · 2024-03-27T10:26:47Z

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

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.

mlbridge · 2024-03-27T15:47:34Z

Webrevs

biboudis · 2024-03-28T11:00:42Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

+         *  to the next available sequence number and entering it under that
+         *  index into the vars array.
+         */
+        void newVar(JCTree pos,VarSymbol sym) {


This is used only twice. One from void newVar(JCVariableDecl varDecl) which is very intuitive and one with newVar(null, component); which I understand. But, is there any reason to create a var in the future with something else than null (unrelated to sym?). Maybe the comment needs to be updated to document what should be the relation (if any) between pos and sym.

Oops, that's actually an oversight - there should be a pos specified in all cases. Should be fixed now.

biboudis · 2024-03-28T11:02:16Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

@@ -3233,6 +3256,11 @@ public void analyzeTree(Env<?> env, JCTree tree, TreeMaker make) {
            try {
                startPos = tree.pos().getStartPosition();

+                if (vars == null)


Curly braces here?

This is a copy of the similar code for vardecls. I was thinking if I could unify the codepaths, but does not seem to be so simple, so I created a duplicate. So, it might be better to keep the code the same/similar as it was before, to minimize disruptions.

biboudis · 2024-03-28T11:27:33Z

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java

@@ -3570,6 +3604,7 @@ public abstract static class Visitor {
        public void visitNewArray(JCNewArray that)           { visitTree(that); }
        public void visitLambda(JCLambda that)               { visitTree(that); }
        public void visitParens(JCParens that)               { visitTree(that); }
+        public void visitReconstruction(JCDerivedInstance that) { visitTree(that); }


Maybe visitDerivedInstance to be in sync with the JCDerivedInstance world?

Done. Thanks!

openjdk · 2024-04-05T12:31:02Z

@lahodaj
Contributor Joe Darcy <darcy@openjdk.org> successfully added.

lahodaj · 2024-04-05T12:45:39Z

/issue add JDK-8329556

openjdk · 2024-04-05T12:46:26Z

@lahodaj
Adding additional issue to issue list: 8329556: javax.lang.model support for Derived Record Creation (Preview).

mcimadamore · 2024-04-05T13:02:49Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java

@@ -4908,6 +4909,92 @@ private boolean patternDominated(JCPattern existingPattern, JCPattern currentPat
            return false;
        }

+    void checkDerivedInstanceBlockStructure(JCDerivedInstance instance) {
+        new TreeScanner() {


I note that here we don't care about leaving locals declared inside a block in the seenVariables after the block ends (e.g. I see no visitBlock). I think that's probably ok, given that every variable has a fresh symbol, so checking against the seenVariables set will fail anyway for two variables that have same name, but different scopes.

mcimadamore · 2024-04-05T13:09:56Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

         */
-        protected JCVariableDecl[] vardecls;
+        protected VarAndDeclarationTree[] vars;


So... normally, a JCVariableDecl would have a symbol attached, which is how the old code used to work. Is it correct that this change is needed because the componentLocalVariables list is a list of symbols and not of JCVariableDecl? Would it be too complex to create a declaration in Attr and save those in the derived creation tree?

Generating the JCVariableDecls in Attr in 1465135.
Thanks for the comment!

mcimadamore · 2024-04-05T13:10:34Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

@@ -4440,6 +4440,65 @@ public void visitTry(JCTry tree) {
        super.visitTry(tree);
    }

+    @Override


As usual, I suggest to add some brief comment with the shape of the generated code.

Done here:
1465135#diff-bc0df6dce7f74078bfca1e90bec75d7bb3d8b338933be725da78f0a8a7a0c78dR4445

Please let me know if that needs some tweaks.

Thanks for the comment!

Looks great!

mcimadamore

Good job!

- pre-generating the JCVarDecls in Attr, to aid Flow - adding a note on how the desugared code looks like

jddarcy · 2024-04-05T18:30:30Z

src/java.compiler/share/classes/javax/lang/model/element/ElementKind.java

+     * @since 23
+     */
+    @PreviewFeature(feature=PreviewFeature.Feature.DERIVED_RECORD_CREATION, reflective=true)
+    COMPONENT_LOCAL_VARIABLE;

    // Maintenance note: check if the default implementation of


From a quick look, I don't think Elements.getOutermostTypeElement needs updating for COMPONENT_LOCAL_VARIABLE, but it would be good to add a test case.

Tests added: e093068

Thanks!

jddarcy · 2024-04-05T18:32:09Z

src/java.compiler/share/classes/javax/lang/model/util/ElementKindVisitorPreview.java

@@ -86,4 +86,21 @@ protected ElementKindVisitorPreview() {
    protected ElementKindVisitorPreview(R defaultValue) {
        super(defaultValue);
    }
+
+    /**
+     * {@inheritDoc ElementKindVisitor6}


If possible, would be good to add some minimal testing/use of the element kind visitors on component local variables.

Tests added: e093068

Thanks!

vicente-romero-oracle · 2024-04-08T16:36:49Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java

@@ -98,10 +98,12 @@
 import com.sun.tools.javac.tree.JCTree.JCNewClass;
 import com.sun.tools.javac.tree.JCTree.JCPattern;
 import com.sun.tools.javac.tree.JCTree.JCPatternCaseLabel;
+import com.sun.tools.javac.tree.JCTree.JCDerivedInstance;


changes to this file can be removed now

Thanks, removed:
a05480c

vicente-romero-oracle · 2024-04-08T16:42:36Z

test/langtools/tools/javac/patterns/withers/Model.java

@@ -0,0 +1,115 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.


probably this test should be in another location not under patterns, same for other tests added inside patterns folder

Thanks, moved:
a05480c

- reverting unnecessary changes in TransPatterns - moving the patters/withers/Model test to a more appropriate place

vicente-romero-oracle

lgtm

mcimadamore

Looks good!

openjdk · 2024-04-18T05:35:48Z

@lahodaj this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout wthexp
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

bridgekeeper · 2024-06-07T01:59:23Z

@lahodaj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2024-07-05T03:23:15Z

@lahodaj This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

lahodaj added 20 commits September 1, 2023 11:02

A very crude and optimistic experiment on wthrs

588030c

Merge branch 'master' into wthexp

5074587

Improvements.

4212f20

Fixing handling of with variables inside a guard.

3e04899

Fixing a few bugs found by biboudis

a375020

Produce an error when the expression for the derived instance is not …

01040f3

…of a record type.

Adding source level check for withers.

e4d0f3b

Merge branch 'master' into wthexp

86e9268

Pre/post-increment/decrement are also assignments.

309e629

Cleanup.

a8d825d

Adding test.

e4d8aba

Cleanup.

6bf29d1

Adding missing file.

c6806fe

Merge branch 'master' into wthexp

cff0d11

Javadoc cleanup.

ac0b968

More correct handling of exceptions in derived record creation expres…

cf80d76

…sion; optimizing the resulting bytecode.

Adding examples.

96b4214

Adding tests.

871e112

Cleanup, bugfixes.

327ab53

Cleanup.

969938a

openjdk bot added the csr label Mar 27, 2024

openjdk bot added core-libs compiler labels Mar 27, 2024

Removing whitespace

a61682f

openjdk bot added the rfr label Mar 27, 2024

biboudis reviewed Mar 28, 2024

View reviewed changes

JavaCompiler cleanup

c91e87f

mcimadamore reviewed Apr 5, 2024

View reviewed changes

Reflecting review feedback:

1465135

- pre-generating the JCVarDecls in Attr, to aid Flow - adding a note on how the desugared code looks like

jddarcy reviewed Apr 5, 2024

View reviewed changes

Adding tests as suggested.

e093068

vicente-romero-oracle reviewed Apr 8, 2024

View reviewed changes

Reflecting review feedback:

a05480c

- reverting unnecessary changes in TransPatterns - moving the patters/withers/Model test to a more appropriate place

vicente-romero-oracle approved these changes Apr 15, 2024

View reviewed changes

mcimadamore approved these changes Apr 17, 2024

View reviewed changes

openjdk bot added the merge-conflict label Apr 18, 2024

lahodaj added 2 commits April 19, 2024 15:16

Merge branch 'master' into wthexp

16cd427

Post-merge fixes.

e8499df

openjdk bot removed the merge-conflict label Apr 19, 2024

Merge branch 'master' into wthexp

c97eb8e

openjdk bot added ready and removed csr labels May 2, 2024

openjdk bot added merge-conflict and removed ready labels May 9, 2024

openjdk bot removed the rfr label Jun 4, 2024

bridgekeeper bot closed this Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8324651: Compiler Implementation for Derived Record Creation (Preview) #18509

8324651: Compiler Implementation for Derived Record Creation (Preview) #18509

lahodaj commented Mar 27, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 27, 2024

openjdk bot commented Mar 27, 2024 •

edited

Loading

openjdk bot commented Mar 27, 2024

mlbridge bot commented Mar 27, 2024 •

edited

Loading

biboudis Mar 28, 2024

lahodaj Mar 28, 2024

biboudis Mar 28, 2024

lahodaj Mar 28, 2024

biboudis Mar 28, 2024

lahodaj Mar 28, 2024

openjdk bot commented Apr 5, 2024

lahodaj commented Apr 5, 2024

openjdk bot commented Apr 5, 2024

mcimadamore Apr 5, 2024

mcimadamore Apr 5, 2024

lahodaj Apr 5, 2024

mcimadamore Apr 5, 2024

lahodaj Apr 5, 2024

mcimadamore Apr 8, 2024

mcimadamore left a comment

jddarcy Apr 5, 2024

lahodaj Apr 9, 2024

jddarcy Apr 5, 2024

lahodaj Apr 9, 2024

vicente-romero-oracle Apr 8, 2024

lahodaj Apr 9, 2024

vicente-romero-oracle Apr 8, 2024

lahodaj Apr 9, 2024

vicente-romero-oracle left a comment

mcimadamore left a comment

openjdk bot commented Apr 18, 2024

bridgekeeper bot commented Jun 7, 2024

bridgekeeper bot commented Jul 5, 2024

		@@ -0,0 +1,115 @@
		/*
		* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

8324651: Compiler Implementation for Derived Record Creation (Preview) #18509

8324651: Compiler Implementation for Derived Record Creation (Preview) #18509

Conversation

lahodaj commented Mar 27, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Mar 27, 2024

openjdk bot commented Mar 27, 2024 • edited Loading

openjdk bot commented Mar 27, 2024

mlbridge bot commented Mar 27, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Apr 5, 2024

lahodaj commented Apr 5, 2024

openjdk bot commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

mcimadamore left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 18, 2024

bridgekeeper bot commented Jun 7, 2024

bridgekeeper bot commented Jul 5, 2024

lahodaj commented Mar 27, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 27, 2024 •

edited

Loading

mlbridge bot commented Mar 27, 2024 •

edited

Loading