RFR: 8353835: Implement JEP 500: Prepare to Make Final Mean Final [v4]

Volkan Yazici vyazici at openjdk.org
Tue Sep 30 07:31:41 UTC 2025 

On Sun, 28 Sep 2025 16:44:56 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Implementation changes for [JEP 500: Prepare to Make Final Mean Final](https://openjdk.org/jeps/500).
>> 
>> Field.set (and Lookup.unreflectSetter) are changed to allow/warn/debug/deny when mutating a final instance field. JFR event recorded if final field mutated. Spec updates to Field.set, Field.setAccessible and Module.addOpens to align with the proposal in the JEP.
>> 
>> HotSpot is updated to add support for the new command line options. To aid diagnosability, -Xcheck:jni reports a fatal error when a mutating a final field with JNI, and -Xlog:jni=debug can help identity when JNI code mutates finals. For now, JNI code is allowed to set the "write-protected" fields System.in/out/err, we can re-visit once we change the System.setIn/setOut/setErr methods to not use JNI (I prefer to keep this separate to this PR because there is a small startup regression to address when changing System.setXXX).
>> 
>> There are many new tests. A small number of existing tests are changed to run /othervm as reflectively opening a package isn't sufficient. Changing the tests to /othervm means that jtreg will launch the agent with the command line options to open the package.
>> 
>> Testing: tier1-6
>
> Alan Bateman has updated the pull request incrementally with one additional commit since the last revision:
> 
>   RemoveFields(duration) and filter internal frames

I've read the JEP, and reviewed the tests. I hope I manage to contribute meaningfully. 

A common theme I've struggled with: While JEP explicitly mentions of `static` fields, and `MethodHandle`s, they are not always exercised in tests. It very well might be unnecessary given the context, but dropped some comments anyway.

For those who were about to jump the gun after seeing no mentions of `VarHandle`s: _"The 2 APIs that are changed by the JEP are `Field.setXXX` and `Lookup.unreflectSetter`.  A `VarHandle` produced by `Lookup.unreflectVarHandle` has never been allowed write access to final fields, so no change there, `UOE` will continue to be thrown."_ – @AlanBateman

src/hotspot/share/runtime/arguments.cpp line 2281:

> 2279:       }
> 2280:     } else if (match_option(option, "--illegal-final-field-mutation=", &tail)) {
> 2281:       if (strcmp(tail, "allow") == 0 || strcmp(tail, "warn") == 0 || strcmp(tail, "debug") == 0 || strcmp(tail, "deny") == 0) {

Is the `jdk.module.illegal.final.field.mutation` property intended as a public API? If so, where is it documented?

test/jdk/java/lang/reflect/Field/mutateFinals/FinalFieldMutationEventTest.java line 1:

> 1: /*

Do we need to extend the cases for a `static` field?

test/jdk/java/lang/reflect/Field/mutateFinals/FinalFieldMutationEventTest.java line 79:

> 77:             recording.enable(EVENT_NAME).withStackTrace();
> 78: 
> 79:             boolean mutated = false;

Instead of relying on `IAE` to be thrown as expected, you may consider passing `mutated` flag in `@run` (`-DwriteAccess`?) and using it to double-check the programmatically computed `mutated`.

test/jdk/java/lang/reflect/Field/mutateFinals/FinalFieldMutationEventTest.java line 114:

> 112: 
> 113:         try (Recording recording = new Recording()) {
> 114:             recording.enable(EVENT_NAME).withStackTrace();

Above remark on `mutated` applies to `unreflected` too.

test/jdk/java/lang/reflect/Field/mutateFinals/MutateFinalsTest.java line 1:

> 1: /*

Do we need to extend the cases for `static` fields and VH too?

test/jdk/java/lang/reflect/Field/mutateFinals/MutateFinalsTest.java line 33:

> 31:  * @run junit/othervm --illegal-final-field-mutation=debug -DwriteAccess=true MutateFinalsTest
> 32:  * @run junit/othervm --illegal-final-field-mutation=deny -DwriteAccess=false MutateFinalsTest
> 33:  */

Shall we also test the defaults too?

Suggestion:

 * @run junit/othervm -DwriteAccess=true MutateFinalsTest
 */

test/jdk/java/lang/reflect/Field/mutateFinals/cli/CommandLineTest.java line 122:

> 120:      * Test warn on first mutation of a final.
> 121:      */
> 122:     @Test

Shall we also test the other way around (i.e., `testUnreflectSetter+testFieldSetInt`) and assert that the ` has been unreflected ...` warning is emitted?

test/jdk/java/lang/reflect/Field/mutateFinals/cli/CommandLineTest.java line 123:

> 121:      */
> 122:     @Test
> 123:     void testWarn() throws Exception {

While this asserts `WARNING: Final field ...` is emitted, it does not assert it is emitted *once*. You may consider performing this check too.

test/jdk/java/lang/reflect/Field/mutateFinals/cli/CommandLineTest.java line 166:

> 164:             .shouldHaveExitValue(0);
> 165:     }
> 166: 

You may consider repeating the test with `deny, allow` order.

test/jdk/java/lang/reflect/Field/mutateFinals/cli/CommandLineTest.java line 185:

> 183:     }
> 184: 
> 185:     /**

Suggestion:

     * Test setting system property to "allow" at runtime. The saved value from startup

test/jdk/java/lang/reflect/Field/mutateFinals/cli/CommandLineTest.java line 190:

> 188:     @Test
> 189:     void testLastOneWins() throws Exception {
> 190:         test("testFieldSetInt", "--illegal-final-field-mutation=allow", "--illegal-final-field-mutation=deny")

This tests `setProperty+Defaults`. Shall we also test `setProperty+vmOpt`?

test/jdk/java/lang/reflect/Field/mutateFinals/cli/CommandLineTestHelper.java line 1:

> 1: /*

Do we also need to assert the default IAE throw before `f.setAccessible(true)` calls?

test/jdk/java/lang/reflect/Field/mutateFinals/jar/ExecutableJarTest.java line 100:

> 98:         testExecutableJar(jarFile, "testUnreflectSetter")
> 99:                 .shouldNotContain(WARNING_LINE1)
> 100:                 .shouldNotContain(WARNING_UNREFLECTED)

We test

* `manifest={mutation=allow}` & `cliOpts={}`, and
* `manifest={mutation=allow}` & `cliOpts={mutation=deny}`.

Shall we also add a test with `manifest={}` & `cliOpts={mutation=allow}`? That is, in the absence of a manifest entry, do command line arguments still apply?

test/jdk/java/lang/reflect/Field/mutateFinals/jar/ExecutableJarTest.java line 104:

> 102:     }
> 103: 
> 104:     /**

Why don't we verify the output (i.e., no warnings) in this case?

test/jdk/java/lang/reflect/Field/mutateFinals/jar/ExecutableJarTest.java line 110:

> 108:      */
> 109:     @Test
> 110:     void testFieldSetWithAddOpens1() throws Exception {

Suggestion:

     * with the Add-Opens attribute.

test/jdk/java/lang/reflect/Field/mutateFinals/jar/ExecutableJarTest.java line 121:

> 119:     }
> 120: 
> 121:     /**

Why don't we verify the output (i.e., no warnings) in this case?

test/jdk/java/lang/reflect/Field/mutateFinals/jar/ExecutableJarTest.java line 129:

> 127:     void testFieldSetWithAddOpens2() throws Exception {
> 128:         String jarFile = createExecutableJar(Map.of(
> 129:                 "Enable-Final-Field-Mutation", "ALL-UNNAMED",

Instead of `BadValue`, shall we use a valid module name here to stress the following specification from the JEP:

> The only supported value for the `Enable-Final-Field-Mutation`
> manifest entry is `ALL-UNNAMED`; other values cause an
> exception to be thrown.

test/jdk/java/lang/reflect/Field/mutateFinals/jar/ExecutableJarTestHelper.java line 1:

> 1: /*

Do we need to extend the cases for a `static` value and VH?

test/jdk/java/lang/reflect/Field/mutateFinals/jar/ExecutableJarTestHelper.java line 1:

> 1: /*

Do we also need to assert the default IAE throw before `f.setAccessible(true)` calls?

test/jdk/java/lang/reflect/Field/mutateFinals/jni/JNIAttachMutator.java line 59:

> 57:             this.value = value;
> 58:         }
> 59:     }

Do we need to extend this list with a copy of itself where `value` is static?

test/jdk/java/lang/reflect/Field/mutateFinals/jni/JNIAttachMutatorTest.java line 1:

> 1: /*

Similar to `ExecutableJarTest`, do we need to extend this test for named modules?

test/jdk/java/lang/reflect/Field/mutateFinals/modules/Driver.java line 30:

> 28:  *     field is open to m2 and not open to m3.
> 29:  * @build m1/* m2/* m3/*
> 30:  * @run junit/othervm --illegal-final-field-mutation=allow -DallowedToMutate=m1,m2 m1/p1.TestMain

Instead of an explicit `allow`, shall we use (and hence, stress) the defaults?

Suggestion:

 * @run junit/othervm -DallowedToMutate=m1,m2 m1/p1.TestMain

test/jdk/java/lang/reflect/Field/mutateFinals/modules/m1/p1/Mutator.java line 36:

> 34:     void setInt(Field f, Object obj, short value) throws IllegalAccessException;
> 35:     void setInt(Field f, Object obj, int value) throws IllegalAccessException;
> 36:     void setLong(Field f, Object obj, long value) throws IllegalAccessException;

Is

     void setInt(..., short value)

used at all? If so,

1. Where?
2. Do we also need the following?

       setLong(..., short value)
       setLong(..., int value)
       setDouble(..., float value)

test/jdk/java/lang/reflect/Field/mutateFinals/modules/m1/p1/TestMain.java line 1:

> 1: /*

All `C::value` are instance fields, do we need to test class (i.e., `static`) fields too?

test/jdk/java/lang/reflect/Field/mutateFinals/modules/m1/p1/TestMain.java line 42:

> 40:  * Test mutating final fields from different modules.
> 41:  */
> 42: 

Suggestion:

test/jdk/java/lang/reflect/Field/mutateFinals/modules/m1/p1/TestMain.java line 44:

> 42: 
> 43: class TestMain {
> 44:     // the names of modules that can mutate finals in m1/p1

Suggestion:

    // the names of modules that are allowed to mutate finals in m1/p1

test/jdk/java/lang/reflect/Field/mutateFinals/modules/m1/p1/TestMain.java line 88:

> 86:             return mutators.stream();
> 87:         }
> 88:     }

You can consider populating `mutators` and `deniedMutators` in `setup()` using a single loop. This would result in less LoC and render `allowedToMutate` field redundant.

test/jdk/java/lang/reflect/Field/mutateFinals/modules/m1/p1/TestMain.java line 105:

> 103:         var obj = new C(oldValue);
> 104: 
> 105:         f.setAccessible(false);

Shouldn't we avoid tampering the defaults with `f.setAccessible(false)` while validating the default IAE throw?

test/micro/org/openjdk/bench/java/lang/reflect/FieldSet.java line 1:

> 1: /*

Do we need to extend the benchmark cases for a `static` field, VH, and MH too?

test/micro/org/openjdk/bench/java/lang/reflect/FieldSet.java line 110:

> 108:     @Benchmark
> 109:     public void setNonFinalLongField() throws Exception {
> 110:         long newValue = ThreadLocalRandom.current().nextLong();

Double-checking: `ThreadLocalRandom.current().nextLong()` would not eclipse the timing of the subject setter, right?

-------------

PR Review: https://git.openjdk.org/jdk/pull/25115#pullrequestreview-3272168187
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387330308
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389003505
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389008777
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389011029
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389026675
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389031363
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387254039
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387261330
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387272585
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387276982
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387281666
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389067367
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387438980
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387406182
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387407350
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387410454
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2387427536
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389069905
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389070187
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2388859285
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2388872897
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389077443
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2388887478
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2388946831
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2388929369
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2388925794
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2388955439
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389062409
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389091791
PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2389089796

More information about the core-libs-dev mailing list