Fix depth-first search for Java 9 modules

To compute modules to be added to compilation roots in JavaModuleGraph.getAllDependencies, we should look not only for transitive requirements of root modules, but for transitive requirements of _root modules' requirements_. The same logic applies to JavaModuleGraph.reads. In other words, when looking for a path in the module graph between two modules, the first edge's transitiveness doesn't matter, but all other edges after the first must be transitive. There was also a stupid bug in dfs in JavaModuleGraph.getAllDependencies: we continued the DFS only if the module _was not_ added to the "visited" set ("add" returns true if the element was added successfully) #KT-18598 In Progress
2017-06-28 16:13:14 +03:00
parent 72f37a278c
commit 5192f946c5
22 changed files with 135 additions and 4 deletions
@@ -32,13 +32,29 @@ class JavaModuleGraph(finder: JavaModuleFinder) {
            // Automatic modules have no transitive exports, so we only consider explicit modules here
            val moduleInfo = (module(moduleName) as? JavaModule.Explicit)?.moduleInfo ?: return
            for ((dependencyModuleName, isTransitive) in moduleInfo.requires) {
-                if (!visited.add(dependencyModuleName) && isTransitive) {
+                if (isTransitive && visited.add(dependencyModuleName)) {
                    dfs(dependencyModuleName)
                }
            }
        }

-        moduleNames.forEach(::dfs)
+        for (moduleName in moduleNames) {
+            val module = module(moduleName) ?: continue
+            when (module) {
+                is JavaModule.Automatic -> {
+                    // Do nothing; all automatic modules should be added to compilation roots at call site as per java.lang.module javadoc
+                }
+                is JavaModule.Explicit -> {
+                    for ((dependencyModuleName) in module.moduleInfo.requires) {
+                        if (visited.add(dependencyModuleName)) {
+                            dfs(dependencyModuleName)
+                        }
+                    }
+                }
+                else -> error("Unknown module: $module (${module.javaClass})")
+            }
+        }
+
        return visited.toList()
    }

@@ -50,7 +66,7 @@ class JavaModuleGraph(finder: JavaModuleFinder) {
        fun dfs(name: String): Boolean {
            if (!visited.add(name)) return false

-            val module = module(name)
+            val module = module(name) ?: return false
            when (module) {
                is JavaModule.Automatic -> return true
                is JavaModule.Explicit -> {
@@ -60,11 +76,20 @@ class JavaModuleGraph(finder: JavaModuleFinder) {
                    }
                    return false
                }
-                null -> return false
                else -> error("Unsupported module type: $module")
            }
        }

+        val module = module(moduleName) ?: return false
+        when (module) {
+            is JavaModule.Automatic -> return true
+            is JavaModule.Explicit -> {
+                for ((dependencyModuleName) in module.moduleInfo.requires) {
+                    if (dfs(dependencyModuleName)) return true
+                }
+            }
+        }
+
        return dfs(moduleName)
    }
 }
@@ -0,0 +1,3 @@
+package foo;
+
+public class Foo {}
@@ -0,0 +1,3 @@
+module moduleA {
+    exports foo;
+}
@@ -0,0 +1,9 @@
+package bar;
+
+import foo.Foo;
+
+public class Bar {
+    public static Foo getFoo() {
+        return new Foo();
+    }
+}
@@ -0,0 +1,5 @@
+module moduleB {
+    requires transitive moduleA;
+
+    exports bar;
+}
@@ -0,0 +1 @@
+OK
@@ -0,0 +1,3 @@
+module moduleC {
+    requires moduleB;
+}
@@ -0,0 +1,8 @@
+import foo.*
+import bar.*
+
+fun test(): String {
+    val f = Foo()
+    val b = Bar.getFoo()
+    return "$f$b"
+}
@@ -0,0 +1,3 @@
+package a;
+
+public class A {}
@@ -0,0 +1,3 @@
+module moduleA {
+    exports a;
+}
@@ -0,0 +1,3 @@
+module moduleB {
+    requires transitive moduleA;
+}
@@ -0,0 +1,3 @@
+module moduleC {
+    requires moduleB;
+}
@@ -0,0 +1 @@
+OK
@@ -0,0 +1,4 @@
+module moduleD {
+    requires moduleC;
+    requires moduleB;
+}
@@ -0,0 +1,5 @@
+import a.A
+
+fun usage(): String {
+    return A().toString()
+}
@@ -0,0 +1,3 @@
+package foo;
+
+public class Foo {}
@@ -0,0 +1,3 @@
+module moduleA {
+    exports foo;
+}
@@ -0,0 +1,9 @@
+package bar;
+
+import foo.Foo;
+
+public class Bar {
+    public static Foo getFoo() {
+        return new Foo();
+    }
+}
@@ -0,0 +1,5 @@
+module moduleB {
+    requires transitive moduleA;
+
+    exports bar;
+}
@@ -0,0 +1 @@
+OK
@@ -0,0 +1,8 @@
+import foo.*
+import bar.*
+
+fun test(): String {
+    val f = Foo()
+    val b = Bar.getFoo()
+    return "$f$b"
+}
@@ -107,4 +107,27 @@ class Java9ModulesIntegrationTest : AbstractKotlinCompilerIntegrationTest() {
        // because we did not provide -Xadd-modules=moduleA
        module("moduleB", listOf(module("moduleA")), addModules = emptyList())
    }
+
+    fun testNamedReadsTransitive() {
+        val a = module("moduleA")
+        val b = module("moduleB", listOf(a))
+        module("moduleC", listOf(a, b))
+    }
+
+    fun testUnnamedReadsTransitive() {
+        val a = module("moduleA")
+        val b = module("moduleB", listOf(a))
+        module("moduleC", listOf(a, b), addModules = listOf("moduleB"))
+    }
+
+    fun testNonTransitiveDoesNotAffectExplicitDependency() {
+        // In this test, D depends on C (which requires B non-transitively) and on B; also B transitively requires A.
+        // We check that if we depend on both C and B, we still transitively depend on A (via B).
+        // This is a check against an incorrectly implemented DFS which, upon entering C, would write off B as "visited"
+        // and not enter it later even though we explicitly depend on it in D's module-info
+        val a = module("moduleA")
+        val b = module("moduleB", listOf(a))
+        val c = module("moduleC", listOf(a, b))
+        module("moduleD", listOf(c, b, a))
+    }
 }