On Wed, 10 Dec 2025 11:19:40 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> 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
In practice, as long as the `JavacFileManager` and `ClassFinder` agree on the
`JRTIndex` instance (which they should), then it should be fine checking that
the file originates in the one specific instance of JRT FS that is held by the
`JRTIndex`. If the file manager is not a JavacFileManager, `jrtIndex` will be
`null`, so custom file managers are ruled out here, I think.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1761#discussion_r2606723491
