fix(array): a replaced Array.prototype[Symbol.iterator] drives spread, call-spread and Array.from (#7542)

[proggeramlug]

Fixes #7542.

Two corrections to the diagnosis, both of which changed the fix

1. note_array_proto_iterator_write does fire (the issue's open check #1). Instrumented: 49 calls during a run, exactly one matches and sets the flag.

[7542] note_write obj=0x34814a552b8 proto=0x34814a552b8 sym=0x34814b40180 wk=0x34814b40180 already=false
[7542] FLAG SET

So the flag was never the problem, and neither js_get_iterator's path nor this one is dead.

2. The walk does NOT miss for a plain array. The issue says the generic lookup "reads own symbol props only … so for a plain array it misses and falls to the js_array_is_array arm". It doesn't: js_object_get_symbol_property synthesizes the built-in Symbol.iterator for an array receiver (a js_class_method_bind by name) rather than reading the prototype slot — so the walk resolves the builtin, calls it, and returns the element copy.

I know this because I first implemented the fix exactly where the issue points, at the js_array_is_array arm, and it changed nothing. Instrumenting that arm showed it is never reached with the flag set. A guard there is dead code.

The fix

The guard goes before the walk, and at each entry point that reaches an array by a different route — this is why a single-site fix was never going to work:

entry point	reached by	was doing
array_from_spread_value	[...arr]	walk resolves the builtin
js_array_from_value	Array.from(arr)	array arm ends in a raw js_array_clone
js_array_push_spread_f64	[0, ...arr, 9]	direct element copy
js_array_like_to_array	f(...arr)	returns the raw pointer ("fast path, no protocol overhead")

Each delegates to js_get_iterator — the one implementation that consults the patched prototype per GetIterator (read the method off Array.prototype, call it with this === val, TypeError when deleted or non-callable). Delegating rather than restating that sequence four times is the point: two copies of a spec sequence that must agree is how this diverged in the first place.

An own arr[Symbol.iterator] still wins — the shortcut stands aside when the receiver carries its own method, because js_get_iterator's patched branch reads the prototype only and would otherwise throw "not iterable" for an array with an own method once the prototype slot is deleted. (That case caught me: my first cut broke it.)

array_proto_iterator_modified() is sticky-false until user code writes the prototype slot, and #7533's dense fast path already declines when it is set, so ordinary programs are untouched.

Verification

                       node --experimental-strip-types    perry
spread literal:        ["patched"]                        ["patched"]
spread variable:       ["patched"]                        ["patched"]
spread into array:     [0,"patched",9]                    [0,"patched",9]
call spread:           1                                  1
Array.from:            ["patched"]                        ["patched"]
own wins:              ["own"]                            ["own"]

Byte-identical. Before this change the four spread rows were [1,2,3] / [0,1,2,3,9] / 3 / [1,2,3].

• cargo test -p perry-runtime --lib: 1991 passed, 0 failed.
• Targeted parity over the array / iterator / spread gap tests: all pass.
• cargo fmt --all --check, scripts/check_file_size.sh: clean.

No gap test, deliberately

Any test that patches Array.prototype[Symbol.iterator] takes the oracle down under the parity harness: node builds a SafeMap from an iterable, gets the patched value, and exits 1 with

TypeError: Iterator value patched is not an entry object
    at new Map (<anonymous>)
    node:internal/per_context/primordials:449

so the comparison runs against a crashed reference. (Standalone node file.ts is fine, which is why this only showed up once I ran it through the harness rather than by hand.) I tried four ways around it — restore by saved reference, restore via getOwnPropertyDescriptor, a behavioural replacement, a self-iterable replacement, and process.exit(0) — and none works, for reasons that are themselves bugs (below). So the fix is verified by direct comparison instead, and I would rather say that plainly than land a test that passes for the wrong reason.

Filed separately (found here, out of scope)

1. for…of over a typed local ignores the patched method. It is a codegen dense index loop with no runtime call at all (js_array_values_iter_obj / js_array_length appear only in the declare block) — a different mechanism from the four above, needing its own guard.
2. Restoring the original iterator is broken. arrProto[Symbol.iterator] = original then [...arr] throws next is not a function: reading Array.prototype[Symbol.iterator] yields a method bound to the prototype. Pre-existing and independent of this change — it reproduces on for…of over an array literal, which routes through js_get_iterator and is untouched here.
3. Object.getOwnPropertyDescriptor(Array.prototype, Symbol.iterator) returns undefined.

No version bump (maintainer bumps at merge).

Summary by CodeRabbit

• Bug Fixes
  • Array spread operations now respect customized Array.prototype[Symbol.iterator] behavior.
  • Updated behavior applies to Array.from, array spreading, spreading into function calls, array appending, and related conversions.
  • Arrays with their own iterator continue to use that iterator appropriately.
  • Existing fast paths remain available when the standard iterator is unchanged.
  • Improved consistency when custom iterators alter the values or order produced by array operations.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: db0d1129-2557-47bc-a268-a202ed328612

📥 Commits

Reviewing files that changed from the base of the PR and between 4e0c716 and b3568c1.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (4)

• CLAUDE.md
• Cargo.toml
• crates/perry-runtime/src/array/iterator.rs
• crates/perry-runtime/src/object/arguments.rs

---

📝 Walkthrough

Walkthrough

Array spread now honors modified Array.prototype[Symbol.iterator] methods. Array.from, spread-append, and array-to-array conversion paths use iterator-aware materialization when required. Own iterators and unmodified-array fast paths remain supported.

Changes

Patched Array Iterator Semantics

Layer / File(s)	Summary
Iterator protocol dispatch crates/perry-runtime/src/array/iterator.rs, crates/perry-runtime/src/array/mod.rs	Array spread checks for own Symbol.iterator properties and delegates modified prototype iterators to js_get_iterator. The spread clone function is re-exported.
Array conversion paths crates/perry-runtime/src/array/from_concat.rs, crates/perry-runtime/src/object/arguments.rs, crates/perry-runtime/src/array/push_pop.rs, changelog.d/7759-array-proto-iterator-spread.md, CLAUDE.md, Cargo.toml	Array conversion and spread-append paths use iterator-aware materialization when the prototype iterator changes. Direct fast paths remain for unmodified iterators. The changelog records the affected paths and remaining limitations. The project version changes to 0.5.1445.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ArrayOperation
  participant ArrayIteratorRuntime
  participant ArrayPrototype
  ArrayOperation->>ArrayIteratorRuntime: Detect modified prototype iterator
  ArrayIteratorRuntime->>ArrayPrototype: Read patched Symbol.iterator
  ArrayPrototype-->>ArrayIteratorRuntime: Return iterator-produced values
  ArrayIteratorRuntime-->>ArrayOperation: Materialize spread result

Loading

Possibly related issues

• PerryTS/perry#7760 — Covers remaining defects in patched iterator handling, including typed-local for…of, which this change explicitly leaves unresolved.

Possibly related PRs

• PerryTS/perry#7540 — Added the dense-array spread fast-path logic that this change now guards for patched iterator semantics.
• PerryTS/perry#7527 — Also modifies array_from_spread_value for array iterator handling, but addresses a different issue.
• PerryTS/perry#7761 — Overlaps with these spread and Array.from iterator-protocol changes in the same runtime paths.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main fix for replaced Array.prototype[Symbol.iterator] behavior across spread and Array.from.
Description check	✅ Passed	The description explains the fix, related issue, implementation, verification, limitations, and scope, although it omits some template headings.
Linked Issues check	✅ Passed	The changes satisfy #7542 by honoring a replaced prototype iterator for array spread while preserving own-iterator precedence and fast paths.
Out of Scope Changes check	✅ Passed	No unrelated implementation changes are evident; the additional entry points use the same iterator fix and match the stated PR objective.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/7542-array-proto-iterator

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@crates/perry-runtime/src/array/iterator.rs`:
- Around line 820-824: Update the shortcut in the iterator conversion path
around array_proto_iterator_modified and js_array_is_array so Proxy receivers do
not enter the prototype-only js_get_iterator path; route them through generic
iterator lookup or an implementation that performs Proxy [[Get]]. Add coverage
for spread, Array.from, and function-spread calls using a Proxy with both a get
trap and a target-owned iterator.

🪄 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: ca49f96c-6a1d-4d1d-8df1-c3986d25c9f8

📥 Commits

Reviewing files that changed from the base of the PR and between 411a96e and 4e0c716.

📒 Files selected for processing (6)

• changelog.d/7759-array-proto-iterator-spread.md
• crates/perry-runtime/src/array/from_concat.rs
• crates/perry-runtime/src/array/iterator.rs
• crates/perry-runtime/src/array/mod.rs
• crates/perry-runtime/src/array/push_pop.rs
• crates/perry-runtime/src/object/arguments.rs

[proggeramlug]

Merging as v0.5.1445

A/B'd on my own host — new matches node on all six rows, old is wrong on four:

                   node          old            new
spread literal     ["patched"]   [1,2,3]        ["patched"]
spread variable    ["patched"]   [1,2,3]        ["patched"]
spread into array  [0,"p",9]     [0,1,2,3,9]    [0,"patched",9]
call spread        1             3              1
Array.from         ["patched"]   [1,2,3]        ["patched"]
own wins           ["own"]       ["own"]        ["own"]

The two corrections are worth more than the fix

You implemented the issue's suggested fix, measured it, and found the arm is never reached. "I first implemented the fix exactly where the issue points, at the js_array_is_array arm, and it changed nothing. Instrumenting that arm showed it is never reached with the flag set. A guard there is dead code." Both of the issue's open checks were wrong in the same direction — the flag does fire (49 calls, one match), and the walk doesn't miss, because js_object_get_symbol_property synthesizes the builtin rather than reading the prototype slot.

That is the difference between fixing the reported location and fixing the bug, and it is why a single-site fix was never going to work: four entry points reach an array by different routes.

Delegating all four to js_get_iterator rather than restating the sequence is the right structure, and the reason given is the one that matters — two copies of a spec sequence that must agree is how this diverged in the first place.

The own wins row is the one I'd have broken: js_get_iterator's patched branch reads the prototype only, so an array with its own method would throw "not iterable" once the prototype slot is deleted. Your first cut hit exactly that, and it is now the sixth row of the table.

Declining to write a gap test is the right call

Any test that patches Array.prototype[Symbol.iterator] takes the oracle down — node's primordials build a SafeMap from an iterable and exit 1 with "Iterator value patched is not an entry object", so the harness compares against a crashed reference. Four workarounds tried, none viable, and each failure is itself a bug you filed. Verifying by direct comparison and saying so plainly beats landing a test that passes for the wrong reason — which is exactly the failure mode the rest of this repo's gates exist to prevent.

The detail that it only appeared through the harness, not by hand (node file.ts standalone is fine), is worth keeping: it means the trap is invisible to the obvious local check.

Three follow-ups filed rather than folded in

for…of over a typed local ignoring the patched method (a codegen dense index loop with no runtime call at all — a genuinely different mechanism), restoring the original iterator being broken, and getOwnPropertyDescriptor returning undefined. That last one is #7761's subject, which is stacked on this and next.

cargo test -p perry-runtime --lib: 2023 passed, 0 failed. Gates 21/21.