[BUG]: ECJ passes invalid StandardLocations to methods in custom JavaFileManager implementations #3496
Comments
|
For me |
|
Would it be possible to have the latest fixed jar to double check this on the latest fixes? I checked the snapshots earlier today but those lacked the changes. |
The PR builds have been removed upon merge. But the following build should have the fix already: https://download.eclipse.org/eclipse/downloads/drops4/I20241229-0630/#JDTCORE |
|
Perfect, thanks. I have a feeling the forward slashes rather than periods may well be a bug on my side looking into this further, but will check and close the issue if this is the case. |
|
What error were you seeing? I am now seeing issues with MODULE_PATH being passed to functions that expect a package oriented class. |
ascopes
changed the title
|
On my side, I see Digging into the call site of this, I see the following: This is the erroneous call site from ECJ. The docs for JavaFileManager state that the call being made by ECJ is erroneous: https://docs.oracle.com/en/java/javase/11/docs/api/java.compiler/javax/tools/JavaFileManager.html#list(javax.tools.JavaFileManager.Location,java.lang.String,java.util.Set,boolean)
From this, my understanding is that Sounds like the implementation in ECJ needs to be updated to first call As a second note, this call is being made even when the source release is set to Java 8. It probably isn't important given Java 8 is now mostly considered antiquated, but it seems a little surprising that a Java 8 build is trying to resolve modules on the module path given that the expectation is that Java 8 would never support modules. Not sure if this is worth investigating or not as it shouldn't affect my use case. It may be worth updating the internal file manager that ECJ uses by default when being invoked from the commandline to throw an exception in these cases so other edge cases like this can be spotted when working on this fix, as it would avoid further follow-up issues for the same thing potentially. |
|
Additionally, I made the following observations just now:
|
ascopes
changed the title
ascopes
changed the title
|
Upon applying the following patch to the 153c153
< files = this.fileManager.list(this.location, qualifiedPackageName, this.fileTypes, false);
---
> files = list(qualifiedPackageName, this.fileTypes, false);
197c197
< for (JavaFileObject javaFileObject : this.fileManager.list(this.location, NO_PATH, Collections.singleton(JavaFileObject.Kind.SOURCE), false)) {
---
> for (JavaFileObject javaFileObject : list(NO_PATH, Collections.singleton(JavaFileObject.Kind.SOURCE), false)) {
204c204
< for (JavaFileObject javaFileObject : this.fileManager.list(this.location, NO_PATH, Collections.singleton(JavaFileObject.Kind.CLASS), false)) {
---
> for (JavaFileObject javaFileObject : list(NO_PATH, Collections.singleton(JavaFileObject.Kind.CLASS), false)) {
231c231
< Iterable<JavaFileObject> files = this.fileManager.list(this.location, qualifiedPackageName, this.fileTypes, false);
---
> Iterable<JavaFileObject> files = list(qualifiedPackageName, this.fileTypes, false);
239c239
< files = this.fileManager.list(this.location, qualifiedPackageName, this.fileTypes, true);
---
> files = list(qualifiedPackageName, this.fileTypes, true);
256c256
< for (JavaFileObject fileObject : this.fileManager.list(this.location, NO_PATH, this.fileTypes, true)) {
---
> for (JavaFileObject fileObject : list(NO_PATH, this.fileTypes, true)) {
361a362,389
> }
>
> private Set<JavaFileObject> list(String qualifiedPackageName, Set<JavaFileObject.Kind> fileTypes, boolean recursive) throws IOException {
> boolean hasModules = location.isModuleOrientedLocation();
> boolean hasPackages = location.isOutputLocation() || !hasModules;
>
> HashSet<JavaFileObject> fileObjects = new HashSet<>();
>
> if (location.isOutputLocation() || !location.isModuleOrientedLocation()) {
> for (JavaFileObject fileObject : this.fileManager.list(location, qualifiedPackageName, fileTypes, recursive)) {
> fileObjects.add(fileObject);
> }
> }
>
> // XXX(ascopes): Not sure if we can ever have both modules and packages in an output location together. For now, let's
> // ignore that edge case and just assume we can't. This should avoid introducing any new bugs as part of GH-3496. If this
> // becomes problematic, the empty check can be removed.
> if (location.isModuleOrientedLocation() && fileObjects.isEmpty()) {
> for (Set<JavaFileManager.Location> moduleLocationSet : this.fileManager.listLocationsForModules(location)) {
> for (JavaFileManager.Location moduleLocation : moduleLocationSet) {
> for (JavaFileObject fileObject : this.fileManager.list(moduleLocation, qualifiedPackageName, fileTypes, recursive)) {
> fileObjects.add(fileObject);
> }
> }
> }
> }
>
> return fileObjects;I believe this should be the expected behaviour. However, a new bug is hidden behind this, which appears to be that ECJ is passing class names to APIs that only expect package names.
The protected PackageBinding findPackage(char[] name, ModuleBinding module) {
return module.getVisiblePackage(CharOperation.arrayConcat(this.compoundName, name));
}
Edit: checked JLS, that confirms this hypothesis that package names cannot be substituted with fully qualified class names as there is no literature I can find that explicitly specifies packages and binary names are interchangable. @stephan-herrmann should I raise a new bug for that separately? |
Please create self containing error description. It's not clear from this issue alone what the problem is, how to reproduce. And how |
|
Please scroll down the thread to #3496 (comment) and consecutive comments, along with a suggested patch, and the reason for this, along with links to the JSR-199 documentation suggesting this behaviour. As stated in the previous thread, the mentioned package holds a conformant in-memory JavaFileManager implementation that is compatible with Javac. This is part of my library that provides developers of compiler extensions, annotation processors, and code generation tools with an API to produce fluent integration tests that calls JSR-199 compliant compilers with a virtual file system of sources, and assertj support for interrogating diagnostics and outputs. The issue has been raised as I have encountered several bugs around how ECJ interacts with such resources, which was where the original thread came from. In this case, the issue is around the fact ClasspathJsr199 passes invalid module-oriented Locations to JavaFileManager#list. I can provide instructions for reproducing the issue via my library, but a "minimal" reproduction will not be possible since that involves implementing JavaFileManager from scratch with JPMS support, and that is far from a minimal API to satisfy. At the simplest:
|
Follow up of #958 (@stephan-herrmann as requested).
See #3496 (comment) for description.
The text was updated successfully, but these errors were encountered: