Fix bug around registering a module in an empty output location.

java-compiler-testing/src/main/java/io/github/ascopes/jct/containers/impl/ContainerGroupRepositoryImpl.java

@@ -30,6 +30,7 @@
 import javax.tools.JavaFileManager.Location;
 import org.apiguardian.api.API;
 import org.apiguardian.api.API.Status;
+import org.jspecify.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -64,8 +65,13 @@ public ContainerGroupRepositoryImpl(String release) {
    * have all modules extracted from them and registered individually rather than the provided path
    * being stored.
    *
+   * <p>Adding modules to an output location <strong>must</strong> ensure that the output location
+   * has a file system path associated with it as a package path already, otherwise the operation
+   * will likely fail
+   *
    * @param location the location to add.
    * @param pathRoot the path root to register with the location.
+   * @throws IllegalStateException if the location is an output location and is misconfigured.
    */
   public void addPath(Location location, PathRoot pathRoot) {
     if (location instanceof ModuleLocation) {
@@ -139,10 +145,11 @@ public void copyContainers(Location from, Location to) {
   public void createEmptyLocation(Location location) {
     if (location instanceof ModuleLocation) {
       throw new IllegalArgumentException("Cannot ensure a module location exists");
-    }
-
-    if (location.isOutputLocation()) {
-      getOrCreateOutputContainerGroup(location);
+    } else if (location.isOutputLocation()) {
+      throw new IllegalArgumentException(
+          "Cannot create an empty output location. It must be created by "
+              + "registering at least one file system path to enable output to."
+      );
     } else if (location.isModuleOrientedLocation()) {
       getOrCreateModuleContainerGroup(location);
     } else {
@@ -164,6 +171,7 @@ public void flush() {
    * @param location the location to get the container group for.
    * @return the container group, or {@code null} if no group is associated with the location.
    */
+  @Nullable
   public ContainerGroup getContainerGroup(Location location) {
     ContainerGroup group = outputs.get(location);
     if (group == null) {
@@ -172,100 +180,113 @@ public ContainerGroup getContainerGroup(Location location) {
         group = packageInputs.get(location);
       }
     }
-
     return group;
   }
 
   /**
-   * Get a package container group.
+   * Get a module container group.
    *
    * @param location the location associated with the group to get.
    * @return the container group, or {@code null} if no group is associated with the location.
    */
-  public PackageContainerGroup getPackageContainerGroup(Location location) {
-    return packageInputs.get(location);
+  @Nullable
+  public ModuleContainerGroup getModuleContainerGroup(Location location) {
+    return moduleInputs.get(location);
   }
 
   /**
-   * Get a snapshot of all the package container groups for inputs.
+   * Get a snapshot of all the module container groups for inputs.
    *
-   * @return the package container groups.
+   * @return the module container groups.
    */
-  public Collection<PackageContainerGroup> getPackageContainerGroups() {
-    return Set.copyOf(packageInputs.values());
+  public Collection<ModuleContainerGroup> getModuleContainerGroups() {
+    return Set.copyOf(moduleInputs.values());
   }
 
   /**
-   * Get a package-oriented container group from the input packages or the outputs.
-   *
-   * <p>This also accepts {@link ModuleLocation module locations} and will resolve them
-   * correctly.
+   * Get a module-oriented container group from the input modules or the outputs.
    *
    * @param location the location associated with the group to get.
    * @return the container group, or {@code null} if no group is associated with the location.
    */
-  public PackageContainerGroup getPackageOrientedContainerGroup(Location location) {
-    if (location instanceof ModuleLocation) {
-      var moduleLocation = (ModuleLocation) location;
-      var group = getModuleOrientedContainerGroup(moduleLocation.getParent());
-      return group == null
-          ? null
-          : group.getModule(moduleLocation.getModuleName());
-    }
-
-    var group = packageInputs.get(location);
-    return group == null
-        ? outputs.get(location)
-        : group;
+  @Nullable
+  public ModuleContainerGroup getModuleOrientedContainerGroup(Location location) {
+    var group = moduleInputs.get(location);
+    return group == null ? outputs.get(location) : group;
   }
 
   /**
-   * Get a module container group.
+   * Get an output container group.
    *
    * @param location the location associated with the group to get.
    * @return the container group, or {@code null} if no group is associated with the location.
    */
-  public ModuleContainerGroup getModuleContainerGroup(Location location) {
-    return moduleInputs.get(location);
+  @Nullable
+  public OutputContainerGroup getOutputContainerGroup(Location location) {
+    return outputs.get(location);
   }
 
   /**
-   * Get a snapshot of all the module container groups for inputs.
+   * Get a snapshot of all the output container groups.
    *
-   * @return the module container groups.
+   * @return the output container groups.
    */
-  public Collection<ModuleContainerGroup> getModuleContainerGroups() {
-    return Set.copyOf(moduleInputs.values());
+  public Collection<OutputContainerGroup> getOutputContainerGroups() {
+    return Set.copyOf(outputs.values());
   }
 
   /**
-   * Get a module-oriented container group from the input modules or the outputs.
+   * Get a package container group.
    *
    * @param location the location associated with the group to get.
    * @return the container group, or {@code null} if no group is associated with the location.
    */
-  public ModuleContainerGroup getModuleOrientedContainerGroup(Location location) {
-    var group = moduleInputs.get(location);
-    return group == null ? outputs.get(location) : group;
+  @Nullable
+  public PackageContainerGroup getPackageContainerGroup(Location location) {
+    return packageInputs.get(location);
   }
 
   /**
-   * Get an output container group.
+   * Get a snapshot of all the package container groups for inputs.
+   *
+   * @return the package container groups.
+   */
+  public Collection<PackageContainerGroup> getPackageContainerGroups() {
+    return Set.copyOf(packageInputs.values());
+  }
+
+  /**
+   * Get a package-oriented container group from the input packages or the outputs.
+   *
+   * <p>This also accepts {@link ModuleLocation module locations} and will resolve them
+   * correctly.
    *
    * @param location the location associated with the group to get.
    * @return the container group, or {@code null} if no group is associated with the location.
    */
-  public OutputContainerGroup getOutputContainerGroup(Location location) {
-    return outputs.get(location);
+  @Nullable
+  public PackageContainerGroup getPackageOrientedContainerGroup(Location location) {
+    if (location instanceof ModuleLocation) {
+      var moduleLocation = (ModuleLocation) location;
+      var group = getModuleOrientedContainerGroup(moduleLocation.getParent());
+      return group == null
+          ? null
+          : group.getModule(moduleLocation.getModuleName());
+    }
+
+    var group = packageInputs.get(location);
+    return group == null
+        ? outputs.get(location)
+        : group;
   }
 
   /**
-   * Get a snapshot of all the output container groups.
+   * Get the release.
    *
-   * @return the output container groups.
+   * @return the release.
    */
-  public Collection<OutputContainerGroup> getOutputContainerGroups() {
-    return Set.copyOf(outputs.values());
+  public String getRelease() {
+    return release;
   }
 
   /**
@@ -338,12 +359,10 @@ private void addModuleRoot(Location location, PathRoot pathRoot) {
           modules
       );
     } else {
-      var group = location.isOutputLocation()
-          ? getOrCreateOutputContainerGroup(location)
-          : getOrCreateModuleContainerGroup(location);
+      var moduleGroup = getOrCreateModuleContainerGroup(location);
 
       modules.forEach((moduleName, modulePath) -> {
-        group.addModule(moduleName, new WrappingDirectoryImpl(modulePath));
+        moduleGroup.addModule(moduleName, new WrappingDirectoryImpl(modulePath));
       });
     }
   }
@@ -406,6 +425,11 @@ private ModuleContainerGroup getOrCreateModuleContainerGroup(Location location)
     );
   }
 
+  // Note that by itself, this method should be considered unsafe. The caller MUST
+  // ensure at least one package root (which is not a module) gets registered upon
+  // creation to enable writing out files correctly to this location. If this is not
+  // done, then attempting to create a new module output will likely fail with errors
+  // since we will have no place to put the module on the file systems we manage.
   private OutputContainerGroup getOrCreateOutputContainerGroup(Location location) {
     return outputs.computeIfAbsent(
         location,

java-compiler-testing/src/main/java/io/github/ascopes/jct/containers/impl/OutputContainerGroupImpl.java

@@ -146,7 +146,21 @@ private PackageContainerGroup newPackageGroup(ModuleLocation moduleLocation) {
     // For output locations, we only need the first root. We then just put a subdirectory
     // in there, as it reduces the complexity of this tenfold and means we don't have to
     // worry about creating more in-memory locations on the fly.
+    //
+    // The reason we have to do this relates to the fact that we will be provided an output
+    // directory to write to regardless of whether we want to write out modules or normal
+    // packages.
     var release = getRelease();
+    var packages = getPackages();
+
+    if (packages.isEmpty()) {
+      // This *shouldn't* be reachable in most cases.
+      throw new IllegalStateException(
+          "Cannot add module " + moduleLocation + " to outputs. No output path has been "
+              + "provided for this location! Please register a package path to output generated "
+              + "modules to first before running the compiler."
+      );
+    }
 
     // Use an anonymous class here to avoid the constraints that the PackageContainerGroupImpl
     // imposes on us.

java-compiler-testing/src/main/java/io/github/ascopes/jct/filemanagers/JctFileManager.java

@@ -39,18 +39,26 @@
 public interface JctFileManager extends JavaFileManager {
 
   /**
-   * Add a path to a given location.
+   * Add a package-oriented path to a given location.
+   *
+   * <p>To add a module, first obtain the module location using
+   * {@link #getLocationForModule(Location, String)}, and pass that result to this call.
    *
    * @param location the location to use.
    * @param path     the path to add.
+   * @see #getLocationForModule(Location, String)
    */
   void addPath(Location location, PathRoot path);
 
   /**
-   * Add a collection of paths to a given location.
+   * Add a collection of package-oriented paths to a given location.
+   *
+   * <p>To add a module, first obtain the module location using
+   * {@link #getLocationForModule(Location, String)}, and pass that result to this call.
    *
    * @param location the location to use.
    * @param paths    the paths to add.
+   * @see #getLocationForModule(Location, String)
    */
   void addPaths(Location location, Collection<? extends PathRoot> paths);
 
@@ -70,7 +78,15 @@ public interface JctFileManager extends JavaFileManager {
    *
    * <p>If the location already exists, then do not do anything.
    *
+   * <p>If the location is an output location, then this operation does not make any sense, since
+   * an empty location cannot have files output to it. In this case, you will likely get an
+   * exception.
+   *
+   * <p>Likewise, this operation does not make sense for module locations within a module-oriented
+   * location group, so this operation will fail with an error for those inputs as well.
+   *
    * @param location the location to apply an empty container for.
+   * @throws IllegalArgumentException if the location is an output location or a module location.
    */
   void createEmptyLocation(Location location);