fix(array,hir): a replaced Array.prototype[Symbol.iterator] is honoured everywhere, and the method is a real own property (#7760) - #7761
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe runtime now installs ChangesArray iterator patch handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ArrayPrototype
participant RuntimePatchTracker
participant ForOfLowering
participant IteratorProtocol
ArrayPrototype->>RuntimePatchTracker: record iterator replacement
RuntimePatchTracker->>ForOfLowering: publish patch flag
ForOfLowering->>ForOfLowering: read flag at loop entry
ForOfLowering->>IteratorProtocol: use lazy iterator path when patched
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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: 5
🤖 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 `@changelog.d/7759-array-proto-iterator-spread.md`:
- Around line 13-15: Update the “No gap test, deliberately” note to reflect that
test-files/test_gap_array_proto_iterator_replaced_7542.ts now covers the patched
Array.prototype iterator cases and passes through the harness, as documented by
changelog.d/7761-array-proto-iterator-own-property.md. Remove the stale claim
that no such test exists unless it specifically refers to an unrelated,
separately tracked case.
In `@crates/perry-runtime/src/array/iterator.rs`:
- Around line 820-825: Update the fast path around js_array_is_array and
array_has_own_iterator so Proxy-wrapped arrays with a custom Symbol.iterator
from a get trap use ordinary proxy-aware GetIterator behavior. Do not route
these values through the direct Array.prototype lookup in js_get_iterator;
either exclude Proxy arrays from this optimization or perform the iterator
property read through the proxy.
In `@crates/perry-runtime/src/object/global_this/array_error.rs`:
- Around line 655-660: Update array_prototype_values_thunk to retain the
existing native-array fast path while routing non-array object receivers to an
array-like iterator implementation. Ensure generic calls such as
Array.prototype.values.call(...) and the Symbol.iterator alias return an
iterator rather than undefined.
In `@test-files/test_gap_array_proto_iterator_replaced_7542.ts`:
- Around line 47-54: After Object.defineProperty(arrProto, Symbol.iterator,
desc) in the iterator restoration test, read the property descriptor again and
assert that its value, writable, enumerable, and configurable fields all match
desc, in addition to the existing spread check.
- Around line 14-22: Update the iterator verification around original so it
calls arrProto.values.call(...) directly, confirming the Array.prototype.values
behavior with the expected array output. Also assert that
arrProto[Symbol.iterator] and arrProto.values reference the same shared
function, rather than relying only on the iterator property.
🪄 Autofix
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: 8b77ca02-6b35-4df2-bd1d-10e80bcef1e5
📒 Files selected for processing (10)
changelog.d/7759-array-proto-iterator-spread.mdchangelog.d/7761-array-proto-iterator-own-property.mdcrates/perry-runtime/src/array/from_concat.rscrates/perry-runtime/src/array/iterator.rscrates/perry-runtime/src/array/mod.rscrates/perry-runtime/src/array/push_pop.rscrates/perry-runtime/src/object/arguments.rscrates/perry-runtime/src/object/global_this/array_error.rscrates/perry-runtime/src/object/global_this/proto_methods.rstest-files/test_gap_array_proto_iterator_replaced_7542.ts
| pub(crate) extern "C" fn array_prototype_values_thunk( | ||
| _c: *const crate::closure::ClosureHeader, | ||
| _a: f64, | ||
| ) -> f64 { | ||
| let this = crate::object::js_implicit_this_get(); | ||
| crate::array::array_values_iter(this) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make Array.prototype.values generic.
array_values_iter returns undefined when this is not a native ArrayHeader. Therefore, Array.prototype.values.call({ 0: "x", length: 1 }) and Array.prototype[Symbol.iterator].call(...) return undefined instead of an iterator.
Add an array-like iterator path for non-array object receivers. Keep the native-array fast 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/object/global_this/array_error.rs` around lines 655
- 660, Update array_prototype_values_thunk to retain the existing native-array
fast path while routing non-array object receivers to an array-like iterator
implementation. Ensure generic calls such as Array.prototype.values.call(...)
and the Symbol.iterator alias return an iterator rather than undefined.
| const desc: any = Object.getOwnPropertyDescriptor(arrProto, Symbol.iterator); | ||
| console.log("descriptor:", typeof desc.value, desc.writable, desc.enumerable, desc.configurable); | ||
| console.log("hasOwn:", Object.prototype.hasOwnProperty.call(arrProto, Symbol.iterator)); | ||
| console.log("in ownSymbols:", Object.getOwnPropertySymbols(arrProto).indexOf(Symbol.iterator) >= 0); | ||
| console.log("name:", desc.value.name); | ||
|
|
||
| const original = arrProto[Symbol.iterator]; | ||
| // #7760: the value reads `this` at CALL time, so a borrowed reference works. | ||
| console.log("values.call:", JSON.stringify(Array.from(original.call([7, 8]) as any))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Call Array.prototype.values directly.
original comes from arrProto[Symbol.iterator], so this test does not verify Array.prototype.values. A no-op values implementation would still pass. Call arrProto.values.call(...) and verify that both properties reference the shared function.
Suggested coverage
const original = arrProto[Symbol.iterator];
+console.log("values alias:", arrProto.values === original);
-console.log("values.call:", JSON.stringify(Array.from(original.call([7, 8]) as any)));
+console.log("values.call:", JSON.stringify(Array.from(arrProto.values.call([7, 8]) as any)));
+console.log("iterator.call:", JSON.stringify(Array.from(original.call([7, 8]) as any)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const desc: any = Object.getOwnPropertyDescriptor(arrProto, Symbol.iterator); | |
| console.log("descriptor:", typeof desc.value, desc.writable, desc.enumerable, desc.configurable); | |
| console.log("hasOwn:", Object.prototype.hasOwnProperty.call(arrProto, Symbol.iterator)); | |
| console.log("in ownSymbols:", Object.getOwnPropertySymbols(arrProto).indexOf(Symbol.iterator) >= 0); | |
| console.log("name:", desc.value.name); | |
| const original = arrProto[Symbol.iterator]; | |
| // #7760: the value reads `this` at CALL time, so a borrowed reference works. | |
| console.log("values.call:", JSON.stringify(Array.from(original.call([7, 8]) as any))); | |
| const desc: any = Object.getOwnPropertyDescriptor(arrProto, Symbol.iterator); | |
| console.log("descriptor:", typeof desc.value, desc.writable, desc.enumerable, desc.configurable); | |
| console.log("hasOwn:", Object.prototype.hasOwnProperty.call(arrProto, Symbol.iterator)); | |
| console.log("in ownSymbols:", Object.getOwnPropertySymbols(arrProto).indexOf(Symbol.iterator) >= 0); | |
| console.log("name:", desc.value.name); | |
| const original = arrProto[Symbol.iterator]; | |
| console.log("values alias:", arrProto.values === original); | |
| // `#7760`: the value reads `this` at CALL time, so a borrowed reference works. | |
| console.log("values.call:", JSON.stringify(Array.from(arrProto.values.call([7, 8]) as any))); | |
| console.log("iterator.call:", JSON.stringify(Array.from(original.call([7, 8]) as any))); |
🤖 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 `@test-files/test_gap_array_proto_iterator_replaced_7542.ts` around lines 14 -
22, Update the iterator verification around original so it calls
arrProto.values.call(...) directly, confirming the Array.prototype.values
behavior with the expected array output. Also assert that
arrProto[Symbol.iterator] and arrProto.values reference the same shared
function, rather than relying only on the iterator property.
| // #7760: restore by reference, and by descriptor round-trip. | ||
| arrProto[Symbol.iterator] = original; | ||
| console.log("restored spread:", JSON.stringify([...src])); | ||
| console.log("restored Array.from:", JSON.stringify(Array.from(src as any))); | ||
| console.log("restored call spread:", count(...src)); | ||
|
|
||
| Object.defineProperty(arrProto, Symbol.iterator, desc); | ||
| console.log("after defineProperty:", JSON.stringify([...src])); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check descriptor fields after the round-trip.
After Object.defineProperty, the test checks only spread behavior. A regression that restores the method but changes writable, enumerable, or configurable would pass. Read the descriptor again and compare its value and all three flags with desc.
Suggested assertion
Object.defineProperty(arrProto, Symbol.iterator, desc);
+const roundTripDesc: any = Object.getOwnPropertyDescriptor(arrProto, Symbol.iterator);
+console.log(
+ "round-trip descriptor:",
+ roundTripDesc.value === desc.value,
+ roundTripDesc.writable === desc.writable,
+ roundTripDesc.enumerable === desc.enumerable,
+ roundTripDesc.configurable === desc.configurable,
+);
console.log("after defineProperty:", JSON.stringify([...src]));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // #7760: restore by reference, and by descriptor round-trip. | |
| arrProto[Symbol.iterator] = original; | |
| console.log("restored spread:", JSON.stringify([...src])); | |
| console.log("restored Array.from:", JSON.stringify(Array.from(src as any))); | |
| console.log("restored call spread:", count(...src)); | |
| Object.defineProperty(arrProto, Symbol.iterator, desc); | |
| console.log("after defineProperty:", JSON.stringify([...src])); | |
| // `#7760`: restore by reference, and by descriptor round-trip. | |
| arrProto[Symbol.iterator] = original; | |
| console.log("restored spread:", JSON.stringify([...src])); | |
| console.log("restored Array.from:", JSON.stringify(Array.from(src as any))); | |
| console.log("restored call spread:", count(...src)); | |
| Object.defineProperty(arrProto, Symbol.iterator, desc); | |
| const roundTripDesc: any = Object.getOwnPropertyDescriptor(arrProto, Symbol.iterator); | |
| console.log( | |
| "round-trip descriptor:", | |
| roundTripDesc.value === desc.value, | |
| roundTripDesc.writable === desc.writable, | |
| roundTripDesc.enumerable === desc.enumerable, | |
| roundTripDesc.configurable === desc.configurable, | |
| ); | |
| console.log("after defineProperty:", JSON.stringify([...src])); |
🤖 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 `@test-files/test_gap_array_proto_iterator_replaced_7542.ts` around lines 47 -
54, After Object.defineProperty(arrProto, Symbol.iterator, desc) in the iterator
restoration test, read the property descriptor again and assert that its value,
writable, enumerable, and configurable fields all match desc, in addition to the
existing spread check.
…nd values() iterates (#7760)
2f2ca31 to
78f5c0f
Compare
…bol.iterator] (#7760 item 1)
Merging as v0.5.1446A/B'd against a post-#7759 build, so this isolates this PR: The best thing here is that it makes the previous PR testable#7759 shipped without a gap test, and the reason was this bug: a test that patches I verified that end-to-end rather than taking it: One root cause, and the cross-builtin table proves it
Both symptoms then fall out of the one cause rather than needing separate fixes — item 3 because there is no own property to describe, and item 2 because a closure bound to the prototype at read time iterates the prototype when called with Reading ScopeCorrectly not closing #7760 — item 1 stays open, and saying so is right. Same discipline as #7759's three filed follow-ups.
|
proggeramlug
changed the title
1 participant
Fixes #7760 — all three items. (Item 1 was added after the first review pass; the original scope note is gone.) #7759 has merged, so this now applies to
maindirectly.Items 2 and 3: one root cause
Array.prototype[Symbol.iterator]was not an own property at all —js_object_get_symbol_propertysynthesized a receiver-bound method on every read — andArray.prototype.valueswas installed byinstall_noop_proto_methods, so it existed and did nothing. Array-specific; Map/Set/String/%TypedArray%already had real descriptors.That explains both symptoms. No own property ⇒ no descriptor,
hasOwnPropertyfalse, absent fromgetOwnPropertySymbols(item 3). And the synthesized closure is bound to the prototype at read time, so storing it back and calling it withthis === arrthrowsnext is not a function(item 2).One real
array_prototype_values_thunkreadingthisat CALL time, installed asvaluesand as the own[Symbol.iterator]with the spec descriptor{ writable: true, enumerable: false, configurable: true }.Item 1:
for…ofover an arrayfor…ofdesugars to an index loop that never consults the protocol. Two parallel lowerings had to be fixed —lower::stmt_loops(module init) andlower_decl::body_stmt(function bodies) — which is why afor…ofover an array parameter was still wrong after the first half worked, and is worth knowing about for anything else in this area.The patch is a RUNTIME fact and the index-vs-lazy choice is COMPILE-TIME, so both forms are emitted and selected by a branch on a new
Expr::ArrayIterationPatched, lowering to a single volatilei8load ofPERRY_ARRAY_PROTO_ITERATOR_PATCHED— the shapePERRY_ARRAY_INDEX_FAST_PATH_INVALIDATEDalready uses. Three properties shaped it:breakwould over-pull, a side-effecting iterator would run extra steps, an infinite one would hang. The test pinsbreakafter 2 of 3 pulling exactly 2, matching node.else. The smaller alternative (one loop, loop-invariant branch, element through a shared__item) would have broken the HIR pattern matchers behind the element-shape clone (perf(repsel): element-shape versioned loop clone — the first consumer of the element-shape invariant (#7480 / #5093) #7612), the dense spread path (perf: object_deep_clone is 37.5x bun (657ms vs 17.5ms) — the worst row in the public artifact, newly visible now that it runs #7533), and the packed-f64 / i32-counter loop specializations.for…ofperforms GetIterator exactly once, so a patch landing mid-loop must not change the iterator already in hand.Cost
Interleaved, 7 reps each, 20k-iteration loop fixture (three
for…ofloops overnumber[]/ object / string arrays), on a host at load average 12:No measurable difference. Recording a near-miss because it is the more useful part: a first non-interleaved best-of-5 on the same box read 2.64 s vs 2.90 s and I nearly reported a 10% regression from it. On a machine at this load only interleaved runs mean anything, and the number still wants confirming on the pinned bench host before anyone quotes it as a bound.
Validation
test-files/test_gap_array_proto_iterator_replaced_7542.ts— PASS through the harness, byte-identical to node 26.5.1. Covers the descriptor shape,values.call, all four spread forms from spread: a replacedArray.prototype[Symbol.iterator]is ignored by[...arr]#7542, own-@@iteratorprecedence,for…ofover a module const / typed local / array parameter, the lazy-breakpull count, restore-by-reference, and agetOwnPropertyDescriptor→definePropertyround-trip.cargo test -p perry-runtime --lib: 1991 passed.cargo test -p perry-hir --lib: 290 passed.cargo fmt --all --checkclean;scripts/check_file_size.shclean — the guard wrapper moved tolower/for_of_guard.rsbecausestmt_loops.rscrossed the 2000-line cap at 2025.Note on the test
This test could not exist before this PR. Patching
Array.prototype[Symbol.iterator]takes the oracle down — node's primordials build aSafeMapfrom an iterable and get the patched value — so the test must restore the slot, and restoring is exactly what item 2 broke.No version bump (the branch carries the maintainer's 0.5.1446 bump from the earlier push).
Summary by CodeRabbit
Bug Fixes
Array.prototype[Symbol.iterator]and.valuesto behave like standard configurable, writable, non-enumerable properties.Array.from, andfor…ofbehavior when iterators are replaced.Tests
Chores