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

8254105: allow static nested declarations #571

Closed

Conversation

vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Oct 9, 2020

Please review the fix for JDK-8254105. The intention of the fix is to allow static members to be declared inside inner classes. The spec allowing this change can be seen at Local and Nested Static Declarations. This change is part of the Records JEP. The idea is to allow not only records to be defined inside inner classes but also interfaces, enums, static classes and methods.

TIA,
Vicente

PS: the records spec can be accessed here Record Classes. This patch also adds a test to check the changes in the Records spec to Chapter 13 "Binary Compatibility". I'm OK moving that test to a separate PR.


Progress

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

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (6/6 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 9, 2020

👋 Welcome back vromero! 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 Pull request is ready for review label Oct 9, 2020
@@ -2,8 +2,8 @@
* @test /nodynamiccopyright/
* @bug 8242478
* @summary test for local interfaces
* @compile/fail/ref=LocalInterface.out -XDrawDiagnostics LocalInterface.java
* @compile --enable-preview -source ${jdk.version} LocalInterface.java
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Oct 9, 2020

Choose a reason for hiding this comment

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

the current patch is supposed to be applied after the code being reviewed at PR-290 which also modifies this particular line, once PR-290 has been pushed this line and others removing the --enable-preview option from tests will change

@openjdk
Copy link

openjdk bot commented Oct 9, 2020

@vicente-romero-oracle 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.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Oct 9, 2020
@@ -0,0 +1,307 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Oct 9, 2020

Choose a reason for hiding this comment

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

this code is not strictly related to the patch but this test is just basically testing the changes introduced by Chapter 13 in the record spec. I'm OK moving this to another PR if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I bet this is related to the extra disjoint test in Check.java - if so, yes, better moved in a different issue, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really but it is true that it is not strictly related to the patch, will create another issue for it

@mlbridge
Copy link

mlbridge bot commented Oct 9, 2020

Webrevs

@vicente-romero-oracle vicente-romero-oracle changed the title 8254105: allow static members in inner classes, add binary compatibil… 8254105: allow static members in inner classes, add binary compatibility test Oct 9, 2020
@vicente-romero-oracle vicente-romero-oracle changed the title 8254105: allow static members in inner classes, add binary compatibility test 8254105: allow static members in inner classes, add binary compatibility tests Oct 9, 2020
@vicente-romero-oracle
Copy link
Contributor Author

/label remove core-libs

@openjdk openjdk bot removed the core-libs core-libs-dev@openjdk.org label Oct 9, 2020
@openjdk
Copy link

openjdk bot commented Oct 9, 2020

@vicente-romero-oracle
The core-libs label was successfully removed.

@mlbridge
Copy link

mlbridge bot commented Oct 9, 2020

Mailing list message from David Holmes on compiler-dev:

Hi Vincente,

On 9/10/2020 10:10 am, Vicente Romero wrote:

Please review the fix for [JDK-8254105](https://bugs.openjdk.java.net/browse/JDK-8254105). The intention of the fix is
to allow static members to be declared inside inner classes. The spec allowing this change can be seen at [Local and
Nested Static
Declarations](http://cr.openjdk.java.net/~gbierman/8246771/8246771-20200928/specs/local-statics-jls.html). This change
is part of the [Records JEP](https://openjdk.java.net/jeps/395). The idea is to allow not only records to be defined
inside inner classes but also interfaces, enums, static classes and methods.

I'm rather confused. Is this intended to be the issue under which the
implementation of JEP 395 is done? JEP 395 is only a Candidate at the
moment so nothing in relation to that should be getting integrated yet.
It is also unclear how the JDK-8254105 issue relates to the JDK-8246774
issues, which appears to be intended to be the implementation issue for
JEP 395.

???

Thanks,
David

@vicente-romero-oracle
Copy link
Contributor Author

vicente-romero-oracle commented Oct 9, 2020

Hi David,

This issue is a subtask of JEP 395 along with JDK-8246774. JDK-8246774 is basically removing all the preview annotations from record's code and preview options from the related tests so that records can be used as a standard feature in Java. But issues like this one, and probably another I'm still working on related to annotations on record components, was, IMO, complex enough to deserve a separate review process. I'm OK with creating an umbrella issue, if that sounds better to you, that contains all implementation tasks and make JDK-8246774 and this current task subtasks of that umbrella issue.

Thanks,
Vicente

@vicente-romero-oracle
Copy link
Contributor Author

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Oct 9, 2020
@openjdk
Copy link

openjdk bot commented Oct 9, 2020

@vicente-romero-oracle this pull request will not be integrated until the CSR request JDK-8254321 for issue JDK-8254105 has been approved.

@vicente-romero-oracle vicente-romero-oracle changed the title 8254105: allow static members in inner classes, add binary compatibility tests 8254105: allow static members in inner classes Oct 13, 2020
@vicente-romero-oracle vicente-romero-oracle changed the title 8254105: allow static members in inner classes 8254105: allow static nested declarations Oct 15, 2020
Copy link
Contributor

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

The changes look good, but I have one concern: this patch seem to apply the improvements unconditionally, regardless of the source version - is this what we want? While I understand that the risk is low (this patch makes space of compilable programs bigger), at the same times it feels wrong that when compiling with --source 14 I can now nest static classes inside member classes (which is not compatible with what the SE 14 spec says).

if (sym.owner.owner.kind == PCK ||
(sym.owner.flags_field & STATIC) != 0)
mask |= STATIC;
else if ((flags & ENUM) != 0 || (flags & RECORD) != 0) {
log.error(pos, Errors.StaticDeclarationNotAllowedInInnerClasses);
//log.error(pos, Errors.StaticDeclarationNotAllowedInInnerClasses);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you get rid of the else if here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, sure

@@ -1307,7 +1302,10 @@ else if ((sym.kind == TYP ||
SEALED | NON_SEALED)
&& checkDisjoint(pos, flags,
SEALED,
FINAL | NON_SEALED)) {
FINAL | NON_SEALED)
&& checkDisjoint(pos, flags,
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this coming from? This seems outside the scope of the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I will remove it

@@ -0,0 +1,307 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet this is related to the extra disjoint test in Check.java - if so, yes, better moved in a different issue, thanks.

@lahodaj
Copy link
Contributor

lahodaj commented Oct 29, 2020

As @mcimadamore, I think this should only be allowed for -source 16+, right?

@vicente-romero-oracle
Copy link
Contributor Author

As @mcimadamore, I think this should only be allowed for -source 16+, right?

I agree, I'm working on another iteration to fix the issue, thanks for the comments

@vicente-romero-oracle
Copy link
Contributor Author

I have pushed a commit addressing the review comments so far. I have re-added code in Attr but this time behind a guard checking if records are allowed in the current source and I have added back some tests I removed before. The tests are now executed with current source and with source 15. Thanks for the comments so far.

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

Changes look good

@vicente-romero-oracle
Copy link
Contributor Author

Changes look good

thanks!

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 19, 2020
@openjdk
Copy link

openjdk bot commented Nov 19, 2020

@vicente-romero-oracle 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:

8254105: allow static nested declarations

Reviewed-by: mcimadamore

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

  • 57025e6: 8256073: Improve vector rematerialization support
  • 41c0587: 8255367: C2: Deoptimization during vector box construction is broken
  • f79e9d4: 8256061: RegisterSaver::save_live_registers() omits upper halves of ZMM0-15 registers
  • 503590f: 8256056: Deoptimization stub doesn't save vector registers on x86
  • e6fa85b: 8256058: Improve vector register handling in RegisterMap::pd_location() on x86
  • be6c893: 8256183: InputStream.skipNBytes is missing @SInCE 12
  • b21b96d: 8256507: Add a micro benchmark for JDK-8153005

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

➡️ 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 Pull request is ready to be integrated label Nov 19, 2020
@vicente-romero-oracle
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Nov 21, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 21, 2020
@openjdk
Copy link

openjdk bot commented Nov 21, 2020

@vicente-romero-oracle Since your change was applied there have been 9 commits pushed to the master branch:

  • 14de791: 8255934: JConsole 14 and greater fails to connect to older JVM
  • 86f3602: 8256806: Shenandoah: optimize shenandoah/jni/TestPinnedGarbage.java test
  • 57025e6: 8256073: Improve vector rematerialization support
  • 41c0587: 8255367: C2: Deoptimization during vector box construction is broken
  • f79e9d4: 8256061: RegisterSaver::save_live_registers() omits upper halves of ZMM0-15 registers
  • 503590f: 8256056: Deoptimization stub doesn't save vector registers on x86
  • e6fa85b: 8256058: Improve vector register handling in RegisterMap::pd_location() on x86
  • be6c893: 8256183: InputStream.skipNBytes is missing @SInCE 12
  • b21b96d: 8256507: Add a micro benchmark for JDK-8153005

Your commit was automatically rebased without conflicts.

Pushed as commit 9a19eb6.

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

openjdk-notifier bot referenced this pull request Nov 21, 2020
@vicente-romero-oracle vicente-romero-oracle deleted the JDK-8254105 branch June 3, 2021 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants