Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] [JSR-199] ECJ cannot resolve JPMS modules if using a user-provided JavaFileManager #958

Closed
ascopes opened this issue Apr 10, 2023 · 47 comments · Fixed by #3446
Closed
Assignees
Milestone

Comments

@ascopes
Copy link
Contributor

ascopes commented Apr 10, 2023

It appears that when using ClasspathJSR199 (i.e. when a provided file manager does not implement StandardFileManager, only JavaFileManager), using a module-info.java in the root of the compilation sources does not work correctly. Automatic modules that are mentioned in requires entries are not being added to the module lookup table within the compiler, leading to errors such as the following:

module org.example {
  exports org.example;

  requires io.avaje.inject;  /* automatic module that is a JAR on both the CLASS_PATH and MODULE_PATH */

  provides io.avaje.inject.spi.Module with org.example.ExampleModule;
}
 - [ERROR] 8389908
   
       Cannot bind message for problem (id: 1300) "{0} cannot be resolved to a module" with arguments: {}

 - [ERROR] 16777218
   
       io cannot be resolved to a type

 - [ERROR] 268435846
   
       The import jakarta cannot be resolved

 - [ERROR] 16777218
   
       Singleton cannot be resolved to a type

 - [ERROR] 268435846
   
       The import jakarta cannot be resolved

This appears to be some sort of bug, since I can get the exact same configuration to compile successfully under Javac using the exact same configuration. If I remove the module-info.java from my test project, then this will get past this step, so it seems to be a bug in the module resolution in ECJ somewhere.

ECJ appears to be using a ClasspathJsr199 object if the FileManager is not an instance of EclipseFileManager or StandardFileManager, and my guess is that when using this flow rather than the default file manager the compiler would usually create when invoked as a standalone process, the wrong logic is being invoked to handle JARs that have an Automatic-Module-Name entry in their META-INF/MANIFEST.MF.

Thus, to replicate this, you'll need to use a JavaFileManager object that does not implement EclipseFileManager or StandardFileManager, otherwise this logic appears to be skipped.


Minimal working example (Maven project)

ecj-bugs.tar.gz

./mvnw clean package and observe the failure.

This fails because ECJ doesn't seem to support automatic module detection properly in the ClasspathJsr199 class, and this is only enabled if the StandardJavaFileManager ECJ provides is not used by the looks.

I tricked ECJ into doing this by hiding the regular standard file manager behind one that does not implement StandardJavaFileManager at all.

@ascopes ascopes changed the title ClasspathJSR199 does not resolve automatic module members ECJ fails to resolve Automatic Modules in minimal working example Apr 15, 2023
@ascopes ascopes changed the title ECJ fails to resolve Automatic Modules in minimal working example ECJ fails to resolve any Automatic Modules Apr 15, 2023
@ascopes
Copy link
Contributor Author

ascopes commented Apr 15, 2023

Updated issue with simple working reproduction now. Can confirm it affects the ECJ file manager if you wrap it in a JavaFileManager delegate that doesn't implement StandardJavaFileManager.

@ascopes ascopes changed the title ECJ fails to resolve any Automatic Modules ECJ fails to resolve any modules Apr 30, 2023
@ascopes ascopes changed the title ECJ fails to resolve any modules ECJ cannot resolve modules if using a JavaFileManager Apr 30, 2023
@ascopes
Copy link
Contributor Author

ascopes commented Apr 30, 2023

The issue seems to be that ClasspathJsr199 just delegates to the JRT to find any modules, meaning that a ClasspathJsr199 will never report that it holds modules. Seems the EclipseFileManager deals with this logic in a special way normally, which is not going to be applicable to custom file manager implementations.

Ideally, I would probably expect ClasspathJsr199 to be using the ModuleFinder to find the appropriate modules and automatic modules, or to use the file manager itself with listLocationForModules.

@ascopes ascopes changed the title ECJ cannot resolve modules if using a JavaFileManager [JSR-199] ECJ cannot resolve JPMS modules if using a user-provided file manager Apr 30, 2023
@ascopes ascopes changed the title [JSR-199] ECJ cannot resolve JPMS modules if using a user-provided file manager [BUG] [JSR-199] ECJ cannot resolve JPMS modules if using a user-provided file manager Jun 24, 2023
@ascopes
Copy link
Contributor Author

ascopes commented Jul 19, 2024

Bumping to prevent this being closed from inactivity, as it is still an outstanding issue.

@ascopes
Copy link
Contributor Author

ascopes commented Dec 11, 2024

Bump

@srikanth-sankaran
Copy link
Contributor

@stephan-herrmann - Thanks for investigating

@stephan-herrmann
Copy link
Contributor

From a first analysis I don't see, how we could possibly obtain the required information about modules on the build path if the file manager is not a StandardJavaFileManager. The plain JavaFileManager simply doesn't have the methods we otherwise use to properly initialize all locations.

If you change all occurrences of JavaFileManager to StandardJavaFileManager then the auto-module is correctly interpreted.

Seems the EclipseFileManager deals with this logic in a special way normally, which is not going to be applicable to custom file manager implementations.

It should work for custom implementations implementing StandardJavaFileManager.

Ideally, I would probably expect ClasspathJsr199 to be using the ModuleFinder to find the appropriate modules and automatic modules,

From what information should we get/configure a ModuleFinder? I can't find any method in JavaFileManager that would, e.g., provide a Path for use by ModuleFinder.of().

or to use the file manager itself with listLocationForModules.

javaFileManager.listLocationsForModules(StandardLocation.MODULE_PATH) answers a singleton list containing an empty set.

To me all this smells like a won't fix, unless you can demonstrate that the interface JavaFileManager suffices to find modules.

@ascopes
Copy link
Contributor Author

ascopes commented Dec 12, 2024

I suppose my question would be from this that this works with javac, my library at https://github.com/ascopes/java-compiler-testing demonstrates that, as its entire purpose is to provide an integration testing harness for JSR 199 compliant compilers.

The library uses a javac-compliant JSR 199 JavaFileManager at https://github.com/ascopes/java-compiler-testing/blob/main/java-compiler-testing/src/main/java/io/github/ascopes/jct/filemanagers/impl/JctFileManagerImpl.java that enables Javac to fully utilise a Path-based FileSystem outside the default file system.

I was hoping to also support ECJ but I had to shelve that idea as the issues posed within this issue result in ECJ being incompatible with the JavaFileManager for module support (that and lack of Java 11 support now which poses another challenge for me - something I should be able to work around if this is fixed - but that came after I filed the original issues).

The expectation is that javaFileManager.listLocationsForModules(StandardLocation.MODULE_PATH) should not return an empty set if the module path is available correctly, and the compiler relies on that to provide all access to module contents, in the same way javac does. The problem seems to be that ECJ attempts to interact with the file system outside the APIs provided by JavaFileManager when StandardJavaFileManager is not in use (which is probably a bug, since it works on Javac with jigsaw modules as expected).

It'd be a shame if this was a wont-fix as ECJ support has been something I've wanted to provide for a very long time now so that consumers can dynamically verify annotation processors work correctly on both javac and ecj.

I'll admit it has been roughly 20 months since I filed this issue, so it will take me time to become familiar again with the details of this that I provided beforehand.

I think a good starting point would be comparing the differences between how ECJ is handling this and how Javac is, as I appreciate how this works is a bit of a minefield from my own experience with javac. I can try to revive my branch that implemented an ECJ backend for this library if it provides any assistance on this, as I also provide access to internals that report all interactions with the file manager for diagnostics.

It is worth noting I still encounter problems if I use the default file system (tempfs). I have a very ancient branch at https://github.com/ascopes/java-compiler-testing/tree/task/ecj which demonstrates the issues I encountered at the time.

@ascopes
Copy link
Contributor Author

ascopes commented Dec 12, 2024

I've rerun an old build at https://github.com/ascopes/java-compiler-testing/actions/runs/12292635044/job/34303785775#step:6:3227 which should demonstrate some of these differences under the same file manager implementation for javac and ecj if this is any help?

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Dec 12, 2024
file manager eclipse-jdt#958

ClasspathJsr199:
+ various drafty additions for handling modules
ModuleFinder:
+ find automatic modules, too
+ allow invocation without a parser for binary-only scans
EclipseFileManager:
+ allow scanning all packages (by specifying an empty package name)
+ establish symmetry between setLocation() and setLocationFromPaths()
+ catch disruptive IAE from ModuleFinder.scanForModule()
@stephan-herrmann
Copy link
Contributor

stephan-herrmann commented Dec 12, 2024

Despite the connection to JPMS much of this issue actually is beyond my expertise. So in my first analysis I only saw that for the newer protocol ecj works fine, and didn't see where the necessary information should come from using the old protocol. (assuming JavaFileManager = old, StandardJavaFileManager = new, which may be wrong, too).

On second look we had lots of code locations that simply didn't pass on necessary information.

@jarthana with my lack of background regarding JSR 199, the provided draft PR #3446 could use some help. I'm not even sure if the general direction is good. Additionally, I'll comment on some known weaknesses of the PR.

@stephan-herrmann
Copy link
Contributor

PS: Apologies for re-iterating many findings that were already stated in the issue description and previous comments. Looking at EclipseCompilerImpl.handleLocations() I simply had no clue, how information is supposed to flow back and forth between compiler and file manager. The fact that even our own file manager drops module information on one code path didn't help either. ;-P

@ascopes
Copy link
Contributor Author

ascopes commented Dec 12, 2024

No worries, thanks for taking the time to look into it! It is greatly appreciated.

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Dec 14, 2024
file manager

+ change to 1 ClasspathJsr199 per module
+ test with two module dependencies (one auto, one regular)

Fixes eclipse-jdt#958
@ascopes
Copy link
Contributor Author

ascopes commented Dec 15, 2024

I've revived a new version of my branch that integrated with ECJ in the past...

https://github.com/ascopes/java-compiler-testing/tree/task/ecj-2024

... currently the tests are failing due to the fact that ECJ is passing a module-oriented MODULE_PATH location around, rather than passing in the expected locations derived from the MODULE_PATH. This seems to be a violation of the original JSR-199 spec, since I enforce this check in three locations within the JavaFileManager:

  • public String inferBinaryName(Location location, JavaFileObject file)
  • public String inferModuleName(Location location)
  • public Set<JavaFileObject> list(Location location, String packageName, Set<Kind> kinds, boolean recurse) throws IOException

Each of these three methods in the JDK are documented as throwing an IllegalArgumentException when being passed a module-oriented location, so is most likely a separate bug in ECJ. That being said, regardless of this, I don't believe it makes sense for ECJ to be passing a module-oriented location to any of these methods, since the module-oriented locations are just a shim around actual modules that could in theory live in a totally different location on the system depending on the JavaFileManager implementation in use. It would appear that the StandardJavaFileManager implementation being used with ECJ is just overly permissive in ignoring invalid calls in this manner.

This may require walking through these errors on ECJ side until I can get the newest version failing on this same issue to reproduce. If you need a project to reproduce these issues on though, you should be able to build that repository on the given branch on Java 17-Java 23.

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Dec 15, 2024
file manager eclipse-jdt#958

ClasspathJsr199:
+ various drafty additions for handling modules
ModuleFinder:
+ find automatic modules, too
+ allow invocation without a parser for binary-only scans
EclipseFileManager:
+ allow scanning all packages (by specifying an empty package name)
+ establish symmetry between setLocation() and setLocationFromPaths()
+ catch disruptive IAE from ModuleFinder.scanForModule()
stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Dec 15, 2024
file manager

+ change to 1 ClasspathJsr199 per module
+ test with two module dependencies (one auto, one regular)

Fixes eclipse-jdt#958
@stephan-herrmann
Copy link
Contributor

... currently the tests are failing

Which version of ECJ did you use? Was 477cab7 included? Shouldn't that fix the very problem you are describing?

@stephan-herrmann
Copy link
Contributor

This seems to be a violation of the original JSR-199 spec,

Do you know of any complete specification? Or is javadoc spread over various types all we have?

@ascopes
Copy link
Contributor Author

ascopes commented Dec 16, 2024

The version I was using was the most recent tag on Maven central so will not include the fix yet, I do not believe. Wanted to just get something up that could reproduce the issue again as my branch was nearly two years stale.

Regarding spec docs... there isn't much... however the JavaDocs document that JFM should throw an exception if module oriented paths are provided to certain methods, such as inferBinaryName. Other methods like inferModuleName somewhat unhelpfully do not document an exception being raised, but they do specify that the input should be a value supplied by the listLocationsForModule call, which is not module-oriented (since the locations are for each module rather than the group of modules as a whole), so in my case I have handled this defensively on my side rather than straying into the realm of potentially undefined behaviour.

@stephan-herrmann
Copy link
Contributor

Latest logs.

This link gives me an Error "AuthenticationFailed".

@ascopes
Copy link
Contributor Author

ascopes commented Dec 27, 2024

@stephan-herrmann
Copy link
Contributor

Oh thats odd, must have timed out.

Can you see https://github.com/ascopes/java-compiler-testing/actions/runs/12514852290/job/34911552087?pr=773 ?

Yes, that link works, thanks.

@stephan-herrmann
Copy link
Contributor

[INFO] [ERROR]   AvajeHttpTest.httpClientCodeGetsGeneratedAsExpected:47 [Expected all paths in ["org/example/httpclient/GeneratedHttpComponent.class", "org/example/httpclient/UserApiHttpClient.class"] to exist but one or more did not] 
[INFO] The following 2 assertions failed:
[INFO] 1) No file with relative path matching "org/example/httpclient/GeneratedHttpComponent.class" was found. Maybe you meant:
[INFO]   - "org.example"
[INFO]   - "org.example/module-info.class"
[INFO]   - "org.example/org/example"
[INFO]   - "org.example/org/example/User.class"
[INFO]   - "org.example/org/example/httpclient/GeneratedHttpComponent.class"

This is the kind of errors I'm getting with your new branch and my locally produced ecj.jar

Seems that ECJ is emitting multi-source outputs rather than single-source outputs in the presence of JPMS modules. Not 100% sure if that is a problem or expected behaviour.

We ask fileManager.getLocationForModule(StandardLocation.CLASS_OUTPUT, new String(modName)); where JctFileManager answers a ModuleLocation. Next fileManager.getJavaFileForOutput(location, ..) gives a PathFileObjectImpl with a path ending, e.g., in "/org.example/org/example/Pump.class". Are you saying we should not ask getLocationForModule? If so, which API should we use to determine whether or no getLocationForModule() should be used?

@ascopes
Copy link
Contributor Author

ascopes commented Dec 27, 2024

If I turn on verbose file manager interceptor logs and run the same test for just Java 17 for both ECJ and Javac, I get the following output describing the process of the function calls:

ecj.log
javac.log

From this, it appears javac does not make the following calls I can see in the ECJ log:

[ForkJoinPool-1-worker-2] DEBUG io.github.ascopes.jct.filemanagers.impl.LoggingFileManagerProxy - >>> [thread=19] boolean hasLocation(Location) called with (CLASS_OUTPUT)
[ForkJoinPool-1-worker-2] DEBUG io.github.ascopes.jct.filemanagers.impl.LoggingFileManagerProxy - <<< [thread=19] boolean hasLocation(Location) returned true
[ForkJoinPool-1-worker-2] DEBUG io.github.ascopes.jct.filemanagers.impl.LoggingFileManagerProxy - >>> [thread=19] Location getLocationForModule(Location, String) called with (CLASS_OUTPUT, org.example)
[ForkJoinPool-1-worker-2] DEBUG io.github.ascopes.jct.filemanagers.impl.LoggingFileManagerProxy - <<< [thread=19] Location getLocationForModule(Location, String) returned ModuleLocation{parent=CLASS_OUTPUT, moduleName="org.example"}

My understanding of how Javac deals with this is that it only emits class outputs as module locations (i.e. via getLocationForModule) if the inputs were provided via module locations (in this case, this should be able to be determined via the APIs used to read the sources to begin with, and stored as a variable to track this intention somewhere).

For source inputs, that is determined simply by whether or not the inputs came from StandardLocation.SOURCE_PATH (legacy output) or StandardLocation.MODULE_SOURCE_PATH (named-module output).

For source outputs that are being recompiled, this should be able to be determined by whether the source outputs themselves were in a modular structure or not when they were produced.

I cannot see ECJ making any calls to read the source outputs after they are produced (via the file manager) so I assume it is accessing those file contents in a different way, but it should be able to be inferred by whether or not the outputs were within a nested module location or not which could possibly be achieved by checking the Location object corresponding to the file object being written out.

Looking at javax.annotation.processing.Filer#createSourceFile, which is used to create the source outputs, the documentation says the following:

     * The file's name and path (relative to the {@linkplain
     * StandardLocation#SOURCE_OUTPUT root output location for source
     * files}) are based on the name of the item to be declared in
     * that file as well as the specified module for the item (if
     * any).
...
     * <p>The optional module name is prefixed to the type name or
     * package name and separated using a "{@code /}" character. For
     * example, to create a source file for class {@code a.B} in module
     * {@code foo}, use a {@code name} argument of {@code "foo/a.B"}.
     *
     * <p>If no explicit module prefix is given and modules are supported
     * in the environment, a suitable module is inferred. If a suitable
     * module cannot be inferred {@link FilerException} is thrown.
     * An implementation may use information about the configuration of
     * the annotation processing tool as part of the inference.

In Javac, the "inference" of what constitutes a "suitable module name" is defined here: https://github.com/openjdk/jdk/blob/807f6f7fb868240cba5ba117c7059216f69a53f9/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacFiler.java#L446-L485. It seems to hand itself off to a function defined at https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacFiler.java#L686-L703 which appears to use the default "legacy" module location (i.e. "dump it in the output location directly rather than a named module sublocation via getLocationForModule") if no other explicit modules were already defined in the same place.

In ECJ, I can see the following snippet at https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/apt/dispatch/BatchFilerImpl.java#L140 performs the check on ECJ side, and it only considers a named module to have been defined if there is a forward-slash in the binary name that is passed in. I assume this information should then be able to be stored and tracked by whatever ECJ is using to "read" the file contents when it runs the second compilation path.

Perhaps the check in this snippet could be enhanced to track this information?

Location location = mod == null ? StandardLocation.SOURCE_OUTPUT : this._fileManager.getLocationForModule(StandardLocation.SOURCE_OUTPUT, mod);
JavaFileObject jfo = this._fileManager.getJavaFileForOutput(location, name.toString(), JavaFileObject.Kind.SOURCE, null);

My guess is you could store this information within https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/apt/dispatch/HookedJavaFileObject.java#L36

Another approach possibly could be to just replace .java with .class on the file name of that hooked java file object? I assume that'd take in to account the module prefix as well?

I believe you could then pass the module name into the compilation unit directly which you create at https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/apt/dispatch/HookedJavaFileObject.java#L228. This is always set to null right now so I'd assume all outputs should never have a named module associated with them unless ECJ is just ignoring this elsewhere?

I'll admit I am not familiar with this code though so this might be total nonsense.


Edit: I used the following patch on acbca4e8f776d94d858134e385dcac344e14905e to get this log output if you wish to reproduce it locally.

diff --git a/java-compiler-testing/src/it/avaje-http/src/test/java/io/github/ascopes/jct/acceptancetests/avajehttp/AvajeHttpTest.java b/java-compiler-testing/src/it/avaje-http/src/test/java/io/github/ascopes/jct/acceptancetests/avajehttp/AvajeHttpTest.java
index 822d57da..d91cddcf 100644
--- a/java-compiler-testing/src/it/avaje-http/src/test/java/io/github/ascopes/jct/acceptancetests/avajehttp/AvajeHttpTest.java
+++ b/java-compiler-testing/src/it/avaje-http/src/test/java/io/github/ascopes/jct/acceptancetests/avajehttp/AvajeHttpTest.java
@@ -18,6 +18,7 @@ package io.github.ascopes.jct.acceptancetests.avajehttp;
 import static io.github.ascopes.jct.assertions.JctAssertions.assertThatCompilation;
 
 import io.github.ascopes.jct.compilers.JctCompiler;
+import io.github.ascopes.jct.filemanagers.LoggingMode;
 import io.github.ascopes.jct.junit.EcjCompilerTest;
 import io.github.ascopes.jct.junit.JavacCompilerTest;
 import io.github.ascopes.jct.workspaces.Workspaces;
@@ -27,8 +28,8 @@ import org.junit.jupiter.api.DisplayName;
 class AvajeHttpTest {
 
   @DisplayName("HTTP client code gets generated as expected")
-  @EcjCompilerTest(minVersion = 11)
-  @JavacCompilerTest(minVersion = 11)
+  @EcjCompilerTest(minVersion = 17, maxVersion = 17)
+  //@JavacCompilerTest(minVersion = 17, maxVersion = 17)
   void httpClientCodeGetsGeneratedAsExpected(JctCompiler compiler) {
     // Given
     try (var workspace = Workspaces.newWorkspace()) {
@@ -38,6 +39,7 @@ class AvajeHttpTest {
 
       // When
       var compilation = compiler
+          .fileManagerLoggingMode(LoggingMode.ENABLED)
           .compile(workspace);
 
       // Then
diff --git a/pom.xml b/pom.xml
index 2ac642de..5a97fd16 100644
--- a/pom.xml
+++ b/pom.xml
@@ -126,8 +126,8 @@
     <maven-surefire-junit5-tree-reporter.version>1.4.0</maven-surefire-junit5-tree-reporter.version>
 
     <!-- Log verbosity -->
-    <hide-test-logs-in-console>true</hide-test-logs-in-console>
-    <test-log-verbosity>INFO</test-log-verbosity>
+    <hide-test-logs-in-console>false</hide-test-logs-in-console>
+    <test-log-verbosity>DEBUG</test-log-verbosity>
 
     <!--
       This argument is pulled in by Surefire and Failsafe, and JaCoCo will amend it to 

I then ran ./mvnw integration-test -Dmaven.test.skip -Dinvoker.test=avaje-http to see the logs.

@ascopes
Copy link
Contributor Author

ascopes commented Dec 27, 2024

As a side note, it may be worth separately investigating why ECJ flushes the file manager 25 times in a row at the end of the compilation too, as that possibly isn't necessary. I can see javac does it twice, but I assume that is also technically a bit iffy.

@ascopes ascopes changed the title [BUG] [JSR-199] ECJ cannot resolve JPMS modules if using a user-provided file manager [BUG] [JSR-199] ECJ cannot resolve JPMS modules if using a user-provided JavaFileManager Dec 28, 2024
ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 28, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
@stephan-herrmann
Copy link
Contributor

My understanding of how Javac deals with this is that it only emits class outputs as module locations (i.e. via getLocationForModule) if the inputs were provided via module locations

I doubt that this behavior is backed by any specification.

Doc of Filer.createClassFile(CharSequence, Element...) requires that a module prefix be used if "modules are supported in the environment". This precondition is satisfied. In the sequel either a module prefix should be used or a FilerException be thrown.

There still seems to be a bit of a gap: JLS 7.2 specifies: " Each host system also determines which observable compilation units are associated with a module." In my understanding a file manager is such a "host system" and hence the API should unambiguously clarify how such association to modules happens. I couldn't find where this would be specified. Perhaps I missed the correct location.

Another strangeness: A Location defines whether it is module-oriented or package-oriented. We could guess that only module-oriented output locations "support modules", as required by Filer.createClassFile(). If I see correctly, we get the output location using fileManager.getLocationForModule(StandardLocation.CLASS_OUTPUT, new String(modName)). Now if CLASS_OUTPUT should not support modules, then I'd expect this call to return null.

At this point I'm not convinced that ECJ has a bug. Perhaps the test expectation is too narrow vis-a-vis a more permissive specification?

@ascopes
Copy link
Contributor Author

ascopes commented Dec 28, 2024

Perhaps this needs to be raise with the team that maintain Javac for further clarification? The difference in behaviour between ECJ and Javac for this makes it difficult to be able to use ECJ as a drop in replacement, which I assume is desirable behaviour if using Eclipse as an IDE, and is desirable if using ECJ with tools like Maven that expect a specific output layout.

My confusion on this comes from the fact inputting

src/
  module-info.java
  org/
    example/
      Foo.java

will result in the following output:

out/
  my.modulename.here/
    module-info.class
    org/
      example/
        Foo.class

rather than

out/
  module-info.class
  org/
    example/
      Foo.class

but not consistently (as the other tests have shown). If there is a clear spec for how I need to deal with this then I can adjust my implementation, but I am nervous about breaking the existing working implementation for Javac if that should be considered the reference implementation.

I don't think the issue is that CLASS_OUTPUT does or does not support modules, as the spec dictates it always does. The problem seems to be that ECJ promotes sources that were not in a module location to begin with into an output class that is within a module location, purely due to the presence of a module-info.java being present somewhere.

I might be wrong like you say though. I'll have a fiddle with it locally and see what I can come up with.

In the mean time, would the issue regarding flushing be worth raising separately?

ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 28, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
@ascopes
Copy link
Contributor Author

ascopes commented Dec 28, 2024

Ok, on the original topic, I think I've found a new edge case that is still not working.

My test case that tests I can compile one set of files and then use them in a second separate compilation appears to be falling over even outside the case of using module-info.java.

  @DisplayName(
      "I can compile sources to classes and provide them in the classpath to a second compilation"
  )
  @EcjCompilerTest
  @JavacCompilerTest
  void compileSourcesToClassesAndProvideThemInClassPathToSecondCompilation(
      JctCompiler compiler
  ) {
    try (
        var firstWorkspace = Workspaces.newWorkspace();
        var secondWorkspace = Workspaces.newWorkspace()
    ) {
      firstWorkspace
          .createSourcePathPackage()
          .createDirectory("org", "example", "first")
          .copyContentsFrom(resourcesDirectory().resolve("first"));

      var firstCompilation = compiler.compile(firstWorkspace);
      assertThatCompilation(firstCompilation)
          .isSuccessfulWithoutWarnings()
          .classOutputPackages()
          .fileExists("org", "example", "first", "Adder.class")
          .isRegularFile()
          .isNotEmptyFile();
      
      secondWorkspace.addClassPathPackage(firstWorkspace.getClassOutputPackages().get(0).getPath());
      secondWorkspace.createSourcePathPackage()
          .createDirectory("org", "example", "second")
          .copyContentsFrom(resourcesDirectory().resolve("second"));

      var secondCompilation = compiler.compile(secondWorkspace);

      secondWorkspace.dump(System.out);
      firstWorkspace.dump(System.out);
      
      assertThatCompilation(secondCompilation)
          .isSuccessfulWithoutWarnings()
          .classOutputPackages()
          .fileExists("org", "example", "second", "Main.class")
          .isRegularFile()
          .isNotEmptyFile();
    }
  }

The first workspace has this structure:

WorkspaceImpl{id="ab643d78-a0d4-460c-b397-e372a28f0e0a", pathStrategy=RAM_DIRECTORIES, closed=false, numberOfLocations=4}
  Location CLASS_OUTPUT: 
    - memory:36fe3742-7c23-4111-90f6-ce95f1c70c22:///CLASSOUTPUTc7f1902f-5134-4cf8-bdbf-b153501a4eec/ contents:
        org/
        ··example/
        ····first/
        ······Adder.class

  Location SOURCE_OUTPUT: 
    - memory:2672d32b-b721-409a-9a54-f6320bb58d94:///SOURCEOUTPUT806d56d9-3de5-4ac5-9a38-70273aa494ab/ contents:

  Location SOURCE_PATH: 
    - memory:88786661-b3e4-4d56-8def-bd7f80786361:///SOURCEPATH1970f858-9869-4224-bdad-6300ae45c4df/ contents:
        org/
        ··example/
        ····first/
        ······Adder.java

  Location NATIVE_HEADER_OUTPUT: 
    - memory:a32d9479-a8dc-4e20-9999-86c61f8a3c1a:///NATIVEHEADEROUTPUT98d0d5fa-4790-4319-ab7b-9724789edfd3/ contents:

The second workspace has the following structure:

WorkspaceImpl{id="0cec0e8b-8ebf-4df0-a520-5c6f1299ee54", pathStrategy=RAM_DIRECTORIES, closed=false, numberOfLocations=5}
  Location CLASS_OUTPUT: 
    - memory:80361ad6-a750-43a3-9648-4934f78524f3:///CLASSOUTPUT536dbed6-bb4f-4cc7-8590-80effdc043a8/ contents:

  Location SOURCE_OUTPUT: 
    - memory:eba6aa76-9407-4be6-a243-f6f18b07714c:///SOURCEOUTPUTd16196cb-62ed-4e08-b495-068645c83241/ contents:

  Location CLASS_PATH: 
    - memory:36fe3742-7c23-4111-90f6-ce95f1c70c22:///CLASSOUTPUTc7f1902f-5134-4cf8-bdbf-b153501a4eec/ contents:
        org/
        ··example/
        ····first/
        ······Adder.class

  Location SOURCE_PATH: 
    - memory:5fd3447b-62a2-4fec-abca-1861af4319c9:///SOURCEPATHe25ef220-8737-45c6-aaf9-b6b1eaa2b1e2/ contents:
        org/
        ··example/
        ····second/
        ······Main.java

  Location NATIVE_HEADER_OUTPUT: 
    - memory:06e6e007-6d61-43a8-b0dc-7c282c938593:///NATIVEHEADEROUTPUT05f69667-da1f-45ce-bad3-8b43f4e4024b/ contents:

When I compile with -release 8, everything works. As soon as -release 9 or newer is used, it falls over... so it suggests this may be related to how JPMS is managed (even though JPMS is not actually in use here).

java.lang.AssertionError: Expected a successful compilation, but it failed.

Diagnostics:

 - [ERROR] 268435846
   
   The import org.example.first cannot be resolved

 - [ERROR] 16777218
   
   Adder cannot be resolved to a type

 - [ERROR] 16777218
   
   Adder cannot be resolved to a type

Reproduction: ./mvnw clean integration-test -Dinvoker.skip -Dtest=MultiTieredCompilationIntegrationTest

ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 28, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
@stephan-herrmann
Copy link
Contributor

stephan-herrmann commented Dec 28, 2024

Reproduction: ./mvnw clean integration-test -Dinvoker.skip -Dtest=MultiTieredCompilationIntegrationTest

I can reproduce.

@ascopes remote debugging doesn't seem to be prepared for this test?

ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 28, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
@ascopes
Copy link
Contributor Author

ascopes commented Dec 28, 2024

@stephan-herrmann this test is part of the surefire tests rather than those bundled with maven-invoker-plugin (which I use when testing against other third party libraries just to keep my main project pom clear).

./mvnw clean test -Dinvoker.skip -Dtest=MultiTieredCompilationIntegrationTest -Dmaven.surefire.debug="-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=localhost:5005"

Should do the trick (or just debug directly from your IDE if not, it should work there too).

You probably want to repull that branch again though as I've just pushed some stuff to it that fixes a license header issue that might kill the build prematurely for you.

ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 29, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 29, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
@stephan-herrmann
Copy link
Contributor

Ok, on the original topic, I think I've found a new edge case that is still not working.

My test case that tests I can compile one set of files and then use them in a second separate compilation appears to be falling over even outside the case of using module-info.java.
...

When I compile with -release 8, everything works. As soon as -release 9 or newer is used, it falls over... so it suggests this may be related to how JPMS is managed (even though JPMS is not actually in use here).

For me it fails even at --release 8. When we call fileManager.list(CLASSPATH, "org/example/first", ...) we get an empty result. Not sure where the contents gets lost.

@stephan-herrmann stephan-herrmann added this to the 4.35 M1 milestone Dec 29, 2024
ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 31, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 31, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Dec 31, 2024
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Jan 9, 2025
This is still provisional and an active work in progress, please track
eclipse-jdt/eclipse.jdt.core#958 for this work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants