test(shrinker): cover -keep defeating flag dead-code elimination

docs/guides/r8-verification.md

@@ -42,6 +42,10 @@ present or absent, proving that the rule caused (or did not cause) elimination.
 The third test is a control: it proves that elimination is caused by the rule, not by R8's
 own constant-folding.
 
+The fourth test — `dead-branch class survives when a user -keep rule pins it despite the
+assumevalues rule` — covers the consumer pitfall described in
+[Keep rules vs. flag-guarded code](#keep-rules-vs-flag-guarded-code) below.
+
 ### Int flags (`R8IntFlagEliminationTest`)
 
 | Test | Rule | Expected |
@@ -52,6 +56,50 @@ own constant-folding.
 With `-assumevalues return 0`, R8 constant-folds `0 > 0` to `false` and eliminates the
 guarded block entirely.
 
+## Keep rules vs. flag-guarded code
+
+Dead-code elimination for a disabled flag happens in two distinct R8 phases:
+
+1. **Constant folding + branch elimination** (an *optimization*). The `-assumevalues` rule
+   pins the flag accessor to a constant, so R8 folds `if (false) { … }` and removes the dead
+   branch — including the call into the flag-guarded class — from the *caller's* method body.
+2. **Tree-shaking** (a *shrink*). Once nothing references the flag-guarded class anymore, it
+   becomes unreachable and R8 drops it from the output.
+
+Phase 2 is reachability-based, and `-keep` is an **unconditional root** of the reachability
+graph. So if a consumer adds a `-keep` that covers a flag-guarded class:
+
+- Phase 1 **still runs** — the branch is still folded away, the guarded code never executes,
+  and runtime behaviour is unchanged. No crash, no correctness regression.
+- Phase 2 **does not run** for that class — R8 treats it as always-reachable and keeps it in
+  the APK even though nothing references it.
+
+**The net effect is a silent loss of the size benefit:** dead code ships in the APK with no
+visible symptom. This is verified by the `dead-branch class survives when a user -keep rule
+pins it…` test in `R8BooleanFlagEliminationTest`.
+
+A deliberate `-keep` on a specific flag class is rare. The realistic causes are **broad
+rules that accidentally cover flag-guarded code**:
+
+- catch-all wildcards such as `-keep class com.myapp.** { *; }`;
+- `@Keep` annotations applied at a package or base-class level;
+- reflection / serialization keep rules (Gson, Moshi, etc.) and DI keep rules;
+- `consumer-rules.pro` shipped by third-party libraries.
+
+**Recommendation:** keep these rules as narrow as possible and avoid blanket `-keep` over
+packages that contain flag-guarded features.
+
+One related rule is *not* a problem and should not be "fixed":
+
+- `-keep` on the **flag accessor method** — `-assumevalues` still constant-folds the value, so
+  branch elimination is unaffected; `-keep` only prevents removing/renaming the method itself.
+
+A separate hazard, **not** a keep rule, also defeats elimination and should be avoided:
+
+- `-dontoptimize` disables phase 1 entirely and stops all constant folding, so disabled
+  branches are never removed in the first place. Do not enable it in a build that relies on
+  build-time flag DCE.
+
 ## Running the tests
 
 ```bash

featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/assertions/JarAssertions.kt

@@ -1,7 +1,13 @@
 package dev.androidbroadcast.featured.shrinker.assertions
 
+import org.objectweb.asm.ClassReader
+import org.objectweb.asm.ClassVisitor
+import org.objectweb.asm.FieldVisitor
+import org.objectweb.asm.MethodVisitor
+import org.objectweb.asm.Opcodes
 import java.io.File
 import java.util.jar.JarFile
+import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 
@@ -28,3 +34,86 @@ internal fun assertClassPresent(
         )
     }
 }
+
+/**
+ * Asserts that the bytecode of [ownerInternalName] in [jar] contains no reference to
+ * [referencedInternalName] — neither as a `NEW`/type reference nor as a method-call target.
+ *
+ * This proves that R8 actually constant-folded the flag and removed the dead branch's call
+ * site from the *caller*, as opposed to merely keeping the branch-target class alive. A test
+ * that only checked class presence would still pass if folding silently stopped working and
+ * the call site stayed reachable; this assertion closes that gap.
+ */
+internal fun assertClassDoesNotReference(
+    jar: File,
+    ownerInternalName: String,
+    referencedInternalName: String,
+) {
+    val classBytes =
+        JarFile(jar).use { jf ->
+            val entry =
+                assertNotNull(
+                    jf.getJarEntry("$ownerInternalName.class"),
+                    "Expected $ownerInternalName to be present in the output JAR",
+                )
+            jf.getInputStream(entry).use { it.readBytes() }
+        }
+
+    var referenced = false
+    val referencedType = "L$referencedInternalName;"
+    val detector =
+        object : ClassVisitor(Opcodes.ASM9) {
+            override fun visitMethod(
+                access: Int,
+                name: String?,
+                descriptor: String?,
+                signature: String?,
+                exceptions: Array<out String>?,
+            ): MethodVisitor =
+                object : MethodVisitor(Opcodes.ASM9) {
+                    override fun visitTypeInsn(
+                        opcode: Int,
+                        type: String?,
+                    ) {
+                        if (type == referencedInternalName) referenced = true
+                    }
+
+                    override fun visitMethodInsn(
+                        opcode: Int,
+                        owner: String?,
+                        name: String?,
+                        descriptor: String?,
+                        isInterface: Boolean,
+                    ) {
+                        if (owner == referencedInternalName) referenced = true
+                    }
+
+                    override fun visitFieldInsn(
+                        opcode: Int,
+                        owner: String?,
+                        name: String?,
+                        descriptor: String?,
+                    ) {
+                        if (owner == referencedInternalName) referenced = true
+                    }
+                }
+
+            override fun visitField(
+                access: Int,
+                name: String?,
+                descriptor: String?,
+                signature: String?,
+                value: Any?,
+            ): FieldVisitor? {
+                if (descriptor == referencedType) referenced = true
+                return null
+            }
+        }
+    ClassReader(classBytes).accept(detector, ClassReader.SKIP_DEBUG or ClassReader.SKIP_FRAMES)
+
+    assertFalse(
+        referenced,
+        "Expected $ownerInternalName to no longer reference $referencedInternalName after R8 " +
+            "folded the disabled branch, but a reference was found in its bytecode",
+    )
+}

featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt

@@ -1,12 +1,14 @@
 package dev.androidbroadcast.featured.shrinker.r8
 
 import dev.androidbroadcast.featured.shrinker.assertions.assertClassAbsent
+import dev.androidbroadcast.featured.shrinker.assertions.assertClassDoesNotReference
 import dev.androidbroadcast.featured.shrinker.assertions.assertClassPresent
 import dev.androidbroadcast.featured.shrinker.bytecode.BIFURCATED_CALLER_INTERNAL
 import dev.androidbroadcast.featured.shrinker.bytecode.ELSE_BRANCH_CODE_INTERNAL
 import dev.androidbroadcast.featured.shrinker.bytecode.IF_BRANCH_CODE_INTERNAL
 import dev.androidbroadcast.featured.shrinker.harness.R8TestHarness
 import dev.androidbroadcast.featured.shrinker.rules.writeBooleanRules
+import dev.androidbroadcast.featured.shrinker.rules.writeBooleanRulesWithKeptDeadBranch
 import dev.androidbroadcast.featured.shrinker.rules.writeNoBooleanAssumeRules
 import kotlin.test.Test
 
@@ -94,4 +96,32 @@ internal class R8BooleanFlagEliminationTest : R8TestHarness() {
         assertClassPresent(outputJar, IF_BRANCH_CODE_INTERNAL)
         assertClassPresent(outputJar, ELSE_BRANCH_CODE_INTERNAL)
     }
+
+    /**
+     * Regression guard for the consumer pitfall: a user-supplied `-keep` on a class that is
+     * only reachable through the disabled branch defeats dead-code elimination.
+     *
+     * The `-assumevalues … return false` rule is present and still works — R8 constant-folds
+     * the flag and drops the dead branch's call site, so runtime behaviour is unchanged. But
+     * `-keep class IfBranchCode { *; }` is an unconditional GC root, so the class itself can no
+     * longer be tree-shaken: it survives in the output even though nothing reaches it.
+     *
+     * This is the failure mode behind broad wildcard / `@Keep` rules silently inflating the
+     * APK. The control test above proves elimination normally happens; this test proves a
+     * `-keep` is what brings the dead class back.
+     *
+     * The final assertion pins the documented split between the two R8 phases: even though the
+     * class is kept, the `-assumevalues` rule must still have folded the disabled branch, so
+     * `BifurcatedCaller` must no longer reference `IfBranchCode`. Without this, the test would
+     * pass even if folding silently stopped working and the call site stayed reachable.
+     */
+    @Test
+    fun `dead-branch class survives when a user -keep rule pins it despite the assumevalues rule`() {
+        val outputJar = runBooleanR8 { writeBooleanRulesWithKeptDeadBranch(it) }
+
+        assertClassPresent(outputJar, IF_BRANCH_CODE_INTERNAL)
+        assertClassPresent(outputJar, ELSE_BRANCH_CODE_INTERNAL)
+        assertClassPresent(outputJar, BIFURCATED_CALLER_INTERNAL)
+        assertClassDoesNotReference(outputJar, BIFURCATED_CALLER_INTERNAL, IF_BRANCH_CODE_INTERNAL)
+    }
 }

featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt

@@ -49,6 +49,32 @@ internal fun writeBooleanRules(
     )
 }
 
+/**
+ * Same `-assumevalues` block as [writeBooleanRules] with `returnValue = false`, but with an
+ * extra `-keep class … { *; }` on the **dead-branch** class ([IF_BRANCH_CODE_FQN]).
+ *
+ * This models a consumer that — deliberately or, far more commonly, via a broad wildcard or
+ * `@Keep` rule — pins a class that is only reachable through a disabled flag branch.
+ *
+ * The `-assumevalues` rule still lets R8 constant-fold the flag and drop the dead branch's
+ * *call site*, so behaviour is unchanged. But `-keep` is an unconditional GC root: the class
+ * itself can no longer be tree-shaken and survives in the output despite being unreachable,
+ * silently defeating the size benefit of build-time flags.
+ */
+internal fun writeBooleanRulesWithKeptDeadBranch(dest: File) {
+    dest.writeText(
+        """
+        -assumevalues class $BOOL_EXTENSIONS_FQN {
+            boolean $IS_DARK_MODE_ENABLED($CONFIG_VALUES_FQN) return false;
+        }
+        -keep class $BIFURCATED_CALLER_FQN { *; }
+        -keep class $IF_BRANCH_CODE_FQN { *; }
+        -keepclassmembers class $ELSE_BRANCH_CODE_FQN { public static int sideEffect; }
+        -dontwarn **
+        """.trimIndent(),
+    )
+}
+
 /**
  * No `-assumevalues` block — R8 cannot constant-fold the flag value.
  * The `-keepclassmembers` rules ensure the `sideEffect` field is not stripped