On Sun, 28 Sep 2025 16:44:56 GMT, Alan Bateman <[email protected]> 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
