test(shrinker): cover -keep defeating flag dead-code elimination#217
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
kirich1409
force-pushed
the
claude/r8-flag-code-keep-UY7Md
branch
from
a039ed6 to
713340b
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds regression coverage and documentation for how user -keep rules can preserve flag-guarded dead classes even when -assumevalues still enables branch folding.
Changes:
- Adds a ProGuard rules writer scenario with a kept dead-branch class.
- Adds an R8 boolean flag regression test for the kept dead-branch case.
- Documents the two-phase R8 model and guidance around keep rules.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt |
Adds rules for the kept dead-branch test case. |
featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt |
Adds the boolean flag regression test. |
docs/guides/r8-verification.md |
Documents keep-rule behavior and R8 verification scenarios. |
Comment on lines
+116
to
+118
| assertClassPresent(outputJar, IF_BRANCH_CODE_INTERNAL) | ||
| assertClassPresent(outputJar, ELSE_BRANCH_CODE_INTERNAL) | ||
| assertClassPresent(outputJar, BIFURCATED_CALLER_INTERNAL) |
Comment on lines
+89
to
+96
| **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. |
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.
kirich1409
added a commit
that referenced
this pull request
May 31, 2026
kirich1409
added a commit
that referenced
this pull request
May 31, 2026
* ci(codeql): force Kotlin recompile so CodeQL sees source (#218) The Analyze Kotlin job failed with 'no source code seen during build' (exit code 32): assembleDebug compile tasks were served from cache / marked UP-TO-DATE, so CodeQL's tracer observed no Kotlin source. Add --no-build-cache --rerun-tasks to the CodeQL build step to force actual recompilation, giving the tracer source to analyze. Co-authored-by: Claude <noreply@anthropic.com> * chore: release 1.0.0 — Android-stable, docs restructure, CodeQL fix - Bump VERSION_NAME to 1.0.0 - Add [1.0.0] CHANGELOG entry (Android-facing API as primary stable target) - Fix mkdocs: exclude cc-verification/specs, add Known Limitations to nav, move iOS guides to "iOS Preview" section, update site_description - Add "Stable in 1.0" admonition to Android guide - Add "Preview" admonitions to iOS guides - Fix CodeQL workflow: build-mode=manual + --no-build-cache --rerun-tasks * test(shrinker): cover -keep defeating flag dead-code elimination (#217) * test(shrinker): cover -keep defeating flag dead-code elimination 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 * test(shrinker): assert branch folding in keep regression; fix docs 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. --------- Co-authored-by: Claude <noreply@anthropic.com> * chore: update Package.swift checksum for v1.0.0 * Bump develop version to 1.1.0-SNAPSHOT --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
This started from a question: what happens if a user adds
-keeprules covering code that sits behind a disabled build-time flag?Featured eliminates flag-guarded code in two distinct R8 phases:
-assumevaluesrule pins the flag accessor to a constant, so R8 foldsif (false) { … }and removes the dead branch (including the call into the guarded class) from the caller's body.-keepis an unconditional root of the reachability graph. So a-keepcovering a flag-guarded class:Net effect: a silent loss of the size benefit. Dead code ships with no visible symptom. The realistic trigger is rarely a deliberate keep — it's broad wildcards (
-keep class com.myapp.** { *; }), package-level@Keep, reflection/serialization/DI keep rules, or third-partyconsumer-rules.pro.Changes
ProguardRulesWriter.kt— addwriteBooleanRulesWithKeptDeadBranch()modelling the pitfall: the same-assumevalues … return falseblock plus a-keepon the dead-branch class.R8BooleanFlagEliminationTest.kt— add a regression test asserting the dead-branch class survives the keep despite the assumevalues rule. The existing control test (no rule → both branches survive) plus the elimination tests prove the-keepis specifically what brings the dead class back.docs/guides/r8-verification.md— document the two-phase model, the failure mode, and guidance: keep rules narrow; note that-keepon the accessor method is harmless (assumevalues still folds), while-dontoptimizeis a separate lever that suppresses phase 1.Verification
403for Google Maven (Android Gradle Plugin9.1.0and thecom.android.tools.r8artifacts cannot be downloaded).spotlessApply/spotlessCheckcould not be run for the same reason.The new test mirrors the existing passing tests in the same class (same harness, assertions, and constants). Please run
:featured-shrinker-tests:testandspotlessCheckin CI / locally to confirm.https://claude.ai/code/session_01LxuawhwkZuJAAoMkDpSTGZ
Generated by Claude Code