eclipse-jdt / eclipse-jdt/eclipse.jdt.core

Calls from ClasspathJrt* into JRTUtil should respect --release

Open
#238 3 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.