RFR: 8373436: Cleanup JRTIndex usages [v2]

[David Beaumont]

On Thu, 11 Dec 2025 10:35:31 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> The `com.sun.tools.javac.file.JRTIndex` class serves two purposes: a) the keeps a package-oriented cache of directories and files inside the JRT FileSystem; b) allows to get the additional flags for classes in packages on the default classpath. Sadly, it is not easy to completely disentangle these two purposes, as the directories cache also holds the additional flags.
>> 
>> The `JRTIndex` is used on two places, the `ClassFinder`, and in `JavacFileManager`, but the former is only using the additional flags, and the latter is using the cache/index. The logic determining whether the `JRTIndex` is used in `ClassFinder` is also not trivial.
>> 
>> The goal of this PR is to attempt to clean this a bit:
>> - there's a separate abstraction for getting the additional flags (`LegacyCtPropertiesAccess`), with the non-trivial implementation residing in `JavacFileManager`. This allows to concentrate the complex almost completely  inside the file manager.
>> - the `JRTIndex` itself is now a package-private class inside the `....file` package.
>> 
>> It is not a goal of this PR to change the behavior, just make the implementation more understandable.
>> 
>> Note this removal from `ClassFinder`:
>> 
>> -        if (fm instanceof DelegatingJavaFileManager delegatingJavaFileManager) {
>> -            fm = delegatingJavaFileManager.getBaseFileManager();
>> -        }
>> 
>> 
>> The `DelegatingJavaFileManager` is used when `--release` is specified, so this allows to get the underlying base file manager. But, regardless of the underlying base manager, the system classes should always come from the `--release` data, which are not in the underlying base manager, and hence (ultimately), the `JRTIndex` data should not be used by `ClassFinder` when `--release` is used. The removal makes this more obvious.
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Cleanup: JRTIndex implements LegacyCtPropertiesAccess directly, as suggested.

Looks good. A couple of ideas about potentially better encapsulation. Nothing blocking committing though.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/ClassFinder.java line 207:

> 205: 
> 206:         JavaFileManager fm = context.get(JavaFileManager.class);
> 207:         if (fm instanceof JavacFileManager javacFileManager) {

Optional: Feels a bit odd to have one case here for returning a no-op, and one case inside `getLegacyCtPropertiesInfo()`. Obviously this method only exists on `JavacFileManager`, so we can't trivially fold this test into the method, but we could have a `JavacFileManager` static helper taking in a `JavaFileManager`.


/**
 * Returns the legacy <maybe say what 'ct' is here> properties supplier from the given file
 * manager, or a "no-op" instance if legacy properties are not available.
 */
public static LegacyCtPropertiesAccess getLegacyCtPropertiesInfo(JavaFileManager fm) {
}


This puts all the no-op logic in one place and lets the caller assign on declaration of `legacyCtPropertiesInfoAccess`, keeping the "legacy stuff" a bit more encapsulated.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/ClassFinder.java line 250:

> 248:                 ModuleSymbol owningModule = packge.modle;
> 249:                 if (owningModule == syms.noModule) {
> 250:                     LegacyCtPropertiesInfo info = legacyCtPropertiesInfoAccess.getInfo(packge.flatName());

Optional nit: If we are changing the name here, I might suggest `legacyInfo` just to keep the "it's legacy stuff" obvious.

src/jdk.compiler/share/classes/com/sun/tools/javac/file/LegacyCtPropertiesAccess.java line 74:

> 72:      * The info that used to be in ct.sym for classes in a package.
> 73:      */
> 74:     public static class LegacyCtPropertiesInfo {

If we are moving CySym into its own type, and thus needing to hit all users anyway, maybe (depending on the effort) now would be a time to switch it to getter methods instead of public fields.

Additionally, could getCtInfo() from `JrtIndex` reasonably by moved here, probably allowing the constructor and the EMPTY instance to be made private?

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

Marked as reviewed by david-beaumont at github.com (no known OpenJDK username).

PR Review: https://git.openjdk.org/jdk/pull/28761#pullrequestreview-3577671583
PR Review Comment: https://git.openjdk.org/jdk/pull/28761#discussion_r2619037115
PR Review Comment: https://git.openjdk.org/jdk/pull/28761#discussion_r2619010624
PR Review Comment: https://git.openjdk.org/jdk/pull/28761#discussion_r2619072430