8378908: [lworld] Serializations warnings for value classes that have private serialization methods by vicente-romero-oracle · Pull Request #2187 · openjdk/valhalla

vicente-romero-oracle · 2026-03-02T20:50:19Z

This PR is adding a new serialization warning for the case when a value class declares one of the following serialization related methods:

readObject
writeObject
readObjectNoData

They have no effect if declared in value classes, fact that should be notified to the user,

TIA

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8378908: [lworld] Serializations warnings for value classes that have private serialization methods (Bug - P4)

Reviewers

Chen Liang (@liach - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2187

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2187.diff

Using Webrev

Link to Webrev Comment

… private serialization methods

vicente-romero-oracle · 2026-03-02T21:50:28Z

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

-                            case "readObject"       -> checkReadObject(tree,e, method);
-                            case "readObjectNoData" -> checkReadObjectNoData(tree, e, method);
-                            case "readResolve"      -> checkReadResolve(tree, e, method);
+                            case "writeObject"      -> isSerialMethodCorrect[0] = hasAppropriateWriteObject(tree, e, method);


some changes in this method and in the methods invoked here were not strictly necessary but were done for the sake of uniformity

Can we convert this to a switch expression, like:

isSerialMethodCorrect[0] = switch (name) { case "writeObject" -> hasAppropriateWriteObject(tree, e, method);

and so on.

bridgekeeper · 2026-03-03T09:32:34Z

👋 Welcome back vromero! 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 · 2026-03-03T09:41:51Z

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

8378908: [lworld] Serializations warnings for value classes that have private serialization methods

Reviewed-by: liach

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 no new commits pushed to the lworld branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

mlbridge · 2026-03-03T09:52:19Z

Webrevs

liach · 2026-03-04T03:54:28Z

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

-                            case "readObject"       -> checkReadObject(tree,e, method);
-                            case "readObjectNoData" -> checkReadObjectNoData(tree, e, method);
-                            case "readResolve"      -> checkReadResolve(tree, e, method);
+                            case "writeObject"      -> isSerialMethodCorrect[0] = hasAppropriateWriteObject(tree, e, method);


Can we convert this to a switch expression, like:

isSerialMethodCorrect[0] = switch (name) { case "writeObject" -> hasAppropriateWriteObject(tree, e, method);

and so on.

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

…java Co-authored-by: Chen Liang <liach@openjdk.org>

jddarcy · 2026-03-04T16:57:34Z

src/java.base/share/classes/java/time/chrono/HijrahDate.java

     * @throws InvalidObjectException always
     */
    @java.io.Serial
+    @SuppressWarnings("serial")


The other uses of @SuppressSerial in the JDK tend to have a comment explaining why the warning is being suppressed.

jddarcy · 2026-03-04T16:58:50Z

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

            // fields
-            final boolean[] hasWriteReplace = {false};
+            final Map<String, Symbol> declaredSerialMethodNames = new HashMap<>();
+            final boolean[] isSerialMethodCorrect = new boolean[] { false };


Why is a one-element boolean array being used as opposed to a (non-final) boolean?

Because this is passed to a lambda expression, which requires final local variables.

This variable is no longer used outside of runUnderLint, we can convert it to a local at the switch where it is assigned.

jddarcy · 2026-03-04T16:59:33Z

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

                                    LintWarnings.SerializableValueClassWithoutWriteReplace2);
                }
            }
+            if (c.isValueClass()) {


I think a short comment here explaining the rules for serializable value classes would be helpful.

jddarcy · 2026-03-04T17:00:31Z

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


            // Check for missing serialVersionUID; check *not* done
            // for enums or records.
            VarSymbol svuidSym = null;


Do value classes need any distinct serialVersionUID handling?

not sure I get your question. Why should they?

Hi @RogerRiggs, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user RogerRiggs" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

not sure I get your question. Why should they?

I don't know if they do; that's why I was asking :-)

I think my concerns would be addressed by a "here's how serial value classes work."

Oh I see, thanks @RogerRiggs for your answer. I didn't know that specific about migrated value classes. I will add a comment

Hi @RogerRiggs, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user RogerRiggs" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Do value classes need any distinct serialVersionUID handling?

Value classes have the same requirements (non-requirements) as identity classes.
They are used to confirm the match between the class and the serialized stream contents.
Migrated value classes especially need the value class and identity class to have the same SVUID.

Oh I see, thanks @RogerRiggs for your answer. I didn't know that specific about migrated value classes. I will add a comment

Formally, use of the proxy pattern is recommended for value classes. Where-in, the writeReplace method is invoked to provide the proxy object to be serialized. A record is recommended since it has a stable nominative serialized form. When the proxy object is deserialized, its readReplace is invoked to construct the object.
The Valhalla ObjectOutputStream and ObjectInputStream describe the process. (Maybe more words are needed).

For migrated java.base classes there is a private protocol implemented in ObjectInputStream to use a constructor of each class to create the value objects from the field values extracted from the serialized form.
The spec in this area is weak and vague, since it is not intended for general use. More work is needed in this area of the spec.

jddarcy · 2026-03-12T04:43:12Z

@vicente-romero-oracle, updates generally looks good; I can given a final re-review after JavaOne.

liach

Looks all good otherwise.

liach · 2026-04-03T12:58:16Z

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

            // fields
-            final boolean[] hasWriteReplace = {false};
+            final Map<String, Symbol> declaredSerialMethodNames = new HashMap<>();
+            final boolean[] isSerialMethodCorrect = new boolean[] { false };


This variable is no longer used outside of runUnderLint, we can convert it to a local at the switch where it is assigned.

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

Co-authored-by: Chen Liang <liach@openjdk.org>

…e/valhalla into JDK-8378908

openjdk · 2026-04-03T14:35:31Z

⚠️ @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).

liach

Looks good to go!

jddarcy · 2026-04-03T16:22:10Z

src/java.base/share/classes/java/time/chrono/HijrahDate.java

     * @throws InvalidObjectException always
     */
    @java.io.Serial
+    @SuppressWarnings("serial") // this method is not invoked for value classes


Perhaps as future work, it would be technically possible for the serial lint checks to look inside the body of a method like this, notice the method always throws an exception, and elide the warning in that case. (It is a little unfortunate to "penalize" the java.time method for being robust in their serial handing by requiring new suppress warnings.)

Various known enhancements to the serial checking envision that deep inspection of lint checking, as in JDK-7019074.

thanks @jddarcy, I have created: https://bugs.openjdk.org/browse/JDK-8381637 to address this issue. Are you OK with the PR?

Hmm, would it be easy for Javac to determine a method alwys finishes exceptionally?

8378908: [lworld] Serializations warnings for value classes that have…

48c47e3

… private serialization methods

vicente-romero-oracle marked this pull request as draft March 2, 2026 20:58

adding tests plus bug fix

f539950

vicente-romero-oracle marked this pull request as ready for review March 2, 2026 21:47

vicente-romero-oracle commented Mar 2, 2026

View reviewed changes

openjdk bot added the rfr Pull request is ready for review label Mar 3, 2026

liach reviewed Mar 4, 2026

View reviewed changes

vicente-romero-oracle and others added 2 commits March 4, 2026 06:52

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

acbf6b3

…java Co-authored-by: Chen Liang <liach@openjdk.org>

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

03d9776

…java Co-authored-by: Chen Liang <liach@openjdk.org>

jddarcy reviewed Mar 4, 2026

View reviewed changes

vicente-romero-oracle added 3 commits March 5, 2026 18:37

Merge branch 'lworld' into JDK-8378908

e22a52f

Merge branch 'lworld' into JDK-8378908

e3ca779

addressing review comments

936d024

vicente-romero-oracle requested a review from jddarcy March 6, 2026 18:12

doc change

8a539cd

vicente-romero-oracle added 2 commits April 2, 2026 20:18

Merge branch 'lworld' into JDK-8378908

78b0f1c

adding more documentation

162263b

liach reviewed Apr 3, 2026

View reviewed changes

vicente-romero-oracle and others added 5 commits April 3, 2026 10:29

addressing review comments

8a88cbd

Apply suggestion from @liach

8cd3594

Co-authored-by: Chen Liang <liach@openjdk.org>

Apply suggestion from @liach

4fb9859

Co-authored-by: Chen Liang <liach@openjdk.org>

Apply suggestions from code review

461261a

Co-authored-by: Chen Liang <liach@openjdk.org>

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

3630bfb

…e/valhalla into JDK-8378908

liach approved these changes Apr 3, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 3, 2026

jddarcy reviewed Apr 3, 2026

View reviewed changes

Conversation

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

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Mar 3, 2026

Uh oh!

openjdk bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogerRiggs Mar 6, 2026 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle Mar 6, 2026 • 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!

RogerRiggs Mar 6, 2026 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

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!

jddarcy commented Mar 12, 2026

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Apr 3, 2026

Uh oh!

liach 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!

Reviewers

vicente-romero-oracle commented Mar 2, 2026 •

edited by openjdk bot

Loading

openjdk bot commented Mar 3, 2026 •

edited

Loading

mlbridge bot commented Mar 3, 2026 •

edited

Loading

RogerRiggs Mar 6, 2026 •

edited by bridgekeeper bot

Loading

vicente-romero-oracle Mar 6, 2026 •

edited

Loading

RogerRiggs Mar 6, 2026 •

edited by bridgekeeper bot

Loading