8263900: [lworld] Make .default a separate node type in the parser.

[jespersm]

To prepare for making .default a poly expression and to implement further logic, .default-expressions should be split off as a separate AST node.

---

Progress

• Change must not contain extraneous whitespace

Issue

• JDK-8263900: [lworld] Make .default a separate node type in the parser.

Reviewers

• Srikanth Adayapalam (@sadayapalam - Committer)

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/valhalla pull/370/head:pull/370
$ git checkout pull/370

To update a local copy of the PR:
$ git checkout pull/370
$ git pull https://git.openjdk.java.net/valhalla pull/370/head

[bridgekeeper]

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

[openjdk]

@jespersm 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:

8263900: [lworld] Make .default a separate node type in the parser.

Reviewed-by: sadayapalam

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 2 new commits pushed to the lworld branch:

• 94c281e: 8263568: [lworld] Fix residual reference to 'value'
• ff70e7d: 8258038: [AARCH64] [lworld] Fix inline type implementation

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sadayapalam) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge]

Webrevs

• 02: Full - Incremental (0341e68)
• 01: Full - Incremental (adc1372)
• 00: Full (dd883c1)

[sadayapalam]

Thanks Jesper, I will review this one and help you take it forward.

[sadayapalam]

I have made one high level pass over the changes, more verifying on the boiler plate parts. Need to delve deeper in Attr, Gen, Parser, TransValues, will do this tomorrow.

Here are some comments so far:

(1) Is DefaultExpression better named as DefaultInstance or DefaultValue ??
(2) Should com.sun.tools.javac.tree.JCTree.JCDefaultExpression#getClazz - for consistency sake return JCExpression ??
(compare similar cases in JCTree)
(3) Do we need a visitor in SourceComputer in CRTable.java ??

[sadayapalam]

Here are cumulative comments:

(1) Is DefaultExpression better named as DefaultInstance or even better DefaultValue ??
(2) Should com.sun.tools.javac.tree.JCTree.JCDefaultExpression#getClazz - for consistency sake return JCExpression ??
(compare similar cases in JCTree)
(3) Should JCDefaultExpression#getClazz be named getType() a la JCTypeCast, JCNewArray, JCVariableDecl, JCInstanceof ...
(4) Do we need a visitor in SourceComputer in CRTable.java ??
(5) Attr.java:4464: reference to .default needs to be removed from comment
(6) Attr.visitDefaultExpression: sitesym seems to be unused variable declaration.
(7) This program:

primitive class X {
    Object o2 = java.util.default;
}

generates this strange error:
error: package java does not exist
Object o2 = java.util.default;
^
1 error
(That it emits the same message without the patch notwithstanding)

I think we should attribute the clazz as a TYP_PCK and for package case emit an error.
(8) com.sun.source.util.TreeScanner#visitDefaultExpression: Eliminate the local and directly return ?

Let me know if you have any questions/clarifications.

Thanks for your effort, this looks very promising prework for the poly expr typing task

[jespersm]

Regarding (7), I hit that earlier as well - but it is a entirely seperate bug, also available in the released product:

% javac -version
javac 16
% cat X.java 
public class X {
    void x() {
        Class c = java.lang.class;
    }
}
% javac X.java    
X.java:3: error: package java does not exist
        Class c = java.lang.class;
                      ^
1 error

I get the same confusing error with e.g. "new java.lang()"

This bug goes back to at least Java 11, but Java 8 u232 appears to get it right, explaining that there's no java.lang class. I'm guessing it may have come in with Jigsaw.

While I might be able to fix this locally, I think it should really be addressed in the main repo first. I tried finding the bug for it, but didn't find it -- would you like me to report it?

[sadayapalam]

Regarding (7), I hit that earlier as well - but it is a entirely seperate bug, also available in the released product:

[...]

While I might be able to fix this locally, I think it should really be addressed in the main repo first. I tried finding the bug for it, but didn't find it -- would you like me to report it?

I agree this should be addressed in mainline first, yes please, could you raise a defect report for that ? Thanks!

[sadayapalam]

Regarding (7), I hit that earlier as well - but it is a entirely seperate bug, also available in the released product:

[...]

While I might be able to fix this locally, I think it should really be addressed in the main repo first. I tried finding the bug for it, but didn't find it -- would you like me to report it?

I agree this should be addressed in mainline first, yes please, could you raise a defect report for that ? Thanks!

Actually, I think with your work to split Foo.default away from JCFieldAccess, this should be fixed independently in both mainline (for java.lang.class example you report) and in Valhalla for java.lang.default case we are discussing. With the code split, fixing mainline won't automatically fix Valhalla issue with java.lang.default.

However, I am fine with that being handled on a separate ticket by itself without blocking or side tracking this PR/RFR and also the work on treating .default as a poly expression.

[sadayapalam]

Looks good Jesper, Can you issue an /integrate command and I will follow it up with /sponsor

Thanks!

[jespersm]

/integrate

[openjdk]

@jespersm
Your change (at version 0341e68) is now ready to be sponsored by a Committer.

[sadayapalam]

/sponsor

[openjdk]

@sadayapalam @jespersm Since your change was applied there have been 2 commits pushed to the lworld branch:

• 94c281e: 8263568: [lworld] Fix residual reference to 'value'
• ff70e7d: 8258038: [AARCH64] [lworld] Fix inline type implementation

Your commit was automatically rebased without conflicts.

Pushed as commit 91c49e4.

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