fix(object): #5347 — Object.assign/spread with an array source no longer crashes#5527
Conversation
📝 WalkthroughWalkthroughAdds a ChangesObject.assign array-source fix and boxed-String index rejection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/alloc.rs (1)
849-862: 🧹 Nitpick | 🔵 TrivialConsolidate
assign_canonical_indexto reuse the existingcanonical_array_indexhelper.The new
assign_canonical_indexfunction (lines 853–862) duplicates logic already present incrates/perry-runtime/src/object/field_get_set.rs:1539with the same semantics. Both functions:
- Reject empty strings and strings with leading zeros (except
"0")- Reject non-ASCII-digit characters
- Accept the range
0..=u32::MAX-1(canonical's4_294_967_294, assign'su32::MAXcheck)- Implicitly validate the round-trip property through their validation logic
Since
canonical_array_indexis widely used (26+ call sites) and well-tested, replacing the single call at line 885 withsuper::canonical_array_index(name)eliminates this maintenance burden and consolidates the canonical index parsing logic.🤖 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/object/alloc.rs` around lines 849 - 862, The `assign_canonical_index` function duplicates logic that already exists in the `canonical_array_index` helper function in the same module. Remove the entire `assign_canonical_index` function definition and replace the call to `assign_canonical_index` at line 885 with a call to the existing `canonical_array_index` function using `super::canonical_array_index(name)` or `canonical_array_index(name)` depending on module visibility, thereby consolidating the canonical index parsing logic and reducing maintenance burden.
🤖 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 `@crates/perry-runtime/src/object/alloc.rs`:
- Around line 1113-1125: In the loop within the array assignment section that
calls object_assign_set_string_key, add an explicit hole check to skip holes
instead of materializing them as undefined properties. Before the
object_assign_set_string_key call, insert a condition to check if the value
returned from js_array_get is a hole (using value.bits() == TAG_HOLE) and
continue to the next iteration if true, following the same pattern used in other
parts of the codebase like flat_clone.rs and field_get_set.rs.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/alloc.rs`:
- Around line 849-862: The `assign_canonical_index` function duplicates logic
that already exists in the `canonical_array_index` helper function in the same
module. Remove the entire `assign_canonical_index` function definition and
replace the call to `assign_canonical_index` at line 885 with a call to the
existing `canonical_array_index` function using
`super::canonical_array_index(name)` or `canonical_array_index(name)` depending
on module visibility, thereby consolidating the canonical index parsing logic
and reducing maintenance burden.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90ae96e6-7653-4b77-a1b6-2b3a42e00203
📒 Files selected for processing (2)
crates/perry-runtime/src/object/alloc.rstest-files/test_gap_5347_object_assign_array_source.ts
| for i in 0..n { | ||
| // js_array_get yields `undefined` for a hole; array literals and | ||
| // pushed arrays are dense, matching Node for the common case. | ||
| let value = crate::array::js_array_get(arr, i); | ||
| let key = i.to_string(); | ||
| let key_ptr = crate::string::js_string_from_bytes(key.as_ptr(), key.len() as u32); | ||
| object_assign_set_string_key( | ||
| target, | ||
| target_is_array, | ||
| key_ptr, | ||
| f64::from_bits(value.bits()), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a hole-detection mechanism available
rg -n "TAG_HOLE|is_hole" --type rust -C2Repository: PerryTS/perry
Length of output: 32713
🌐 Web query:
Does Object.assign copy holes from sparse arrays as undefined or skip them?
💡 Result:
Object.assign does NOT copy array “holes” from a sparse array as undefined. Instead, it only copies that source’s enumerable own properties; indices that are holes have no own property key, so they are skipped and remain absent in the result. [1][2] In other words: - If the source sparse array has an actual element value at index i (even if that value is undefined), Object.assign will create/overwrite key “i” on the target with that value. [1][2] - If the source has a hole at index i (no own property for “i”), Object.assign does nothing for that key; the target will not get a property for that index (so it won’t become a “filled” undefined slot). [1][2] Example behavior is discussed in practice: when assigning from a sparse array, empty slots are preserved as empty (non-existing) rather than turned into undefined values. [3] To “materialize” holes as actual undefined values, you need a different operation (e.g., some array-copying methods that preserve empties vs ones that convert them); spread/other copying mechanisms can treat holes differently. [3][4]
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global%5FObjects/Object/assign
- 2: https://tc39.es/ecma262/2022/multipage/fundamental-objects.html
- 3: https://stackoverflow.com/questions/45888724/difference-between-object-assign-and-object-spread-using-syntax
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array
Holes in sparse arrays are incorrectly materialized as undefined properties instead of being skipped.
The code calls js_array_get(arr, i) which converts holes to undefined (per issue #323 hole handling), then assigns that to the target object via object_assign_set_string_key. Per the ES spec and Node behavior, Object.assign should skip holes entirely (they are absent own properties), not create undefined properties.
For dense arrays (literals, push), this works correctly since there are no holes. For sparse arrays like Object.assign({}, [1,,3]), index 1 becomes undefined rather than remaining absent.
Add an explicit hole check before assignment:
if (value.bits() == TAG_HOLE) continue;This pattern is used elsewhere in the codebase (e.g., flat_clone.rs, field_get_set.rs) to properly handle holes.
🤖 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/object/alloc.rs` around lines 1113 - 1125, In the
loop within the array assignment section that calls
object_assign_set_string_key, add an explicit hole check to skip holes instead
of materializing them as undefined properties. Before the
object_assign_set_string_key call, insert a condition to check if the value
returned from js_array_get is a hole (using value.bits() == TAG_HOLE) and
continue to the next iteration if true, following the same pattern used in other
parts of the codebase like flat_clone.rs and field_get_set.rs.
…ger crashes
`js_object_assign_one` read an array source's indexed elements through
`(*src).keys_array`, casting the `ArrayHeader` to an `ObjectHeader`. ArrayHeader
has no `keys_array` field (it is `{length, capacity}` followed by inline element
slots), so the read picked up a NaN-boxed element as a pointer, passed the
`>= 0x10000` guard, and `js_array_length` dereferenced it — a hard SIGSEGV on
`Object.assign(target, [..])` and `{ ...[..] }` (object-spread of an array).
Detect an array source (`GC_TYPE_ARRAY`) and enumerate its dense index range
directly via `js_array_get`, then copy its enumerable string expandos from the
named-property side table. The expandos are snapshotted BEFORE the index loop:
that loop allocates, which can trigger a GC that rekeys the side table to the
moved array's new address, after which a stale-address lookup would miss them.
Indices precede expandos, matching `[[OwnPropertyKeys]]` order.
Also: a boxed-String target (`Object.assign('abc', src)` → `ToObject('abc')`)
exposes its code units as non-writable own index properties that aren't in
`keys_array`. A strict `Set` to an in-range index must throw `TypeError`; detect
it up front in `object_assign_throw_if_set_rejected`. With the array-source fix,
`Object.assign('a', [1])` now reaches this check and throws.
test262 built-ins/Object/assign: 31→32 pass (the readonly-target negative now
passes; the rest of the cluster is Proxy-source forwarding, out of scope).
New gap test `test_gap_5347_object_assign_array_source.ts` verified byte-for-byte
against Node; 8 existing spread/assign parity tests still pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proggeramlug
force-pushed
the
fix/5347-object-assign-array-source
branch
from
378a130 to
21abc74
Compare
1 participant
What
Object.assign(target, [..])and{ ...[..] }(object-spread of an array) segfaulted.js_object_assign_oneread an array source's elements through(*src).keys_array, casting theArrayHeaderto anObjectHeader— butArrayHeaderis just{length, capacity}+ inline element slots, with nokeys_arrayfield. The read picked up a NaN-boxed element as a pointer, passed the>= 0x10000guard, andjs_array_lengthdereferenced it → hard SIGSEGV on a common operation.How
GC_TYPE_ARRAYand enumerate the dense index range directly viajs_array_get, then copy enumerable string expandos (arr.foo) from the named-property side table. Expandos are snapshotted before the index loop — that loop allocates, which can trigger a GC that rekeys the side table to the moved array's new address, after which a stale-address lookup would miss them. Indices precede expandos, matching[[OwnPropertyKeys]]order.Object.assign('abc', src)doesToObject('abc'), whose code units are non-writable own index properties not stored inkeys_array. A strictSetto an in-range index must throwTypeError; detected up front inobject_assign_throw_if_set_rejected. With the array-source fix,Object.assign('a', [1])now reaches this check and throws.The non-array-source path is untouched (
elsebranch), so plain object/class/string sources behave exactly as before.Testing
built-ins/Object/assign: 31 → 32 pass. The readonly-target negative (assignment-to-readonly-property-of-target-must-throw-a-typeerror-exception) now passes; the remaining 5 are Proxy-source trap forwarding (out of scope).test_gap_5347_object_assign_array_source.ts— array source (indices + expando), array target growth, boxed-String readonly negatives, normal merges — verified byte-for-byte againstnode --experimental-strip-types.main).Chips the Object cluster of the #5347 umbrella and fixes a real crash.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Object.assignuses an array as the sourceObject.assign