diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/AndroidModule.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/AndroidModule.kt index 39a70b9cc6..c12d27711d 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/AndroidModule.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/AndroidModule.kt @@ -141,8 +141,18 @@ open class AndroidModule( addAll(getSelectedVariant()?.mainArtifact?.classJars ?: emptyList()) } - override fun getCompileClasspaths(excludeSourceGeneratedClassPath: Boolean): Set { + override fun getCompileClasspaths( + excludeSourceGeneratedClassPath: Boolean, + visited: MutableSet, + ): Set { val project = IProjectManager.getInstance().workspace ?: return emptySet() + + // Guard against cyclic project-dependency graphs: contribute each module's classpaths at most + // once so a cycle (:a -> :b -> :a) terminates instead of recursing until the stack overflows. + if (!visited.add(path)) { + return emptySet() + } + val result = mutableSetOf() if (excludeSourceGeneratedClassPath) { // TODO: The mainArtifact.classJars are technically generated from source files @@ -159,6 +169,8 @@ open class AndroidModule( libraries = variantDependencies.mainArtifact?.compileDependencyList ?: emptyList(), result = result, excludeSourceGeneratedClassPath = excludeSourceGeneratedClassPath, + visited = HashSet(), + moduleVisited = visited, ) return result } @@ -234,14 +246,34 @@ open class AndroidModule( return result } + /** + * Recursively collect the compile classpath entries contributed by the given dependency-graph + * [libraries] into [result], guarding against cycles. + * + * @param root The workspace used to resolve project dependencies by path. + * @param libraries The dependency-graph nodes to expand at this level. + * @param result The accumulating set of classpath files. + * @param excludeSourceGeneratedClassPath Whether to exclude source-generated classpath entries. + * @param visited Keys of dependency-graph nodes already expanded within this module; each node is + * expanded at most once so a cyclic graph terminates. + * @param moduleVisited Module paths already expanded across modules; threaded into cross-module + * recursion so a cyclic PROJECT graph terminates. + */ private fun collectLibraries( root: Workspace, libraries: List, result: MutableSet, - excludeSourceGeneratedClassPath: Boolean = false, + excludeSourceGeneratedClassPath: Boolean, + visited: MutableSet, + moduleVisited: MutableSet, ) { val libraryMap = variantDependencies.librariesMap for (library in libraries) { + // Guard against cyclic dependency graphs within this module: expand each graph node once. + if (!visited.add(library.key)) { + continue + } + val lib = libraryMap[library.key] ?: continue if (lib.type == AndroidModels.LibraryType.Project) { val module = root.findByPath(lib.projectInfo!!.projectPath) ?: continue @@ -249,39 +281,71 @@ open class AndroidModule( continue } - result.addAll(module.getCompileClasspaths(excludeSourceGeneratedClassPath)) + // Cross-module recursion threads moduleVisited so a cyclic PROJECT graph terminates. + result.addAll(module.getCompileClasspaths(excludeSourceGeneratedClassPath, moduleVisited)) } else if (lib.type == AndroidModels.LibraryType.ExternalAndroidLibrary && lib.hasAndroidLibraryData()) { result.addAll(lib.androidLibraryData.compileJarFiles) } else if (lib.type == AndroidModels.LibraryType.ExternalJavaLibrary && lib.hasArtifactPath()) { result.add(lib.artifact) } - collectLibraries(root, library.dependencyList, result) + // Note: the recursive call intentionally keeps the original behavior of not propagating + // `excludeSourceGeneratedClassPath` (it defaults to false here); only the visited-set + // cycle guard is added. + collectLibraries( + root = root, + libraries = library.dependencyList, + result = result, + excludeSourceGeneratedClassPath = false, + visited = visited, + moduleVisited = moduleVisited, + ) } } - override fun getCompileModuleProjects(): List { + override fun getCompileModuleProjects( + visited: MutableSet, + recursionPath: ArrayDeque, + ): List { val root = IProjectManager.getInstance().workspace ?: return emptyList() - val result = mutableListOf() - val libraries = variantDependencies.mainArtifact.compileDependencyList - val libraryMap = variantDependencies.librariesMap - for (library in libraries) { - val lib = libraryMap[library.key] ?: continue - if (lib.type != AndroidModels.LibraryType.Project) { - continue - } + // True cycle: this module is already an ancestor on the current recursion path + // (e.g. :a -> :b -> :a). Report it and break the cycle. + if (recursionPath.contains(path)) { + reportDependencyCycle(recursionPath, path) + return emptyList() + } - val module = root.findByPath(lib.projectInfo!!.projectPath) ?: continue - if (module !is ModuleProject) { - continue + // Already fully expanded elsewhere (a diamond / shared dependency, not a cycle): dedup. + if (!visited.add(path)) { + return emptyList() + } + + recursionPath.addLast(path) + try { + val result = mutableListOf() + + val libraries = variantDependencies.mainArtifact.compileDependencyList + val libraryMap = variantDependencies.librariesMap + for (library in libraries) { + val lib = libraryMap[library.key] ?: continue + if (lib.type != AndroidModels.LibraryType.Project) { + continue + } + + val module = root.findByPath(lib.projectInfo!!.projectPath) ?: continue + if (module !is ModuleProject) { + continue + } + + result.add(module) + result.addAll(module.getCompileModuleProjects(visited, recursionPath)) } - result.add(module) - result.addAll(module.getCompileModuleProjects()) + return result + } finally { + recursionPath.removeLast() } - - return result } override fun hasExternalDependency( diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/JavaModule.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/JavaModule.kt index e24dc9079a..8d11a09e2c 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/JavaModule.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/JavaModule.kt @@ -86,15 +86,21 @@ class JavaModule( override fun getModuleClasspaths(): Set = mutableSetOf(classesJar) - override fun getCompileClasspaths(excludeSourceGeneratedClassPath: Boolean): Set { + override fun getCompileClasspaths( + excludeSourceGeneratedClassPath: Boolean, + visited: MutableSet, + ): Set { + // Guard against cyclic project-dependency graphs: contribute each module's classpaths once. + if (!visited.add(path)) { + return emptySet() + } + val classpaths = if (excludeSourceGeneratedClassPath) mutableSetOf() else getModuleClasspaths().toMutableSet() getCompileModuleProjects().forEach { classpaths.addAll( - it.getCompileClasspaths( - excludeSourceGeneratedClassPath - ) + it.getCompileClasspaths(excludeSourceGeneratedClassPath, visited) ) } @@ -121,8 +127,25 @@ class JavaModule( override fun getRuntimeDexFiles(): Set = emptySet() - override fun getCompileModuleProjects(): List { + override fun getCompileModuleProjects( + visited: MutableSet, + recursionPath: ArrayDeque, + ): List { val root = IProjectManager.getInstance().workspace ?: return emptyList() + + // True cycle guard (defensive: this override is non-recursive — it returns only direct + // compile-scope module dependencies — so it cannot itself extend the recursion path, but the + // check keeps behavior consistent with AndroidModule if that ever changes). + if (recursionPath.contains(path)) { + reportDependencyCycle(recursionPath, path) + return emptyList() + } + + // Already fully expanded elsewhere (a diamond / shared dependency, not a cycle): dedup. + if (!visited.add(path)) { + return emptyList() + } + return this.dependencyList .filter { it.hasModule() && it.scope == SCOPE_COMPILE } .mapNotNull { root.findByPath(it.module.projectPath) } diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/ModuleProject.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/ModuleProject.kt index 5f45acc408..bbef5fcdaf 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/ModuleProject.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/ModuleProject.kt @@ -56,6 +56,17 @@ abstract class ModuleProject( companion object { private val log = LoggerFactory.getLogger(ModuleProject::class.java) + /** + * Format a dependency cycle as a human-readable chain, e.g. `:a -> :b -> :a`. [recursionPath] + * is the current DFS path of module paths; [repeated] is the module that closes the cycle. The + * chain starts at the first occurrence of [repeated] on the path and ends with [repeated] + * again, so it shows exactly the loop the user needs to break. + */ + internal fun formatDependencyCycle( + recursionPath: Collection, + repeated: String, + ): String = (recursionPath.dropWhile { it != repeated } + repeated).joinToString(" -> ") + @JvmStatic val COMPLETION_MODULE_KEY = Lookup.Key() } @@ -102,9 +113,24 @@ abstract class ModuleProject( * source files of this module or its dependencies. Defaults to `false`. * @return The source directories. */ - abstract fun getCompileClasspaths(excludeSourceGeneratedClassPath: Boolean): Set + fun getCompileClasspaths(excludeSourceGeneratedClassPath: Boolean): Set = + getCompileClasspaths(excludeSourceGeneratedClassPath, HashSet()) fun getCompileClasspaths() = getCompileClasspaths(false) + /** + * Variant of [getCompileClasspaths] that guards against cyclic project-dependency graphs. + * + * @param visited module paths already expanded in this traversal. Each module contributes its + * classpaths at most once; a module already in [visited] returns an empty set, so a cyclic graph + * (e.g. `:a -> :b -> :a`) terminates instead of recursing until the stack overflows. The + * user-facing cycle report is emitted once by [getCompileModuleProjects]; this method only breaks + * the cycle. + */ + abstract fun getCompileClasspaths( + excludeSourceGeneratedClassPath: Boolean, + visited: MutableSet, + ): Set + /** * Get the intermediate build output classpaths for this module. * This includes compiled .class files from the build directory that aren't packaged into JARs yet. @@ -127,7 +153,46 @@ abstract class ModuleProject( * Get the list of module projects with compile scope. This includes transitive module projects as * well. */ - abstract fun getCompileModuleProjects(): List + fun getCompileModuleProjects(): List = + getCompileModuleProjects(HashSet(), ArrayDeque()) + + /** + * Get the list of module projects with compile scope (including transitive ones), guarding against + * cyclic project-dependency graphs. + * + * @param visited The set of already-expanded module paths. Each module is expanded at most once, + * keyed by its [path], so a diamond/shared dependency is not re-expanded and a cyclic graph + * terminates instead of recursing until the stack overflows. Implementations must add their own + * [path] before recursing into dependencies and must thread the same set through every recursive + * call. + * @param recursionPath The ordered stack of module paths currently being expanded (the DFS path + * from the root call down to this module). Implementations must push their own [path] before + * recursing and pop it afterwards. It distinguishes a *true cycle* (a module that is already an + * ancestor on this path) from a harmless diamond/shared dependency (a module merely seen before + * but not an ancestor), so only real cycles are reported via [reportDependencyCycle]. + */ + abstract fun getCompileModuleProjects( + visited: MutableSet, + recursionPath: ArrayDeque, + ): List + + /** + * Report a detected project-dependency cycle as an actionable error. Called when a module is found + * to be its own ancestor in the compile-dependency graph (e.g. `:a -> :b -> :a`). The traversal + * breaks the cycle (returns without recursing again) to keep the IDE responsive; reporting it + * surfaces the misconfiguration to the user instead of silently swallowing it. + * + * @param recursionPath The current DFS path of module paths. + * @param repeated The module path that was found to already be on [recursionPath]. + */ + protected fun reportDependencyCycle(recursionPath: ArrayDeque, repeated: String) { + log.error( + "Module dependency cycle detected: {}. Breaking the cycle to keep the IDE responsive; " + + "the project's module/classpath graph may be incomplete until you remove one of these " + + "dependencies (check the build.gradle of the modules in the cycle).", + formatDependencyCycle(recursionPath, repeated), + ) + } /** * Check if the given module is a dependency of this module. diff --git a/subprojects/projects/src/test/java/com/itsaky/androidide/projects/api/CompileModuleProjectsCycleTest.kt b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/api/CompileModuleProjectsCycleTest.kt new file mode 100644 index 0000000000..5e1cfacee0 --- /dev/null +++ b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/api/CompileModuleProjectsCycleTest.kt @@ -0,0 +1,213 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.projects.api + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.project.AndroidModels +import com.itsaky.androidide.project.GradleModels +import com.itsaky.androidide.projects.ProjectManagerImpl +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Verifies [AndroidModule.getCompileModuleProjects] (via [ModuleProject.getCompileModuleProjects]) + * terminates on a *cyclic* project-dependency graph instead of recursing until the stack overflows + * (ADFA-4329 / Sentry). + * + * This builds a cyclic graph of *real* [AndroidModule] instances backed by protobuf project models + * (each module's [AndroidModels.VariantDependencies] references the next module as a `Project` + * library), registers them in a real [Workspace] on the production [ProjectManagerImpl], and drives + * the actual production traversal in [AndroidModule.getCompileModuleProjects]. + * + * On the pre-fix code (no visited-set guard) a cycle a -> b -> a recurses + * `a.getCompileModuleProjects -> b.getCompileModuleProjects -> a.getCompileModuleProjects -> ...` + * forever and blows the stack with [StackOverflowError]. With the fix it terminates and each module + * is expanded at most once. + */ +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE) +class CompileModuleProjectsCycleTest { + + @After + fun tearDown() { + runCatching { ProjectManagerImpl.getInstance().workspace = null } + } + + /** + * Build a real [AndroidModule] at Gradle [path] whose compile-scope project dependencies are + * [moduleDeps] (Gradle project paths). The dependency graph is encoded exactly the way the tooling + * layer encodes it: a [AndroidModels.GraphItem] in the main artifact's compile graph keyed to a + * [AndroidModels.Library] of type [AndroidModels.LibraryType.Project] that points at the dependency + * module via [AndroidModels.ProjectInfo.getProjectPath]. + */ + private fun androidModule(path: String, moduleDeps: List): AndroidModule { + val mainArtifact = AndroidModels.ArtifactDependencies.newBuilder() + val variantDeps = AndroidModels.VariantDependencies.newBuilder().setName("debug") + + for (depPath in moduleDeps) { + val key = "project$depPath" + mainArtifact.addCompileDependency( + AndroidModels.GraphItem.newBuilder().setKey(key).build(), + ) + variantDeps.putLibraries( + key, + AndroidModels.Library.newBuilder() + .setKey(key) + .setType(AndroidModels.LibraryType.Project) + .setProjectInfo( + AndroidModels.ProjectInfo.newBuilder() + .setBuildId(":") + .setProjectPath(depPath) + .build(), + ) + .build(), + ) + } + variantDeps.setMainArtifact(mainArtifact.build()) + + val androidProject = AndroidModels.AndroidProject.newBuilder() + .setProjectType(AndroidModels.ProjectType.LibraryProject) + .setVariantDependencies(variantDeps.build()) + .build() + + val gradleProject = GradleModels.GradleProject.newBuilder() + .setName(path.trimStart(':')) + .setPath(path) + .setProjectDirPath("/tmp/cycle-test${path.replace(':', '/')}") + .setBuildDirPath("/tmp/cycle-test${path.replace(':', '/')}/build") + .setBuildScriptPath("/tmp/cycle-test${path.replace(':', '/')}/build.gradle") + .setAndroidProject(androidProject) + .build() + + return AndroidModule(gradleProject) + } + + /** Install a real [Workspace] containing the given [modules] on the production [ProjectManagerImpl]. */ + private fun installWorkspace(vararg modules: ModuleProject) { + val root = GradleProject( + GradleModels.GradleProject.newBuilder().setName("root").setPath(":").build(), + ) + ProjectManagerImpl.getInstance().workspace = + Workspace(rootProject = root, subProjects = modules.toList(), syncIssues = emptyList()) + } + + /** A two-node cycle (`:a -> :b -> :a`) terminates and expands each module exactly once. */ + @Test(timeout = 30_000) + fun `terminates on a two-node module dependency cycle`() { + // a -> b -> a (compile scope, real AndroidModule instances) + val a = androidModule(":a", listOf(":b")) + val b = androidModule(":b", listOf(":a")) + installWorkspace(a, b) + + // Pre-fix: a -> b -> a -> b -> ... until StackOverflowError. + // With the fix: terminates; each module expanded at most once. + val result = a.getCompileModuleProjects() + + assertThat(result.map { it.path }).containsExactly(":b", ":a") + } + + /** A self-dependency (`:a -> :a`) terminates and reports the module once. */ + @Test(timeout = 30_000) + fun `terminates on a self dependency cycle`() { + val a = androidModule(":a", listOf(":a")) + installWorkspace(a) + + val result = a.getCompileModuleProjects() + + assertThat(result.map { it.path }).containsExactly(":a") + } + + /** A longer cycle (`:a -> :b -> :c -> :a`) terminates and expands each module at most once. */ + @Test(timeout = 30_000) + fun `terminates on a longer cycle and expands each module at most once`() { + // a -> b -> c -> a + val a = androidModule(":a", listOf(":b")) + val b = androidModule(":b", listOf(":c")) + val c = androidModule(":c", listOf(":a")) + installWorkspace(a, b, c) + + val result = a.getCompileModuleProjects() + + assertThat(result.map { it.path }).containsExactly(":b", ":c", ":a") + } + + /** A diamond (shared dependency reached by two paths) terminates and is not flagged as a cycle. */ + @Test(timeout = 30_000) + fun `diamond shared dependency terminates and is not treated as a cycle`() { + // a -> b -> d, a -> c -> d. `d` is a shared dependency reached by two paths, NOT a cycle: + // it must be expanded (no infinite loop) and must not trip the cycle guard. + val a = androidModule(":a", listOf(":b", ":c")) + val b = androidModule(":b", listOf(":d")) + val c = androidModule(":c", listOf(":d")) + val d = androidModule(":d", emptyList()) + installWorkspace(a, b, c, d) + + val result = a.getCompileModuleProjects() + + // Terminates (timeout would fail otherwise) and reaches every module in the diamond. + assertThat(result.map { it.path }.toSet()).containsExactly(":b", ":c", ":d") + } + + /** [AndroidModule.getCompileClasspaths] terminates on a cyclic module graph (`:a -> :b -> :a`). */ + @Test(timeout = 30_000) + fun `getCompileClasspaths terminates on a cyclic module graph`() { + // a -> b -> a. Pre-fix the sibling classpath traversal had no cross-module guard, so + // a.getCompileClasspaths -> b.getCompileClasspaths -> a.getCompileClasspaths -> ... overflowed + // the stack just like getCompileModuleProjects did. With the fix it terminates. + val a = androidModule(":a", listOf(":b")) + val b = androidModule(":b", listOf(":a")) + installWorkspace(a, b) + + val result = a.getCompileClasspaths() + + // Terminates (timeout would fail otherwise) and still contributes this module's own classpaths. + assertThat(result).contains(a.getGeneratedJar()) + } + + /** [AndroidModule.getCompileClasspaths] terminates on a self-dependency cycle (`:a -> :a`). */ + @Test(timeout = 30_000) + fun `getCompileClasspaths terminates on a self dependency cycle`() { + val a = androidModule(":a", listOf(":a")) + installWorkspace(a) + + val result = a.getCompileClasspaths() + + assertThat(result).contains(a.getGeneratedJar()) + } + + /** + * [ModuleProject.formatDependencyCycle] renders the loop starting at the first occurrence of the + * repeated module, for two-node, self, and longer cycles. + */ + @Test + fun `formatDependencyCycle renders the loop from the first occurrence of the repeated module`() { + // Two-node, self, and longer cycles render as the chain the user must break. + assertThat(ModuleProject.formatDependencyCycle(listOf(":a", ":b"), ":a")) + .isEqualTo(":a -> :b -> :a") + assertThat(ModuleProject.formatDependencyCycle(listOf(":a"), ":a")) + .isEqualTo(":a -> :a") + assertThat(ModuleProject.formatDependencyCycle(listOf(":a", ":b", ":c"), ":a")) + .isEqualTo(":a -> :b -> :c -> :a") + // The chain starts at the FIRST occurrence of the repeated node, dropping any prefix before it. + assertThat(ModuleProject.formatDependencyCycle(listOf(":a", ":b", ":c"), ":b")) + .isEqualTo(":b -> :c -> :b") + } +}