8338981: Access to private classes should be permitted inside the permits clause of the enclosing top-level class

[Evemose]

Fix involves adding new flag to after context that indicates that currently resolved symbol is in permits clause

---

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-8341669 to be approved

Issues

• JDK-8338981: Access to private classes should be permitted inside the permits clause of the enclosing top-level class (Bug - P3)
• JDK-8341669: Access to private classes and interfaces should be permitted inside the permits clause of the enclosing class or interface (CSR)

Reviewers

• Vicente Romero (@vicente-romero-oracle - Reviewer)
• Maurizio Cimadamore (@mcimadamore - Reviewer) Review applies to 441adf9f

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20718

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back Evemose! 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]

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

8338981: Access to private classes should be permitted inside the permits clause of the enclosing top-level class

Reviewed-by: vromero, 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 156 new commits pushed to the master branch:

• 3ccd2f7: 8342458: More consistent constant instruction handling
• 29ae265: 8341861: GHA: Use only retention mechanism to remove bundles
• e5bbad0: 8334999: RISC-V: implement AES single block encryption/decryption intrinsics
• c40bb76: 8343296: IGV: Show pre/main/post at CountedLoopNodes
• dee0982: 8343132: Remove temporary transitions from Virtual thread implementation
• 2f1ba5e: 8343307: Throw ZipException instead of IAE in ZipFile.Source::initCEN
• 388d44f: 8342156: C2: Compilation failure with fewer arguments after JDK-8329032
• 688e92e: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
• f340ab2: 8342181: Update tests to use stronger Key and Salt size
• cc2fb4d: 8337276: jcmd man page update for PID in output filenames
• ... and 146 more: https://git.openjdk.org/jdk/compare/309b929147e7dddfa27879ff31b1eaad271def85...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.

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 (@vicente-romero-oracle, @lahodaj, @mcimadamore) 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).

[openjdk]

@Evemose The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 18: Full - Incremental (0889133b)
• 17: Full - Incremental (5fdfdf32)
• 16: Full - Incremental (38dd1b5a)
• 15: Full - Incremental (91689784)
• 14: Full - Incremental (7efb56e9)
• 13: Full - Incremental (efefc762)
• 12: Full - Incremental (441adf9f)
• 11: Full - Incremental (439aa2c4)
• 10: Full - Incremental (18d346e2)
• 09: Full - Incremental (70ba2755)
• 08: Full - Incremental (a699206d)
• 07: Full - Incremental (a81efa3d)
• 06: Full - Incremental (3d6d5749)
• 05: Full - Incremental (b879275e)
• 04: Full - Incremental (f0021f43)
• 03: Full - Incremental (b7a5c47e)
• 02: Full - Incremental (7d05c664)
• 01: Full - Incremental (fd6e673d)
• 00: Full (6df5e3f5)

[lahodaj]

Others may have a different opinion, but this seems a bit too hacky to me (given we would need to maintain this for a long time).

I would suggest to try to add a new flag into AttrContext, along the lines of permitPrivateAccessInHeader, and set it while attributing the permitted types. Not sure how invasive that'd be, but hopefully not terribly much. And it should be clear (and documented) what's happening.

[Evemose]

Others may have a different opinion, but this seems a bit too hacky to me (given we would need to maintain this for a long time).

I would suggest to try to add a new flag into AttrContext, along the lines of permitPrivateAccessInHeader, and set it while attributing the permitted types. Not sure how invasive that'd be, but hopefully not terribly much. And it should be clear (and documented) what's happening.

Honestly, I agree with you, I just didnt know how much invasive changes to existing code could be. I will look into AttrContext later today

[Evemose]

Others may have a different opinion, but this seems a bit too hacky to me (given we would need to maintain this for a long time).

I would suggest to try to add a new flag into AttrContext, along the lines of permitPrivateAccessInHeader, and set it while attributing the permitted types. Not sure how invasive that'd be, but hopefully not terribly much. And it should be clear (and documented) what's happening.

I have decided to write tests before reworking fix. Could you please suggest where could i put it? I have found tools/javac/diags/examples folder, but it seems to only contain negative tests, while I need positive ones. Is there any suitable place for this kind of thing?

[vicente-romero-oracle]

Others may have a different opinion, but this seems a bit too hacky to me (given we would need to maintain this for a long time).
I would suggest to try to add a new flag into AttrContext, along the lines of permitPrivateAccessInHeader, and set it while attributing the permitted types. Not sure how invasive that'd be, but hopefully not terribly much. And it should be clear (and documented) what's happening.

I have decided to write tests before reworking fix. Could you please suggest where could i put it? I have found tools/javac/diags/examples folder, but it seems to only contain negative tests, while I need positive ones. Is there any suitable place for this kind of thing?

most tests for sealed classes are under:
test/langtools/tools/javac/sealed/

[Evemose]

@lahodaj could you review please?

[vicente-romero-oracle]

I think this change needs more test cases so that we can be sure that we are testing most of the possible combinations

[Evemose]

I think this change needs more test cases so that we can be sure that we are testing most of the possible combinations

I have tried to imagine some test cases where something could go wrong. Not really sure how to verify everything concidering the fact that permits is the last clause before attributing class code, which is allowed to access any private members, but I added some test cases veryfying everything from a different angles

[jddarcy]

If this fix is proposing to align javac behavior with the JLS, while no spec change would be needed. A CSR would need to be filed for the behavioral change.

[Evemose]

If this fix is proposing to align javac behavior with the JLS, while no spec change would be needed. A CSR would need to be filed for the behavioral change.

Hello! I am not familiar with CSR process. I tried to find a way to file it using java bug database, but could not find anything. Could you tell me what should I do and where, please?

[lahodaj]

Overall, I think this is good. Left some comments to make the test and style similar to what is generally used in javac, at least in my opinion. Thanks for doing this change!

[lahodaj]

If this fix is proposing to align javac behavior with the JLS, while no spec change would be needed. A CSR would need to be filed for the behavioral change.

Hello! I am not familiar with CSR process. I tried to find a way to file it using java bug database, but could not find anything. Could you tell me what should I do and where, please?

In JBS, in the bug, there is "More", and if you click on that, there is "Create CSR". Click on that, and fill in the details in the text, as described in the template. When reviewing, I'll try to fill any flags that need to be filled (so that you know what gets filled next time).

[Evemose]

By the way, does anyone know why ci fails? It fails on generate report, not on tests. I tried to sync branch with master but it seems like it has not had any affect

[vicente-romero-oracle]

@Evemose I was waiting for the results of the tests after your last change, all green, we are good to go

[vicente-romero-oracle]

thanks!

[Evemose]

/integrate

[openjdk]

@Evemose
Your change (at version 0889133) is now ready to be sponsored by a Committer.

[Evemose]

@Evemose I was waiting for the results of the tests after your last change, all green, we are good to go

Glad we are finally done with this one. Got a bit busy lately, pr could`ve taken much less time, thanks for your patience. Would you mind sponsoring?

[vicente-romero-oracle]

/sponsor

[vicente-romero-oracle]

@Evemose I was waiting for the results of the tests after your last change, all green, we are good to go

Glad we are finally done with this one. Got a bit busy lately, pr could`ve taken much less time, thanks for your patience. Would you mind sponsoring?

thanks to you!

[openjdk]

Going to push as commit b269493.
Since your change was applied there have been 156 commits pushed to the master branch:

• 3ccd2f7: 8342458: More consistent constant instruction handling
• 29ae265: 8341861: GHA: Use only retention mechanism to remove bundles
• e5bbad0: 8334999: RISC-V: implement AES single block encryption/decryption intrinsics
• c40bb76: 8343296: IGV: Show pre/main/post at CountedLoopNodes
• dee0982: 8343132: Remove temporary transitions from Virtual thread implementation
• 2f1ba5e: 8343307: Throw ZipException instead of IAE in ZipFile.Source::initCEN
• 388d44f: 8342156: C2: Compilation failure with fewer arguments after JDK-8329032
• 688e92e: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
• f340ab2: 8342181: Update tests to use stronger Key and Salt size
• cc2fb4d: 8337276: jcmd man page update for PID in output filenames
• ... and 146 more: https://git.openjdk.org/jdk/compare/309b929147e7dddfa27879ff31b1eaad271def85...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vicente-romero-oracle @Evemose Pushed as commit b269493.

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