On Tue, 25 Nov 2025 17:03:57 GMT, David Beaumont <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove note about StableValue (not possible)
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/file/JRTIndex.java line
> 341:
>
>> 339:
>> 340: public boolean isInJRT(FileObject fo) {
>> 341: if (fo instanceof PathFileObject pathFileObject) {
>
> This is surprisingly subtle and needs someone much more familiar with javac,
> FileObject and other things to look at. This method is used exactly once in
> ClassFinder to see if the path of a FileObject "came from" the JRT index.
>
> Previously any FileObject created with a path that come from the global jrt
> file system would cause this to return "true". Now, it might not, because
> preview mode and non-preview mode no longer share a file system instance.
>
> This feels correct in the sense that "the two classes could have different
> content in different modes", but it's bad if anyone wants flags for a class
> that wasn't changed by preview mode. In that case, the code will now fail to
> do anything (whereas before it would):
>
> --------
>
> long getSupplementaryFlags(ClassSymbol c) {
> if (jrtIndex == null || !jrtIndex.isInJRT(c.classfile) || c.name ==
> names.module_info) {
> return 0;
> }
> ...
> }
>
> --------
>
> So it's not at all clear to me if this is an issue or not. If ClassSymbols
> for classes in the JRT image which get passed to getSupplementaryFlags()
> always come from **this index instance**, then it should be okay, but
> otherwise it's subtly broken. It depends how people can define ClassSymbol
> instances, and what control they have over the paths of the file objects they
> use.
My feeling here is that this shouldn't matter.
First, I think the method seems consistent -- e.g. `isInJRT` is an instance
method on a JRTIndex, and returns true if the provided file is defined in the
file system backing _that specific_ JRTIndex. Which seems correct.
Second, in reality there's only one ClassFinder per javac instantiation. Which
means the class finder will either run in preview mode or not -- effectively
only seeing one type of file system. In other words, I don't think it's
possible for javac to witness different classfiles for the same symbol.
This might be possible in cases like combo tests, where we try to reuse the
existing compilation context for multiple compilation rounds (which allows us
to run thousands of javac compilation in seconds). But in that test framework,
whenever a "sensitive" option is detected, javac dutifully marks its context as
"not good for reuse", and so it throws it away and creates a new one (and a new
classfinder).
But, @lahodaj please chime in too
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1761#discussion_r2606254293
