Skip to content

8344943: Mark not subclassable classes final in java.base exported classes#22389

Closed
eirbjo wants to merge 8 commits intoopenjdk:masterfrom
eirbjo:non-subclassable-final
Closed

8344943: Mark not subclassable classes final in java.base exported classes#22389
eirbjo wants to merge 8 commits intoopenjdk:masterfrom
eirbjo:non-subclassable-final

Conversation

@eirbjo
Copy link
Contributor

@eirbjo eirbjo commented Nov 26, 2024

Please review this PR which adds the final modifier to non-subclassable classes in java.base.

The classes were identified using an automated analysis. See CSR for details.

Besides simply adding the final access modifier, the PR:

  • Updates a note in java.lang.constant.DynamicCallSiteDesc to not reference subtypes. See CSR for discussion.
  • Removes the class java.lang.Runtime from the test test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java
  • Updates the copyright year of affected source files

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

Issues

  • JDK-8344943: Mark not subclassable classes final in java.base exported classes (Enhancement - P4)
  • JDK-8345054: Mark not subclassable classes final in java.base exported classes (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22389

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 26, 2024

👋 Welcome back eirbjo! 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
Copy link

openjdk bot commented Nov 26, 2024

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

8344943: Mark not subclassable classes final in java.base exported classes

Reviewed-by: weijun, liach, djelinski

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

  • 17e3df6: 8347763: [doc] Add documentation of module options for JEP 483
  • 6cc1c0a: 8347422: Crash during safepoint handler execution with -XX:+UseAPX
  • baca7da: 8347733: Replace SIZE_FORMAT in runtime code
  • 2ca1b4d: 8347840: Fix testlibrary compilation warnings
  • 2c41f5a: 8347959: ThreadDumper leaks memory
  • 15e06b6: 8345618: javax/swing/text/Caret/8163124/CaretFloatingPointAPITest.java leaves Caret is not complete
  • 3ab521e: 8347924: Replace SIZE_FORMAT in memory and metaspace
  • 8460072: 8342881: RISC-V: secondary_super_cache does not scale well: C1 and interpreter
  • f5573f5: 8330851: C2: More efficient TypeFunc creation
  • a3eef6c: 8346875: Test jdk/jdk/jfr/event/os/TestCPULoad.java fails on macOS
  • ... and 4 more: https://git.openjdk.org/jdk/compare/07a0dcbabcb1216211c1da75f6dd2582da194df9...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 csr Pull request needs approved CSR before integration label Nov 26, 2024
@openjdk
Copy link

openjdk bot commented Nov 26, 2024

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

  • core-libs
  • net
  • nio
  • security

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 security security-dev@openjdk.org nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org labels Nov 26, 2024
* {@code invokedynamic} call site.
*
* <p>Concrete subtypes of {@linkplain DynamicCallSiteDesc} should be immutable
* and their behavior should not rely on object identity.
Copy link
Member

Choose a reason for hiding this comment

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

Please reword this to something like:

{@code DynamicCallSiteDesc} is immutable and its behavior does not rely on object identity.

This is given in ConstantDesc, but DynamicCallSiteDesc does not extend ConstantDesc so the removal is dubious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've updated the PR and CSR to retain the note about immutability and object identity, removing the reference to subtypes:

This class is immutable and its behavior does not rely on object identity

I opted to replace the self-reference {@code DynamicCallSiteDesc} with just "This class" here. Let me know if you prefer spelling out the class name.

*
* <p>Concrete subtypes of {@linkplain DynamicCallSiteDesc} should be immutable
* and their behavior should not rely on object identity.
* <p>This class is immutable and its behavior does not rely on object identity
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* <p>This class is immutable and its behavior does not rely on object identity
* <p>A {@code DynamicCallSiteDesc} is immutable and its behavior does not
* rely on object identity.

This describes an object, not a class, and please close sentences with a period.
(We use fragments for param and return block tags instead)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed in 6156b46 :)

@eirbjo
Copy link
Contributor Author

eirbjo commented Nov 26, 2024

Following some offline discussion with @liach, we decided to leave out the constructor access updates in ModuleDescriptor and InterfaceAddress for now. They are not considered a specification change and adds noise for the CSR review.

These changes may be revisited in this PR pending CSR approval, or can be addressed in follow-up PRs.

Copy link
Member

@liach liach left a comment

Choose a reason for hiding this comment

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

Looks good for 25.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Jan 16, 2025
@eirbjo eirbjo marked this pull request as ready for review January 16, 2025 18:06
@eirbjo eirbjo marked this pull request as draft January 16, 2025 18:07
@eirbjo eirbjo marked this pull request as ready for review January 16, 2025 20:37
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 16, 2025
@eirbjo
Copy link
Contributor Author

eirbjo commented Jan 16, 2025

Now that the CSR for this change is "final-ly" approved, I'm marking this PR ready for review.

Since the CSR approval, the PR has merged with a recent master and the copyright headers are updated for 2025.

@mlbridge
Copy link

mlbridge bot commented Jan 16, 2025

Webrevs

Copy link
Member

@djelinski djelinski left a comment

Choose a reason for hiding this comment

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

LGTM, the class changes are pretty trivial.
I suppose the CheckCSMs test could be cleaned up a little (will we ever need KNOWN_NON_FINAL_CSMS again?), but that can be done in a separate PR.

@seanjmullan
Copy link
Member

@bradfordwetmore @wangweij please review the change to DRBGParameters and make sure we are ok with it.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jan 17, 2025
@wangweij
Copy link
Contributor

The DRBGParameters change looks good. It’s a utility class with static methods and a private constructor, so any attempt to extend it should result in a compile-time error.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 17, 2025
@eirbjo
Copy link
Contributor Author

eirbjo commented Jan 24, 2025

Thanks everyone for your patient reviews in this PR. I'll integrate this now.

/integrate

@openjdk
Copy link

openjdk bot commented Jan 24, 2025

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

  • 76f792b: 8348203: [JVMCI] Make eager JVMCI initialization observable in the debugger
  • 909cef5: 8339891: Several sun/security/ssl/SSLSessionImpl/* tests override test.java.opts
  • 56a1877: 8348170: Unnecessary Hashtable usage in CSS.styleConstantToCssMap
  • 9768f60: 8345249: Apply some conservative cleanups in FileURLConnection
  • c5a69b6: 8331723: Serial: Remove the unused parameter of the method SerialHeap::gc_prologue
  • 9c55e25: 8347981: RISC-V: Add Zfa zli imm loads
  • e20bd01: 8344361: Restore null return for invalid services from legacy providers
  • 5a0bdd0: 8346890: AArch64: Type profile counters generate suboptimal code
  • 4a375e5: 8344232: [PPC64] secondary_super_cache does not scale well: C1 and interpreter
  • 0df9dcb: 8346572: Check is_reserved() before using ReservedSpace instances
  • ... and 100 more: https://git.openjdk.org/jdk/compare/07a0dcbabcb1216211c1da75f6dd2582da194df9...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 24, 2025
@openjdk openjdk bot closed this Jan 24, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 24, 2025
@openjdk
Copy link

openjdk bot commented Jan 24, 2025

@eirbjo Pushed as commit 8e8f800.

💡 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

Labels

core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated net net-dev@openjdk.org nio nio-dev@openjdk.org security security-dev@openjdk.org

Development

Successfully merging this pull request may close these issues.

6 participants

Comments