fix(runtime): give iterator helpers their own class id (#7576) - #7583
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughIterator helper dispatch now uses a unique class ID, own ChangesIterator dispatch correction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IteratorFrom
participant iterator_step
participant OwnNext
IteratorFrom->>iterator_step: request next step
iterator_step->>IteratorFrom: read own next field
iterator_step->>OwnNext: invoke with iterator as this
OwnNext-->>iterator_step: return iterator result
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ 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 |
`ITERATOR_HELPER_CLASS_ID` and `STRING_ITERATOR_CLASS_ID` were both
`0xFFFF_0009`. The two constants were introduced independently, each with a
comment reading "sits just past the Set iterator id (0xFFFF0008)", and neither
author checked whether the slot was taken.
Every dispatch tower matches these ids in a fixed order and tests the String
arm before the helper arm, so EVERY iterator-helper object was dispatched as a
String iterator:
* `.next()` read the helper's op-kind field as a cursor index against a null
backing array, so it answered `{ done: true }` on its first step —
`Iterator.from(x)` was exhausted before it started, silently and for every
source kind.
* every other helper method (`map`/`filter`/`take`/`drop`/`flatMap`/
`toArray`/`reduce`/`forEach`/`some`/`every`/`find`) fell into the String
dispatcher's `_ => undefined` arm, so `Iterator.from(g).map(f)` was
`undefined` and `.filter` on it threw.
The helper takes `0xFFFF_000B` (`0xFFFF_000A` is the RegExp-string iterator).
Second, independent defect on the same path: `iterator_step` resolved the
source's `next` with the INHERITING getter. Since #321 every built-in iterator
inherits `.next` from its shared `%…IteratorPrototype%` singleton, and that
inherited `next` is a thunk that resolves its receiver from
`js_implicit_this_get()`. So the lookup found a callable closure for an
array/Map/Set/String iterator source, took the raw-closure-call branch, and ran
the thunk with whatever `this` was left in the thread-local — `done` on the
first step, or `Method %IteratorPrototype%.next called on incompatible
receiver`. `js_iterator_to_array` already carries the own-field version of this
fix; `iterator_step` now uses `js_object_get_own_field_or_undef` too, and binds
`this` to the iterator around the call per `IteratorNext`'s
`Call(next, iterator)`. Both halves are load-bearing: with the class id fixed
and `iterator_step` reverted, the new tests abort on the brand-check throw.
Why nobody noticed: `test_gap_iterator_helpers_2874.ts` DID catch this, and has
been listed in `test-parity/known_failures.json` since 2026-07-04. It passes
byte-for-byte now and is un-skipped here.
Coverage, per CLAUDE.md #5960 (integration suites under `crates/*/tests/` only
run nightly/tag):
* `crates/perry-runtime/src/iterator_helpers/tests.rs` — 16 `cargo-test`
visible unit tests. They all drive `js_native_call_method`, the tower a
compiled program reaches, NOT `dispatch_iterator_helper_method` directly:
a test that called the helper dispatcher directly would have been green
throughout the outage, because the collision lives in the tower's arm
order. Includes `iterator_class_ids_are_pairwise_distinct`, which fails on
any duplicate in the family, and
`an_own_next_method_is_called_with_the_iterator_as_this`, which pins the
`this`-binding half independently.
* `test-files/test_gap_iterator_helpers_7576.ts` — 28 lines byte-identical
to `node --experimental-strip-types` (26.5.1), covering the issue's
reproducer, a stored hand-stepped helper, all six source kinds, every
combinator and terminal, laziness over an unbounded generator, spread, and
the String iterator itself.
Sabotage-verified both ways: restoring `0xFFFF_0009` fails
`iterator_class_ids_are_pairwise_distinct` plus 6 behavioural tests; reverting
`iterator_step` aborts the suite on the brand check.
cdf996d to
71317ab
Compare
Audit before merge — verified, merged as v0.5.1330Reproduced the fix end-to-end against node on my own build, with a test I Byte-identical to Sabotage-verified: restoring The root cause is worth restating because it is so cheap and so destructive: two The new test cannot catch the next one — and there IS a next one
A distinctness test that enumerates one family is structurally unable to fail |
Closes #7576.
Root cause: a class-id collision
crates/perry-runtime/src/iterator_helpers.rs:45andcrates/perry-runtime/src/string/iter_object.rs:25both defined0xFFFF_0009:Two constants, introduced independently, carrying the same comment. Neither
author checked whether the slot was taken.
Every dispatch tower matches these ids in a fixed order, and the String arm
comes first —
object/native_call_method/handle_methods.rs:838and.../collection_methods.rs:396, both ahead of the helper arm at:855/:410.So every iterator-helper object was dispatched as a String iterator:
.next()dispatch_string_iterator_method, reading the helper's op-kind field as a cursor index against a null backing array{ done: true }on the first step, for every source kind.map/.filter/.take/.drop/.flatMap/.toArray/.reduce/.forEach/.some/.every/.find_ => undefinedarmundefined, so the next link in the chain threwThat is both symptoms in the issue from one cause, and it explains why row C
(the bare
{next}driven directly) worked: it never went near the helper.The helper now takes
0xFFFF_000B.0xFFFF_000Ais the RegExp-string iterator.Second, independent defect on the same path
iterator_stepresolved the source iterator'snextwith the inheritinggetter (
js_object_get_field_by_name). Since #321 every built-in iteratorinherits
.nextfrom its shared%…IteratorPrototype%singleton(
object/iterator_prototypes.rs), and that inheritednextis a thunkthat resolves its receiver from
js_implicit_this_get()and dispatches byclass id.
So for an array / Map / Set / String iterator source the lookup found a
perfectly callable closure,
iterator_steptook the raw-closure-call branch —js_closure_call1, which binds nothis— and the thunk ran against whateverwas left in the thread-local. Observed directly:
Method %IteratorPrototype%.next called on incompatible receiver.array/iterator.rs::js_iterator_to_arrayalready carries the own-field versionof this fix and documents exactly this hazard.
iterator_stepnow matches it:js_object_get_own_field_or_undef(also allocation-free, so it drops an internfrom the per-step path), and the call binds
thisto the iterator perIteratorNext'sCall(next, iterator)— which an ownnext() { return this.#impl.next(); }needs too.Both halves are load-bearing. With the class id fixed and
iterator_stepreverted, the new suite aborts on the brand-check throw.
Why this survived: the test existed and was skipped
test-files/test_gap_iterator_helpers_2874.tscovers most of this surface andhas been in
test-parity/known_failures.jsonsince 2026-07-04(
"reason": "iterator helpers (#2874); standing per #5917."). It passesbyte-for-byte now, and this PR removes the skip.
Coverage
Per CLAUDE.md #5960 (integration suites under
crates/*/tests/only runnightly/tag), the acceptance coverage is
cargo-test-visible plus a gap test.crates/perry-runtime/src/iterator_helpers/tests.rs— 16 unit tests.Every behavioural one drives
js_native_call_method, the tower a compiledprogram reaches, not
dispatch_iterator_helper_methoddirectly. That isdeliberate: a test that called the helper dispatcher directly would have been
green throughout the entire outage, because the collision lives in the
tower's arm order, not in the dispatcher. Two structural tests:
iterator_class_ids_are_pairwise_distinct— enumerates all sevenruntime-defined iterator class ids and fails on any duplicate, with an error
message naming the pair. This is the test that would have caught the bug at
the moment it was introduced.
an_own_next_method_is_called_with_the_iterator_as_this— pins thethis-binding half independently of the own-vs-inherited lookup.test-files/test_gap_iterator_helpers_7576.ts— 28 output lines, byte-identicalto
node --experimental-strip-typeson the pinned 26.5.1. Covers the issue'sreproducer verbatim (rows A–D), a helper stored in a local and hand-stepped
(the shape
test_gap_iterator_helpers_2874.tsnever exercised — it is allinline chains), all six source kinds (bare
{next}, generator, array, arrayiterator, Map iterator, Set iterator, string iterator), every combinator and
every terminal, laziness over an unbounded generator, spread, and the String
iterator itself.
Validation
Local; CI has a deep backlog and has not reported.
cargo test -p perry-runtime --no-fail-fast— 1834 passed, 0 failed.Node: 30/30 pass, including
test_gap_7563_array_iterator_class_id_confusion,the three
test_gap_7564_iter_result_*, andtest_gap_iterator_helpers_2874(the un-skipped one).
python3 scripts/raw_handle_debt.py→ 998 (baseline 998).scripts/check_file_size.sh→ OK.cargo fmt --all -- --check→ clean.Sabotage evidence
ITERATOR_HELPER_CLASS_ID = 0xFFFF_0009:iterator_class_ids_are_pairwise_distinctfails naming theiterator-helper/stringpair, andto_array_drains_the_chain,take_...,reduce_...,some_every_find_...,map_...,filter_...all go red.
iterator_stepto the inherited lookup + unboundjs_closure_call1(class id left fixed): the suite aborts on
Method %IteratorPrototype%.next called on incompatible receiver.js_implicit_this_setpair:an_own_next_method_is_called_with_the_iterator_as_thisreports a receiverof
undefined.Out of scope, but found while here
Two things a reviewer may want to pick up separately:
RAW_JSON_CLASS_IDandJSX_NODE_CLASS_IDare both0xFFFF_00A0(
json/raw_json.rs:23,jsx.rs:33). Same shape as this bug. They are notin the iterator family so the new distinctness test does not cover them, and
I have not established whether any tower matches both.
python3 scripts/addr_class_inventory.pyfails onmain(verified on aclean tree at
origin/main1e971d2), oncrates/perry-runtime/src/iter_result.rs:139— agcheader-castintroducedby perf(iterator): build one object per .next(), not five (#7564) #7579. It needs a mutable header write, so
try_read_gc_header(whichreturns
&'static) is not a drop-in; it wants either an allowlist entry ora
gc_flagssetter. Left alone here to keep this PR scoped.Summary by CodeRabbit
Bug Fixes
Tests
Chores