fix(runtime): pass the property key to JSON.stringify toJSON, omit undefined members (#5909) - #6686
Conversation
…defined members (PerryTS#5909) SerializeJSONProperty (ECMA-262 §25.5.2.2 step 2) now hands `toJSON` the correct property key — the empty String at the root, an object's own key for a member, the stringified index for an array element — instead of the hardcoded empty string. The key travels from each serialization loop to the `object_get_to_json` / `array_get_to_json` / `bigint_apply_to_json` probes through a new `TO_JSON_KEY` thread-local (an owned Rust String, so it never roots a movable GC pointer), reset at each top-level entry and threaded through the object-property, array-element, shape-template, and replacer (`apply_to_json_keyed`) paths. A heap-valued object member's `toJSON` is now applied by `member_to_json` BEFORE the member key is written, so a member whose `toJSON` returns `undefined`/function/Symbol is omitted per spec instead of emitting `"k":null`. `member_to_json` returns `Some` only when a `toJSON` actually ran, so a plain data-object member keeps the PerryTS#6009 fast path and the one-shot suppression guard is never leaked into a sibling member. Flips test262 built-ins/JSON/stringify/value-tojson-arguments fail→pass with zero regressions across the built-ins/JSON slice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 6 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughJSON serialization now tracks the current property key or array index in thread-local state, passes it to ChangesJSON toJSON key propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant stringify_object_inner
participant member_to_json
participant object_get_to_json
participant toJSON_callback
stringify_object_inner->>member_to_json: set property key and probe member
member_to_json->>object_get_to_json: resolve toJSON with current key
object_get_to_json->>toJSON_callback: invoke toJSON(key)
toJSON_callback-->>member_to_json: return replacement or omitted value
member_to_json-->>stringify_object_inner: serialize replacement or skip member
🚥 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/json/stringify.rs (1)
1258-1296: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord the property key before primitive BigInt fallback
write_numberalready routes BigInt throughserialize_bigint; the gap is that these object-member and shape-template primitive paths don’t set the pendingtoJSONkey first. A field like{ a: 1n }can reachBigInt.prototype.toJSONwith the old/root key instead of"a".🤖 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 `@crates/perry-runtime/src/json/stringify.rs` around lines 1258 - 1296, The inline primitive dispatch for object members must record the current property key before BigInt reaches write_number and serialize_bigint. Update the surrounding member/shape-template serialization paths, including the field_bits dispatch near write_number, to set the pending toJSON key using the existing key-tracking mechanism before serializing primitive values, while preserving current handling for null, booleans, strings, pointers, and numbers.
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/json/stringify.rs`:
- Around line 1258-1296: The inline primitive dispatch for object members must
record the current property key before BigInt reaches write_number and
serialize_bigint. Update the surrounding member/shape-template serialization
paths, including the field_bits dispatch near write_number, to set the pending
toJSON key using the existing key-tracking mechanism before serializing
primitive values, while preserving current handling for null, booleans, strings,
pointers, and numbers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 975be661-22c3-400a-ada2-40b67cdd0d5f
📒 Files selected for processing (6)
crates/perry-runtime/src/json/mod.rscrates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/json/stringify.rscrates/perry-runtime/src/json/stringify_api.rscrates/perry-runtime/src/json/stringify_scalars.rscrates/perry-runtime/src/json/stringify_tojson_probe.rs
…emplate fields (PerryTS#5909) A BigInt field reaches `serialize_bigint` → `bigint_apply_to_json` (which reads the pending `toJSON` key) through `write_number`'s numeric fallback, which the object-member and shape-template primitive dispatch paths took without first recording the member's key — so `{ a: 1n }` with a userland `BigInt.prototype.toJSON` observed the previous/root key instead of "a". The array-element path already set it. Set the key in the three primitive fallback branches when the value is a BigInt (zero cost for ordinary numbers). Addresses CodeRabbit review on PerryTS#6686. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai — good catch. Addressed in b8329f4: the object-member and shape-template primitive dispatch paths now record the member's |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
Fixes
built-ins/JSON/stringify/value-tojson-arguments.jsfrom the test262 JSON worklist (#5909).SerializeJSONProperty(ECMA-262 §25.5.2.2 step 2) now:toJSON— the empty String at the root, an object's own key for a member, the stringified index for an array element. Perry previously always passed"".toJSONto an object member before its key is written, so a member whosetoJSONreturnsundefinedis omitted per spec instead of being emitted as"k":null.Both are the same root: getting
Call(toJSON, value, « key »)and the "undefined member is omitted" rule right.What changed (perry-runtime
json/)TO_JSON_KEYthread-local (mod.rs) — an owned RustString(so it never roots a movable GC pointer) holding the key to handtoJSON. Reset to""at each top-levelJSON.stringifyentry; set before each child recursion by the object-property loop, the array-element loops, the shape-template fast path, and the replacer walk (apply_to_json_keyed, which already carried the key but discarded it). The three probes (object_get_to_json/array_get_to_json/bigint_apply_to_json) read it instead of allocating a hardcoded empty string.member_to_json(stringify.rs) — applies a heap-valued member'stoJSONup front in the object walk, returningSome(result)only when atoJSONactually ran. The caller omits the member when the result isundefined/function/Symbol, else serializes the result withSUPPRESS_NEXT_TO_JSONarmed (so it isn't re-probed). ReturnsNone— "serialize normally" — for plain objects without atoJSONand for non-object/array values, so a plain data-object member keeps theJSON.stringifyis too slow then nodejs #6009 fast path (no added probe) and the one-shot guard is never leaked into a sibling member.Verification
built-ins/JSONslice (scripts/test262_subset.py --dir built-ins/JSON), baseline vs. patched built the same way (target/release):value-tojson-arguments: fail → passtoJSONobjects wrappingtoJSONobjects,toJSON→undefined omission with surrounding keys, replacer +toJSONkeys, class instances, Date/Buffer/RegExp members, shape-template arrays) all match Node 26 byte-for-byte.cargo fmtclean on the changed files;scripts/check_file_size.shpasses (stringify.rs1959 < 2000).Scope note
The
toJSON-key threading also covers the compact replacer walk (viaapply_to_json_keyed). The pretty-print serializer (stringify_value_pretty, used when aspaceargument is given) pre-builds its member list with comma placement keyed on that list's length, so applying the same "omit anundefined-toJSONmember" rule there is a separate change; it keeps its prior (pre-existing) behavior in this PR. The target test and every case in thebuilt-ins/JSONslice use the compact form, so this is a follow-up, not a regression.Code-only per the issue: no version bump, no
CHANGELOG/CLAUDE.mdedits.Refs #5909.
🤖 Generated with Claude Code
Summary by CodeRabbit
toJSONcallbacks receive the correct property name or array index.JSON.stringifycalls use the correct root key and do not inherit stale key values.toJSONreturnsundefined, functions, or Symbols.