ADFA-3604: Fix 2 more R8 shrink bugs (Gson templates, JDI debugger) - #1610
Open
davidschachterADFA wants to merge 2 commits into
Open
ADFA-3604: Fix 2 more R8 shrink bugs (Gson templates, JDI debugger)#1610davidschachterADFA wants to merge 2 commits into
davidschachterADFA wants to merge 2 commits into
Conversation
Found via a "Failed to load template archive ... Abstract classes can't be instantiated!" error on-device. Same root cause as the other shrink bugs fixed in this branch: Gson deserializes these classes only via reflection (gson.fromJson(..., X::class.java)), never a direct `new` R8 can trace, so it strips the constructor and Gson's runtime then reports the class as abstract. Fixes templates-impl's TemplatesIndex/TemplateJson/etc., and two more instances found by auditing every gson.fromJson call site in the repo: OpenedFilesCache/OpenedFile (no prior rule at all) and the breakpoint persistence models. Verified on-device: templates load cleanly, zero FATAL EXCEPTION in logcat, Kotlin project init still succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found via a "Network access error" dialog on-device -- misleading, since
the app's debug-connect failure handler always appends a network-access
suggestion regardless of actual cause. The real error was buried in
logcat: "java.lang.Error: no Connectors loaded" from
com.sun.tools.jdi.VirtualMachineManagerImpl, caused by a
ServiceConfigurationError failing to instantiate
SocketAttachingConnector/SocketListeningConnector. Same root cause as
the other shrink bugs: JDI loads these via ServiceLoader, which R8
can't trace, so it stripped their no-arg constructors.
This exact fix was already anticipated and left commented out in this
file ("Initial rules to enable when R8 is shrinking to address
exceptions") from before shrinking was ever genuinely enabled -- just
needed uncommenting now that it is.
Verified on-device: JDWP listener starts successfully, no dialog, zero
FATAL EXCEPTION in logcat.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📝 Walkthrough
WalkthroughThe ProGuard configuration now keeps Gson-deserialized model packages and template ZIP models. It also enables retention for JDI connector classes and constructors loaded through ServiceLoader. ChangesShrinker retention rules
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/proguard-rules.pro`:
- Around line 117-118: Correct the positional reference in the comment
describing templates.impl.zip near the OpenedFilesCache/OpenedFile rules: change
“above” to “below” to reflect the template rule’s location, or remove the
positional wording entirely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d45ea7d7-01a5-4824-b9f3-3f25873e5b37
📒 Files selected for processing (1)
app/proguard-rules.pro
Comment on lines
+117
to
+118
| # templates.impl.zip above. Covers OpenedFilesCache/OpenedFile (no prior | ||
| # rule) as well as the APK metadata classes already listed individually. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the relative rule reference.
The template rule is at Line 174, below this block. Change above to below, or remove the positional reference.
Proposed fix
-# templates.impl.zip above. Covers OpenedFilesCache/OpenedFile (no prior
+# templates.impl.zip below. Covers OpenedFilesCache/OpenedFile (no prior📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # templates.impl.zip above. Covers OpenedFilesCache/OpenedFile (no prior | |
| # rule) as well as the APK metadata classes already listed individually. | |
| # templates.impl.zip below. Covers OpenedFilesCache/OpenedFile (no prior | |
| # rule) as well as the APK metadata classes already listed individually. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/proguard-rules.pro` around lines 117 - 118, Correct the positional
reference in the comment describing templates.impl.zip near the
OpenedFilesCache/OpenedFile rules: change “above” to “below” to reflect the
template rule’s location, or remove the positional wording entirely.
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
1 participant
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.
Follow-up to #1609 — two more R8 shrink bugs found after that PR merged
#1609 (fixes 1–5 of the R8 shrink/optimize bugs found while verifying ADFA-3604 on-device) was merged to
stagewhile this session was still actively testing. This PR carries the two additional fixes found afterward, cherry-picked cleanly onto currentstage.What this PR fixes
gson.fromJson(..., X::class.java)reflection have no traceablenewcall site, so R8 strips their constructor and Gson's runtime then reports them as abstract (Failed to load template archive ... Abstract classes can't be instantiated!onTemplatesIndex, surfaced as a user-facing error dialog). Found and fixed for everygson.fromJsoncall site in the repo, including two more instances with no prior keep rule at all (OpenedFilesCache/OpenedFile, breakpoint persistence models).com.sun.tools.jdi'sSocketAttachingConnector/SocketListeningConnectorare loaded viaServiceLoader, which R8 can't trace; stripping their constructors broke the debugger withError: no Connectors loaded, which the app then surfaced to the user as a misleading "Network access error" (the debug-connect failure handler always appends a network-access suggestion regardless of actual cause). This exact fix was already anticipated and left commented out in this file since before shrinking was ever genuinely enabled — just needed uncommenting.Verification
Built and installed on a physical ARM device (Samsung Galaxy Note20 Ultra):
Starting JDWP listener,startListening), no dialogFATAL EXCEPTIONin logcatRecommendation
A broader smoke test (Java LSP, XML LSP, actually building/running a project) is still recommended before considering ADFA-3604 fully closed — everything verified so far has been found via targeted manual exploration, not exhaustive coverage of the app's core workflows.
🤖 Generated with Claude Code