From 5192f946c5134f1c0f32f77dba933a66e01ea32a Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Wed, 28 Jun 2017 16:13:14 +0300 Subject: [PATCH] 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 --- .../resolve/jvm/modules/JavaModuleGraph.kt | 33 ++++++++++++++++--- .../namedReadsTransitive/moduleA/foo/Foo.java | 3 ++ .../moduleA/module-info.java | 3 ++ .../namedReadsTransitive/moduleB/bar/Bar.java | 9 +++++ .../moduleB/module-info.java | 5 +++ .../namedReadsTransitive/moduleC.txt | 1 + .../moduleC/module-info.java | 3 ++ .../namedReadsTransitive/moduleC/usage.kt | 8 +++++ .../moduleA/a/A.java | 3 ++ .../moduleA/module-info.java | 3 ++ .../moduleB/module-info.java | 3 ++ .../moduleC/module-info.java | 3 ++ .../moduleD.txt | 1 + .../moduleD/module-info.java | 4 +++ .../moduleD/usage.kt | 5 +++ .../moduleA/foo/Foo.java | 3 ++ .../moduleA/module-info.java | 3 ++ .../moduleB/bar/Bar.java | 9 +++++ .../moduleB/module-info.java | 5 +++ .../unnamedReadsTransitive/moduleC.txt | 1 + .../unnamedReadsTransitive/moduleC/usage.kt | 8 +++++ .../compiler/Java9ModulesIntegrationTest.kt | 23 +++++++++++++ 22 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 compiler/testData/javaModules/namedReadsTransitive/moduleA/foo/Foo.java create mode 100644 compiler/testData/javaModules/namedReadsTransitive/moduleA/module-info.java create mode 100644 compiler/testData/javaModules/namedReadsTransitive/moduleB/bar/Bar.java create mode 100644 compiler/testData/javaModules/namedReadsTransitive/moduleB/module-info.java create mode 100644 compiler/testData/javaModules/namedReadsTransitive/moduleC.txt create mode 100644 compiler/testData/javaModules/namedReadsTransitive/moduleC/module-info.java create mode 100644 compiler/testData/javaModules/namedReadsTransitive/moduleC/usage.kt create mode 100644 compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/a/A.java create mode 100644 compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/module-info.java create mode 100644 compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleB/module-info.java create mode 100644 compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleC/module-info.java create mode 100644 compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD.txt create mode 100644 compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/module-info.java create mode 100644 compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/usage.kt create mode 100644 compiler/testData/javaModules/unnamedReadsTransitive/moduleA/foo/Foo.java create mode 100644 compiler/testData/javaModules/unnamedReadsTransitive/moduleA/module-info.java create mode 100644 compiler/testData/javaModules/unnamedReadsTransitive/moduleB/bar/Bar.java create mode 100644 compiler/testData/javaModules/unnamedReadsTransitive/moduleB/module-info.java create mode 100644 compiler/testData/javaModules/unnamedReadsTransitive/moduleC.txt create mode 100644 compiler/testData/javaModules/unnamedReadsTransitive/moduleC/usage.kt diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleGraph.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleGraph.kt index d0d166c15dc..fa95af796aa 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleGraph.kt +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleGraph.kt @@ -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) } } diff --git a/compiler/testData/javaModules/namedReadsTransitive/moduleA/foo/Foo.java b/compiler/testData/javaModules/namedReadsTransitive/moduleA/foo/Foo.java new file mode 100644 index 00000000000..7f566bafd45 --- /dev/null +++ b/compiler/testData/javaModules/namedReadsTransitive/moduleA/foo/Foo.java @@ -0,0 +1,3 @@ +package foo; + +public class Foo {} diff --git a/compiler/testData/javaModules/namedReadsTransitive/moduleA/module-info.java b/compiler/testData/javaModules/namedReadsTransitive/moduleA/module-info.java new file mode 100644 index 00000000000..cbfe65e9b40 --- /dev/null +++ b/compiler/testData/javaModules/namedReadsTransitive/moduleA/module-info.java @@ -0,0 +1,3 @@ +module moduleA { + exports foo; +} diff --git a/compiler/testData/javaModules/namedReadsTransitive/moduleB/bar/Bar.java b/compiler/testData/javaModules/namedReadsTransitive/moduleB/bar/Bar.java new file mode 100644 index 00000000000..56b7fc1cf14 --- /dev/null +++ b/compiler/testData/javaModules/namedReadsTransitive/moduleB/bar/Bar.java @@ -0,0 +1,9 @@ +package bar; + +import foo.Foo; + +public class Bar { + public static Foo getFoo() { + return new Foo(); + } +} diff --git a/compiler/testData/javaModules/namedReadsTransitive/moduleB/module-info.java b/compiler/testData/javaModules/namedReadsTransitive/moduleB/module-info.java new file mode 100644 index 00000000000..1f7e2873c37 --- /dev/null +++ b/compiler/testData/javaModules/namedReadsTransitive/moduleB/module-info.java @@ -0,0 +1,5 @@ +module moduleB { + requires transitive moduleA; + + exports bar; +} diff --git a/compiler/testData/javaModules/namedReadsTransitive/moduleC.txt b/compiler/testData/javaModules/namedReadsTransitive/moduleC.txt new file mode 100644 index 00000000000..d86bac9de59 --- /dev/null +++ b/compiler/testData/javaModules/namedReadsTransitive/moduleC.txt @@ -0,0 +1 @@ +OK diff --git a/compiler/testData/javaModules/namedReadsTransitive/moduleC/module-info.java b/compiler/testData/javaModules/namedReadsTransitive/moduleC/module-info.java new file mode 100644 index 00000000000..401e33a50c7 --- /dev/null +++ b/compiler/testData/javaModules/namedReadsTransitive/moduleC/module-info.java @@ -0,0 +1,3 @@ +module moduleC { + requires moduleB; +} diff --git a/compiler/testData/javaModules/namedReadsTransitive/moduleC/usage.kt b/compiler/testData/javaModules/namedReadsTransitive/moduleC/usage.kt new file mode 100644 index 00000000000..fe77f806c41 --- /dev/null +++ b/compiler/testData/javaModules/namedReadsTransitive/moduleC/usage.kt @@ -0,0 +1,8 @@ +import foo.* +import bar.* + +fun test(): String { + val f = Foo() + val b = Bar.getFoo() + return "$f$b" +} diff --git a/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/a/A.java b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/a/A.java new file mode 100644 index 00000000000..e3676563ca3 --- /dev/null +++ b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/a/A.java @@ -0,0 +1,3 @@ +package a; + +public class A {} diff --git a/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/module-info.java b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/module-info.java new file mode 100644 index 00000000000..8e8f5b048e3 --- /dev/null +++ b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleA/module-info.java @@ -0,0 +1,3 @@ +module moduleA { + exports a; +} diff --git a/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleB/module-info.java b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleB/module-info.java new file mode 100644 index 00000000000..57fa8b07dd7 --- /dev/null +++ b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleB/module-info.java @@ -0,0 +1,3 @@ +module moduleB { + requires transitive moduleA; +} diff --git a/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleC/module-info.java b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleC/module-info.java new file mode 100644 index 00000000000..401e33a50c7 --- /dev/null +++ b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleC/module-info.java @@ -0,0 +1,3 @@ +module moduleC { + requires moduleB; +} diff --git a/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD.txt b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD.txt new file mode 100644 index 00000000000..d86bac9de59 --- /dev/null +++ b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD.txt @@ -0,0 +1 @@ +OK diff --git a/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/module-info.java b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/module-info.java new file mode 100644 index 00000000000..2608009a8c3 --- /dev/null +++ b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/module-info.java @@ -0,0 +1,4 @@ +module moduleD { + requires moduleC; + requires moduleB; +} diff --git a/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/usage.kt b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/usage.kt new file mode 100644 index 00000000000..fd8c2645b11 --- /dev/null +++ b/compiler/testData/javaModules/nonTransitiveDoesNotAffectExplicitDependency/moduleD/usage.kt @@ -0,0 +1,5 @@ +import a.A + +fun usage(): String { + return A().toString() +} diff --git a/compiler/testData/javaModules/unnamedReadsTransitive/moduleA/foo/Foo.java b/compiler/testData/javaModules/unnamedReadsTransitive/moduleA/foo/Foo.java new file mode 100644 index 00000000000..7f566bafd45 --- /dev/null +++ b/compiler/testData/javaModules/unnamedReadsTransitive/moduleA/foo/Foo.java @@ -0,0 +1,3 @@ +package foo; + +public class Foo {} diff --git a/compiler/testData/javaModules/unnamedReadsTransitive/moduleA/module-info.java b/compiler/testData/javaModules/unnamedReadsTransitive/moduleA/module-info.java new file mode 100644 index 00000000000..cbfe65e9b40 --- /dev/null +++ b/compiler/testData/javaModules/unnamedReadsTransitive/moduleA/module-info.java @@ -0,0 +1,3 @@ +module moduleA { + exports foo; +} diff --git a/compiler/testData/javaModules/unnamedReadsTransitive/moduleB/bar/Bar.java b/compiler/testData/javaModules/unnamedReadsTransitive/moduleB/bar/Bar.java new file mode 100644 index 00000000000..56b7fc1cf14 --- /dev/null +++ b/compiler/testData/javaModules/unnamedReadsTransitive/moduleB/bar/Bar.java @@ -0,0 +1,9 @@ +package bar; + +import foo.Foo; + +public class Bar { + public static Foo getFoo() { + return new Foo(); + } +} diff --git a/compiler/testData/javaModules/unnamedReadsTransitive/moduleB/module-info.java b/compiler/testData/javaModules/unnamedReadsTransitive/moduleB/module-info.java new file mode 100644 index 00000000000..1f7e2873c37 --- /dev/null +++ b/compiler/testData/javaModules/unnamedReadsTransitive/moduleB/module-info.java @@ -0,0 +1,5 @@ +module moduleB { + requires transitive moduleA; + + exports bar; +} diff --git a/compiler/testData/javaModules/unnamedReadsTransitive/moduleC.txt b/compiler/testData/javaModules/unnamedReadsTransitive/moduleC.txt new file mode 100644 index 00000000000..d86bac9de59 --- /dev/null +++ b/compiler/testData/javaModules/unnamedReadsTransitive/moduleC.txt @@ -0,0 +1 @@ +OK diff --git a/compiler/testData/javaModules/unnamedReadsTransitive/moduleC/usage.kt b/compiler/testData/javaModules/unnamedReadsTransitive/moduleC/usage.kt new file mode 100644 index 00000000000..fe77f806c41 --- /dev/null +++ b/compiler/testData/javaModules/unnamedReadsTransitive/moduleC/usage.kt @@ -0,0 +1,8 @@ +import foo.* +import bar.* + +fun test(): String { + val f = Foo() + val b = Bar.getFoo() + return "$f$b" +} diff --git a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/Java9ModulesIntegrationTest.kt b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/Java9ModulesIntegrationTest.kt index b077063b794..ff1ad2446ef 100644 --- a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/Java9ModulesIntegrationTest.kt +++ b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/Java9ModulesIntegrationTest.kt @@ -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)) + } }