fix(runtime): typed-array integer-indexed [[Set]] spec rejection + coercion#6059
Merged
Conversation
…jection + coercion rules
A typed array is an Integer-Indexed exotic object; its [[Set]] (ECMA-262
§10.4.5.5) and the underlying TypedArraySetElement (§10.4.5.16) have two
observable behaviours Perry was swallowing:
1. Ordinary (non-index) property [[Set]] must report the OrdinarySet
boolean. A non-writable own data expando, a setter-less accessor, or a
new key on a non-extensible view must be REJECTED (return false → a
strict `ta.k = v` throws, `Reflect.set(ta, k, v)` returns false).
Previously every path returned `true`, so the rejection was silent.
`typed_array_set_property_by_name` now returns the correct boolean, and
`own_set_descriptor` consults the typed-array side tables directly
(they are invisible to the generic address-keyed descriptor lookups,
which are deliberately gated off for typed arrays) so the
receiver-threading OrdinarySet reports the right result.
2. TypedArraySetElement runs ToNumber / ToBigInt on the value BEFORE the
IsValidIntegerIndex bounds check, then drops the store for an invalid
index. Perry short-circuited on an out-of-bounds index and never
coerced, so `ta[100] = { valueOf() { … } }` never fired the hook and a
throwing coercion never threw. The invalid-index paths
(`js_typed_array_set`, `typed_array_set_numeric_index`, the
IntegerIndex arm) now coerce for side effects first. The hot
in-bounds / plain-number path is untouched — the out-of-bounds
coercion is gated on a non-plain-number value.
Verified on an internal Linux sweep host against test262
built-ins/TypedArray + TypedArrayConstructors (2184 cases): +7 pass,
0 regressions. built-ins/Reflect + Proxy + Object/{defineProperty,freeze,
preventExtensions} (1688 cases): byte-identical, 0 regressions. Newly
passing: Set/key-is-not-numeric-index, key-is-not-canonical-index,
tonumber-value-throws, key-is-out-of-bounds-receiver-is-proto (+ BigInt
variants).
📝 WalkthroughWalkthroughModifies TypedArray ChangesTypedArray Set Semantics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant js_typed_array_set
participant CoercionCheck as value_needs_coercion_side_effect
participant BigIntCoerce as bigint::to_bigint_for_store
participant NumberCoerce as jsvalue_to_f64
Caller->>js_typed_array_set: set(index, value)
js_typed_array_set->>js_typed_array_set: validate index bounds
alt index out of bounds
js_typed_array_set->>CoercionCheck: check value tag
CoercionCheck-->>js_typed_array_set: needs_coercion?
alt needs coercion, BigInt view
js_typed_array_set->>BigIntCoerce: coerce value
else needs coercion, Number view
js_typed_array_set->>NumberCoerce: coerce value
end
js_typed_array_set-->>Caller: return without store
else index in bounds
js_typed_array_set-->>Caller: perform store
end
sequenceDiagram
participant Proxy as own_set_descriptor
participant Probe as typed_array_own_set_descriptor
participant SideTables
Proxy->>Proxy: target is typed array?
alt is typed array
Proxy->>Probe: query(owner, name)
Probe->>SideTables: read accessor/data state
SideTables-->>Probe: descriptor state
Probe-->>Proxy: TypedArrayOwnSetDescriptor or None
Proxy-->>Proxy: map to OwnSetDescriptor
else not typed array
Proxy-->>Proxy: fall back to generic descriptor lookup
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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/typedarray_props.rs`:
- Around line 572-576: The numeric-index guard in typed_array_set_numeric_index
is missing the canonical-invalid -0 case, so -0.0 currently falls through and
writes to index 0. Update the validation in typed_array_set_numeric_index to
treat negative zero as invalid alongside the existing finite/integer/range
checks, so the path used by js_typed_array_index_set_dynamic and
js_object_set_index_polymorphic still coerces via
typed_array_coerce_element_for_side_effects and then drops the write.
In `@crates/perry-runtime/src/typedarray/access.rs`:
- Around line 137-156: The out-of-bounds branch in typedarray/access.rs within
the typed array set path should not skip coercion for BigInt views when the
value is a plain string or other primitive that still requires ToBigInt. Update
js_typed_array_set so the invalid-index handling always runs
bigint::to_bigint_for_store for KIND_BIGINT64/KIND_BIGUINT64 before returning,
while keeping jsvalue_to_f64 for numeric views; use
value_needs_coercion_side_effect only for the non-BigInt fast-path if needed.
🪄 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: ac23cddf-6d06-4096-a76d-5361daa23149
📒 Files selected for processing (4)
crates/perry-runtime/src/proxy.rscrates/perry-runtime/src/typedarray/access.rscrates/perry-runtime/src/typedarray/bigint.rscrates/perry-runtime/src/typedarray_props.rs
Comment on lines
572
to
576
| if !index.is_finite() || index.fract() != 0.0 || index < 0.0 || index > u32::MAX as f64 { | ||
| // Canonical-invalid index: coerce the value for side effects, then drop. | ||
| typed_array_coerce_element_for_side_effects(owner, value); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=rust -C4 '\btyped_array_set_numeric_index\s*\(' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 2545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,260p' crates/perry-runtime/src/object/polymorphic_index.rs
printf '\n----\n'
sed -n '650,730p' crates/perry-runtime/src/typedarray_props.rs
printf '\n----\n'
rg -n "numeric_key_u32_index|IsValidIntegerIndex|-0|negative zero|canonical-invalid" crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 28775
Reject -0 on the numeric-index path typed_array_set_numeric_index receives raw f64 values from both js_typed_array_index_set_dynamic and js_object_set_index_polymorphic, so -0.0 reaches this guard unchanged and currently stores at index 0. IsValidIntegerIndex treats -0 as canonical-invalid, so this path should coerce for side effects and drop the write.
🤖 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/typedarray_props.rs` around lines 572 - 576, The
numeric-index guard in typed_array_set_numeric_index is missing the
canonical-invalid -0 case, so -0.0 currently falls through and writes to index
0. Update the validation in typed_array_set_numeric_index to treat negative zero
as invalid alongside the existing finite/integer/range checks, so the path used
by js_typed_array_index_set_dynamic and js_object_set_index_polymorphic still
coerces via typed_array_coerce_element_for_side_effects and then drops the
write.
Comment on lines
+137
to
156
| let kind = (*ta).kind; | ||
| if index < 0 || index as u32 >= (*ta).length { | ||
| // TypedArraySetElement (§10.4.5.16) runs `ToNumber`/`ToBigInt` on | ||
| // the value BEFORE the IsValidIntegerIndex bounds check, then drops | ||
| // the store for an invalid index. The coercion is only observable | ||
| // for a value whose conversion has a side effect or throws — an | ||
| // object (`valueOf`/`Symbol.toPrimitive` hook), a Symbol / BigInt | ||
| // into a numeric view, or a Number into a BigInt view. Gate the | ||
| // out-of-bounds coercion on a non-plain-number value so the hot | ||
| // in-bounds and primitive-value paths are untouched (test262 | ||
| // Set/tonumber-value-throws, key-is-out-of-bounds-*). | ||
| if value_needs_coercion_side_effect(value) { | ||
| if kind == KIND_BIGINT64 || kind == KIND_BIGUINT64 { | ||
| let _ = bigint::to_bigint_for_store(value); | ||
| } else { | ||
| let _ = jsvalue_to_f64(value); | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== access.rs outline ==\n'
ast-grep outline crates/perry-runtime/src/typedarray/access.rs --view expanded || true
printf '\n== typedarray_props.rs outline ==\n'
ast-grep outline crates/perry-runtime/src/typedarray/typedarray_props.rs --view expanded || true
printf '\n== Relevant search terms ==\n'
rg -n "value_needs_coercion_side_effect|typed_array_coerce_element_for_side_effects|KIND_BIGINT64|KIND_BIGUINT64|key-is-out-of-bounds|ToBigInt|ToNumber" crates/perry-runtime/src crates -SRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,180p' crates/perry-runtime/src/typedarray/access.rs
printf '\n---\n'
sed -n '1,220p' crates/perry-runtime/src/typedarray/typedarray_props.rs
printf '\n---\n'
rg -n "key-is-out-of-bounds|Set/tonumber-value-throws|tonumber-value-throws|BigInt64|BigUint64" . -SRepository: PerryTS/perry
Length of output: 4535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,180p' crates/perry-runtime/src/typedarray/access.rs
printf '\n---\n'
sed -n '1,260p' crates/perry-runtime/src/typedarray/typedarray_props.rs
printf '\n---\n'
rg -n "key-is-out-of-bounds|Set/tonumber-value-throws|tonumber-value-throws|BigInt64|BigUint64" . -SRepository: PerryTS/perry
Length of output: 4535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== crates/perry-runtime/src/typedarray/access.rs ==='
nl -ba crates/perry-runtime/src/typedarray/access.rs | sed -n '90,180p'
echo
echo '=== crates/perry-runtime/src/typedarray/typedarray_props.rs ==='
nl -ba crates/perry-runtime/src/typedarray/typedarray_props.rs | sed -n '1,260p'
echo
echo '=== test references ==='
rg -n "key-is-out-of-bounds|Set/tonumber-value-throws|tonumber-value-throws|BigInt64|BigUint64|TypedArrayConstructors/.*/Set" . -SRepository: PerryTS/perry
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== typedarray_props.rs relevant slice ===\n'
sed -n '480,590p' crates/perry-runtime/src/typedarray_props.rs
printf '\n=== set-related references ===\n'
rg -n "key-is-out-of-bounds|Set/tonumber-value-throws|typed_array_coerce_element_for_side_effects|coerce_element_for_side_effects|ToBigInt" crates/perry-runtime/src/typedarray_props.rs crates/perry-runtime/src/typedarray/access.rs -S
printf '\n=== test262 or test references ===\n'
rg -n "key-is-out-of-bounds|tonumber-value-throws|BigInt64Array|BigUint64Array" . -g '!target' -g '!dist' -g '!build' -SRepository: PerryTS/perry
Length of output: 31271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== crates/perry-runtime/src/typedarray/access.rs (100-170) ==='
sed -n '100,170p' crates/perry-runtime/src/typedarray/access.rs
echo
echo '=== crates/perry-runtime/src/typedarray/bigint.rs (1-180) ==='
sed -n '1,180p' crates/perry-runtime/src/typedarray/bigint.rs
echo
echo '=== crates/perry-runtime/src/typedarray_props.rs (540-570) ==='
sed -n '540,570p' crates/perry-runtime/src/typedarray_props.rsRepository: PerryTS/perry
Length of output: 11763
Coerce out-of-bounds writes on BigInt views before returning value_needs_coercion_side_effect skips plain strings, so js_typed_array_set can turn ta[100] = 5 or ta[100] = "bad" on a BigInt64Array/BigUint64Array into a silent no-op instead of running ToBigInt and propagating the expected TypeError/SyntaxError. The invalid-index path should coerce unconditionally for BigInt views, matching the property-set path.
🤖 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/typedarray/access.rs` around lines 137 - 156, The
out-of-bounds branch in typedarray/access.rs within the typed array set path
should not skip coercion for BigInt views when the value is a plain string or
other primitive that still requires ToBigInt. Update js_typed_array_set so the
invalid-index handling always runs bigint::to_bigint_for_store for
KIND_BIGINT64/KIND_BIGUINT64 before returning, while keeping jsvalue_to_f64 for
numeric views; use value_needs_coercion_side_effect only for the non-BigInt
fast-path if needed.
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.
Summary
A typed array is an Integer-Indexed exotic object. Its
[[Set]](ECMA-262 §10.4.5.5) and the underlyingTypedArraySetElement(§10.4.5.16) have two observable behaviours Perry was swallowing:1. Ordinary (non-index) property
[[Set]]must report the OrdinarySet boolean.A non-writable own data expando, a setter-less accessor, or a new key on a non-extensible view must be rejected — a strict
ta.k = vshould throw andReflect.set(ta, k, v)should returnfalse. Previously every typed-array set path returnedtrue, so the rejection was silent.typed_array_set_property_by_namenow returns the correct spec boolean, andown_set_descriptorconsults the typed-array side tables directly (they're invisible to the generic address-keyed descriptor lookups, which are deliberately gated off for typed arrays) so the receiver-threading OrdinarySet reports the right result.2.
TypedArraySetElementcoerces the value BEFORE the bounds check.ToNumber/ToBigIntruns on the value before theIsValidIntegerIndexcheck, then the store is dropped for an invalid index. Perry short-circuited on an out-of-bounds index and never coerced, sota[100] = { valueOf() { … } }never fired the hook and a throwing coercion never threw. The invalid-index paths (js_typed_array_set,typed_array_set_numeric_index, and theIntegerIndexarm) now run the per-content-type coercion for side effects first. The hot in-bounds / plain-number path is untouched — the out-of-bounds coercion is gated on a non-plain-number value.Validation
Built and tested on an internal Linux sweep host (
--release).test262
built-ins/TypedArray+TypedArrayConstructors(2184 cases): +7 pass, 0 regressions (baseline vs. patched failure-set diff was removal-only).test262
built-ins/Reflect+Proxy+Object/{defineProperty,freeze,preventExtensions}(1688 cases): byte-identical pass/fail between baseline and patched — 0 regressions (as expected; the newown_set_descriptorbranch only activates for a typed-array target).Newly passing:
Set/key-is-not-numeric-index.jsSet/key-is-not-canonical-index.jsSet/tonumber-value-throws.jsSet/key-is-out-of-bounds-receiver-is-proto.jscargo fmt --all -- --checkclean; address-classification audit passes; all four touched files stay under 2000 lines;perry-runtimetypedarray unit tests pass.Scope notes
The remaining receiver-threading cases in this internals slice were deliberately left out of this PR:
key-is-symbol.jsdepends on a separate, non-typed-array-specific bug (a non-writable symbol data property whose value isundefinedisn't detected on plain objects either),resized-out-of-bounds-to-in-bounds-index.jsneeds resizable ArrayBuffers, and theObject.create(ta)+ Proxy-receiver prototype-chain cases touch several subsystems (one currently SIGSEGVs) — all bigger, riskier mechanisms best handled separately.Summary by CodeRabbit