Extract R8 DCE verification into featured-shrinker-tests module#165
Open
kirich1409 wants to merge 1 commit intomainfrom
Open
Extract R8 DCE verification into featured-shrinker-tests module#165kirich1409 wants to merge 1 commit intomainfrom
kirich1409 wants to merge 1 commit intomainfrom
Conversation
Move R8EliminationTest from featured-gradle-plugin into a dedicated non-published JVM module. Split the monolithic test (~660 lines) into focused units: bytecode generators, JAR assembly, ProGuard rules, test harness, assertions, and two test classes (boolean/int flags). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Summary by QodoExtract R8 dead-code-elimination tests into featured-shrinker-tests module
WalkthroughsDescription• Extracted 657-line R8 DCE test into dedicated featured-shrinker-tests module • Split monolithic test into 8 focused files across 5 packages by concern • Removed R8/ASM dependencies from featured-gradle-plugin • Added comprehensive R8 verification guide with test scenarios and usage Diagramflowchart LR
A["featured-gradle-plugin<br/>R8EliminationTest<br/>657 lines"] -->|Extract| B["featured-shrinker-tests<br/>8 focused files"]
B -->|Bytecode| C["SyntheticBytecodeFactory"]
B -->|Rules| D["ProguardRulesWriter"]
B -->|Harness| E["R8TestHarness"]
B -->|Tests| F["R8BooleanFlagEliminationTest<br/>R8IntFlagEliminationTest"]
B -->|Assertions| G["JarAssertions"]
H["docs/guides/r8-verification.md"] -->|Documents| B
File Changes1. featured-gradle-plugin/build.gradle.kts
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+30
to
+47
| * `-keepclassmembers` pins the `sideEffect` field of the **surviving** branch class so | ||
| * that R8 cannot treat the `doWork()` call as a no-op and eliminate the class via | ||
| * write-only field optimisation. The dead branch class intentionally has no such rule, | ||
| * so R8 is free to eliminate it once the branch becomes unreachable. | ||
| */ | ||
| internal fun writeBooleanRules( | ||
| dest: File, | ||
| returnValue: Boolean, | ||
| ) { | ||
| val survivingClass = if (returnValue) 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 $returnValue; | ||
| } | ||
| -keep class $BIFURCATED_CALLER_FQN { *; } | ||
| -keepclassmembers class $survivingClass { public static int sideEffect; } | ||
| -dontwarn ** |
There was a problem hiding this comment.
1. Dce tests can false-pass 🐞 Bug ≡ Correctness
writeBooleanRules (and writeIntRules) only protects the expected-surviving class’s sideEffect field, while the synthetic doWork() has no observable effect other than incrementing that field. R8 can eliminate the unprotected branch/guarded class even when it is still reachable (by stripping the write-only field/writes), causing the tests to pass without actually validating that -assumevalues drove the DCE outcome.
Agent Prompt
## Issue description
The R8 DCE verification tests can produce false positives because a branch/guarded class may be removed due to write-only field optimization, not because `-assumevalues` made the code unreachable.
## Issue Context
`doWork()` only increments a static `sideEffect` field. In the assumevalues scenarios, the ProGuard rules pin `sideEffect` only for the *expected* surviving class (or not at all for the int assumevalues case), so the other class can still be optimized away even if reachable.
## Fix Focus Areas
- featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt[35-85]
## Concrete fix
- In `writeBooleanRules(...)`, add `-keepclassmembers` for **both** `IfBranchCode` and `ElseBranchCode` (not just the computed `survivingClass`).
- This prevents write-only stripping from deciding class presence, while still allowing the unreachable class to be removed (because `-keepclassmembers` does not keep the class itself).
- In `writeIntRules(...)`, add `-keepclassmembers class PositiveCountCode { public static int sideEffect; }` so `PositiveCountCode` cannot disappear unless the guarded branch is proven unreachable.
- Keep the existing control tests; after these changes, they will more directly validate that `-assumevalues` is what changes reachability/class presence.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Extracts the R8 dead-code-elimination (DCE) verification tests from :featured-gradle-plugin into a dedicated :featured-shrinker-tests module, keeping the Gradle plugin’s test suite lighter while preserving deterministic verification of -assumevalues behavior.
Changes:
- Added new
:featured-shrinker-testsmodule with an R8 test harness, ASM-generated bytecode inputs, ProGuard rules writer, and jar assertions. - Removed the monolithic
R8EliminationTest(and its R8/ASM test deps) from:featured-gradle-plugin. - Added an MkDocs guide and navigation entry for R8 DCE verification.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Registers the new :featured-shrinker-tests module in the build. |
| mkdocs.yml | Adds documentation nav entry for the new R8 verification guide. |
| featured-shrinker-tests/build.gradle.kts | New JVM test-only module with R8 + ASM dependencies. |
| featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/rules/ProguardRulesWriter.kt | Writes test ProGuard rules (including -assumevalues) for the synthetic scenarios. |
| featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt | Boolean-flag DCE verification tests. |
| featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8IntFlagEliminationTest.kt | Int-flag DCE verification tests. |
| featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/harness/R8TestHarness.kt | Shared harness for building inputs, invoking R8, and managing temp dirs. |
| featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/bytecode/SyntheticBytecodeFactory.kt | ASM bytecode factory for synthetic class graphs used in tests. |
| featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/bytecode/JarAssembler.kt | Assembles the generated classes into input JARs for R8. |
| featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/assertions/JarAssertions.kt | Asserts presence/absence of classes in the R8 output JAR. |
| featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/R8EliminationTest.kt | Deletes the old monolithic R8 test from the plugin module. |
| featured-gradle-plugin/build.gradle.kts | Removes R8/ASM test dependencies now that tests moved modules. |
| docs/guides/r8-verification.md | Adds a guide describing what the shrinker tests verify and how to run/extend them. |
| .gitignore | Ignores swarm-report/ output. |
Comment on lines
+19
to
+20
| * write rules files in the exact format `ProguardRulesGenerator` produces, run R8 | ||
| * programmatically, and assert presence/absence of flag-guarded classes in the output JAR. |
There was a problem hiding this comment.
The KDoc says the rules files are written in the exact format produced by ProguardRulesGenerator, but the test rules include additional scaffolding directives (e.g., -keep, -dontwarn) and omit the generator’s header comments. Please reword to clarify that the -assumevalues block matches the generator output and the rest is test-only scaffolding.
Suggested change
| * write rules files in the exact format `ProguardRulesGenerator` produces, run R8 | |
| * programmatically, and assert presence/absence of flag-guarded classes in the output JAR. | |
| * write test rule files whose `-assumevalues` block matches what `ProguardRulesGenerator` | |
| * produces, add the surrounding `-keep`/`-dontwarn` directives as test-only scaffolding, | |
| * run R8 programmatically, and assert presence/absence of flag-guarded classes in the | |
| * output JAR. |
Comment on lines
+23
to
+24
| 2. **Rules generation** — `ProguardRulesWriter` writes `.pro` files in the exact format | ||
| `ProguardRulesGenerator` produces, optionally including the `-assumevalues` block. |
There was a problem hiding this comment.
This guide states that ProguardRulesWriter writes .pro files in the exact format ProguardRulesGenerator produces, but the test rules intentionally add extra directives (e.g., -keep, -dontwarn) and don’t include the generator’s header comments. Consider clarifying that the -assumevalues block matches the plugin output while the rest is test scaffolding.
Suggested change
| 2. **Rules generation** — `ProguardRulesWriter` writes `.pro` files in the exact format | |
| `ProguardRulesGenerator` produces, optionally including the `-assumevalues` block. | |
| 2. **Rules generation** — `ProguardRulesWriter` writes test `.pro` files whose | |
| `-assumevalues` block matches the format produced by `ProguardRulesGenerator`, | |
| optionally including that block. The surrounding directives are test scaffolding | |
| (for example `-keep` and `-dontwarn`) rather than a byte-for-byte copy of the | |
| plugin's full output. |
Comment on lines
+9
to
+11
| import org.junit.After | ||
| import org.junit.Before | ||
| import java.io.File |
There was a problem hiding this comment.
This harness uses JUnit4 lifecycle annotations (org.junit.Before/After). Most tests in this repo use kotlin.test.BeforeTest/AfterTest instead (e.g. featured-registry/.../FlagRegistryTest.kt:10, providers/javaprefs/...Test.kt:32). Consider switching to the kotlin.test lifecycle annotations for consistency and to avoid depending on JUnit APIs in the test sources.
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.
Summary
featured-gradle-plugininto standalonefeatured-shrinker-testsmoduleR8EliminationTest(~660 lines) into 8 focused files across 5 packagesfeatured-gradle-plugindocs/guides/r8-verification.mdTest plan
./gradlew :featured-shrinker-tests:test— 5 tests pass./gradlew :featured-gradle-plugin:test— 144 tests pass🤖 Generated with Claude Code