refactor(derive): serialize tagged WireEnum variants as structs - #1096
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ChangesTagged enum serialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WireEnumDerive
participant SerializeStruct
participant serde_json
WireEnumDerive->>SerializeStruct: generate tagged discriminator and fields
SerializeStruct->>serde_json: serialize JSON object
serde_json-->>SerializeStruct: provide serialized representation
Possibly related PRs
Suggested labels: 🚥 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}
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 `@wacore/derive/src/lib.rs`:
- Around line 1652-1671: Update the optional-field detection used by the
serialized length accounting around is_option_type so only fields explicitly
proven to be Option<T> are treated as optional. Avoid relying solely on the
final path-segment name; use the existing type-resolution mechanism or an
explicit optional marker, then ensure constant_count and per-field increments
apply consistently to the resolved classification.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1ec13c9c-e6aa-44df-b7d2-7b4f0f57666d
📒 Files selected for processing (2)
wacore/derive/src/lib.rswacore/tests/wire_enum_serde_test.rs
|
| Filename | Overview |
|---|---|
| wacore/derive/src/lib.rs | Serialization path refactored from SerializeMap to SerializeStruct; field-count formula and Option-aware len_prelude are correct; tuple-variant compile_error! fix properly scopes the pattern to avoid shadowing. |
| wacore/tests/wire_enum_serde_test.rs | Adds a purpose-built field-count-checking serializer that validates declared vs. written counts, JSON-pins all variant shapes, and includes a meta-test that guards the checker itself against false negatives. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["serialize"] --> B{"match self"}
B --> C["Unit/Fallback variant"]
B --> D["Named variant"]
C --> G["let __len = 1"]
D --> H{"any Option fields?"}
H -- no --> I["let __len = constant_count"]
H -- yes --> J["let mut __len = constant_count; increment per Some"]
G --> K["serialize_struct(name, __len)"]
I --> K
J --> K
K --> L["serialize_field(discriminator, wire_tag())"]
L --> M["serialize remaining fields, skip None"]
M --> N["SerializeStruct::end"]
Reviews (2): Last reviewed commit: "refactor(derive): serialize tagged WireE..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Baseline: |
The tagged branch emitted its payload as a map, so every field name went through the serializer's value path even though each one is a compile-time constant. Serializers that intern struct keys could not intern them and had to materialize a fresh key per field per value; in a host-object backend that is a new string plus a UTF-8 decode on every event, while the `#[derive(Serialize)]` structs alongside them cost nothing after the first. Switch to `serialize_struct` with an exact field count computed per variant (constant fields plus the discriminator, then one per present `Option`), so length-prefixed formats stay valid. `#[wire(skip)]` filtering, the omission of `None` fields and `Deserialize` are unchanged, and self-describing formats produce byte-identical output. The tuple-variant diagnostic is scoped to its own variant pattern instead of a wildcard arm, which would have shadowed every arm declared after it. The new tests pin the exact JSON for the tagged enums in the repo, covering all-fields-present, omitted `None`, skipped and unit variants, and the fallback tag. Since JSON discards the declared field count, a small serializer checks that count against the fields actually written, and a deliberately miscounted struct proves the check bites.
b8d1437 to
23f1187
Compare
|
Worth noting separately: the job is meant to be informational, but Generated by Claude Code |
2 participants
The tagged branch emitted its payload as a map, so every field name went through the serializer's value path even though each one is a compile-time constant. Serializers that intern struct keys could not intern them and had to materialize a fresh key per field per value; in a host-object backend that is a new string plus a UTF-8 decode on every event, while the
#[derive(Serialize)]structs alongside them cost nothing after the first.Switch to
serialize_structwith an exact field count computed per variant (constant fields plus the discriminator, then one per presentOption), so length-prefixed formats stay valid.#[wire(skip)]filtering, the omission ofNonefields andDeserializeare unchanged, and self-describing formats produce byte-identical output.The new tests pin the exact JSON for the tagged enums in the repo, covering all-fields-present, omitted
None, skipped and unit variants, and the fallback tag.