fix(array): respect deletion of Array.prototype[Symbol.iterator]#462
Conversation
The `get_computed_property_impl` Array arm had a hard-coded synthesis block that returned a default array iterator for Symbol.iterator lookups after the prototype chain walk found nothing. This meant that `delete Array.prototype[Symbol.iterator]` had no effect: arrays remained iterable, so ArrayBindingPattern destructuring (§7.4.1 GetIterator) never threw the expected TypeError. The block was described as a fallback for "before setup_iterator_protocol is called", but setup_iterator_protocol writes symbol_properties[iterator_sym.id] on Array.prototype (builtins_iterator.mbt:60), so the prototype chain walk at lines 935-936 handles all normal cases and the synthesis block is unreachable in that path — it only fires in the delete case, which is the bug. The instance-level checks at lines 906-918 already cover own-property Symbol.iterator overrides (accessors and values), so removing the synthesis block does not regress any custom-iterator use case. Fixes 40 strict + 40 non-strict test262 failures (language/.../dstr/ary-init-iter-get-err-array-prototype across arrow-function, generators, class methods, function params, etc.) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 3 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6cacec520
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| ) | ||
| } | ||
| return Undefined |
There was a problem hiding this comment.
When an array's prototype override is another array or a Proxy, get_array_prototype returns Array(...)/Proxy(...), so this return Undefined is now reached because the preceding walk only handles Object(proto_data). After deleting the fallback, cases like Object.setPrototypeOf(a, []); a[Symbol.iterator] or Object.setPrototypeOf(a, new Proxy(Array.prototype, {})); for (const x of a) {} lose the inherited Array.prototype iterator and throw, even though OrdinaryGet should keep walking through those prototype objects; route this path through the existing symbol-prototype walker or handle non-Object prototypes before returning.
Useful? React with 👍 / 👎.
Benchmark ResultsRun: https://github.com/dowdiness/js_engine/actions/runs/28181387137
Stage summary
Focused bytecode base-vs-head comparison
Base-vs-head comparison
Mean-time chart (log scale)
Closure-conversion comparison
|
… overrides
`get_array_prototype` can return `Array(...)`, `Proxy(...)`, or any other
Value when the prototype was overridden via `Object.setPrototypeOf`. The
previous `_ => ()` wildcard silently swallowed all these cases, causing
`get_computed_property` to return `Undefined` even when Symbol.iterator
was reachable through the prototype chain.
Replace the wildcard with:
- `Null | Undefined` → end of chain (same semantics as before)
- any other Value → delegate to `self.get_computed_property`, which
already dispatches through Array, Proxy, Map, Set, and Object arms
This makes the following cases work correctly:
- `Object.setPrototypeOf(a, []); a[Symbol.iterator]`
- `Object.setPrototypeOf(a, new Proxy(Array.prototype, {})); for (x of a)`
while preserving the fix from the parent commit: after a real
`delete Array.prototype[Symbol.iterator]`, the prototype chain walk via
the `Object(proto_data)` arm finds nothing, falls through to `Null | Undefined`,
and returns Undefined as required by GetIterator §7.4.1.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Two complementary fixes to
get_computed_property_impl's Array symbol lookup arm ininterpreter/runtime/property.mbt.Fix 1 — Remove synthesis block so
delete Array.prototype[Symbol.iterator]is respected:The Array arm had a hard-coded block that synthesized a default array iterator after the prototype chain walk found nothing. This made
delete Array.prototype[Symbol.iterator]a no-op: arrays remained iterable, soArrayBindingPatterndestructuring (§7.4.1 GetIterator) never threw the required TypeError.setup_iterator_protocolalready writessymbol_properties[iterator_sym.id]on Array.prototype (builtins_iterator.mbt:60), so the chain walk handles the normal case without this fallback.Fix 2 — Continue lookup through non-Object prototype overrides (
Object.setPrototypeOf(arr, [])):The
_ => ()wildcard following thematch get_array_prototype(...)silently droppedArray(...),Proxy(...), and other walkable prototype values. Replaced withNull | Undefined => ()(end of chain) and aproto_val => get_computed_property(proto_val, ...)delegate that routes through the existing full-dispatch path. This fixes:Object.setPrototypeOf(a, []); for (const x of a) {}Object.setPrototypeOf(a, new Proxy(Array.prototype, {})); a[Symbol.iterator]The delete fix is preserved: after
delete Array.prototype[Symbol.iterator], the chain walk finds nothing in theObject(proto_data)branch and falls through toNull | Undefined => (), returning Undefined as GetIterator requires.Lookup order after these fixes
Test Results
ary-init-iter-get-err-array-prototypefilter: 80/80 ✓ (was 0/80)language/statements/for-of: 1358/1414, zero new regressions vs CI baselinelanguage/expressions/array: no new regressions vs CI baselineFixes 40 strict + 40 non-strict test262 failures across arrow-function/dstr, generators/dstr, class/dstr, function/dstr, and ~12 other destructuring families.
Test plan
moon test2219/2219make test262-filter FILTER=ary-init-iter-get-err-array-prototype80/80make test262-filter FILTER=language/statements/for-ofzero new regressions vs CI baselinemake test262-filter FILTER=language/expressions/arrayzero new regressions vs CI baseline🤖 Generated with Claude Code