diff --git a/docs/guides/r8-verification.md b/docs/guides/r8-verification.md index 51cffe8..ec03556 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,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 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 fda8aba..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,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) + } } 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