8376045: [lworld] Treat @NullRestricted fields as ACC_STRICT_INIT on load by liach · Pull Request #1951 · openjdk/valhalla

liach · 2026-01-22T21:40:30Z

This is the 2nd PR in the strict removal series. 1st PR is #1952, and 3rd PR is #1959.

Automatically inject strict for NR annotations, and remove tests that previously kept them separate. Also add behavior where NR is only accepted for preview class files.

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-8376045: [lworld] Treat @NullRestricted fields as ACC_STRICT_INIT on load (Bug - P4)

Reviewers

Frederic Parain (@fparain - Committer)

Contributors

Frederic Parain <fparain@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1951

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…load

bridgekeeper · 2026-01-22T21:41:38Z

👋 Welcome back liach! 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-01-22T21:43:05Z

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

8376045: [lworld] Treat @NullRestricted fields as ACC_STRICT_INIT on load

Co-authored-by: Frederic Parain <fparain@openjdk.org>
Reviewed-by: fparain

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

3eab1e0: 8374959: [lworld] runtime/cds/appcds/cacheObject/ArchivedIntegerCacheTest.java fails with --enable-preview
70f4419: 8375777: [lworld] jdk/javadoc/doccheck/checks/jdkCheckLinks.java failed with missing ids
2cea9c8: 8372599: [lworld] runtime/cds/serviceability/ReplaceCriticalClassesForSubgraphs.java fails with --enable-preview
... and 8 more: https://git.openjdk.org/valhalla/compare/c259bd40ebf1a37d607411282202d579fbd3c593...lworld

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 lworld branch, type /integrate in a new comment.

mlbridge · 2026-01-22T21:45:53Z

Webrevs

liach · 2026-01-22T22:45:11Z

Testing: tier1 clear on Linux.

dansmithcode · 2026-01-22T23:44:52Z

I want to confirm that this works on a non-preview class file, and that nothing downstream is going to break. (Hard to prove that second point definitively, but looking for some confirmation from a runtime expert.)

liach · 2026-01-22T23:53:18Z

Currently @NullRestricted requires annotated field type to be a value class, which means the source code always uses preview features and would be marked as such, at least in our code base.

The non-preview class scenario is possible with users annotating a migrated value class field plus opening up jdk internals, don't know what we should do in this case - Given this annotation is expected to be revisited in bworld, wonder if it's worth adding extra checks.

fparain · 2026-01-23T17:36:53Z

With this change, the double annotation @strict + @NullRestricted can be replaced by a single @NullRestricted annotation in all the tests. Is there a reason why this simplification is not part or this PR?

liach · 2026-01-23T19:22:45Z

The annotation @Strict actually has a javac side effect - it makes all initializer statements and constructor bodies early-construction by default. This is not compatible with the planned language evolution with ! types. The PR #1952 fixes that javac side effect.

After both #1951 and #1952 are merged, I plan to move on to to the simplification you mentioned.

fparain · 2026-01-23T21:14:47Z

I want to confirm that this works on a non-preview class file, and that nothing downstream is going to break. (Hard to prove that second point definitively, but looking for some confirmation from a runtime expert.)

The annotation is currently parsed and processed without checking if the class file has a preview version number or not. Looks like an issue that can easily be fixed.

When a field is annotated with @NullRestricted, it is automatically marked as a null-free value field, which will trigger two actions later in the loading of the declaring class:

the JVM will try to load the type of the field with circularity detection
the JVM will check that the loaded type is a concrete value class
This is a remain of the previous lworld model, when strict fields didn't exist yet. This is not the behavior we want with null-restricted fields being strict. But this code is not part of JEP 401, so we haven't updated it yet.

For the JEP 401 preview, the simplest solution would be to simply reject or ignore the annotation when the class file hasn't a preview version number.

…ture/cfp-nr-auto-strict

dansmithcode · 2026-01-23T23:01:15Z

My worry is that if the annotation gets ignored in a non-preview class file, we'll have to be sure javac is setting the preview version number for the relevant tests. But Chen is right, I think, that a value-class-typed field should already trigger LoadableDescriptors and a preview version number, so maybe there's nothing to worry about here.

Alternatively, we can interpret the annotation in all class file versions. That's what I thought we wanted, and I was just confirming that nothing later is going to break because of an unexpected version number. Sounds like probably not.

So it seems good either way.

liach · 2026-01-23T23:17:30Z

Per Fred's suggestion I have added a filter to block NR on non-preview classes. This is experimental; Running tier 1-2 to see whether our "already preview" assumption stands.

liach · 2026-01-25T05:00:14Z

I added !HAS_PENDING_EXCEPTION because otherwise it seems to run into resource leaks; early return seems to have resource leaks too.

fparain · 2026-01-27T14:14:01Z

              class_name()->as_C_string(), name->as_C_string());
          }
+          const bool is_strict = (flags & JVM_ACC_STRICT) != 0;
+          if (!is_strict && !HAS_PENDING_EXCEPTION) {


This test should not include exception checking.
There's a bug just above, the call to Exceptions::fthrow() should be followed by a return; statement.

I tried to add return; to both fthrow() occurrences. The problem is that they hit some weird assertions in runtime like when metaspace is being teared down. I couldn't diagnose them so I resorted to HAS_PENDING_EXCEPTION which runs successfully instead.

…ture/cfp-nr-auto-strict

fparain · 2026-01-27T17:52:04Z

Could you try the patch below that enabled early returns in the parse_fields() method by removing the assumption in Annotations deallocation code that the arrays are fully populated, and by ensuring the ownership transfer of field_annotations is completed before all return to prevent a double free.
It has been tested with only the two tests of this PR, so more testing is needed.

diff --git a/src/hotspot/share/classfile/classFileParser.cpp b/src/hotspot/share/classfile/classFileParser.cpp
index 8af14a0d984..0cd354401e3 100644
--- a/src/hotspot/share/classfile/classFileParser.cpp
+++ b/src/hotspot/share/classfile/classFileParser.cpp
@@ -1461,6 +1461,7 @@ void ClassFileParser::parse_fields(const ClassFileStream* const cfs,
                                              CHECK);
         }
         _fields_annotations->at_put(n, parsed_annotations.field_annotations());
+        parsed_annotations.set_field_annotations(nullptr);
         if (parsed_annotations.has_annotation(AnnotationCollector::_jdk_internal_NullRestricted)) {
           if (!Signature::has_envelope(sig)) {
             Exceptions::fthrow(
@@ -1468,6 +1469,7 @@ void ClassFileParser::parse_fields(const ClassFileStream* const cfs,
               vmSymbols::java_lang_ClassFormatError(),
               "Illegal use of @jdk.internal.vm.annotation.NullRestricted annotation on field %s.%s with signature %s (primitive types can never be null)",
               class_name()->as_C_string(), name->as_C_string(), sig->as_C_string());
+              return;
           }
           if (!supports_inline_types()) {
             Exceptions::fthrow(
@@ -1475,9 +1477,10 @@ void ClassFileParser::parse_fields(const ClassFileStream* const cfs,
               vmSymbols::java_lang_ClassFormatError(),
               "Illegal use of @jdk.internal.vm.annotation.NullRestricted annotation on field %s.%s in non-preview class file",
               class_name()->as_C_string(), name->as_C_string());
+              return;
           }
           const bool is_strict = (flags & JVM_ACC_STRICT) != 0;
-          if (!is_strict && !HAS_PENDING_EXCEPTION) {
+          if (!is_strict) {
             // Inject STRICT_INIT and validate in context
             const jint patched_flags = flags | JVM_ACC_STRICT;
             verify_legal_field_modifiers(patched_flags, class_access_flags, CHECK);
@@ -1485,7 +1488,6 @@ void ClassFileParser::parse_fields(const ClassFileStream* const cfs,
           }
           is_null_restricted = true;
         }
-        parsed_annotations.set_field_annotations(nullptr);
       }
       if (parsed_annotations.field_type_annotations() != nullptr) {
         if (_fields_type_annotations == nullptr) {
diff --git a/src/hotspot/share/oops/annotations.cpp b/src/hotspot/share/oops/annotations.cpp
index c4e49d52383..8fb6f3b7824 100644
--- a/src/hotspot/share/oops/annotations.cpp
+++ b/src/hotspot/share/oops/annotations.cpp
@@ -41,7 +41,9 @@ Annotations* Annotations::allocate(ClassLoaderData* loader_data, TRAPS) {
 void Annotations::free_contents(ClassLoaderData* loader_data, Array<AnnotationArray*>* p) {
   if (p != nullptr) {
     for (int i = 0; i < p->length(); i++) {
-      MetadataFactory::free_array<u1>(loader_data, p->at(i));
+      if (p->at(i) != nullptr) {
+        MetadataFactory::free_array<u1>(loader_data, p->at(i));
+      }
     }
     MetadataFactory::free_array<AnnotationArray*>(loader_data, p);
   }

liach · 2026-01-27T18:08:19Z

/contributor add @fparain

openjdk · 2026-01-27T18:09:35Z

@liach
Contributor Frederic Parain <fparain@openjdk.org> successfully added.

liach · 2026-01-27T19:37:16Z

Ran tier 1-3 on the CI, no failure on linux-x64 platform. I think this patch is safe.

fparain

Looks good to me.

liach · 2026-01-29T23:04:30Z

Thanks for the reviews. I will integrate now to facilitate testing for the subsequent test fixes.

/integrate

openjdk · 2026-01-29T23:05:41Z

Going to push as commit e7af4bd.
Since your change was applied there have been 11 commits pushed to the lworld branch:

3eab1e0: 8374959: [lworld] runtime/cds/appcds/cacheObject/ArchivedIntegerCacheTest.java fails with --enable-preview
70f4419: 8375777: [lworld] jdk/javadoc/doccheck/checks/jdkCheckLinks.java failed with missing ids
2cea9c8: 8372599: [lworld] runtime/cds/serviceability/ReplaceCriticalClassesForSubgraphs.java fails with --enable-preview
... and 8 more: https://git.openjdk.org/valhalla/compare/c259bd40ebf1a37d607411282202d579fbd3c593...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2026-01-29T23:05:52Z

@liach Pushed as commit e7af4bd.

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

8376045: [lworld] Treat @NullRestricted fields as ACC_STRICT_INIT on …

fcf2735

…load

openjdk Bot added the rfr Pull request is ready for review label Jan 22, 2026

Merge branch 'lworld' of https://github.com/openjdk/valhalla into fea…

947cd90

…ture/cfp-nr-auto-strict

Require NR users to be preview

1390ad4

Try again?

8dd8fe9

This was referenced Jan 24, 2026

8376046: [lworld] Adjust @NullRestricted tests to remove @Strict #1959

Closed

8376033: [lworld] Remove auto early construction from strict fields #1952

Closed

liach added 2 commits January 24, 2026 22:43

Missing returns?

0ebdcce

Fix conditionals

dac3449

fparain reviewed Jan 27, 2026

View reviewed changes

liach added 2 commits January 27, 2026 09:09

Merge branch 'lworld' of https://github.com/openjdk/valhalla into fea…

2111f5f

…ture/cfp-nr-auto-strict

Correctly fix resource leak this time

0adaa3a

Restructure according to fred

5589cc4

fparain approved these changes Jan 29, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Jan 29, 2026

openjdk Bot added the integrated Pull request has been integrated label Jan 29, 2026

openjdk Bot closed this Jan 29, 2026

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 29, 2026

Conversation

liach commented Jan 22, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper Bot commented Jan 22, 2026

Uh oh!

openjdk Bot commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

liach commented Jan 22, 2026

Uh oh!

dansmithcode commented Jan 22, 2026

Uh oh!

liach commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fparain commented Jan 23, 2026

Uh oh!

liach commented Jan 23, 2026

Uh oh!

fparain commented Jan 23, 2026

Uh oh!

dansmithcode commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach commented Jan 23, 2026

Uh oh!

liach commented Jan 25, 2026

Uh oh!

fparain Jan 27, 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!

liach Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

fparain commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach commented Jan 27, 2026

Uh oh!

openjdk Bot commented Jan 27, 2026

Uh oh!

liach commented Jan 27, 2026

Uh oh!

fparain left a comment

Choose a reason for hiding this comment

Uh oh!

liach commented Jan 29, 2026

Uh oh!

openjdk Bot commented Jan 29, 2026

Uh oh!

openjdk Bot commented Jan 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

liach commented Jan 22, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Jan 22, 2026 •

edited

Loading

mlbridge Bot commented Jan 22, 2026 •

edited

Loading

liach commented Jan 22, 2026 •

edited

Loading

dansmithcode commented Jan 23, 2026 •

edited

Loading

fparain Jan 27, 2026 •

edited

Loading

fparain commented Jan 27, 2026 •

edited

Loading