From 533d8643b2fef33c4d40ab384042c14530679658 Mon Sep 17 00:00:00 2001 From: Atanas Oreshkov Date: Mon, 10 Aug 2026 22:02:12 +0300 Subject: [PATCH] Hold the stability tiers with a gate instead of a script in a private directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 23 split the Kotlin API into a stable core and an explicit `@RaboshExperimental` tier, and gave the published modules a module-wide opt-in so the engine's use of its own internals did not need several hundred `@OptIn`s. That opt-in has a consequence which was noticed then and left open: the compiler will not report a public signature that *names* an experimental type without carrying the marker. Inside the library every such use is permitted, so a consumer meets the signature and is handed an experimental type with nothing having asked them to opt in. An ABI dump cannot see it either — the JVM format writes signatures and never annotations. The gap is not theoretical. Run by hand in phase 23 it found four leaks, and phase 24 then added public surface to every module in the chain with nothing but an awk script in a private directory standing behind the claim. `ApiTierAudit` is that script, promoted: plain Kotlin over `File` beside `PublishedModules` and `CentralBundleReport`, with fifteen unit tests, and a root `checkApiTiers` task hung off `check` so `./gradlew build` runs it and CI needs no new step. Root rather than per-module because the leak is cross-module — a type marked in `rabosh-index` leaks through a signature in `rabosh-query`'s dump. Both halves are derived and neither is listed. The marker set comes from the sources, the surface from the committed dumps. A hand-maintained list of experimental types in a build script would be a second list free to disagree with the annotations, and it would disagree exactly once, silently, in the direction of not reporting a leak. That is the `PublishedModules` rule applied to a different question, and it is why the awk script is deleted rather than kept beside this one. Three decisions inside it. An annotation the scanner cannot attribute is a failure and never a shorter set, because under-reporting is the only failure mode that matters here: an audit that misses a leak passes, and passing is what it is read for. Nesting follows indentation rather than counting braces, because Kotlin string templates put braces inside string literals and a counter needs a lexer to be right. And a missing dump is skipped rather than failed, because `checkKotlinAbi` already owns that. Promoting it found a bug the hand-written version never had to have. The awk script carried a hard-coded type list, so it never had to work out which type a marked *member* belonged to; the derived version does, and its first attempt took the most recent type declaration above the annotation — which put `IndexCatalog.read` inside a `private class` declared two hundred lines earlier and reported a correctly marked method as a leak. A false positive is what gets a gate switched off, so the sibling case is a test rather than an assumption, and the comparison is now strictly less than the declaration's column. Verified by breaking it: removing the marker from `IndexCatalog.read` fails `checkApiTiers` naming the method, the type it exposes and the module. Co-Authored-By: Claude Opus 5 --- .claude/rules/build-and-release.md | 34 +- CHANGELOG.md | 17 + CLAUDE.md | 4 + CONTRIBUTING.md | 7 + README.md | 1 + STABILITY.md | 15 +- .../app/oreshkov/rabosh/build/ApiTierAudit.kt | 328 +++++++++++++++++ .../main/kotlin/rabosh.api-tiers.gradle.kts | 66 ++++ .../kotlin/rabosh.kotlin-library.gradle.kts | 5 + .../oreshkov/rabosh/build/ApiTierAuditTest.kt | 342 ++++++++++++++++++ build.gradle.kts | 4 +- 11 files changed, 812 insertions(+), 11 deletions(-) create mode 100644 build-logic/src/main/kotlin/app/oreshkov/rabosh/build/ApiTierAudit.kt create mode 100644 build-logic/src/main/kotlin/rabosh.api-tiers.gradle.kts create mode 100644 build-logic/src/test/kotlin/app/oreshkov/rabosh/build/ApiTierAuditTest.kt diff --git a/.claude/rules/build-and-release.md b/.claude/rules/build-and-release.md index e40a390..800a207 100644 --- a/.claude/rules/build-and-release.md +++ b/.claude/rules/build-and-release.md @@ -115,13 +115,33 @@ experimental type to be marked too, and that cascade ends with the stable core i tier. `SegmentObserver` is that cascade caught at one step: `RaboshOptions`' constructor names it, so marking the interface would have put `RaboshOptions(...)` behind an opt-in. It is stable, deliberately. -**The gate is `rabosh-samples` not opting in, and the ABI dumps are not the gate.** The JVM dump -format writes signature lines and never annotations — verified in the dumper, and confirmed by the -markers changing the committed dumps by exactly one entry, the annotation class itself — so a -declaration changing tier is invisible to `checkKotlinAbi`. What catches it is the samples module: -`:rabosh-api` and nothing else, `allWarningsAsErrors`, part of `build`, and the one module the -opt-in is deliberately withheld from. Do not "tidy" that asymmetry by giving every module the same -compiler options; `rabosh.kotlin-library`, `rabosh-testkit` and `rabosh-bench` opt in, samples do not. +**Two gates, and neither is `checkKotlinAbi`.** The JVM dump format writes signature lines and never +annotations — verified in the dumper, and confirmed by the markers changing the committed dumps by +exactly one entry, the annotation class itself — so a declaration changing tier is invisible to it. + +The first gate is **`rabosh-samples` not opting in**: `:rabosh-api` and nothing else, +`allWarningsAsErrors`, part of `build`, and the one module the opt-in is deliberately withheld from. +Do not "tidy" that asymmetry by giving every module the same compiler options; +`rabosh.kotlin-library`, `rabosh-testkit` and `rabosh-bench` opt in, samples do not. + +The second is **`checkApiTiers`**, and it exists because the first one only sees what a sample +happens to call. Module-wide opt-in blinds the compiler to a public signature that *names* an +experimental type without carrying the marker — a consumer meeting it gets handed an experimental +type with nothing having asked them to opt in. `ApiTierAudit` reads the marker set from the +**sources** and the surface from the **committed dumps**, both derived and neither listed, for the +`PublishedModules` reason: a hand-maintained list of experimental types would disagree with the +annotations exactly once, silently, in the direction of not reporting a leak. It is a root task, +because the leak is cross-module — a type marked in `rabosh-index` leaks through a signature in +`rabosh-query`'s dump — and it hangs off the root `check`, so `./gradlew build` runs it. + +Three things about it that are decisions. **An annotation the scanner cannot attribute is a failure, +not a shorter set**: under-reporting is the only failure mode that matters, because an audit that +misses a leak passes and passing is what it is read for. **Nesting is followed by indentation rather +than by counting braces**, because Kotlin string templates put braces inside string literals and a +counter needs a lexer to be right; the comparison is *strictly* less than the declaration's column, so +a sibling `private class` declared beside a marked function is not mistaken for its enclosing scope — +which the first version did, reporting `IndexCatalog.read` as a leak. And **a missing dump is skipped +rather than failed**, because `checkKotlinAbi` already owns that and owning it twice is worse. ## Native access: the flag nobody needs diff --git a/CHANGELOG.md b/CHANGELOG.md index c4c98a8..afd343a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,23 @@ else may change in any release. That claim lives in [STABILITY.md](STABILITY.md) ### Added +- **`checkApiTiers` — the stability tiers are now held by a gate rather than by a script.** The + module-wide opt-in that keeps the engine from needing several hundred `@OptIn`s also blinds the + compiler to a public signature that *names* an experimental type without carrying the marker, and + an ABI dump writes signatures and never annotations — so between them nothing was checking that + `STABILITY.md`'s claim was still true. Run by hand it had already found four such leaks. + + `ApiTierAudit` reads the marker set from the **sources** and the surface from the **committed + dumps**, both derived and neither listed: a hand-maintained list of experimental types would + disagree with the annotations exactly once, silently, in the direction of not reporting a leak. It + is a root task, because a type marked in one module leaks through another module's dump, and it + hangs off `check`, so `./gradlew build` runs it. + + Promoting it found a bug the hand-written version never had to have: attributing a marked member to + the *most recent* type declaration rather than the enclosing one put `IndexCatalog.read` inside a + `private class` two hundred lines above it. Nesting now follows indentation, and the sibling case is + a test. + - **`Rabosh.checkpoint(target)` — a consistent copy, taken while you are writing.** The recipe it replaces was *stop writing and copy the directory*, which a desktop application cannot do because it is the writer. The database is flushed, a snapshot is pinned, and the copy is of what that snapshot diff --git a/CLAUDE.md b/CLAUDE.md index b97e9da..a9d5fe2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,6 +57,10 @@ them at the latest stable release; do not adopt pre-releases (e.g. Kotlin `-Beta dumps live at `/api/.api`. - **`build-logic/` is an included build, and its tests are not part of the root `build`** — CI runs `./gradlew -p build-logic check` as its own step. +- **`checkApiTiers` is the gate `checkKotlinAbi` cannot be**: a dump carries signatures and never + annotations, and module-wide opt-in blinds the compiler, so a public signature exposing a + `@RaboshExperimental` type without carrying the marker is invisible to both. It is a root task and + runs under `build`. - **`gradle.properties` stays `0.1.0-SNAPSHOT`**: `release.yml` derives the release version from the git tag and nowhere else. Do not "fix" it to a release number. - **The format claim lives in `COMPATIBILITY.md` and the API claim in `STABILITY.md`, each in one diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 88efe68..509e25f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,6 +53,13 @@ and commit the result. The tasks are `checkKotlinAbi` and `updateKotlinAbi`; the that Kotlin still registers are deprecated shims and using them is a build that breaks on the next upgrade for no benefit today. +**A dump says nothing about stability tiers**, so a second check runs beside it. `checkApiTiers` — +also part of `build` — fails when a public signature exposes a `@RaboshExperimental` type without +carrying the marker itself. If it names your declaration, either mark it or move the type into the +stable core and say so in [STABILITY.md](STABILITY.md); the one thing not to do is leave a consumer +holding an experimental type that nothing asked them to opt in to. It reads the marker set from the +sources, so adding a marker is all that is needed to teach it. + **2. A new dependency needs agreement first, and the runtime scope is closed.** "No runtime dependencies at all" is a claim the README makes, so it has to stay true — the JSON parser, the compressed bitmap, the HyperLogLog, the bloom filter and the property-test harness are all in-repo diff --git a/README.md b/README.md index f4858fd..aa4d9f1 100644 --- a/README.md +++ b/README.md @@ -636,6 +636,7 @@ Requires JDK 25. ./gradlew build # compile, test, and check the public ABI against the committed dumps ./gradlew test # tests only ./gradlew updateKotlinAbi # after an intentional public API change +./gradlew checkApiTiers # no unmarked signature exposes an experimental type (part of build) ./gradlew dokkaGenerate # the aggregated API site, into build/dokka/html ``` diff --git a/STABILITY.md b/STABILITY.md index f3aedf4..bd3fb81 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -122,16 +122,25 @@ they do **not** carry is the markers: the JVM dump format writes signature lines annotations, and the synthetic method Kotlin emits for an annotated property is filtered out as synthetic. A declaration changing tier is invisible to it. -**`rabosh-samples` is what holds the tiers.** It depends on `:rabosh-api` and nothing else, it +**`rabosh-samples` is what holds the stable core.** It depends on `:rabosh-api` and nothing else, it compiles with `allWarningsAsErrors`, it is part of `./gradlew build`, and — unlike every other module in the repository — it deliberately does **not** opt in to `@RaboshExperimental`. It is therefore a real consumer compiling against the stable core with no opt-in. A stable declaration that silently acquires the marker fails there, and so does a sample that reaches past the facade. That asymmetry is load-bearing and should not be tidied away by giving every module the same build configuration. -Verified by breaking it, which is this repository's standing rule for a check nobody has watched +**`checkApiTiers` holds the other direction**, which a sample cannot: that no unmarked public +signature *exposes* an experimental type. Module-wide opt-in means the compiler permits exactly that +inside the library, so a consumer could be handed a `ColumnReader` by a method carrying no marker at +all — the tier statement above quietly ceasing to be true. The audit reads the marker set from the +sources and the surface from the committed dumps, both derived rather than listed, and runs at the +root because the leak is cross-module. It found four leaks the first time it was run by hand. + +Verified by breaking them, which is this repository's standing rule for a check nobody has watched fail: adding `db.store.flush()` to a sample fails `./gradlew build` with the opt-in error naming the -marker, and commenting out the opt-in in `rabosh.kotlin-library` fails the published modules. +marker; commenting out the opt-in in `rabosh.kotlin-library` fails the published modules; and removing +the marker from `IndexCatalog.read` fails `checkApiTiers` naming the method, the type it exposes and +the module. ## Reporting diff --git a/build-logic/src/main/kotlin/app/oreshkov/rabosh/build/ApiTierAudit.kt b/build-logic/src/main/kotlin/app/oreshkov/rabosh/build/ApiTierAudit.kt new file mode 100644 index 0000000..89db54c --- /dev/null +++ b/build-logic/src/main/kotlin/app/oreshkov/rabosh/build/ApiTierAudit.kt @@ -0,0 +1,328 @@ +package app.oreshkov.rabosh.build + +import java.io.File + +/** + * Whether any declaration outside the experimental tier exposes a type that is inside it. + * + * **This exists because the compiler cannot do it, and the reason is worth stating precisely.** + * `STABILITY.md` marks the entrances to the experimental tier rather than every member, and the + * published modules opt in to their own marker module-wide so that the engine's internal use of its + * own internals does not need several hundred `@OptIn`s. That opt-in is what blinds the compiler: + * inside the library every use of an experimental type is permitted, *including a use in a public + * signature that carries no marker of its own*. A consumer meeting that signature gets an + * experimental type handed to them with nothing having asked them to opt in — the tier statement + * quietly stops being true, and `checkKotlinAbi` cannot see it because the JVM dump format writes + * signatures and never annotations. + * + * The gap is real rather than theoretical: it was found by hand in phase 23 with four leaks in it, + * and phase 24 added public surface to every module in the chain with nothing but a script in a + * private directory standing behind the claim. This is that script, promoted to a gate. + * + * **Both halves are derived, neither is listed.** The experimental set comes from the *sources* — + * every `@RaboshExperimental` and the declaration under it — and the surface comes from the + * *committed ABI dumps*. A hand-maintained list of experimental types in a build script would be a + * second list free to disagree with the annotations, and it would disagree exactly once, silently, + * in the direction of not reporting a leak. That is the [PublishedModules] rule applied to a + * different question. + * + * **A scanner that does not understand an annotation is the defect this class exists to prevent, one + * level up**, so [markedIn] counts the annotations it found against the declarations it attributed + * and reports the difference rather than carrying on with a short set. Under-reporting is the only + * failure mode that matters here: an audit that misses a leak passes, and passing is what it is + * consulted for. + * + * Plain Kotlin over [File] with unit tests, for the reason [BenchmarkRunReport] and + * [CentralBundleReport] are: an included build's tests are run by `./gradlew -p build-logic check`, + * and a decision that fails the build is worth testing. + */ +object ApiTierAudit { + + /** The opt-in marker, by simple name. Its package is not needed: nothing else is called this. */ + const val MARKER: String = "RaboshExperimental" + + /** What the sources say is experimental. */ + class Tier( + /** Simple names of types the marker is on. */ + val types: Set, + /** `Type.member` for members the marker is on; a constructor is spelled `Type.constructor`. */ + val members: Set, + ) { + val isEmpty: Boolean get() = types.isEmpty() && members.isEmpty() + + override fun toString(): String = "Tier(${types.size} type(s), ${members.size} member(s))" + } + + /** One public signature naming an experimental type without being marked itself. */ + class Leak( + val module: String, + val owner: String, + val member: String, + val referenced: String, + ) { + override fun toString(): String = + "$module: $owner.$member exposes $referenced, which is @$MARKER, without being marked itself" + } + + /** + * The declarations [MARKER] is applied to, read from the Kotlin sources under [modules]. + * + * Two spellings are recognised because both are in use: the annotation on its own line above a + * declaration, and inline as `class Foo @RaboshExperimental constructor(`. A third would be a + * silent hole, which is what [UnreadableMarker] is for. + * + * @throws IllegalStateException if an annotation was found that no declaration could be + * attributed to. See the class documentation: a short set is worse than a failure. + */ + fun markedIn(modules: List): Tier { + val types = sortedSetOf() + val members = sortedSetOf() + val unreadable = ArrayList() + + for (module in modules) { + val sourceRoot = File(module, "src/main/kotlin") + if (!sourceRoot.isDirectory) continue + for (file in sourceRoot.walkTopDown()) { + if (!file.isFile || file.extension != "kt") continue + scan(file, types, members, unreadable) + } + } + + check(unreadable.isEmpty()) { + "the tier audit found ${unreadable.size} @$MARKER annotation(s) it could not attribute to a " + + "declaration, so its experimental set is incomplete and would under-report:\n" + + unreadable.joinToString("\n") { " $it" } + + "\nTeach ApiTierAudit the spelling, or spell the declaration the way the others are." + } + return Tier(types, members) + } + + /** + * Every unmarked public signature in [dumps] that names a type in [tier]. + * + * Empty is the passing answer. The check runs over the *committed* dumps rather than over + * compiled classes because the dumps are the artefact `checkKotlinAbi` already maintains: a + * signature that is not in them is not published, and one that is has been reviewed in a diff. + */ + fun leaks(dumps: Map, tier: Tier): List { + if (tier.isEmpty) return emptyList() + val found = ArrayList() + for ((module, dump) in dumps) { + if (!dump.isFile) continue + var owner = "" + var ownerNames = emptyList() + var ownerIsExperimental = false + + for (line in dump.readLines()) { + if (line.isBlank() || line == "}") continue + if (!line.startsWith("\t")) { + owner = binaryNameOf(line) ?: "" + ownerNames = simpleNamesOf(owner) + ownerIsExperimental = ownerNames.any { it in tier.types } + // A supertype list is on this line too, so an unmarked class implementing an + // experimental interface is caught here rather than by any member below it. + // Matched as a *bare* binary name rather than as a descriptor: a header spells a + // supertype `: app/oreshkov/…/Bitmap`, with none of the `L…;` a signature has. + if (!ownerIsExperimental) { + for (referenced in experimentalTypesIn(line.substringAfter(owner), tier, descriptors = false)) { + found += Leak(module, simpleOwner(ownerNames), "(supertype)", referenced) + } + } + continue + } + if (ownerIsExperimental) continue + + val member = memberNameOf(line) ?: continue + if (isMarked(ownerNames, member, tier)) continue + for (referenced in experimentalTypesIn(line, tier)) { + found += Leak(module, simpleOwner(ownerNames), member, referenced) + } + } + } + return found.distinctBy { "${it.module}|${it.owner}|${it.member}|${it.referenced}" } + } + + /** The published modules' dumps, by module name. The universe is [PublishedModules]'. */ + fun dumpsUnder(root: File): Map = + PublishedModules.under(root).associateWith { File(root, "$it/api/$it.api") } + + /** The published modules' directories. */ + fun modulesUnder(root: File): List = + PublishedModules.under(root).map { File(root, it) } + + // --- reading the sources ---------------------------------------------------------------------- + + private val TYPE_DECLARATION = + Regex("""^\s*(?:public |internal |private |protected )?(?:[\w@.]+ )*?(class|interface|object)\s+(\w+)""") + + private val FUNCTION_DECLARATION = Regex("""\bfun\s+(?:<[^>]*>\s*)?(\w+)\s*[(<]""") + private val PROPERTY_DECLARATION = Regex("""\b(?:val|var)\s+(\w+)\s*[:=]""") + private val INLINE_CONSTRUCTOR = Regex("""@$MARKER\s+constructor\s*\(""") + + /** One open type declaration and the column it was declared at. */ + private class Scope(val name: String, val indent: Int) + + /** + * Scans one file, tracking which type each declaration is inside. + * + * **Nesting is followed by indentation rather than by counting braces**, and the choice is a + * decision. Kotlin string templates put `{` and `}` inside string literals, so a brace counter + * needs a lexer to be right and is wrong in a way nobody notices until it mis-attributes one + * member. Indentation needs no lexer and is exact for any code laid out the way this repository's + * is: a declaration at column *n* is inside the nearest type declared at a column below *n*. + * + * Getting this wrong was demonstrated rather than imagined — the first version took the *most + * recent* type declaration, and `IndexCatalog.read` came out attributed to a `private class` + * declared two hundred lines above it, which reports a marked member as a leak. A false positive + * is what gets a gate switched off, so it is worth the ten lines. + * + * A `companion object` re-pushes its **outer** name: in a dump its members appear on + * `Foo$Companion` and statically on `Foo`, and attributing them to `Foo` is what makes both + * spellings match the one source declaration. + */ + private fun scan(file: File, types: MutableSet, members: MutableSet, unreadable: MutableList) { + val lines = file.readLines() + val scopes = ArrayList() + + fun enclosing(indent: Int): String = scopes.lastOrNull { it.indent < indent }?.name.orEmpty() + + for ((index, line) in lines.withIndex()) { + val indent = line.indexOfFirst { !it.isWhitespace() } + if (indent >= 0) { + val declared = typeNameOf(line) + val companion = line.contains("companion object") + if (declared != null || companion) { + while (scopes.isNotEmpty() && scopes.last().indent >= indent) scopes.removeLast() + scopes += Scope(declared ?: enclosing(indent), indent) + } + } + + if (!line.contains("@$MARKER")) continue + val at = if (indent >= 0) indent else 0 + + // Inline: `public class SchemaCatalog @RaboshExperimental constructor(`. + if (INLINE_CONSTRUCTOR.containsMatchIn(line)) { + val owner = typeNameOf(line) ?: enclosing(at) + if (owner.isEmpty()) { + unreadable += "${file.name}:${index + 1} — inline constructor with no type on the line" + } else { + members += "$owner.constructor" + } + continue + } + // An import, a KDoc reference, or an `@OptIn` naming it: not an application of it. + if (!isAnnotationApplication(line)) continue + + val declaration = declarationAfter(lines, index) + if (declaration == null) { + unreadable += "${file.name}:${index + 1} — no declaration follows the annotation" + continue + } + val type = typeNameOf(declaration) + if (type != null) { + types += type + continue + } + val name = FUNCTION_DECLARATION.find(declaration)?.groupValues?.get(1) + ?: PROPERTY_DECLARATION.find(declaration)?.groupValues?.get(1) + // The declaration's own column, and the comparison is **strictly** less than it. A + // sibling declared at the same column — `private class CountedObservation` beside + // `public fun read` — is not an enclosing scope, and treating it as one is exactly the + // mis-attribution this tracking exists to avoid. + val owner = enclosing(declaration.indexOfFirst { !it.isWhitespace() }.coerceAtLeast(0)) + if (name == null || owner.isEmpty()) { + unreadable += "${file.name}:${index + 1} — cannot name the declaration: ${declaration.trim()}" + } else { + members += "$owner.$name" + } + } + } + + /** + * Whether this line *applies* the marker rather than merely mentioning it. + * + * An `import`, a KDoc line and an `@OptIn(RaboshExperimental::class)` all contain the word. Only + * the first token being the annotation means it is being applied here. + */ + private fun isAnnotationApplication(line: String): Boolean { + val trimmed = line.trim() + if (trimmed.startsWith("import ") || trimmed.startsWith("*") || trimmed.startsWith("//")) return false + return trimmed.startsWith("@$MARKER") + } + + /** The next line that is neither blank, a comment, nor another annotation. */ + private fun declarationAfter(lines: List, from: Int): String? { + for (index in from + 1 until lines.size) { + val trimmed = lines[index].trim() + if (trimmed.isEmpty() || trimmed.startsWith("//") || trimmed.startsWith("*") || + trimmed.startsWith("/*") || trimmed.startsWith("@") + ) { + continue + } + return lines[index] + } + return null + } + + private fun typeNameOf(line: String): String? { + if (line.contains("companion object")) return null + val match = TYPE_DECLARATION.find(line) ?: return null + // `object : Foo {` and `enum class` both land here; only a named declaration counts. + return match.groupValues[2].takeIf { it.isNotEmpty() } + } + + // --- reading the dumps ------------------------------------------------------------------------ + + /** `public final class app/oreshkov/rabosh/index/Bitmap : … {` -> the binary name. */ + private fun binaryNameOf(header: String): String? = + Regex("""\b(app/oreshkov/[\w/$]+)""").find(header)?.groupValues?.get(1) + + /** `app/oreshkov/rabosh/query/Predicate$And` -> `[Predicate, And]`. */ + private fun simpleNamesOf(binaryName: String): List = + binaryName.substringAfterLast('/').split('$').filter { it.isNotEmpty() } + + private fun simpleOwner(names: List): String = names.joinToString(".").ifEmpty { "?" } + + /** `\tpublic final fun getStore ()L…;` -> `getStore`. Fields and constructors included. */ + private fun memberNameOf(line: String): String? = + Regex("""\b(?:fun|field)\s+([\w$<>]+)""").find(line)?.groupValues?.get(1) + + /** + * Whether [member] of a class named by [ownerNames] carries the marker in the sources. + * + * The dump's spelling and the source's differ in three ways and all three are normalised here: a + * property is `getFoo`/`setFoo` against `foo`, a constructor is `` against `constructor`, + * and a default-argument bridge is `foo$default` against `foo`. Every enclosing name is tried, so + * a member on `Foo$Companion` matches a source declaration attributed to `Foo`. + */ + private fun isMarked(ownerNames: List, member: String, tier: Tier): Boolean { + val candidates = LinkedHashSet() + candidates += member + candidates += member.substringBefore("\$default") + if (member == "") candidates += "constructor" + for (prefix in listOf("get", "set")) { + if (member.length > prefix.length && member.startsWith(prefix) && member[prefix.length].isUpperCase()) { + candidates += member.removePrefix(prefix).replaceFirstChar { it.lowercase() } + } + } + return ownerNames.any { owner -> candidates.any { "$owner.$it" in tier.members } } + } + + /** + * Every experimental type this line names, in order of appearance. + * + * Two spellings, because the dump has two. A *signature* carries JVM descriptors — + * `Lapp/oreshkov/rabosh/index/Bitmap;` — and matching those with delimiters is what stops + * `…/BitmapView` being read as `Bitmap`. A *class header* carries bare binary names after the + * colon, with no `L` and no `;`, so the supertype case has to ask for the looser form; there the + * `$`-split of a whole name is the boundary instead. + */ + private fun experimentalTypesIn(line: String, tier: Tier, descriptors: Boolean = true): List { + val pattern = if (descriptors) Regex("""L(app/oreshkov/[\w/$]+);""") else Regex("""\b(app/oreshkov/[\w/$]+)""") + return pattern.findAll(line) + .flatMap { simpleNamesOf(it.groupValues[1]).asSequence() } + .filter { it in tier.types } + .distinct() + .toList() + } +} diff --git a/build-logic/src/main/kotlin/rabosh.api-tiers.gradle.kts b/build-logic/src/main/kotlin/rabosh.api-tiers.gradle.kts new file mode 100644 index 0000000..50b28b9 --- /dev/null +++ b/build-logic/src/main/kotlin/rabosh.api-tiers.gradle.kts @@ -0,0 +1,66 @@ +import app.oreshkov.rabosh.build.ApiTierAudit + +// Applied at the root and nowhere else, because the question is cross-module: a type marked +// experimental in `rabosh-index` can leak through a signature in `rabosh-query`'s dump, and no +// per-module task can see both. +// +// This is the gate `checkKotlinAbi` cannot be. The JVM dump format writes signatures and never +// annotations, so a declaration changing tier is invisible to it — and the published modules opt in +// to their own marker module-wide, so it is invisible to the *compiler* too. Between them that +// leaves the tier statement in `STABILITY.md` held up by nothing, which is what this fixes. + +/** + * Fails when a public signature exposes an experimental type without being marked itself. + * + * The rule lives in [ApiTierAudit] rather than here, for the reason `CentralBundleReport` and + * `BenchmarkRunReport` do: a decision that fails a build is worth unit tests, and an included build's + * tests are run by `./gradlew -p build-logic check` rather than by the root `build`. + * + * Both inputs are declared, so this is up to date when neither the sources nor the dumps have moved — + * and it re-runs when either has, which is exactly when the answer can change. + */ +val checkApiTiers = tasks.register("checkApiTiers") { + group = "verification" + description = "Checks that no unmarked public signature exposes a @RaboshExperimental type." + + val root = layout.projectDirectory.asFile + val modules = ApiTierAudit.modulesUnder(root) + val dumps = ApiTierAudit.dumpsUnder(root) + + inputs.files(dumps.values.filter { it.isFile }).withPropertyName("abiDumps") + inputs.files(modules.map { File(it, "src/main/kotlin") }.filter { it.isDirectory }) + .withPropertyName("sources") + .withPathSensitivity(PathSensitivity.RELATIVE) + // Nothing is produced, so up-to-date needs somewhere to record that it ran. + outputs.file(layout.buildDirectory.file("reports/api-tiers/checked.txt")) + + val report = layout.buildDirectory.file("reports/api-tiers/checked.txt") + + doLast { + val tier = ApiTierAudit.markedIn(modules) + val leaks = ApiTierAudit.leaks(dumps, tier) + + val summary = buildString { + appendLine("experimental tier: $tier") + appendLine("dumps checked: ${dumps.size}") + appendLine("leaks: ${leaks.size}") + for (leak in leaks) appendLine(" $leak") + } + report.get().asFile.apply { parentFile.mkdirs() }.writeText(summary) + + if (leaks.isNotEmpty()) { + throw GradleException( + "${leaks.size} public signature(s) expose a @${ApiTierAudit.MARKER} type without carrying " + + "the marker, so a consumer reaches the experimental tier with nothing asking them to " + + "opt in:\n" + leaks.joinToString("\n") { " $it" } + + "\n\nMark the declaration, or move the type into the stable core and say so in " + + "STABILITY.md. See ApiTierAudit for why the compiler cannot report this.", + ) + } + logger.lifecycle("API tiers: ${dumps.size} dump(s) checked against $tier, no leaks.") + } +} + +// `check` is the root's, from `base` by way of the Dokka plugin, and `build` depends on it — so this +// runs under the `./gradlew build` that CI already invokes rather than needing a step of its own. +tasks.named("check") { dependsOn(checkApiTiers) } diff --git a/build-logic/src/main/kotlin/rabosh.kotlin-library.gradle.kts b/build-logic/src/main/kotlin/rabosh.kotlin-library.gradle.kts index f8f1f3b..e098600 100644 --- a/build-logic/src/main/kotlin/rabosh.kotlin-library.gradle.kts +++ b/build-logic/src/main/kotlin/rabosh.kotlin-library.gradle.kts @@ -32,6 +32,11 @@ kotlin { * silently changes tier fails there, and so does a sample that reaches past the facade. The ABI * dumps cannot do this job: the JVM dump format writes signatures only and never annotations, so * a tier change is invisible to `checkKotlinAbi`. + * + * What a sample cannot catch is a public signature that *exposes* an experimental type without + * carrying the marker — this opt-in permits exactly that inside the library, and a sample only + * sees what it happens to call. `checkApiTiers` at the root is that half, and the two together + * are what hold `STABILITY.md`'s claim up. */ compilerOptions { optIn.add("app.oreshkov.rabosh.RaboshExperimental") diff --git a/build-logic/src/test/kotlin/app/oreshkov/rabosh/build/ApiTierAuditTest.kt b/build-logic/src/test/kotlin/app/oreshkov/rabosh/build/ApiTierAuditTest.kt new file mode 100644 index 0000000..59c84f6 --- /dev/null +++ b/build-logic/src/test/kotlin/app/oreshkov/rabosh/build/ApiTierAuditTest.kt @@ -0,0 +1,342 @@ +package app.oreshkov.rabosh.build + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows +import org.junit.jupiter.api.io.TempDir +import java.io.File + +/** + * The tier audit, against fixtures rather than against the repository. + * + * **A check that reports nothing has to be shown capable of reporting something**, which is most of + * this file: every "clean" case sits beside the same shape with one thing changed so that it leaks. + * An audit whose parser quietly stopped understanding the codebase would go green for ever, and going + * green is the only thing anybody reads it for. + */ +class ApiTierAuditTest { + + @TempDir + lateinit var root: File + + // --- reading the sources ---------------------------------------------------------------------- + + @Test + fun `a marker on a type, a member and a constructor is found in each spelling`() { + module( + "rabosh-index", + "Bitmap.kt" to """ + package app.oreshkov.rabosh.index + + import app.oreshkov.rabosh.RaboshExperimental + + @RaboshExperimental + public class Bitmap private constructor(private val keys: IntArray) { + public fun cardinality(): Int = 0 + } + """, + "IndexCatalog.kt" to """ + package app.oreshkov.rabosh.index + + public class IndexCatalog @RaboshExperimental constructor( + public val directory: String, + ) { + /** Doc mentioning @RaboshExperimental should not count. */ + @RaboshExperimental + public fun read(handle: Int): Bitmap = Bitmap() + + public fun indexes(): List = emptyList() + } + """, + ) + + val tier = ApiTierAudit.markedIn(listOf(File(root, "rabosh-index"))) + + assertEquals(setOf("Bitmap"), tier.types) + assertEquals(setOf("IndexCatalog.constructor", "IndexCatalog.read"), tier.members) + } + + /** An import and an `@OptIn` both contain the word and neither applies it. */ + @Test + fun `mentioning the marker is not applying it`() { + module( + "rabosh-core", + "DocumentStore.kt" to """ + package app.oreshkov.rabosh.core + + import app.oreshkov.rabosh.RaboshExperimental + + /** + * See [RaboshExperimental] for what this means. + */ + @OptIn(RaboshExperimental::class) + public class DocumentStore { + public fun put(key: String) {} + } + """, + ) + + assertTrue(ApiTierAudit.markedIn(listOf(File(root, "rabosh-core"))).isEmpty) + } + + /** + * A marked member is attributed to the type it is **inside**, not to the last one declared. + * + * The shape is `IndexCatalog`'s, and it is here because the first version of this scanner got it + * wrong: it took the most recent type declaration, so `read` came out as + * `CountedObservation.read` and the audit reported a marked member as a leak. A false positive is + * what gets a gate switched off, so the sibling case is pinned rather than assumed. + */ + @Test + fun `a member is attributed to the type it is inside, not the last one declared`() { + module( + "rabosh-index", + "IndexCatalog.kt" to """ + package app.oreshkov.rabosh.index + + public class IndexCatalog { + + private class CountedObservation(private val count: Int) { + fun observe() {} + } + + @RaboshExperimental + public fun read(handle: Int): Bitmap = Bitmap() + } + """, + ) + + val tier = ApiTierAudit.markedIn(listOf(File(root, "rabosh-index"))) + assertEquals(setOf("IndexCatalog.read"), tier.members) + } + + /** A companion's members belong to the outer type, because that is where the dump puts them. */ + @Test + fun `a companion member is attributed to its outer type`() { + module( + "rabosh-core", + "DocumentStore.kt" to """ + package app.oreshkov.rabosh.core + + public class DocumentStore { + + private class Pinned(val live: Set) + + public companion object { + @RaboshExperimental + public fun open(directory: String): DocumentStore = DocumentStore() + } + } + """, + ) + + val tier = ApiTierAudit.markedIn(listOf(File(root, "rabosh-core"))) + assertEquals(setOf("DocumentStore.open"), tier.members) + } + + /** + * A spelling the scanner cannot attribute is a **failure**, never a shorter set. + * + * This is the direction that matters. An audit that silently dropped an annotation would build a + * short experimental set and then report no leaks against it — passing, for the worst reason. + */ + @Test + fun `an annotation the scanner cannot attribute fails loudly`() { + module( + "rabosh-core", + "Odd.kt" to """ + package app.oreshkov.rabosh.core + + public class Odd { + @RaboshExperimental + } + """, + ) + + val failure = assertThrows { + ApiTierAudit.markedIn(listOf(File(root, "rabosh-core"))) + } + assertTrue("could not attribute" in failure.message!!, failure.message) + assertTrue("Odd.kt" in failure.message!!, failure.message) + } + + // --- reading the dumps ------------------------------------------------------------------------ + + private val tier = ApiTierAudit.Tier( + types = setOf("Bitmap", "ColumnReader"), + members = setOf("Rabosh.store", "IndexCatalog.read", "DocumentStore.open", "SchemaCatalog.constructor"), + ) + + @Test + fun `a marked member returning an experimental type is not a leak`() { + val dump = dump( + "rabosh-index", + """ + public final class app/oreshkov/rabosh/index/IndexCatalog { + public final fun read (I)Lapp/oreshkov/rabosh/index/Bitmap; + public final fun indexes ()Ljava/util/List; + } + """, + ) + assertEquals(emptyList(), ApiTierAudit.leaks(dump, tier)) + } + + /** The same dump with the marker taken off `read`: the audit must now see it. */ + @Test + fun `an unmarked member returning an experimental type is a leak`() { + val dump = dump( + "rabosh-index", + """ + public final class app/oreshkov/rabosh/index/IndexCatalog { + public final fun read (I)Lapp/oreshkov/rabosh/index/Bitmap; + } + """, + ) + val leaks = ApiTierAudit.leaks(dump, ApiTierAudit.Tier(tier.types, emptySet())) + + assertEquals(1, leaks.size, leaks.toString()) + assertEquals("IndexCatalog", leaks.single().owner) + assertEquals("read", leaks.single().member) + assertEquals("Bitmap", leaks.single().referenced) + } + + /** A member *taking* an experimental type leaks exactly as one returning it does. */ + @Test + fun `an experimental parameter is a leak too`() { + val dump = dump( + "rabosh-query", + """ + public final class app/oreshkov/rabosh/query/Plan { + public final fun intersect (Lapp/oreshkov/rabosh/index/Bitmap;)V + } + """, + ) + assertEquals(1, ApiTierAudit.leaks(dump, tier).size) + } + + /** Everything inside an experimental class is already behind the opt-in that got you there. */ + @Test + fun `members of an experimental class are not leaks`() { + val dump = dump( + "rabosh-index", + """ + public final class app/oreshkov/rabosh/index/Bitmap { + public final fun copy ()Lapp/oreshkov/rabosh/index/Bitmap; + public final fun reader ()Lapp/oreshkov/rabosh/index/ColumnReader; + } + """, + ) + assertEquals(emptyList(), ApiTierAudit.leaks(dump, tier)) + } + + /** An unmarked class *implementing* an experimental interface is a leak on the header line. */ + @Test + fun `an experimental supertype is a leak`() { + val dump = dump( + "rabosh-index", + """ + public final class app/oreshkov/rabosh/index/Mask : app/oreshkov/rabosh/index/Bitmap { + public final fun size ()I + } + """, + ) + val leaks = ApiTierAudit.leaks(dump, tier) + assertEquals(1, leaks.size, leaks.toString()) + assertEquals("(supertype)", leaks.single().member) + } + + /** + * The three spellings the dump and the sources disagree on, each pinned. + * + * A property is `getStore` against `store`; a constructor is `` against `constructor`; a + * default-argument bridge is `read$default` against `read`. Getting any of them wrong makes the + * audit report a marked declaration as a leak, which is the failure that gets a gate switched off. + */ + @Test + fun `the dump's spellings are normalised to the source's`() { + val dump = dump( + "rabosh-api", + """ + public final class app/oreshkov/rabosh/api/Rabosh { + public final fun getStore ()Lapp/oreshkov/rabosh/index/Bitmap; + } + + public final class app/oreshkov/rabosh/catalog/SchemaCatalog { + public fun (Lapp/oreshkov/rabosh/index/Bitmap;)V + } + + public final class app/oreshkov/rabosh/index/IndexCatalog { + public static synthetic fun read${'$'}default (Lapp/oreshkov/rabosh/index/Bitmap;)Lapp/oreshkov/rabosh/index/Bitmap; + } + """, + ) + assertEquals(emptyList(), ApiTierAudit.leaks(dump, tier)) + } + + /** A companion's members appear on `Foo${'$'}Companion` and are declared inside `Foo`. */ + @Test + fun `a companion member matches a marker attributed to its outer type`() { + val dump = dump( + "rabosh-core", + """ + public final class app/oreshkov/rabosh/core/DocumentStore${'$'}Companion { + public final fun open (Ljava/lang/String;)Lapp/oreshkov/rabosh/index/Bitmap; + } + """, + ) + assertEquals(emptyList(), ApiTierAudit.leaks(dump, tier)) + } + + /** A signature naming nothing experimental is never a leak, however much else it names. */ + @Test + fun `an ordinary signature is not a leak`() { + val dump = dump( + "rabosh-core", + """ + public final class app/oreshkov/rabosh/core/Key { + public final fun successor ()Lapp/oreshkov/rabosh/core/Key; + public static final fun of (Ljava/lang/String;)Lapp/oreshkov/rabosh/core/Key; + } + """, + ) + assertEquals(emptyList(), ApiTierAudit.leaks(dump, tier)) + } + + /** With nothing marked there is nothing to leak, and the audit says so rather than scanning. */ + @Test + fun `an empty tier reports nothing`() { + val dump = dump( + "rabosh-index", + """ + public final class app/oreshkov/rabosh/index/IndexCatalog { + public final fun read (I)Lapp/oreshkov/rabosh/index/Bitmap; + } + """, + ) + assertEquals(emptyList(), ApiTierAudit.leaks(dump, ApiTierAudit.Tier(emptySet(), emptySet()))) + } + + /** A missing dump is not a failure here: `checkKotlinAbi` owns that, and owning it twice is worse. */ + @Test + fun `a module with no dump is skipped`() { + assertEquals( + emptyList(), + ApiTierAudit.leaks(mapOf("rabosh-ghost" to File(root, "nowhere.api")), tier), + ) + } + + // --- fixtures --------------------------------------------------------------------------------- + + private fun module(name: String, vararg sources: Pair) { + val directory = File(root, "$name/src/main/kotlin/app/oreshkov/rabosh") + directory.mkdirs() + for ((file, text) in sources) File(directory, file).writeText(text.trimIndent()) + } + + private fun dump(module: String, text: String): Map { + val file = File(root, "$module.api") + file.writeText(text.trimIndent()) + return mapOf(module to file) + } +} diff --git a/build.gradle.kts b/build.gradle.kts index b8b5848..e5c8304 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -7,7 +7,9 @@ plugins { // Applied: a Central deployment is one archive over every published module, so it is assembled // and checked from the only project that can see all of them. The aggregated API documentation - // is one site over the same set, for the same reason. + // is one site over the same set, for the same reason — and so is the stability-tier audit, since + // a type marked experimental in one module leaks through a signature in another's dump. id("rabosh.central-bundle") id("rabosh.api-docs") + id("rabosh.api-tiers") }