eclipse-jdt / eclipse-jdt/eclipse.jdt.core
Calls from ClasspathJrt* into JRTUtil should respect --release
- Dominant language
- Java
- Stars
- 237
- Forks
- 195
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 47
Description
Class `JRTUtil` contains several static methods, all starting with `getJrtSystem(jrt)` and then delegating to that JrtFileSystem. Calling the single arg variant implies that `null` will be passed as the release.
For all call paths from any of the `ClasspathJrt*` classes, this ignores release information that might be present in the caller. This is relevant for all methods that are *not overridden* in `ClasspathJep247*` / `ClasspathJrtWithReleaseOption`.
I suggest to let each of `ClasspathJrt*` store a link to the one corresponding `JrtFileSystem`:
* those instances have a long life time anyway via static `JRTUtil images`, so we are not increasing "permanent" heap usage.
* holding a reference in a field avoids overhead from repeatedly calling `JRTUtil.getJrtSystem(..)` which could be relevant due to the file access inside `readJdkReleaseFile()`
* at the same time this reduces the number of calls into static methods of `JRTUtil`, which brings us closer to verifying that the remaining calls actually provide the `release` argument whenever suitable.
For a straight forward example see `ClasspathJrt.hasCompilationUnit(..)` (in both variants of this class): we call into `JRTUtil` without passing release information, *even when* `this` is in fact `ClasspathJep247*` or `ClasspathJrtWithReleaseOption`.
For the sibling case of `isPackage()` it should be possible to construct a test where `o.e.j.i.c.builder.ClasspathJrtWithReleaseOption.findClass(String, String, String, String, boolean, Predicate)` would not find a class that exists in the specified release, simply because the package has been removed in the current JDK, but release information is ignored during the `isPackage()` check (I just don't have an example at hand, where a *package* was removed, has this happened for a public package, yet? Note that --add-exports is off limits with --release :) ).
Contributor guide
Assessment
This issue has not been assessed yet.