8354556: Expand value-based class warnings to java.lang.ref API #24746

vicente-romero-oracle · 2025-04-18T00:06:26Z

This PR is defining a new internal annotation, @jdk.internal.RequiresIdentity, with target types PARAMETER and TYPE_PARAMETER. The @RequiresIdentity annotation expresses the expectation that an argument to a given method or constructor parameter will be an object with a unique identity, not an instance of a value-based class; or that the type argument to a given type parameter will not be a value-based class type.

For more details please refer to the complete description in the corresponding JIRA entry [1]

TIA

[1] https://bugs.openjdk.org/browse/JDK-8354556

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

Issues

JDK-8354556: Expand value-based class warnings to java.lang.ref API (Enhancement - P4)
JDK-8356539: Expand value-based class warnings to java.lang.ref API (CSR)

Reviewers

Jan Lahoda (@lahodaj - Reviewer)

Contributors

Archie Cobbs <acobbs@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24746

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-18T00:07:10Z

👋 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 · 2025-04-18T00:07:13Z

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

8354556: Expand value-based class warnings to java.lang.ref API

Co-authored-by: Archie Cobbs <acobbs@openjdk.org>
Reviewed-by: jlahoda

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

6a58af3: 8357143: New test AOTCodeCompressedOopsTest.java fails on platforms without AOT Code Cache support
84a98ab: 8357166: Many AOT tests failed with VM crash
fbc12be: 8349151: Refactor test/java/security/cert/CertificateFactory/slowstream.sh to java test

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 · 2025-04-18T00:07:51Z

@vicente-romero-oracle The following labels will be automatically applied to this pull request:

build
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.

liach · 2025-04-18T02:25:24Z

make/langtools/src/classes/build/tools/symbolgenerator/CreateSymbols.java

Need to add handling of RequiresIdentity in createAnnotation

update on CreateSymbols, the complete implementation with the changes for CreateSymbols should be included in the PR for [1]. Which will be developed in parallel to this proposal

[1] https://bugs.openjdk.org/browse/JDK-8356894

liach · 2025-04-18T02:32:06Z

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties

@@ -4249,6 +4249,14 @@ compiler.warn.declared.using.preview=\
 compiler.warn.attempt.to.synchronize.on.instance.of.value.based.class=\
    attempt to synchronize on an instance of a value-based class

+# lint: identity
+compiler.warn.attempt.to.synchronize.on.instance.of.value.based.class2=\


How is this different from the message with no 2 suffix? Is there a reason why one localization entry is required for one type of lint?

the requirement is for the identity lint warning to be an alias of the synchronization warning

…e/jdk into JDK-8354556

openjdk · 2025-04-24T21:39:17Z

⚠️ @vicente-romero-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

archiecobbs · 2025-05-10T20:38:36Z

FYI, this issue now effectively fixes JDK-7004476 Lint.LintCategory should support aliases, which happened to be closed as "Rejected" on Monday. Not sure if anything needs to be done.

vicente-romero-oracle · 2025-05-10T22:46:20Z

FYI, this issue now effectively fixes JDK-7004476 Lint.LintCategory should support aliases, which happened to be closed as "Rejected" on Monday. Not sure if anything needs to be done.

I guess we can add a note to that bug once this one goes through

kimbarrett · 2025-05-11T19:11:40Z

I'm coming here from "far away" and am not familiar with all the work leading
up to this proposal, so maybe there's some rationale that I don't know about.
(I did look, but didn't find anything other than this draft JEP:
https://openjdk.org/jeps/8340476. And that doesn't really deal with the issue
I discuss below.)

Keeping that in mind, I'm surprised by some parts of this change. Specifically

Apply the annotation to the following:

The referent parameter of all constructors in classes PhantomReference, SoftReference, and WeakReference
...
The type parameter T of Reference, PhantomReference, SoftReference, WeakReference, and ReferenceQueue

I wonder why it is not sufficient to apply the annotation to the type
parameter T of Reference, and leave it at that?

The type parameter of the subclasses is passed as the type parameter to
Reference in the extends clause. I would expect if the type parameter for a
subclass didn't satisfy the annotation that would be noticed there. Indeed, if
that isn't the case then many uses of Reference types won't benefit from this
annotation, since they are using classes derived from one of those subclasses
and won't have this internal annotation. That would significantly limit the
utility of this annotation in this area.

Similarly, I wonder why the constructors need the annotation. And why don't
the return type of get or the argument for Reference::refersTo need it?
(Maybe because those values can be null? But there's nothing that prevents
passing null to one of the constructors. It's not very useful to do so, but
not forbidden. I've even seen a use-case for doing so.)

lahodaj · 2025-05-14T18:02:10Z

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

@@ -5667,4 +5668,193 @@ private <E extends Element> Void runUnderLint(E symbol, JCClassDecl p, BiConsume

    }

+    void checkRequiresIdentity(JCTree tree, Lint lint) {
+        switch (tree) {
+            case JCClassDecl classDecl : {


Nit - unless fall-through is needed (it does not seem to be, on the first sight), I would suggest to use -> instead of :, esp. given the cases use blocks anyway.

lahodaj

javac changes look overall good to me. One question and some comments for consideration inline.

lahodaj · 2025-05-14T18:23:09Z

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

@@ -2672,6 +2670,9 @@ public void visitApply(JCMethodInvocation tree) {
            Type capturedRes = resultInfo.checkContext.inferenceContext().cachedCapture(tree, restype, true);
            result = check(tree, capturedRes, KindSelector.VAL, resultInfo);
        }
+        if (env.info.lint.isEnabled(LintCategory.IDENTITY)) {


This is the only place where there's a check whether the lint is enabled before the call to checkRequiresIdentity. Is there a reason for that?

left over of the initial approach I guess. I will remove it

lahodaj · 2025-05-14T18:25:26Z

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

@@ -5667,4 +5668,193 @@ private <E extends Element> Void runUnderLint(E symbol, JCClassDecl p, BiConsume

    }

+    void checkRequiresIdentity(JCTree tree, Lint lint) {


For consideration: as far as I can see, we have a sharp(er) type when we call checkRequiresIdentity, but we give up the type, and re-instante it here using the pattern matching switch. I wonder if it would be more elegant (and hopefully not really too much longer) if the checkRequiresIdentity method would have multiple overloads, with the sharp(er) types, like JCClassDecl/JCVariableDecl, etc.

Or is there a reason to given up the sharp(er) type and re-create it using the switch?

I guess the motivation was to have all this related code in one place, if this code is split into several overloaded methods then I guess an entity, a class, should probably be defined to contain all this code. Dunno I guess the current solution seemed more self-contained to me

lahodaj · 2025-05-14T18:27:18Z

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

+            if (t != null && t.tsym != null) {
+                SymbolMetadata sm = t.tsym.getMetadata();
+                if (sm != null && !t.getTypeArguments().isEmpty()) {
+                    for (Attribute.TypeCompound ta: sm.getTypeAttributes().stream()


The code here, and the code in checkIfTypeParamsRequiresIdentity look similar a lot (although they manipulate List<Type> and List<JCExpression>, of course. I wonder if there's a chance to share the code, at least partially.

I have rewritten both using streams, technically we could still reuse some code but not a big amount of it, dunno

lahodaj

javac changes look good to me, with one trivial nit in Check.

lahodaj · 2025-05-19T08:27:55Z

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

+                                CompilerProperties.LintWarnings.AttemptToUseValueBasedWhereIdentityExpected));
+        }
+    }
+ }


Nit - there appears to be an extra space at the beginning of this line.

jdlib · 2025-05-19T11:51:15Z

make/langtools/src/classes/build/tools/symbolgenerator/CreateSymbols.java

@@ -304,13 +304,18 @@ public void createSymbols(String ctDescriptionFileExtra, String ctDescriptionFil
            "Ljdk/internal/ValueBased;";
    private static final String VALUE_BASED_ANNOTATION_INTERNAL =
            "Ljdk/internal/ValueBased+Annotation;";
+    private static final String REQUIRES_IDENTITY_ANNOTATION =
+            "Ljdk/internal/RequieresIdentity;";


typo: RequieresIdentity -> RequiresIdentity

lahodaj

javac changes look good to me.

vicente-romero-oracle · 2025-05-19T19:19:24Z

thanks for the reviews, rerunning tests before pushing

vicente-romero-oracle · 2025-05-19T22:45:56Z

/integrate

vicente-romero-oracle · 2025-05-19T22:46:17Z

thanks guys for the reviews and contributions

openjdk · 2025-05-19T22:47:18Z

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

fbeea1d: 8356844: Missing @serial annotation for sun.print.CustomOutputBin#serialVersionUID
27a4243: 8354560: Exponentially delay subsequent native thread creation in case of EAGAIN
bd99525: 8356193: Remove tests from ProblemList-enable-preview.txt fixed by JDK-8344706
... and 4 more: https://git.openjdk.org/jdk/compare/265d630125db448ba0cdc3ab7e938beb50e93ed0...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-19T22:47:24Z

@vicente-romero-oracle Pushed as commit 637e9d1.

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

8354556: Expand value-based class warnings to java.lang.ref API

950f391

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Apr 18, 2025

liach reviewed Apr 18, 2025

View reviewed changes

vicente-romero-oracle added 12 commits April 21, 2025 10:29

minor fixes

4c4aebe

Merge branch 'master' into JDK-8354556

0979978

fixing bug

d35ef50

removing the REQUIRES_IDENTITY flag

ed64f22

removing requiresIdentityInternalType

e6c17f7

removing unnecessary changes

d40e285

fixing bug

5a0b6bf

only do extra analysis if lint warning enable

434dda7

minor chantes to CreateSymbol

9f0407a

additional coverage for other uses of types

2697858

additional coverage for other uses of types

b4f5329

Merge branch 'JDK-8354556' of https://github.com/vicente-romero-oracl…

ac28815

…e/jdk into JDK-8354556

vicente-romero-oracle added 8 commits April 24, 2025 22:20

covering more cases plus refactoring in order to simplify the code

5491f39

refactoring

09b8331

refactoring

9d6af78

more refactorings and tests

4a6b9bc

more refactorings

1c3cd61

removing unneeded changes

84b52a6

additional refactorings

70e0311

final adjustments

5e3acc9

vicente-romero-oracle marked this pull request as ready for review May 1, 2025 15:01

Merge branch 'master' into JDK-8354556

22acaf2

lahodaj mentioned this pull request May 14, 2025

8356894: Adjust CreateSymbols to properly handle the newly added @jdk.internal.RequiresIdentity #25232

Closed

3 tasks

lahodaj reviewed May 14, 2025

View reviewed changes

update warning message

bdbcd33

openjdk bot removed the csr Pull request needs approved CSR before integration label May 15, 2025

vicente-romero-oracle added 2 commits May 15, 2025 16:26

addressing review comments

6c8d2b1

addressing review comments

def2505

lahodaj approved these changes May 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 19, 2025

jdlib reviewed May 19, 2025

View reviewed changes

review comments

cf92614

openjdk bot removed the ready Pull request is ready to be integrated label May 19, 2025

Merge branch 'master' into JDK-8354556

904bd4c

lahodaj approved these changes May 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 19, 2025

openjdk bot added the integrated Pull request has been integrated label May 19, 2025

openjdk bot closed this May 19, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 19, 2025

graalvmbot mentioned this pull request May 27, 2025

[GR-65267] Update labsjdk to 25+24-jvmci-b01 oracle/graal#11267

Closed

archiecobbs mentioned this pull request Jun 17, 2025

8359596: Behavior change when both -Xlint:options and -Xlint:-options flags are given #25840

Closed

4 tasks

		@@ -5667,4 +5668,193 @@ private <E extends Element> Void runUnderLint(E symbol, JCClassDecl p, BiConsume

		}

		void checkRequiresIdentity(JCTree tree, Lint lint) {

8354556: Expand value-based class warnings to java.lang.ref API #24746

8354556: Expand value-based class warnings to java.lang.ref API #24746

Uh oh!

Conversation

vicente-romero-oracle commented Apr 18, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Apr 18, 2025

Uh oh!

openjdk bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 24, 2025

Uh oh!

archiecobbs commented May 10, 2025

Uh oh!

vicente-romero-oracle commented May 10, 2025

Uh oh!

kimbarrett commented May 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vicente-romero-oracle commented May 19, 2025

Uh oh!

vicente-romero-oracle commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

Uh oh!

vicente-romero-oracle commented Apr 18, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 18, 2025 •

edited

Loading

vicente-romero-oracle May 15, 2025 •

edited

Loading

vicente-romero-oracle commented May 19, 2025 •

edited

Loading