fix(adapter): normalize structured r2flutter superclass metadata - #99
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe Python adapter adds Suggested reviewers: Merge Risk: ⚪ Minimal · up to Superclass metadata is normalized for structured r2flutter output, with coverage for named and unresolved references. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, the normalization behavior, the error, and the real-binary validation result. It does not follow the required template because it omits the Summary, Validation, and Scope sections and does not include the required validation and scope checklists. Resolution Rewrite the description using the repository template. Add Summary, Validation, and Scope sections. Mark each validation item, including cargo fmt, cargo clippy, cargo test, and the real-binary check. Mark the atomic commits, documentation, and unrelated-refactor scope items.
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
There was a problem hiding this comment.
Nice find. The fix does what it says, I ran it over the shapes r2flutter can actually emit and it holds up.
Small correction for the description though: it isn't that r2flutter "may" return structured metadata, it always does. The object is unconditional in dart_pool_classes.c:
if (ci->super_class_name || ci->super_class_ref || ci->super_type_ref) {
pj_k (pj, "super");
pj_o (pj);
if (ci->super_class_ref) pj_kn (pj, "ref", ci->super_class_ref);
if (ci->super_type_ref) pj_kn (pj, "type_ref", ci->super_type_ref);
if (ci->super_class_name) pj_ks (pj, "name", ci->super_class_name);So (c.get("super") or "Object") was never right for this backend. It only survived because nothing exercises the r2flutter class projection, which is also my one real ask here: this needs a test, for the same reason it broke in the first place. The pattern to copy is run_adapter_blutter_backend_synthesizes_entrypoint_candidate in crates/flutterdec-adapter/src/lib.rs, which writes a fake backend executable and runs the real template through it. A fake r2flutter answering -jH, -ji, -jxz and -jc, with one class whose super is {"type_ref": 35836}, would pin it. Ping me if stubbing the four flags is a pain, I have most of that lying around.
Commit subject needs reformatting too, CONTRIBUTING wants type(scope): description, so something like fix(adapter): normalize structured r2flutter superclass metadata.
On the Object fallback, I'm keeping it but I want it commented. {"type_ref": 35836} means r2flutter couldn't resolve the super to a class, and answering Object claims the class extends Object directly, which for most Dart classes is false. Under the v3 schema super is a required non-null string so there's genuinely nowhere else to go, and or "Object" is already what this file does in four other places, so you're being consistent rather than introducing anything. Just drop a line at the return saying it's a placeholder and not recovered data, so nobody reads it as fact later.
Ignoring ref when there's no name is the right call, by the way. r2flutter resolves the name from ctx->refs[super_class_ref] itself, so a ref with no name means its own lookup already failed, and re-matching inside -jc won't do better.
One heads up, and this part is my problem rather than yours. #97/#98 rewrite this producer for ProgramModel v4 and delete the function you're patching. I tried the merge: if this lands and I then rebase, there's a single conflict in adapter_template.py whose only sensible resolution is taking the v4 side, and _r2flutter_super_name quietly goes with it. My rewrite has the same bug in a nastier form anyway, it does _sanitize_class_name(raw_super or "") and that calls .strip(), so your exact payload gives AttributeError: 'dict' object has no attribute 'strip'. Same bug in two places, and mine needs its own fix rather than a copy of yours. v4 can at least be honest about it, super_class is Optional[int] there and class_relationships already carries PARTIAL/UNAVAILABLE, so unresolved can just be None.
Point being, this should go in first. Sixteen lines against a crash that's live on main today, and I'll pick it up on my side afterwards. The other order strands your PR when the code it patches stops existing.
20635fb to
e4dcb2b
Compare
e4dcb2b to
6d6012d
Compare
caverav
left a comment
caverav
left a comment
There was a problem hiding this comment.
Subject and the placeholder comment are good, and the fake backend is exactly the shape I had in mind. Requesting changes on two things in the test rather than in the fix itself, both suggested inline.
I ran the test, then tried to break it. Reverting the call site to (c.get("super") or "Object") fails it with the original invalid type: map, expected a string, so it does pin the bug you reported. But deleting the whole dict branch from the helper, all six lines of the name lookup, leaves the test passing, because the fixed and the gutted version both return Object for a type_ref-only super. The half of the helper that actually recovers a name is the half that isn't guarded. Adding one resolved class to the fake covers it, and I checked that version passes on your code and fails with the dict branch deleted.
The other one is hermeticity. The wrapper sets FLUTTERDEC_R2FLUTTER_BIN, but _resolve_r2flutter_runner checks FLUTTERDEC_R2FLUTTER_CMD first, so anyone with that exported in their shell gets the fake ignored and the test fails with r2flutter -jH failed (1). Reproduced here with FLUTTERDEC_R2FLUTTER_CMD=/bin/false. Setting _CMD in the wrapper is enough, and that is why the blutter test doesn't hit this, it already uses the highest-precedence variable.
If you take all three suggestions as one commit, test(adapter): harden the r2flutter superclass test works as the subject.
Everything I said before about merge order and my own v4 rebase still stands, none of that is on you.
|
Updated, thanks. 310306f added. |
There was a problem hiding this comment.
Thank you for the PR! All three taken as-is, and the commit subject is right. This looks good to me.
I re-ran the mutation checks against 310306f and both now behave the way they should. Reverting the call site to (c.get("super") or "Object") still fails with the original invalid type: map, expected a string, so the reported bug stays pinned. Deleting the dict branch from the helper now fails too, on right: "StatefulWidget", where before it passed. That was the gap, and it's closed. The stray FLUTTERDEC_R2FLUTTER_CMD=/bin/false case passes now instead of blowing up on r2flutter -jH failed (1), so the fake is properly pinned. cargo fmt --check and clippy -D warnings are clean on the adapter crate, and all four CI jobs are green.
One thing I deliberately did not ask for, so nobody else asks later: the isinstance(value, str) branch is still uncovered, and that's fine. It's unreachable for any current r2flutter, since super is always emitted as an object. It's there as cheap insurance against the JSON shape changing, and a test asserting a code path the tool can't produce would just be noise.
Merging this ahead of #97/#98 as discussed. I'll carry the fix into the v4 producer on my side, where the crash is an AttributeError rather than a parse error and the honest answer is None plus a degraded class_relationships instead of a placeholder. Thanks for the clean turnaround on this, and for finding it in the first place.
caverav
changed the title
r2flutter emits class `super` as a JSON object, so the v4 projection's
`_sanitize_class_name(raw_super or "")` raised `AttributeError: 'dict' object
has no attribute 'strip'` on any snapshot carrying a `{"type_ref": N}`
superclass. #99 fixed the same root cause in the v3 producer, which this branch
replaces, so the fix does not survive the rebase and is reimplemented here.
v4 does not need the `Object` placeholder v3's schema forced: `super_class` is
an optional id, so an unresolved reference is reported as no edge. r2flutter
fills in `name` itself when a reference resolves, which makes a bare
`ref`/`type_ref` a lookup its own resolver already failed.
Covered by a fake-r2flutter producer test asserting both shapes, and by an
assertion that `class_relationships` stays unavailable when no superclass
resolves rather than claiming partial off a class table alone.
r2flutter emits class `super` as a JSON object, so the v4 projection's
`_sanitize_class_name(raw_super or "")` raised `AttributeError: 'dict' object
has no attribute 'strip'` on any snapshot carrying a `{"type_ref": N}`
superclass. #99 fixed the same root cause in the v3 producer, which this branch
replaces, so the fix does not survive the rebase and is reimplemented here.
v4 does not need the `Object` placeholder v3's schema forced: `super_class` is
an optional id, so an unresolved reference is reported as no edge. r2flutter
fills in `name` itself when a reference resolves, which makes a bare
`ref`/`type_ref` a lookup its own resolver already failed.
Covered by a fake-r2flutter producer test asserting both shapes, and by an
assertion that `class_relationships` stays unavailable when no superclass
resolves rather than claiming partial off a class table alone.
2 participants
The r2flutter adapter currently assumes that superclass metadata is always a string.
On newer Dart snapshots, r2flutter may return structured superclass metadata such as:
Passing this object directly into ProgramModel causes deserialization to fail with:
This change normalizes superclass metadata before building ProgramModel:
Validated against a Dart 3.6.2 FullAOT snapshot using the r2flutter backend. Before this change, adapter output parsing failed. After the change, decompilation completes successfully with all 20,105 discovered functions disassembled.