diff --git a/build-logic/conventions/src/main/kotlin/ApiContractRules.kt b/build-logic/conventions/src/main/kotlin/ApiContractRules.kt new file mode 100644 index 00000000..a46717e4 --- /dev/null +++ b/build-logic/conventions/src/main/kotlin/ApiContractRules.kt @@ -0,0 +1,106 @@ +/** + * Rules for contract (`:*:api`) modules, kept free of Gradle types so they can be unit tested. + * + * The rule here exists because of a real crash. `GitPanelApi.Panel` was declared with default + * argument values. For a `@Composable` interface member the Compose compiler emits a + * `ComposeDefaultImpls.$default` bridge that invokes the abstract method through a synthesised + * signature. While the interface and its implementation shared a module that resolved; once the + * interface moved to its own api module and the implementation stayed behind, the bridge no longer + * matched the override. It compiled, passed detekt, passed every unit test, and threw + * `AbstractMethodError` the first time the panel was composed. + * + * Nothing on the JVM side can see that, so it is checked at the source level instead. + */ +data class ApiContractViolation( + val file: String, + val line: Int, + val declaration: String, +) { + fun render(): String = + " $file:$line\n `$declaration` is a @Composable interface member with default argument " + + "values. Across a module boundary the generated defaults bridge does not match the " + + "implementation, which fails at runtime with AbstractMethodError. Require every argument." +} + +/** + * @param sources file path to file contents. Only contract modules should be passed in. + */ +fun findApiContractViolations(sources: Map): List = + sources.flatMap { (path, text) -> violationsIn(path, text) } + +private fun violationsIn(path: String, text: String): List { + val found = mutableListOf() + val lines = text.lines() + var interfaceDepth = -1 + var braceDepth = 0 + var composableSeen = false + + var index = 0 + while (index < lines.size) { + val raw = lines[index] + val line = raw.substringBefore("//").trim() + + if (interfaceDepth < 0 && Regex("""\binterface\s+\w+""").containsMatchIn(line)) { + interfaceDepth = braceDepth + } + if (line.startsWith("@Composable")) composableSeen = true + + if (composableSeen && interfaceDepth >= 0 && Regex("""\bfun\s+\w+""").containsMatchIn(line)) { + val (signature, consumed) = readSignature(lines, index) + if (hasDefaultArgument(signature)) { + val name = Regex("""\bfun\s+(\w+)""").find(signature)?.groupValues?.get(1).orEmpty() + found += ApiContractViolation(path, index + 1, "fun $name(...)") + } + index += consumed + composableSeen = false + continue + } + + if (line.isNotEmpty() && !line.startsWith("@")) { + braceDepth += line.count { it == '{' } - line.count { it == '}' } + if (interfaceDepth >= 0 && braceDepth <= interfaceDepth) interfaceDepth = -1 + } + index++ + } + return found +} + +/** Collects a possibly multi-line signature up to the closing paren of its parameter list. */ +private fun readSignature(lines: List, start: Int): Pair { + val builder = StringBuilder() + var depth = 0 + var consumed = 0 + var opened = false + for (i in start until lines.size) { + val line = lines[i].substringBefore("//") + builder.append(line).append('\n') + consumed++ + depth += line.count { it == '(' } - line.count { it == ')' } + if (line.contains('(')) opened = true + if (opened && depth <= 0) break + } + return builder.toString() to consumed +} + +/** True when the parameter list contains a top-level `=`, i.e. a default value. */ +private fun hasDefaultArgument(signature: String): Boolean { + val params = signature.substringAfter('(', "").substringBeforeLast(')', "") + var depth = 0 + var index = 0 + while (index < params.length) { + when (params[index]) { + '(', '<', '[' -> depth++ + ')', '>', ']' -> depth-- + '-' -> if (index + 1 < params.length && params[index + 1] == '>') index++ // lambda arrow + '=' -> { + val next = params.getOrNull(index + 1) + // `=` that is not part of `==`, `>=`, `<=` or `->` + if (depth == 0 && next != '=' && params.getOrNull(index - 1) !in listOf('!', '<', '>', '=')) { + return true + } + } + } + index++ + } + return false +} diff --git a/build-logic/conventions/src/main/kotlin/AslModuleBoundariesConventionPlugin.kt b/build-logic/conventions/src/main/kotlin/AslModuleBoundariesConventionPlugin.kt index 4ee671b1..be0724d2 100644 --- a/build-logic/conventions/src/main/kotlin/AslModuleBoundariesConventionPlugin.kt +++ b/build-logic/conventions/src/main/kotlin/AslModuleBoundariesConventionPlugin.kt @@ -5,6 +5,7 @@ import org.gradle.api.Project import org.gradle.api.artifacts.ProjectDependency import org.gradle.api.file.RegularFileProperty import org.gradle.api.provider.ListProperty +import org.gradle.api.provider.MapProperty import org.gradle.api.provider.SetProperty import org.gradle.api.tasks.Input import org.gradle.api.tasks.OutputFile @@ -41,9 +42,21 @@ class AslModuleBoundariesConventionPlugin : Plugin { // regardless of what is declared. The provider is realised once every project is // evaluated, and only Strings are stored, which keeps it configuration-cache safe. edges.set(target.provider { target.collectProjectEdges() }) + contractSources.set(target.provider { target.collectContractSources() }) } } + /** Kotlin sources of every contract module, keyed by a repo-relative path. */ + private fun Project.collectContractSources(): Map = allprojects + .filter { it.path.endsWith(":api") } + .flatMap { project -> + project.file("src/main").walkTopDown() + .filter { it.isFile && it.extension == "kt" } + .map { it.relativeTo(rootDir).path to it.readText() } + .toList() + } + .toMap() + private fun Project.collectProjectEdges(): List = allprojects .flatMap { project -> project.configurations.flatMap { configuration -> @@ -69,6 +82,9 @@ abstract class VerifyModuleBoundariesTask : DefaultTask() { @get:Input abstract val baseline: SetProperty + @get:Input + abstract val contractSources: MapProperty + @get:OutputFile abstract val report: RegularFileProperty @@ -102,6 +118,17 @@ abstract class VerifyModuleBoundariesTask : DefaultTask() { staleBaseline.sorted().forEach { logger.lifecycle(" $it") } } + val contractViolations = findApiContractViolations(contractSources.get()) + if (contractViolations.isNotEmpty()) { + throw GradleException( + buildString { + appendLine("Contract module violations (${contractViolations.size}):") + appendLine() + contractViolations.forEach { appendLine(it.render()) } + }, + ) + } + if (violations.isNotEmpty()) { throw GradleException( buildString { diff --git a/build-logic/conventions/src/test/kotlin/ApiContractRulesTest.kt b/build-logic/conventions/src/test/kotlin/ApiContractRulesTest.kt new file mode 100644 index 00000000..842f8e6d --- /dev/null +++ b/build-logic/conventions/src/test/kotlin/ApiContractRulesTest.kt @@ -0,0 +1,137 @@ +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class ApiContractRulesTest { + + private fun violations(source: String) = findApiContractViolations(mapOf("Api.kt" to source)) + + @Test + fun `composable interface member with a default argument is a violation`() { + val found = violations( + """ + interface PanelApi { + @Composable + fun Panel(id: String, onClose: () -> Unit = {}) + } + """.trimIndent(), + ) + assertEquals(1, found.size) + assertTrue(found.single().declaration.contains("Panel")) + } + + @Test + fun `the real regression is caught across multiple lines`() { + // The exact shape that shipped an AbstractMethodError. + val found = violations( + """ + interface GitPanelApi { + @Composable + fun Panel( + projectId: String, + onClose: () -> Unit, + onOpenDiff: (String, GitDiffTarget) -> Unit = { _, _ -> }, + onOpenHistory: () -> Unit = {}, + ) + } + """.trimIndent(), + ) + assertEquals(1, found.size) + } + + @Test + fun `composable interface member without defaults is fine`() { + val found = violations( + """ + interface PanelApi { + @Composable + fun Panel( + projectId: String, + onClose: () -> Unit, + onOpenDiff: (String, GitDiffTarget) -> Unit, + ) + } + """.trimIndent(), + ) + assertTrue(found.isEmpty()) + } + + // ---- cases that must NOT be reported -------------------------------------------------- + + @Test + fun `a non-composable interface member may keep its defaults`() { + // Plain interface methods do not go through the Compose defaults bridge, so they are safe. + val found = violations( + """ + interface BuildRunApi { + suspend fun clear(buildId: String? = null) + } + """.trimIndent(), + ) + assertTrue(found.isEmpty()) + } + + @Test + fun `a composable that is not an interface member may keep its defaults`() { + val found = violations( + """ + @Composable + fun Standalone(text: String, modifier: Modifier = Modifier) { } + """.trimIndent(), + ) + assertTrue(found.isEmpty()) + } + + @Test + fun `a data class default value is not mistaken for one`() { + val found = violations( + """ + data class BuildClientMeta( + val projectId: String, + val autoLaunchAfterInstall: Boolean = true, + ) + """.trimIndent(), + ) + assertTrue(found.isEmpty()) + } + + @Test + fun `a class implementing the interface may keep its overrides`() { + val found = violations( + """ + class PanelApiImpl : PanelApi { + @Composable + override fun Panel(id: String, onClose: () -> Unit) { } + } + """.trimIndent(), + ) + assertTrue(found.isEmpty()) + } + + @Test + fun `comparison operators in a default expression are not read as defaults`() { + val found = violations( + """ + interface PanelApi { + @Composable + fun Panel(id: String, onClose: () -> Unit) + fun compare(a: Int, b: Int): Boolean + } + """.trimIndent(), + ) + assertTrue(found.isEmpty()) + } + + @Test + fun `each offending declaration is reported once with its file and line`() { + val found = findApiContractViolations( + mapOf( + "A.kt" to "interface A {\n @Composable\n fun X(a: Int = 1)\n}", + "B.kt" to "interface B {\n @Composable\n fun Y(b: Int)\n}", + ), + ) + assertEquals(1, found.size) + assertEquals("A.kt", found.single().file) + assertEquals(3, found.single().line) + } +}