changeset 13544:01989b3e4652

Move post-resolution to check exports from Layer.create to Configuration.resolve and bind
author alanb
date Thu, 30 Jul 2015 10:12:06 +0100
parents c7eb744c853a
children d766c19978ef
files src/java.base/share/classes/java/lang/module/Configuration.java src/java.base/share/classes/java/lang/module/Layer.java src/java.base/share/classes/java/lang/module/Resolver.java test/jdk/jigsaw/module/ConfigurationTest.java test/jdk/jigsaw/module/LayerTest.java
diffstat 5 files changed, 231 insertions(+), 194 deletions(-) [+]
line wrap: on
line diff
--- a/src/java.base/share/classes/java/lang/module/Configuration.java	Wed Jul 29 17:27:58 2015 +0100
+++ b/src/java.base/share/classes/java/lang/module/Configuration.java	Thu Jul 30 10:12:06 2015 +0100
@@ -41,15 +41,14 @@
  *
  * <pre>{@code
  *
- *     ModuleFinder finder =
- *         ModuleFinder.of(dir1, dir2, dir3);
+ *     ModuleFinder finder = ModuleFinder.of(dir1, dir2, dir3);
  *
  *     Configuration cf =
- *         Configuration.resolve(ModuleFinder.empty(),
- *                               Layer.boot(),
- *                               finder,
- *                               "myapp")
- *                      .bind();
+ *         = Configuration.resolve(ModuleFinder.empty(),
+ *                                 Layer.boot(),
+ *                                 finder,
+ *                                 "myapp")
+ *                        .bind();
  *
  * }</pre>
  *
@@ -67,12 +66,23 @@
     }
 
     /**
-     * Resolves the given named modules. The given root modules are located
-     * using {@code beforeFinder} or if not found then using {@code afterFinder}.
-     * Module dependences are resolved by locating them (in order) using the
-     * given {@code beforeFinder}, {@code layer}, and {@code afterFinder}.
+     * Resolves the collection of root modules, specified by module names,
+     * returning the resulting configuration.
      *
-     * <p> Resolution can fail for several reasons including: </p>
+     * <p> The root modules and their transitive dependences are located using
+     * the given {@code beforeFinder}, parent {@code Layer} and {@code
+     * afterFinder}, in this order. Dependences located in the parent {@code
+     * Layer} are resolved no further. </p>
+     *
+     * <p> When all modules have been resolved then the resulting <em>dependency
+     * graph</em> is checked to ensure that it does not contain cycles. A
+     * <em>readability graph</em> is then constructed to take account of
+     * implicitly declared dependences (requires public). The readability
+     * graph and modules exports are checked to ensure that two or more modules
+     * do not export the same package to a module that reads both. </p>
+     *
+     * <p> Resolution and the post-resolution consistency checks may fail for
+     * several reasons: </p>
      *
      * <ul>
      *     <li> A root module, or a direct or transitive dependency, is not
@@ -81,17 +91,23 @@
      *     <li> Some other error occurs when attempting to find a module.
      *          Possible errors include I/O errors, errors detected parsing a
      *          module descriptor ({@code module-info.class}) or two versions
-     *          of the same module found in the same directory. </li>
+     *          of the same module are found in the same directory. </li>
      *
-     *     <li> A cycle is detected, say where module {@code m1} requires module
-     *          {@code m2} and {@code m2} requires {@code m1}. </li>
+     *     <li> A cycle is detected, say where module {@code m1} requires
+     *          module {@code m2} and {@code m2} requires {@code m1}. </li>
      *
-     *     <li> Implementation specific checks, for example referential integrity
-     *          checks that fail where incompatible versions of modules may not
-     *          be combined in the same configuration. </li>
+     *     <li> Two or more modules in the configuration export the same
+     *          package to a module that reads both. This includes the case
+     *          where a module {@code M} containing package {@code P} reads
+     *          another module that exports {@code P} to {@code M}. </li>
+     *
+     *     <li> Other implementation specific checks, for example referential
+     *          integrity checks that fail where incompatible versions of
+     *          modules may not be combined in the same configuration. </li>
      * </ul>
      *
-     * @throws ResolutionException  If resolution fails
+     * @throws ResolutionException If resolution or the post-resolution checks
+     *         fail for any of the reasons listed
      */
     public static Configuration resolve(ModuleFinder beforeFinder,
                                         Layer parent,
@@ -109,11 +125,11 @@
     }
 
     /**
-     * Resolves the given named modules. Module dependences are resolved by
-     * locating them (in order) using the given {@code beforeFinder}, {@code
-     * layer}, and {@code afterFinder}.
+     * Resolves the root modules, specified by module names, returning the
+     * resulting configuration.
      *
-     * @throws ResolutionException   If resolution fails
+     * @throws ResolutionException If resolution or the post-resolution checks
+     *         fail for any of the reasons listed
      */
     public static Configuration resolve(ModuleFinder beforeFinder,
                                         Layer layer,
@@ -139,7 +155,8 @@
      * provider modules. It may therefore fail with {@code ResolutionException}
      * for exactly the same reasons as the {@link #resolve resolve} methods.
      *
-     * @throws ResolutionException  If resolution fails
+     * @throws ResolutionException If resolution or the post-resolution checks
+     *         fail for any of the reasons listed
      *
      * @apiNote This method is not thread safe
      */
--- a/src/java.base/share/classes/java/lang/module/Layer.java	Wed Jul 29 17:27:58 2015 +0100
+++ b/src/java.base/share/classes/java/lang/module/Layer.java	Thu Jul 30 10:12:06 2015 +0100
@@ -110,9 +110,10 @@
      * given {@code Configuration}, to the Java virtual machine.
      *
      * <p> Modules are mapped to module-capable class loaders by means of the
-     * given {@code ClassLoaderFinder}. This method registers modules to their
-     * class loader by invoking the class loader's {@link
-     * ModuleCapableLoader#register register} method. </p>
+     * given {@code ClassLoaderFinder} and defined to the Java virtual machine.
+     * This method also registers modules to their class loader by invoking
+     * the class loader's {@link ModuleCapableLoader#register register} method.
+     * </p>
      *
      * <p> Creating a {@code Layer} may fail for several reasons: </p>
      *
@@ -120,12 +121,6 @@
      *     <li> Two or more modules with the same package (exported or
      *          concealed) are mapped to the same class loader. </li>
      *
-     *     <li> Two or more modules in the configuration export the same
-     *          package to a module that reads both. </li>
-     *
-     *     <li> A module {@code M} containing package {@code P} reads another
-     *          module that exports {@code P} to {@code M}. </li>
-     *
      *     <li> A module is mapped to a class loader that already has a module
      *          of the same name defined to it. </li>
      *
@@ -158,7 +153,6 @@
             checkBootModulesForDuplicatePkgs(cf);
         } else {
             checkForDuplicatePkgs(cf, clf);
-            checkExportSuppliers(cf);
         }
 
         Layer layer;
@@ -221,51 +215,6 @@
     }
 
     /**
-     * Checks a configuration to ensure that no two modules export the same
-     * package to a module.
-     *
-     * @throws LayerInstantiationException
-     */
-    private static void checkExportSuppliers(Configuration cf) {
-
-        for (ModuleDescriptor descriptor1 : cf.descriptors()) {
-
-            // the map of packages that are local or exported to descriptor1
-            Map<String, ModuleDescriptor> packageToExporter = new HashMap<>();
-
-            // local packages
-            descriptor1.packages()
-                .forEach(p -> packageToExporter.put(p, descriptor1));
-
-            // descriptor1 reads descriptor2
-            for (ModuleDescriptor descriptor2 : cf.reads(descriptor1)) {
-
-                for (ModuleDescriptor.Exports export : descriptor2.exports()) {
-
-                    Optional<Set<String>> otargets = export.targets();
-                    if (otargets.isPresent()) {
-                        if (!otargets.get().contains(descriptor1.name()))
-                            continue;
-                    }
-
-                    // pkg is exported to descriptor2
-                    String pkg = export.source();
-                    ModuleDescriptor other
-                        = packageToExporter.put(pkg, descriptor2);
-                    if (other != null && other != descriptor2) {
-                        throw fail("Modules %s and %s export package %s to module %s",
-                                    descriptor2.name(),
-                                    other.name(),
-                                    pkg,
-                                    descriptor1.name());
-                    }
-                }
-            }
-
-        }
-    }
-
-    /**
      * Creates a LayerInstantiationException with the a message formatted from
      * the given format string and arguments.
      */
@@ -295,9 +244,6 @@
 
     /**
      * Returns a set of the {@code Module}s in this layer.
-     *
-     * @apiNote This method is for discussion purposes. We also need to
-     * consider Module::getLayer.
      */
     public Set<Module> modules() {
         return nameToModule.values().stream().collect(Collectors.toSet());
--- a/src/java.base/share/classes/java/lang/module/Resolver.java	Wed Jul 29 17:27:58 2015 +0100
+++ b/src/java.base/share/classes/java/lang/module/Resolver.java	Thu Jul 30 10:12:06 2015 +0100
@@ -211,6 +211,12 @@
         Map<ModuleDescriptor, Set<ModuleDescriptor>> graph = makeGraph();
         cachedGraph = graph;
 
+        // As Layer.create will ensure that the boot Layer does not contain any
+        // duplicate packages (even concealed) then we can skip this check to
+        // reduce overhead at startup
+        //if (Layer.boot() != null)
+        checkExportSuppliers(graph);
+
         Resolution r = new Resolution(selected,
                                       nameToReference,
                                       graph,
@@ -404,6 +410,8 @@
 
             graph = makeGraph();
 
+            checkExportSuppliers(graph);
+
         } else {
             graph = cachedGraph;
         }
@@ -740,6 +748,56 @@
 
 
     /**
+     * Checks the readability graph to ensure that no two modules export the
+     * same package to a module.
+     */
+    private static void
+    checkExportSuppliers(Map<ModuleDescriptor, Set<ModuleDescriptor>> graph) {
+
+        for (Map.Entry<ModuleDescriptor, Set<ModuleDescriptor>> e : graph.entrySet()) {
+            ModuleDescriptor descriptor1 = e.getKey();
+
+            // the map of packages that are local or exported to descriptor1
+            Map<String, ModuleDescriptor> packageToExporter = new HashMap<>();
+
+            // local packages
+            for (String pn : descriptor1.packages()) {
+                packageToExporter.put(pn, descriptor1);
+            }
+
+            // descriptor1 reads descriptor2
+            Set<ModuleDescriptor> reads = e.getValue();
+            for (ModuleDescriptor descriptor2 : reads) {
+
+                for (ModuleDescriptor.Exports export : descriptor2.exports()) {
+
+                    Optional<Set<String>> otargets = export.targets();
+                    if (otargets.isPresent()) {
+                        if (!otargets.get().contains(descriptor1.name()))
+                            continue;
+                    }
+
+                    // source is exported to descriptor2
+                    String source = export.source();
+                    ModuleDescriptor other
+                        = packageToExporter.put(source, descriptor2);
+                    if (other != null && other != descriptor2) {
+                        fail("Modules %s and %s export package %s to module %s",
+                                descriptor2.name(),
+                                other.name(),
+                                source,
+                                descriptor1.name());
+
+                    }
+                }
+            }
+
+        }
+
+    }
+
+
+    /**
      * Tracing support, limited to boot layer for now.
      */
 
--- a/test/jdk/jigsaw/module/ConfigurationTest.java	Wed Jul 29 17:27:58 2015 +0100
+++ b/test/jdk/jigsaw/module/ConfigurationTest.java	Thu Jul 30 10:12:06 2015 +0100
@@ -30,6 +30,7 @@
  */
 
 import java.lang.module.Configuration;
+import java.lang.module.Layer;
 import java.lang.module.ModuleDescriptor;
 import java.lang.module.ModuleDescriptor.Requires.Modifier;
 import java.lang.module.ModuleFinder;
@@ -96,6 +97,7 @@
         assertTrue(cf.toString().contains("m3"));
     }
 
+
     /**
      * Basic test of "requires public"
      */
@@ -139,6 +141,7 @@
         assertTrue(cf.reads(descriptor3).size() == 0);
     }
 
+
     /**
      * Basic test of binding services
      */
@@ -219,6 +222,7 @@
         Configuration.resolve(empty(), boot(), empty(), "m1");
     }
 
+
     /**
      * Direct dependency not found
      */
@@ -231,6 +235,7 @@
         Configuration.resolve(finder, boot(), empty(), "m1");
     }
 
+
     /**
      * Transitive dependency not found
      */
@@ -245,6 +250,7 @@
         Configuration.resolve(finder, boot(), empty(), "m1");
     }
 
+
     /**
      * Service provider dependency not found
      */
@@ -274,6 +280,7 @@
         cf.bind();
     }
 
+
     /**
      * Simple cycle.
      */
@@ -291,6 +298,7 @@
         Configuration.resolve(finder, boot(), empty(), "m1");
     }
 
+
     /**
      * Basic test for detecting cycles involving a service provider module
      */
@@ -325,4 +333,108 @@
         cf.bind();
     }
 
+
+    /**
+     * Test two modules exporting package p to a module that reads both.
+     */
+    @Test(expectedExceptions = { ResolutionException.class })
+    public void testPackageSuppliedByTwoOthers() {
+
+        ModuleDescriptor descriptor1
+            =  new ModuleDescriptor.Builder("m1")
+                .requires("m2")
+                .requires("m3")
+                .build();
+
+        ModuleDescriptor descriptor2
+            =  new ModuleDescriptor.Builder("m2")
+                .exports("p")
+                .build();
+
+        ModuleDescriptor descriptor3
+            =  new ModuleDescriptor.Builder("m3")
+                .exports("p", "m1")
+                .build();
+
+        ModuleFinder finder
+            = ModuleUtils.finderOf(descriptor1, descriptor2, descriptor3);
+
+        Configuration.resolve(finder, Layer.boot(), empty(), "m1");
+    }
+
+
+    /**
+     * Test the scenario where a module has a concealed package p and reads
+     * a module that exports package p.
+     */
+    @Test(expectedExceptions = { ResolutionException.class })
+    public void testPackageSuppliedBySelfAndOther() {
+
+        ModuleDescriptor descriptor1
+            =  new ModuleDescriptor.Builder("m1")
+                .requires("m2")
+                .conceals("p")
+                .build();
+
+        ModuleDescriptor descriptor2
+            =  new ModuleDescriptor.Builder("m2")
+                .exports("p")
+                .build();
+
+        ModuleFinder finder = ModuleUtils.finderOf(descriptor1, descriptor2);
+
+        Configuration.resolve(finder, Layer.boot(), empty(), "m1");
+    }
+
+
+    /**
+     * Test the scenario where a module has a concealed package p and reads
+     * a module that also has a concealed package p.
+     */
+    public void testPackagePrivateToSelfAndOther() {
+        ModuleDescriptor descriptor1
+            =  new ModuleDescriptor.Builder("m1")
+                .requires("m2")
+                .conceals("p")
+                .build();
+
+        ModuleDescriptor descriptor2
+            =  new ModuleDescriptor.Builder("m2")
+                .conceals("p")
+                .build();
+
+        ModuleFinder finder = ModuleUtils.finderOf(descriptor1, descriptor2);
+
+        Configuration cf
+            = Configuration.resolve(finder, Layer.boot(), empty(), "m1");
+
+        assertTrue(cf.descriptors().size() == 2);
+        assertTrue(cf.descriptors().contains(descriptor1));
+        assertTrue(cf.descriptors().contains(descriptor2));
+
+        // m1 reads m2
+        assertTrue(cf.reads(descriptor1).contains(descriptor2));
+
+        // m2 reads nothing
+        assertTrue(cf.reads(descriptor2).isEmpty());
+    }
+
+
+    /**
+     * Test the scenario where a module that exports a package that is also
+     * exported by a module that it reads in a parent layer.
+     */
+    @Test(expectedExceptions = { ResolutionException.class })
+    public void testExportSamePackageAsBootLayer() {
+        ModuleDescriptor descriptor
+            =  new ModuleDescriptor.Builder("m1")
+                .requires("java.base")
+                .exports("java.lang")
+                .build();
+
+        ModuleFinder finder = ModuleUtils.finderOf(descriptor);
+
+        Configuration.resolve(finder, Layer.boot(), empty(), "m1");
+    }
+
 }
--- a/test/jdk/jigsaw/module/LayerTest.java	Wed Jul 29 17:27:58 2015 +0100
+++ b/test/jdk/jigsaw/module/LayerTest.java	Thu Jul 30 10:12:06 2015 +0100
@@ -75,6 +75,7 @@
         assertTrue(bootLayer.parent().get() == Layer.empty());
     }
 
+
     /**
      * Exercise Layer.empty()
      */
@@ -100,6 +101,7 @@
         assertTrue(!emptyLayer.parent().isPresent());
     }
 
+
     /**
      * Exercise Layer.create, created on an empty layer
      */
@@ -177,6 +179,7 @@
         assertTrue(layer.parent().get() == Layer.empty());
     }
 
+
     /**
      * Exercise Layer.create, created over the boot layer
      */
@@ -237,6 +240,7 @@
         assertTrue(layer.parent().get() == Layer.boot());
     }
 
+
     /**
      * Layer.create with a configuration of two modules that have the same
      * module-private package.
@@ -271,87 +275,6 @@
         } catch (LayerInstantiationException expected) { }
     }
 
-    /**
-     * Layer.create with a configuration of two modules that export the
-     * same package.
-     */
-    public void testSameExportedPackage() {
-        ModuleDescriptor descriptor1
-            =  new ModuleDescriptor.Builder("m1")
-                .requires("m2")
-                .exports("p")
-                .build();
-
-        ModuleDescriptor descriptor2
-            = new ModuleDescriptor.Builder("m2")
-                .exports("p")
-                .build();
-
-        ModuleFinder finder
-            = ModuleUtils.finderOf(descriptor1, descriptor2);
-
-        Configuration cf
-            = Configuration.resolve(finder, Layer.empty(), ModuleFinder.empty(), "m1");
-        assertTrue(cf.descriptors().size() == 2);
-
-        // one loader per module
-        try {
-            Layer.create(cf, m -> new ModuleClassLoader());
-            assertTrue(false);
-        } catch (LayerInstantiationException expected) { }
-
-        // same class loader
-        try {
-            ClassLoader loader = new ModuleClassLoader();
-            Layer.create(cf, m -> loader);
-            assertTrue(false);
-        } catch (LayerInstantiationException expected) { }
-    }
-
-    /**
-     * Layer.create with a configuration of two modules that export the
-     * same package to another module (that reads both).
-     */
-    public void testSameExportToModule() {
-
-        ModuleDescriptor descriptor1
-            =  new ModuleDescriptor.Builder("m1")
-                .requires("m2")
-                .requires("m3")
-                .build();
-
-        // exports p
-        ModuleDescriptor descriptor2
-            = new ModuleDescriptor.Builder("m2")
-                .exports("p")
-                .build();
-
-        // exports p to m1
-        ModuleDescriptor descriptor3
-            = new ModuleDescriptor.Builder("m3")
-                .exports("p", "m1")
-                .build();
-
-        ModuleFinder finder
-            = ModuleUtils.finderOf(descriptor1, descriptor2, descriptor3);
-
-        Configuration cf
-            = Configuration.resolve(finder, Layer.empty(), ModuleFinder.empty(), "m1");
-        assertTrue(cf.descriptors().size() == 3);
-
-        // one loader per module
-        try {
-            Layer.create(cf, m -> new ModuleClassLoader());
-            assertTrue(false);
-        } catch (LayerInstantiationException expected) { }
-
-        // same class loader
-        try {
-            ClassLoader loader = new ModuleClassLoader();
-            Layer.create(cf, m -> loader);
-            assertTrue(false);
-        } catch (LayerInstantiationException expected) { }
-    }
 
     /**
      * Layer.create with a configuration with a partitioned graph. The same
@@ -411,17 +334,18 @@
         } catch (LayerInstantiationException expected) { }
     }
 
+
     /**
-     * Layer.create with a configuration that contains a module that exports
-     * the same package as java.base.
+     * Layer.create with a configuration that contains a module that has a
+     * concealed package that is the same name as a non-exported package
+     * in a parent layer.
      */
-    @Test(expectedExceptions = { LayerInstantiationException.class })
-    public void testExportSamePackageAsBootLayer() {
+    public void testConcealSamePackageAsBootLayer() {
         ModuleDescriptor descriptor
-            =  new ModuleDescriptor.Builder("m1")
-                .requires("java.base")
-                .exports("java.lang")
-                .build();
+            = new ModuleDescriptor.Builder("m1")
+               .requires("java.base")
+               .conceals("sun.launcher")
+               .build();
 
         ModuleFinder finder = ModuleUtils.finderOf(descriptor);
 
@@ -429,28 +353,8 @@
             = Configuration.resolve(finder, Layer.boot(), ModuleFinder.empty(), "m1");
         assertTrue(cf.descriptors().size() == 1);
 
-        Layer.create(cf, m -> new ModuleClassLoader());
-    }
-
-    /**
-     * Layer.create with a configuration that contains a module that has a
-     * concealed package that is the same name as a non-exported package
-     * in java.base.
-     */
-    public void testConcealSamePackageAsBootLayer() {
-        ModuleDescriptor descriptor
-            =  new ModuleDescriptor.Builder("m1")
-                .requires("java.base")
-                .conceals("sun.launcher")
-                .build();
-
-        ModuleFinder finder = ModuleUtils.finderOf(descriptor);
-
-        Configuration cf
-            = Configuration.resolve(finder, Layer.boot(), ModuleFinder.empty(), "m1");
-        assertTrue(cf.descriptors().size() == 1);
-
-        Layer.create(cf, m -> new ModuleClassLoader());
-    }
+        Layer layer = Layer.create(cf, m -> new ModuleClassLoader());
+        assertTrue(layer.modules().size() == 1);
+   }
 
 }