On Tue, 30 Sep 2025 15:06:11 GMT, Alan Bateman <[email protected]> wrote:
>> 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 > >> I've read the JEP, and reviewed the tests. I hope I manage to contribute >> meaningfully. > > @vy Thank you very much for your thorough review and walk through of all of > the tests. You clearly spent a lot of time on this. I went through your > comments and pushed an update [5acb1d727c5e0feee52c7a1f47665264eb534489]( > https://github.com/openjdk/jdk/pull/25115/commits/5acb1d727c5e0feee52c7a1f47665264eb534489) > to add more tests, improves some comments, and remove the unused setInt > methods that you spotted in the x-module test. Many of the tests are focused > on specific parts of the solution (APIs, access checks, CLI options, JAR file > manifest, JNI, ...) and I've resisted going further with some of these tests > to avoid too much overlap. You have rightly identified a few opportunities > for more tests, e.g. JNI attached thread doing upcall to mutate a final field > in named module as the tests only checks the unnamed module case right now. > This are good suggestions but they require a good bit of setup, I'll try to > get to it (or find someone) so that we have more tests before we are done. > @AlanBateman - Sorry should have realized this sooner. This is only the > preparatory step so writing to final fields is still allowed at the moment, > in which case -Xcheck:jni should only be issuing a warning, otherwise code > that still writes to finals (but which knows it has to stop before > final-really-means-final) won't be able to use -Xcheck:jni to watch for other > problems. JEP 500 is about using deep reflection to mutate final instance fields. It defines the CLI option to enable final field mutation for specific modules and the CLI option to determine the action when illegal final field mutation is attempted. The proposal is to start with the the action "warn" as the default, some future JEP will propose to change it to "deny" and drop the "allow" action. The JEP does not propose to change JNI. Native code that uses JNI to mutate final instance or final static fields will work as before and in the future. So JNI is outside of the fold and the CLI option to allow/warn/deny does not apply to JNI. The roadmap to move to "deny" by default does not include JNI. The change to the JNI implementation is limited to logging with -Xlog:jni=debug and a fatal error when running with -Xcheck:jni. I think what you are suggesting is that the JEP could instead have -Xcheck:jni emit a warning when JNI setter functions mutate final fields, and maybe change it to be a fatal error in the future, maybe as part of the future JEP that proposes to move "deny" the default. I don't see any issue with changing that section of the JEP but only if Ron/Alex agree. TBH, aside from the write-protected fields that are System.in/out/errr, it should be very rare to encounter native code mutating final fields. A more gentle transition for -Xcheck:jni users might not make any difference. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25115#issuecomment-3369072100
