From a569243f9321a4ddb66b76509a0fd98cb2e51ab7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 10:54:25 +0000 Subject: [PATCH] Add R8 -checkdiscard verification for disabled-flag dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local boolean flags (default = false) can now declare discard(...) class specs. The plugin emits R8 -checkdiscard rules that *verify* the flag-guarded code was actually stripped from the optimized release binary — the missing verification half of the -assumevalues dead-code-elimination story. Unit tests see the pre-R8 classpath and androidTest runs without minify, so without -checkdiscard nothing checks the final DEX; a forgotten DI reference, manifest entry, reflection, or over-broad -keep that keeps a "disabled" feature alive is now caught at build time (Discard checks failed) instead of shipping. - discard(classSpec) on FlagSpec; rejected on non-boolean, default=true, and remote flags (a value never pinned at build time can't be guaranteed). - CheckDiscardRulesGenerator + generateFeaturedCheckDiscardRules task, wired into R8 like -assumevalues: app proguardFiles and library/KMP consumer rules. - Real-R8 shrinker test proving the check passes when code is eliminated and fails the build when a checked class survives. - Unit + registration + DSL-validation tests; docs in both CLAUDE.md files. https://claude.ai/code/session_01HqnfagV6KswrzeqKLywpaS --- CLAUDE.md | 5 +- featured-gradle-plugin/CLAUDE.md | 17 +++ .../featured/gradle/AndroidProguardWiring.kt | 34 ++++- .../gradle/CheckDiscardRulesGenerator.kt | 74 +++++++++++ .../featured/gradle/FeaturedPlugin.kt | 32 ++++- .../featured/gradle/FlagContainer.kt | 6 + .../featured/gradle/FlagSpec.kt | 49 ++++++++ .../gradle/GenerateCheckDiscardRulesTask.kt | 86 +++++++++++++ .../gradle/CheckDiscardRulesGeneratorTest.kt | 116 ++++++++++++++++++ .../featured/gradle/FlagDiscardDslTest.kt | 98 +++++++++++++++ ...teCheckDiscardRulesTaskRegistrationTest.kt | 68 ++++++++++ .../shrinker/r8/R8CheckDiscardTest.kt | 45 +++++++ .../shrinker/rules/ProguardRulesWriter.kt | 31 +++++ 13 files changed, 651 insertions(+), 10 deletions(-) create mode 100644 featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGenerator.kt create mode 100644 featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTask.kt create mode 100644 featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGeneratorTest.kt create mode 100644 featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/FlagDiscardDslTest.kt create mode 100644 featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTaskRegistrationTest.kt create mode 100644 featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8CheckDiscardTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index 986df70..a352a90 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -66,6 +66,7 @@ Sample build / install: - `generateConfigParam` — typed `ConfigParam` objects + `ConfigValues` extensions - `generateFeaturedProguardRules` — R8 `-assumevalues` rules for local flags +- `generateFeaturedCheckDiscardRules` — R8 `-checkdiscard` rules that *verify* flag-guarded code was stripped (opt-in via `discard(...)` on a local boolean default-`false` flag; fails the build if a "disabled" feature survives) - `generateIosConstVal` / `generateXcconfig` — Swift DCE inputs - `generateFeaturedManifest` — emits `featured-manifest.json` consumed by the aggregator - `generateFeaturedRegistry` (aggregator-only) — produces `GeneratedFeaturedRegistry.kt` @@ -110,12 +111,12 @@ For non-reactive reads (logging, eager-conditional paths) use `configValues.getV - **Branching:** `develop` is the integration branch; PRs go to `develop`, not `main`. `main` is updated only on releases. One logical change per PR — do not bundle. - **Comment language:** English (per `.github/copilot-instructions.md`). - **iOS:** SKIE is applied in `:core`; the XCFramework is named `FeaturedCore`. SKIE config is `skie.toml` at repo root. -- **R8:** the project relies on `android.enableR8.fullMode=true` and `android.r8.strictInputValidation=true`. The generated ProGuard rules + `-assumevalues` are what make DCE work. +- **R8:** the project relies on `android.enableR8.fullMode=true` and `android.r8.strictInputValidation=true`. The generated ProGuard rules + `-assumevalues` are what make DCE work; the opt-in `discard(...)` → `-checkdiscard` rules *prove* the disabled-flag code left the binary (the final DEX is otherwise checked by nothing — unit tests see the pre-R8 classpath, `androidTest` runs without minify). ## Where to Look First When… - "Find how the DSL is parsed" → `featured-gradle-plugin/src/main/kotlin/.../FeaturedExtension.kt`, `FlagSpec.kt`, `FlagContainer.kt`. -- "Find codegen output shape" → `ConfigParamGenerator.kt`, `ExtensionFunctionGenerator.kt`, `ProguardRulesGenerator.kt`, `XcconfigGenerator.kt`, `IosConstValGenerator.kt` (all in `featured-gradle-plugin/src/main/kotlin/`). +- "Find codegen output shape" → `ConfigParamGenerator.kt`, `ExtensionFunctionGenerator.kt`, `ProguardRulesGenerator.kt`, `CheckDiscardRulesGenerator.kt`, `XcconfigGenerator.kt`, `IosConstValGenerator.kt` (all in `featured-gradle-plugin/src/main/kotlin/`). - "Find aggregator wiring" → `FeaturedApplicationPlugin.kt` + `aggregation/` subpackage. - "Find manifest format" → `manifest/` subpackage (`GenerateFeaturedManifestTask.kt`, `SCHEMA_VERSION`). - "Verify R8 DCE behaviour" → `featured-shrinker-tests/` (integration tests over real `assembleRelease`). diff --git a/featured-gradle-plugin/CLAUDE.md b/featured-gradle-plugin/CLAUDE.md index 62ccb67..a369c5e 100644 --- a/featured-gradle-plugin/CLAUDE.md +++ b/featured-gradle-plugin/CLAUDE.md @@ -23,6 +23,10 @@ featured { boolean("dark_mode", default = false) { category = "UI" } int("max_retries", default = 3) enum("checkout_variant", typeFqn = "com.example.CheckoutVariant", default = "LEGACY") + boolean("new_checkout", default = false) { + // assert the guarded code is gone from release binaries (R8 -checkdiscard) + discard("com.example.checkout.newflow.**") + } } remoteFlags { boolean("promo_banner", default = false) { description = "Show promo banner" } @@ -39,6 +43,18 @@ featured { - ProGuard rules and the iOS const-val files follow the **local** section's effective package. - The iOS expect/actual `const val` declarations stay `public` regardless of `visibility`. +`discard(...)` notes (per local boolean flag, default `false` only): + +- Emits an R8 `-checkdiscard class { *; }` rule that **verifies** the flag-guarded code + was actually dead-code-eliminated in release builds — the companion check to the `-assumevalues` + rule that causes the elimination. If anything keeps the code alive (a forgotten DI reference, a + manifest entry, reflection, an over-broad `-keep`), R8 fails the build with `Discard checks + failed`; diagnose with `-whyareyoukeeping`. +- Wired like `-assumevalues`: into the app's own R8 for application modules, and as a consumer + ProGuard file for library modules so the check runs in the consuming app's R8. +- Rejected on non-boolean flags, on `default = true` flags, and on remote flags (a runtime-resolved + value is never pinned at build time). Covers **code only** — resource shrinking is not verified. + ## Tasks registered per module | Task | Output | @@ -46,6 +62,7 @@ featured { | `resolveFeatureFlags` | `build/featured/flags.txt` | | `generateConfigParam` | `build/generated/featured/commonMain/Generated{Local,Remote}Flags.kt` + `Generated{Local,Remote}FlagExtensions.kt` (file names follow custom `className`s) | | `generateFeaturedProguardRules` | `build/featured/proguard-featured.pro` | +| `generateFeaturedCheckDiscardRules` | `build/featured/proguard-featured-checkdiscard.pro` | | `generateIosConstVal` | iOS constant value files | | `generateXcconfig` | `build/featured/FeatureFlags.generated.xcconfig` | diff --git a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/AndroidProguardWiring.kt b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/AndroidProguardWiring.kt index 16e7b44..ce9f91f 100644 --- a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/AndroidProguardWiring.kt +++ b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/AndroidProguardWiring.kt @@ -7,16 +7,22 @@ import org.gradle.api.Project import org.gradle.api.tasks.TaskProvider /** - * Wires the generated ProGuard rules file into every application variant via the AGP Variant API. + * Wires the generated ProGuard rules files into every application variant via the AGP Variant API. * * Called lazily — only when `com.android.application` is present on the project. * + * Both the `-assumevalues` rules ([proguardTask]) and the `-checkdiscard` rules + * ([checkDiscardTask]) are added: the first drives flag dead-code elimination, the second + * verifies it in the same R8 run. The check-discard file is empty unless a flag declares + * `discard(...)`, and an empty ProGuard configuration is a harmless no-op. + * * AGP 9.x does not propagate implicit Gradle task dependencies through [Variant.proguardFiles], - * so [proguardTask] is also wired explicitly as a dependency of every `minify*WithR8` task. + * so both tasks are also wired explicitly as dependencies of every `minify*WithR8` task. */ internal fun wireProguardToApplicationVariants( project: Project, proguardTask: TaskProvider, + checkDiscardTask: TaskProvider, ) { val androidComponents = project.extensions @@ -25,32 +31,39 @@ internal fun wireProguardToApplicationVariants( variant.proguardFiles.add( proguardTask.flatMap { it.outputFile }, ) + variant.proguardFiles.add( + checkDiscardTask.flatMap { it.outputFile }, + ) } // AGP 9.x does not propagate implicit task dependencies through variant.proguardFiles, // so we wire an explicit dependsOn on every R8 minify task. project.tasks.configureEach { task -> if (task.name.startsWith("minify") && task.name.endsWith("WithR8")) { task.dependsOn(proguardTask) + task.dependsOn(checkDiscardTask) } } } /** - * Wires the generated ProGuard rules file as consumer ProGuard rules for every library variant. + * Wires the generated ProGuard rules files as consumer ProGuard rules for every library variant. * * Called lazily — only when `com.android.library` is present on the project. * * Library modules do not run R8 themselves, so [Variant.proguardFiles] would never be applied. * Consumer ProGuard rules are bundled into the AAR and forwarded to every consuming app's R8, * which is where the `-assumevalues` rules need to run for flag dead-code elimination to work. + * The `-checkdiscard` rules ([checkDiscardTask]) ride along the same path so the discard check + * runs in the consuming app's R8 — exactly where the matching `-assumevalues` rule applies. * * AGP 9.x does not propagate implicit Gradle task dependencies through - * [LibraryVariant.consumerProguardFiles], so [proguardTask] is also wired explicitly as a + * [LibraryVariant.consumerProguardFiles], so both tasks are also wired explicitly as a * dependency of every `export*ConsumerProguardFiles` task. */ internal fun wireProguardToLibraryVariants( project: Project, proguardTask: TaskProvider, + checkDiscardTask: TaskProvider, ) { val libraryComponents = project.extensions @@ -59,6 +72,9 @@ internal fun wireProguardToLibraryVariants( variant.consumerProguardFiles.add( proguardTask.flatMap { it.outputFile }, ) + variant.consumerProguardFiles.add( + checkDiscardTask.flatMap { it.outputFile }, + ) } // AGP 9.x does not propagate implicit task dependencies through // variant.consumerProguardFiles, so we wire an explicit dependsOn on every @@ -66,6 +82,7 @@ internal fun wireProguardToLibraryVariants( project.tasks.configureEach { task -> if (task.name.startsWith("export") && task.name.endsWith("ConsumerProguardFiles")) { task.dependsOn(proguardTask) + task.dependsOn(checkDiscardTask) } } } @@ -81,14 +98,17 @@ internal fun wireProguardToLibraryVariants( * * Consumer ProGuard rules are bundled into the AAR and forwarded to every consuming app's R8, * which is where the `-assumevalues` rules need to run for flag dead-code elimination to work. + * The `-checkdiscard` rules ([checkDiscardTask]) ride along the same path so the discard check + * runs in the consuming app's R8. * * AGP 9.x does not propagate implicit Gradle task dependencies through - * [KotlinMultiplatformAndroidVariant.consumerProguardFiles], so [proguardTask] is also wired + * [KotlinMultiplatformAndroidVariant.consumerProguardFiles], so both tasks are also wired * explicitly as a dependency of every `export*ConsumerProguardFiles` task. */ internal fun wireProguardToKmpLibraryVariants( project: Project, proguardTask: TaskProvider, + checkDiscardTask: TaskProvider, ) { val kmpComponents = project.extensions @@ -97,10 +117,14 @@ internal fun wireProguardToKmpLibraryVariants( variant.consumerProguardFiles.add( proguardTask.flatMap { it.outputFile }, ) + variant.consumerProguardFiles.add( + checkDiscardTask.flatMap { it.outputFile }, + ) } project.tasks.configureEach { task -> if (task.name.startsWith("export") && task.name.endsWith("ConsumerProguardFiles")) { task.dependsOn(proguardTask) + task.dependsOn(checkDiscardTask) } } } diff --git a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGenerator.kt b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGenerator.kt new file mode 100644 index 0000000..0d0d0ee --- /dev/null +++ b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGenerator.kt @@ -0,0 +1,74 @@ +package dev.androidbroadcast.featured.gradle + +/** + * A single `-checkdiscard` target: a class specification that must be absent from the + * optimized release binary, together with the flag that guards it. + * + * @property flagKey The local boolean flag (default `false`) that guards [classSpec]. Used only + * for the generated comment so a failing check points at the responsible flag. + * @property classSpec An R8/ProGuard class specification (e.g. `com.example.featurex.**`). + */ +public data class CheckDiscardEntry( + public val flagKey: String, + public val classSpec: String, +) + +/** + * Generates R8 `-checkdiscard` rules that **verify** flag-guarded code was actually removed + * from the optimized release binary. + * + * This is the verification half of the dead-code-elimination story. [ProguardRulesGenerator] + * emits `-assumevalues` rules that *cause* R8 to fold a local flag to its default and drop the + * disabled branch; `-checkdiscard` then *asserts* the result — if any listed class survives, + * R8 fails the build with `Discard checks failed` and prints the retention path (diagnose with + * `-whyareyoukeeping`). + * + * Unit tests see the full classpath before R8 runs and `androidTest` is assembled without + * minification, so without `-checkdiscard` nothing checks the final DEX. A forgotten DI + * reference, a manifest entry, or reflection that keeps the feature alive is caught at build + * time instead of shipping dead code (or worse, a still-reachable disabled feature). + * + * Each target is declared via `discard(...)` on a local boolean flag whose default is `false` + * (see [FlagSpec.discard]); the matching `-assumevalues` rule is what makes the discard + * guaranteed to hold in release builds. + * + * Example output for `boolean("new_checkout", default = false) { discard("com.example.checkout.newflow.**") }`: + * ```proguard + * # guarded by flag: new_checkout + * -checkdiscard class com.example.checkout.newflow.** { *; } + * ``` + * + * The directive covers **code only** — it does not verify that resources are stripped. + */ +public object CheckDiscardRulesGenerator { + /** + * Generates `-checkdiscard` rules for all [entries]. + * + * Entries with a blank [CheckDiscardEntry.classSpec] are skipped. Returns a blank string + * when no entry contributes a rule, so the output file can be wired into R8 unconditionally + * (an empty ProGuard configuration is a valid no-op). + */ + public fun generate(entries: List): String { + val valid = + entries.mapNotNull { entry -> + val spec = entry.classSpec.trim() + if (spec.isEmpty()) null else entry.copy(classSpec = spec) + } + if (valid.isEmpty()) return "" + + val header = + listOf( + "# Auto-generated by Featured Gradle Plugin — do not edit manually.", + "# -checkdiscard asserts these classes are absent from the optimized (R8) release binary.", + "# Each target is guarded by a local boolean flag pinned to false via the generated", + "# -assumevalues rule, so the flag-guarded code must be dead-code-eliminated.", + "# If a target survives, R8 fails the build with \"Discard checks failed\" and the", + "# retention path. Diagnose what keeps a class alive with -whyareyoukeeping.", + ) + val rules = + valid.map { entry -> + "# guarded by flag: ${entry.flagKey}\n-checkdiscard class ${entry.classSpec} { *; }" + } + return (header + rules).joinToString("\n") + } +} diff --git a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FeaturedPlugin.kt b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FeaturedPlugin.kt index b695b0a..3152b1d 100644 --- a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FeaturedPlugin.kt +++ b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FeaturedPlugin.kt @@ -14,6 +14,7 @@ import org.gradle.api.tasks.TaskProvider internal const val RESOLVE_FLAGS_TASK_NAME = "resolveFeatureFlags" internal const val VERIFY_EXPIRED_FLAGS_TASK_NAME = "verifyExpiredFlags" internal const val GENERATE_PROGUARD_TASK_NAME = "generateFeaturedProguardRules" +internal const val GENERATE_CHECK_DISCARD_TASK_NAME = "generateFeaturedCheckDiscardRules" internal const val GENERATE_IOS_CONST_VAL_TASK_NAME = "generateIosConstVal" internal const val GENERATE_XCCONFIG_TASK_NAME = "generateXcconfig" internal const val GENERATE_CONFIG_PARAM_TASK_NAME = "generateConfigParam" @@ -47,18 +48,19 @@ public class FeaturedPlugin : Plugin { val verifyTask = registerVerifyExpiredFlagsTask(target, extension, resolveTask) registerConfigParamTask(target, extension, resolveTask, verifyTask) val proguardTask = registerProguardTask(target, extension, resolveTask, verifyTask) + val checkDiscardTask = registerCheckDiscardTask(target, extension, resolveTask, verifyTask) registerIosConstValTask(target, extension, resolveTask, verifyTask) registerXcconfigTask(target, resolveTask, verifyTask) val manifestTask = registerManifestTask(target, resolveTask) registerFeaturedManifestConfiguration(target, manifestTask) target.plugins.withId("com.android.application") { - wireProguardToApplicationVariants(target, proguardTask) + wireProguardToApplicationVariants(target, proguardTask, checkDiscardTask) } target.plugins.withId("com.android.library") { - wireProguardToLibraryVariants(target, proguardTask) + wireProguardToLibraryVariants(target, proguardTask, checkDiscardTask) } target.plugins.withId("com.android.kotlin.multiplatform.library") { - wireProguardToKmpLibraryVariants(target, proguardTask) + wireProguardToKmpLibraryVariants(target, proguardTask, checkDiscardTask) } } @@ -136,6 +138,30 @@ public class FeaturedPlugin : Plugin { task.dependsOn(verifyTask) } + private fun registerCheckDiscardTask( + target: Project, + extension: FeaturedExtension, + resolveTask: TaskProvider, + verifyTask: TaskProvider, + ): TaskProvider = + target.tasks.register(GENERATE_CHECK_DISCARD_TASK_NAME, GenerateCheckDiscardRulesTask::class.java) { task -> + task.group = "featured" + task.description = "Generates R8 -checkdiscard rules for local flag discard(...) targets in '${target.path}'." + // Read straight from the DSL (not the resolved-flags report) so discard targets and + // the remote-flag guard participate in the @Input fingerprint and invalidate caching. + task.discardDescriptors.set(target.provider { extension.localFlags.discardDescriptors() }) + task.remoteFlagsWithDiscards.set( + target.provider { + extension.remoteFlags.flags + .filter { it.discards.isNotEmpty() } + .map { it.key } + }, + ) + task.outputFile.set(target.layout.buildDirectory.file("featured/proguard-featured-checkdiscard.pro")) + task.dependsOn(resolveTask) + task.dependsOn(verifyTask) + } + private fun registerIosConstValTask( target: Project, extension: FeaturedExtension, diff --git a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagContainer.kt b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagContainer.kt index af1779b..7226f5e 100644 --- a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagContainer.kt +++ b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagContainer.kt @@ -154,4 +154,10 @@ public class FlagContainer { /** Serialises all flags to pipe-delimited descriptors for [ResolveFlagsTask] inputs. */ internal fun toDescriptors(): List = _flags.map { it.toDescriptor() } + + /** + * Serialises every `discard(...)` target to a `flagKey|classSpec` descriptor for + * [GenerateCheckDiscardRulesTask] inputs. Flags without discard targets contribute nothing. + */ + internal fun discardDescriptors(): List = _flags.flatMap { flag -> flag.discards.map { spec -> "${flag.key}|$spec" } } } diff --git a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagSpec.kt b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagSpec.kt index 8e6d0b9..81bce71 100644 --- a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagSpec.kt +++ b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/FlagSpec.kt @@ -20,6 +20,55 @@ public class FlagSpec( public var category: String? = null public var expiresAt: String? = null + private val _discards = mutableListOf() + + /** + * R8/ProGuard class specifications that must be discarded from release binaries when this + * flag is at its (false) default. Populated via [discard]; consumed by + * [GenerateCheckDiscardRulesTask] to emit `-checkdiscard` rules. + */ + public val discards: List get() = _discards.toList() + + /** + * Asserts that [classSpec] is dead-code-eliminated from release binaries when this flag is + * disabled, by emitting an R8 `-checkdiscard class { *; }` rule. + * + * This is the verification companion to the `-assumevalues` rule generated for the flag: + * `-assumevalues` pins the flag to `false` so R8 drops the disabled branch, and + * `-checkdiscard` fails the build if anything (a forgotten DI reference, a manifest entry, + * reflection, an over-broad `-keep`) keeps the guarded code alive. The failure surfaces at + * build time — `Discard checks failed` plus the retention path — instead of in production. + * + * Only valid on a **local boolean** flag whose default is `false`: a runtime-resolved + * (remote) flag is never pinned at build time, and a flag that defaults to `true` keeps its + * guarded code, so in neither case can the discard be guaranteed. + * + * Repeatable to list several class specifications for the same flag: + * ```kotlin + * boolean("new_checkout", default = false) { + * discard("com.example.checkout.newflow.**") + * discard("com.example.checkout.NewCheckoutEntryPoint") + * } + * ``` + * + * @param classSpec An R8/ProGuard class specification, e.g. `com.example.featurex.**`. + */ + public fun discard(classSpec: String) { + require(type == "Boolean") { + "discard(...) is only supported on boolean flags, but flag \"$key\" has type \"$type\". " + + "-checkdiscard verifies code is removed while a flag is off, which is only well-defined " + + "for a boolean flag whose default is false." + } + require(defaultValue == "false") { + "discard(...) requires boolean flag \"$key\" to have default = false (got $defaultValue). " + + "A flag that defaults to true keeps its guarded code in release builds, so that code " + + "cannot be verified as discarded." + } + val spec = classSpec.trim() + require(spec.isNotEmpty()) { "discard(...) classSpec for flag \"$key\" must not be blank." } + _discards += spec + } + /** Serialises this spec to a pipe-delimited descriptor consumed by [ResolveFlagsTask]. */ internal fun toDescriptor(): String = "$key|$defaultValue|$type|${description.orEmpty()}|${category.orEmpty()}|${expiresAt.orEmpty()}" } diff --git a/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTask.kt b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTask.kt new file mode 100644 index 0000000..0f1beda --- /dev/null +++ b/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTask.kt @@ -0,0 +1,86 @@ +package dev.androidbroadcast.featured.gradle + +import org.gradle.api.DefaultTask +import org.gradle.api.GradleException +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.provider.ListProperty +import org.gradle.api.tasks.CacheableTask +import org.gradle.api.tasks.Input +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.TaskAction + +/** + * Gradle task that generates R8 `-checkdiscard` rules for every `discard(...)` target declared + * on a local boolean flag (see [FlagSpec.discard]). + * + * The rules are the verification companion to the `-assumevalues` rules from + * [GenerateProguardRulesTask]: where `-assumevalues` *forces* a flag to its default and lets R8 + * drop the disabled branch, `-checkdiscard` *asserts* the guarded classes are gone from the + * optimized output and fails the build otherwise. + * + * For Android application modules the generated file is wired into every variant's R8 run; for + * library modules it is exported as a consumer ProGuard file so the check runs in the consuming + * app's R8 — wherever the matching `-assumevalues` rule applies. + * + * Inputs are read directly from the `featured { }` DSL (not the resolved-flags report) so that + * the discard targets and the remote-flag guard participate in the task's `@Input` fingerprint. + */ +@CacheableTask +public abstract class GenerateCheckDiscardRulesTask : DefaultTask() { + /** + * Discard targets for the module's local flags, one `flagKey|classSpec` line per target. + * Wired from [FlagContainer.discardDescriptors] by [FeaturedPlugin]. + */ + @get:Input + public abstract val discardDescriptors: ListProperty + + /** + * Keys of any **remote** flags that declared `discard(...)`. Remote flag values are resolved + * at runtime and never pinned at build time, so a discard target on a remote flag can never + * be guaranteed — the task fails with a precise message instead of emitting a rule that R8 + * would always reject. + */ + @get:Input + public abstract val remoteFlagsWithDiscards: ListProperty + + /** Generated rules file. Written to `build/featured/proguard-featured-checkdiscard.pro`. */ + @get:OutputFile + public abstract val outputFile: RegularFileProperty + + @TaskAction + public fun generate() { + val remoteOffenders = remoteFlagsWithDiscards.get() + if (remoteOffenders.isNotEmpty()) { + throw GradleException( + "discard(...) is only supported on local flags, but it was declared on remote " + + "flag(s): ${remoteOffenders.joinToString(", ")}. Remote flag values are resolved " + + "at runtime and are never pinned at build time, so R8 cannot guarantee the " + + "guarded code is removed. Move the flag into localFlags { } or drop the discard(...) call.", + ) + } + + val entries = discardDescriptors.get().mapNotNull { it.toCheckDiscardEntry() } + val rules = CheckDiscardRulesGenerator.generate(entries) + + val out = outputFile.get().asFile + out.parentFile?.mkdirs() + out.writeText(rules) + + if (rules.isBlank()) { + logger.lifecycle("[featured] No discard(...) targets — proguard-featured-checkdiscard.pro is empty.") + } else { + logger.lifecycle("[featured] Generated -checkdiscard rules for ${entries.size} target(s) → ${out.path}") + } + } +} + +/** + * Parses a `flagKey|classSpec` descriptor produced by [FlagContainer.discardDescriptors]. + * Returns `null` when the line has no `|` separator or an empty key/spec. + */ +private fun String.toCheckDiscardEntry(): CheckDiscardEntry? { + val key = substringBefore('|', missingDelimiterValue = "") + val spec = substringAfter('|', missingDelimiterValue = "") + if (key.isEmpty() || spec.isBlank()) return null + return CheckDiscardEntry(flagKey = key, classSpec = spec) +} diff --git a/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGeneratorTest.kt b/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGeneratorTest.kt new file mode 100644 index 0000000..9cbdf10 --- /dev/null +++ b/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/CheckDiscardRulesGeneratorTest.kt @@ -0,0 +1,116 @@ +package dev.androidbroadcast.featured.gradle + +import kotlin.test.Test +import kotlin.test.assertContains +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class CheckDiscardRulesGeneratorTest { + // ── empty / no-op cases ────────────────────────────────────────────────── + + @Test + fun `returns blank for empty entries`() { + assertTrue(CheckDiscardRulesGenerator.generate(emptyList()).isBlank()) + } + + @Test + fun `returns blank when all class specs are blank`() { + val entries = + listOf( + CheckDiscardEntry(flagKey = "new_checkout", classSpec = " "), + CheckDiscardEntry(flagKey = "promo", classSpec = ""), + ) + assertTrue(CheckDiscardRulesGenerator.generate(entries).isBlank()) + } + + // ── rule generation ────────────────────────────────────────────────────── + + @Test + fun `generates a checkdiscard rule for a class spec`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf(CheckDiscardEntry("new_checkout", "com.example.checkout.newflow.**")), + ) + assertContains(rules, "-checkdiscard class com.example.checkout.newflow.** { *; }") + } + + @Test + fun `names the guarding flag in a comment`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf(CheckDiscardEntry("new_checkout", "com.example.checkout.newflow.**")), + ) + assertContains(rules, "# guarded by flag: new_checkout") + } + + @Test + fun `output starts with comment header`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf(CheckDiscardEntry("flag", "com.example.Foo")), + ) + assertTrue(rules.trimStart().startsWith("#"), "Output must start with a comment") + } + + @Test + fun `mentions whyareyoukeeping for diagnostics`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf(CheckDiscardEntry("flag", "com.example.Foo")), + ) + assertContains(rules, "-whyareyoukeeping") + } + + @Test + fun `trims surrounding whitespace from the class spec`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf(CheckDiscardEntry("flag", " com.example.Foo ")), + ) + assertContains(rules, "-checkdiscard class com.example.Foo { *; }") + assertFalse(rules.contains(" com.example.Foo "), "Class spec must be trimmed") + } + + // ── multiple targets ───────────────────────────────────────────────────── + + @Test + fun `generates one rule per target`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf( + CheckDiscardEntry("flag_a", "com.example.a.**"), + CheckDiscardEntry("flag_b", "com.example.b.**"), + ), + ) + assertEquals( + 2, + rules.lines().count { it.startsWith("-checkdiscard ") }, + "Expected exactly one -checkdiscard line per target", + ) + assertContains(rules, "com.example.a.**") + assertContains(rules, "com.example.b.**") + } + + @Test + fun `skips blank specs but keeps valid ones`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf( + CheckDiscardEntry("flag_a", "com.example.a.**"), + CheckDiscardEntry("flag_b", " "), + ), + ) + assertContains(rules, "com.example.a.**") + assertFalse(rules.contains("flag_b"), "Blank-spec entries must not appear") + } + + @Test + fun `output has no trailing newline`() { + val rules = + CheckDiscardRulesGenerator.generate( + listOf(CheckDiscardEntry("flag", "com.example.Foo")), + ) + assertFalse(rules.endsWith("\n"), "Output must not end with a trailing newline") + } +} diff --git a/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/FlagDiscardDslTest.kt b/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/FlagDiscardDslTest.kt new file mode 100644 index 0000000..b2ebdd8 --- /dev/null +++ b/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/FlagDiscardDslTest.kt @@ -0,0 +1,98 @@ +package dev.androidbroadcast.featured.gradle + +import kotlin.test.Test +import kotlin.test.assertContains +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertTrue + +class FlagDiscardDslTest { + // ── happy path ─────────────────────────────────────────────────────────── + + @Test + fun `discard target is recorded on a boolean false flag`() { + val container = + FlagContainer().apply { + boolean("new_checkout", default = false) { + discard("com.example.checkout.newflow.**") + } + } + assertEquals(listOf("com.example.checkout.newflow.**"), container.flags.single().discards) + } + + @Test + fun `multiple discard targets accumulate in order`() { + val container = + FlagContainer().apply { + boolean("new_checkout", default = false) { + discard("com.example.a.**") + discard("com.example.b.Entry") + } + } + assertEquals(listOf("com.example.a.**", "com.example.b.Entry"), container.flags.single().discards) + } + + @Test + fun `discard class spec is trimmed`() { + val container = + FlagContainer().apply { + boolean("flag", default = false) { discard(" com.example.Foo ") } + } + assertEquals(listOf("com.example.Foo"), container.flags.single().discards) + } + + @Test + fun `discardDescriptors serialises flagKey and class spec`() { + val container = + FlagContainer().apply { + boolean("new_checkout", default = false) { + discard("com.example.a.**") + discard("com.example.b.Entry") + } + boolean("dark_mode", default = false) // no discards + } + assertEquals( + listOf("new_checkout|com.example.a.**", "new_checkout|com.example.b.Entry"), + container.discardDescriptors(), + ) + } + + @Test + fun `discardDescriptors is empty when no flag declares discard`() { + val container = FlagContainer().apply { boolean("dark_mode", default = false) } + assertTrue(container.discardDescriptors().isEmpty()) + } + + // ── validation ─────────────────────────────────────────────────────────── + + @Test + fun `discard on a boolean true flag is rejected`() { + val error = + assertFailsWith { + FlagContainer().apply { + boolean("legacy_path", default = true) { discard("com.example.Foo") } + } + } + assertContains(error.message!!, "default = false") + } + + @Test + fun `discard on a non-boolean flag is rejected`() { + val error = + assertFailsWith { + FlagContainer().apply { + int("max_retries", default = 3) { discard("com.example.Foo") } + } + } + assertContains(error.message!!, "only supported on boolean flags") + } + + @Test + fun `discard with a blank class spec is rejected`() { + assertFailsWith { + FlagContainer().apply { + boolean("flag", default = false) { discard(" ") } + } + } + } +} diff --git a/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTaskRegistrationTest.kt b/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTaskRegistrationTest.kt new file mode 100644 index 0000000..32c328b --- /dev/null +++ b/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/GenerateCheckDiscardRulesTaskRegistrationTest.kt @@ -0,0 +1,68 @@ +package dev.androidbroadcast.featured.gradle + +import org.gradle.testfixtures.ProjectBuilder +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +class GenerateCheckDiscardRulesTaskRegistrationTest { + @Test + fun `plugin registers generateFeaturedCheckDiscardRules task`() { + val project = ProjectBuilder.builder().build() + project.plugins.apply("dev.androidbroadcast.featured") + + assertNotNull( + project.tasks.findByName(GENERATE_CHECK_DISCARD_TASK_NAME), + "Expected '$GENERATE_CHECK_DISCARD_TASK_NAME' task to be registered", + ) + } + + @Test + fun `generateFeaturedCheckDiscardRules task is of correct type`() { + val project = ProjectBuilder.builder().build() + project.plugins.apply("dev.androidbroadcast.featured") + + val task = project.tasks.findByName(GENERATE_CHECK_DISCARD_TASK_NAME) + assertNotNull(task) + assertTrue( + task is GenerateCheckDiscardRulesTask, + "Expected GenerateCheckDiscardRulesTask, was ${task::class.simpleName}", + ) + } + + @Test + fun `generateFeaturedCheckDiscardRules task is in featured group`() { + val project = ProjectBuilder.builder().build() + project.plugins.apply("dev.androidbroadcast.featured") + + val task = project.tasks.findByName(GENERATE_CHECK_DISCARD_TASK_NAME) + assertNotNull(task) + assertEquals("featured", task.group) + } + + @Test + fun `generateFeaturedCheckDiscardRules task has outputFile configured`() { + val project = ProjectBuilder.builder().build() + project.plugins.apply("dev.androidbroadcast.featured") + + val task = project.tasks.findByName(GENERATE_CHECK_DISCARD_TASK_NAME) as? GenerateCheckDiscardRulesTask + assertNotNull(task) + assertTrue(task.outputFile.isPresent, "Expected outputFile to be configured") + } + + @Test + fun `generateFeaturedCheckDiscardRules task depends on resolveFeatureFlags task`() { + val project = ProjectBuilder.builder().build() + project.plugins.apply("dev.androidbroadcast.featured") + + val generateTask = project.tasks.findByName(GENERATE_CHECK_DISCARD_TASK_NAME) + val resolveTask = project.tasks.findByName(RESOLVE_FLAGS_TASK_NAME) + assertNotNull(generateTask) + assertNotNull(resolveTask) + assertTrue( + generateTask.taskDependencies.getDependencies(generateTask).contains(resolveTask), + "Expected '$GENERATE_CHECK_DISCARD_TASK_NAME' to depend on '$RESOLVE_FLAGS_TASK_NAME'", + ) + } +} diff --git a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8CheckDiscardTest.kt b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8CheckDiscardTest.kt new file mode 100644 index 0000000..86d81cf --- /dev/null +++ b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8CheckDiscardTest.kt @@ -0,0 +1,45 @@ +package dev.androidbroadcast.featured.shrinker.r8 + +import com.android.tools.r8.CompilationFailedException +import dev.androidbroadcast.featured.shrinker.assertions.assertClassAbsent +import dev.androidbroadcast.featured.shrinker.bytecode.IF_BRANCH_CODE_INTERNAL +import dev.androidbroadcast.featured.shrinker.harness.R8TestHarness +import dev.androidbroadcast.featured.shrinker.rules.writeBooleanRulesWithCheckDiscard +import kotlin.test.Test +import kotlin.test.assertFailsWith + +/** + * Verifies the verification half of the flag dead-code-elimination story: the `-checkdiscard` + * rules emitted by `CheckDiscardRulesGenerator` actually make R8 fail the build when + * flag-guarded code is *not* removed. + * + * Unlike unit tests (which see the full pre-R8 classpath) and `androidTest` (assembled without + * minification), `-checkdiscard` inspects the optimized output, so it is the only thing that can + * catch a forgotten reference keeping a "disabled" feature alive. These tests run real R8 over + * the same synthetic bytecode used by [R8BooleanFlagEliminationTest] and assert both outcomes. + */ +internal class R8CheckDiscardTest : R8TestHarness() { + /** + * `-assumevalues … return false` makes `IfBranchCode` unreachable and eliminated, so the + * `-checkdiscard class IfBranchCode { *; }` assertion holds and R8 completes successfully. + */ + @Test + fun `checkdiscard passes and build succeeds when the disabled branch class is eliminated`() { + val outputJar = runBooleanR8 { writeBooleanRulesWithCheckDiscard(it, assume = false) } + + assertClassAbsent(outputJar, IF_BRANCH_CODE_INTERNAL) + } + + /** + * `-assumevalues … return true` keeps `IfBranchCode` reachable, so it survives R8. The + * `-checkdiscard` assertion on it then fails and R8 aborts with `Discard checks failed`, + * surfacing as a [CompilationFailedException]. This is the build-time guarantee: a flag that + * fails to strip its guarded code breaks the build instead of shipping dead/live code. + */ + @Test + fun `checkdiscard fails the build when a checked class survives`() { + assertFailsWith { + runBooleanR8 { writeBooleanRulesWithCheckDiscard(it, assume = true) } + } + } +} diff --git a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt index 2bce2b0..875a1e3 100644 --- a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt +++ b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt @@ -91,6 +91,37 @@ internal fun writeNoBooleanAssumeRules(dest: File) { ) } +/** + * Combines a Boolean `-assumevalues` rule with a `-checkdiscard` assertion on the + * if-branch class, mirroring what `ProguardRulesGenerator` + `CheckDiscardRulesGenerator` + * emit together for a `boolean(..., default = false) { discard(...) }` flag. + * + * - [assume] `= false` pins `isDarkModeEnabled` to `false`, so the if-branch (`IfBranchCode`) + * becomes dead and is eliminated; the `-checkdiscard` on it then **passes** and R8 succeeds. + * - [assume] `= true` keeps the if-branch reachable, so `IfBranchCode` survives and the + * `-checkdiscard` on it **fails**, making R8 abort with `Discard checks failed`. + * + * The surviving branch's `sideEffect` field is pinned so R8 cannot eliminate it as a no-op, + * keeping the scenario deterministic. + */ +internal fun writeBooleanRulesWithCheckDiscard( + dest: File, + assume: Boolean, +) { + val survivingClass = if (assume) IF_BRANCH_CODE_FQN else ELSE_BRANCH_CODE_FQN + dest.writeText( + """ + -assumevalues class $BOOL_EXTENSIONS_FQN { + boolean $IS_DARK_MODE_ENABLED($CONFIG_VALUES_FQN) return $assume; + } + -keep class $BIFURCATED_CALLER_FQN { *; } + -keepclassmembers class $survivingClass { public static int sideEffect; } + -checkdiscard class $IF_BRANCH_CODE_FQN { *; } + -dontwarn ** + """.trimIndent(), + ) +} + /** * Approximates `ProguardRulesGenerator` output for an Int flag `"max_retries"` in * module `":int-test"` with the given [returnValue].