From 713340b6d9fee4f784b1c8adce2abaf7c734a2e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 06:13:28 +0000 Subject: [PATCH 1/2] test(shrinker): cover -keep defeating flag dead-code elimination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A consumer -keep rule (often a broad wildcard or @Keep) that covers a flag-guarded class defeats R8 tree-shaking: -assumevalues still folds the disabled branch (behaviour unchanged), but the class itself is pinned as an unconditional GC root and ships in the APK despite being unreachable — silently losing the size benefit of build-time flags. - Add writeBooleanRulesWithKeptDeadBranch() modelling the pitfall - Add a regression test asserting the dead-branch class survives the keep - Document the two-phase elimination model and keep-rule guidance in the R8 verification guide --- docs/guides/r8-verification.md | 43 +++++++++++++++++++ .../r8/R8BooleanFlagEliminationTest.kt | 23 ++++++++++ .../shrinker/rules/ProguardRulesWriter.kt | 26 +++++++++++ 3 files changed, 92 insertions(+) diff --git a/docs/guides/r8-verification.md b/docs/guides/r8-verification.md index 51cffe8..153326a 100644 --- a/docs/guides/r8-verification.md +++ b/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,45 @@ 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. Two related rules are *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. +- `-dontoptimize` is a different lever entirely: it disables phase 1 and stops all folding. It + is not a keep rule, but it has the same end result of suppressing elimination. + ## Running the tests ```bash diff --git a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt index fda8aba..3c75423 100644 --- a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt +++ b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt @@ -7,6 +7,7 @@ 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 +95,26 @@ 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. + */ + @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) + } } 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 69f409d..2bce2b0 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 @@ -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 From 7b928eff27b0210ab0941a8afd41bd86937dc23e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 08:07:55 +0000 Subject: [PATCH 2/2] test(shrinker): assert branch folding in keep regression; fix docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on the -keep regression test and guide: - The keep test now also asserts BifurcatedCaller no longer references IfBranchCode, proving R8 still folded the disabled branch (phase 1) rather than only keeping the class alive via the kept caller. Adds assertClassDoesNotReference() (ASM bytecode inspection). - Move -dontoptimize out of the 'not a problem' list in the R8 guide into a distinct hazard note — it suppresses elimination and must not be grouped with the harmless accessor-method keep. --- docs/guides/r8-verification.md | 13 ++- .../shrinker/assertions/JarAssertions.kt | 89 +++++++++++++++++++ .../r8/R8BooleanFlagEliminationTest.kt | 7 ++ 3 files changed, 105 insertions(+), 4 deletions(-) diff --git a/docs/guides/r8-verification.md b/docs/guides/r8-verification.md index 153326a..ec03556 100644 --- a/docs/guides/r8-verification.md +++ b/docs/guides/r8-verification.md @@ -87,13 +87,18 @@ rules that accidentally cover flag-guarded code**: - `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. Two related rules are *not* a problem and should -not be "fixed": +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. -- `-dontoptimize` is a different lever entirely: it disables phase 1 and stops all folding. It - is not a keep rule, but it has the same end result of suppressing elimination. + +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 diff --git a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/assertions/JarAssertions.kt b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/assertions/JarAssertions.kt index 9148399..bd633f4 100644 --- a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/assertions/JarAssertions.kt +++ b/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?, + ): 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", + ) +} diff --git a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt index 3c75423..7aa4a70 100644 --- a/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt +++ b/featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt @@ -1,6 +1,7 @@ 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 @@ -108,6 +109,11 @@ internal class R8BooleanFlagEliminationTest : R8TestHarness() { * 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`() { @@ -116,5 +122,6 @@ internal class R8BooleanFlagEliminationTest : R8TestHarness() { 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) } }