fix(set-iterator): brand check + done-flag for SetIteratorPrototype.next (§23.2.5.2.1)#441
Conversation
…ext (§23.2.5.2.1) Two bugs fixed in make_set_values_iterator and make_set_entries_iterator: 1. this-not-object-throw: §23.2.5.2.1 step 2 requires TypeError when `this` is not an Object. Switched closure from NativeCallable to make_method_func (MethodCallable) so the next function receives this_val; added Object check and class_name "Set Iterator" brand check. 2. iteration-mutable: §23.2.5.2.1 step 12 requires setting [[IteratedSet]] to undefined after exhaustion so subsequent next() calls return done=true even if items are added later. Added done_ref that is set on first exhaustion and checked on entry (step 7 analogue). SetIteratorPrototype/next: 6/20 → 20/20. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 56 minutes and 1 second. 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 (4)
📝 WalkthroughWalkthroughString Iterator and Set Iterator are refactored from per-instance closure-backed ChangesString and Set Iterator slot-based refactor
Possibly related issues
Possibly related PRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…22.1.5.2.1) Object.create(stringIterator) should throw TypeError when next() is called on it — the created object inherits next from the instance (a closure) but has no String Iterator internal slots. Switched make_string_iterator_value_with_proto from NativeCallable (ignores this) to make_method_func (MethodCallable) and added Object check + class_name "String Iterator" brand check per §22.1.5.2.1 steps 2-3. Also simplified result objects to create_iter_result. StringIteratorPrototype/next: 46/48 → 48/48. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: efd542c0c2
ℹ️ 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".
Benchmark ResultsRun: https://github.com/dowdiness/js_engine/actions/runs/28095050753
Stage summary
Focused bytecode base-vs-head comparison
Base-vs-head comparison
Mean-time chart (log scale)
Closure-conversion comparison
|
The previous brand check (class_name == Set Iterator) was insufficient:
SetIteratorPrototype itself carries that class_name, so calling
iter.next.call(Object.getPrototypeOf(iter)) passed the guard and advanced
the closure-captured iter instead of throwing for a receiver with no
iterator slots.
Migrate Set iterators to the same internal-slot design as Map iterators:
- Add negative-ID symbol constants SET_ITERATOR_{NEXT_INDEX,TARGET,KIND}_SYMBOL_ID
- set_iterator_next(this_val) reads slots from this_val; missing slots throw TypeError
- Iterator next lives on SetIteratorPrototype, not per-instance
- Collapse make_set_values_iterator + make_set_entries_iterator into make_set_iterator
Object.getPrototypeOf(iter).next() now correctly throws TypeError because the
prototype has no SET_ITERATOR_TARGET_SYMBOL_ID slot.
All 22 SetIteratorPrototype test262 tests pass; 2183 unit tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eab6ed1b6c
ℹ️ 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".
…state %StringIteratorPrototype% has class_name "String Iterator", so the prior class_name-only brand check accepted the prototype itself as |this|. That allowed iter.next.call(Object.getPrototypeOf(iter)) to advance the closure-captured state on the original iterator instead of throwing. Migration to internal-slot design (STRING_ITERATOR_NEXT_INDEX_SYMBOL_ID=-136, STRING_ITERATOR_STRING_SYMBOL_ID=-137): - string_iterator_next() reads [[StringIteratorNextIndex]] and [[IteratedString]] from the receiver's hidden symbol slots; missing STRING slot -> TypeError. - next() moves from per-instance closure to %StringIteratorPrototype%.next. - Factory stores s and index 0 as slots; uses unsafe_substring() for O(1) character slicing with correct surrogate-pair handling. StringIteratorPrototype/next (all): 40/40 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…actories
Three changes from post-review:
- string_iterator_next: `if index >= len { return }` -> `guard` (AGENTS.md convention)
- string_iterator_next: cache `s[index+1].to_int()` as `next_code` (was evaluated twice)
- make_string_iterator_value_with_proto: call array_iterator_internal_slot_descriptor()
instead of inlining the same PropDescriptor literal
- make_set_iterator: call map_iterator_internal_slot_descriptor() instead of inlining
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@interpreter/stdlib/builtins_map_set.mbt`:
- Around line 1874-1922: `set_iterator_next` is using a raw array index cursor,
so Set mutations between `next()` calls can shift `set_data.values` and cause
skipped entries. Update the Set iterator logic in `set_iterator_next` to follow
the same stable-slot/tombstone approach used during live iteration, or otherwise
make the cursor track stable entry identities instead of array positions. Also
ensure `Set.prototype.delete` and `Set.prototype.clear` preserve iterator
correctness by not compacting in a way that invalidates existing
`SET_ITERATOR_NEXT_INDEX_SYMBOL_ID` state.
🪄 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
Run ID: cf43179e-d239-4f69-8c26-363be364de2f
📒 Files selected for processing (2)
interpreter/runtime/iterators.mbtinterpreter/stdlib/builtins_map_set.mbt
set_iterator_next never incremented set_data.iteration_depth, so delete/clear saw depth==0 during for..of and compacted (remove+shift) instead of tombstoning. This caused the iterator cursor to skip elements adjacent to deleted entries. Fix: add SET_ITERATOR_ACTIVE_SYMBOL_ID=-138 slot (initialized false). - First non-done next(): set ACTIVE=true, increment iteration_depth. - Exhaustion: set ACTIVE=false, decrement iteration_depth; compact tombstones if depth reaches 0 (mirrors forEach cleanup at line 1107). Abandoned iterators (for..of break/return) leave iteration_depth elevated; there is no finalizer to decrement it. SetIteratorPrototype: 22/22 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63542ee639
ℹ️ 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".
| data.bag.symbol_properties[SET_ITERATOR_ACTIVE_SYMBOL_ID] = Bool( | ||
| true, | ||
| ) | ||
| set_data.iteration_depth += 1 |
There was a problem hiding this comment.
When a Set iterator is only partially consumed (for example const it = s.values(); it.next(); or a for...of loop that breaks), this increment is only undone if that same iterator is later exhausted. From then on delete/clear keep tombstones in data.values, and methods that are not tombstone-aware still use the raw backing array (e.g. isSubsetOf compares data.values.length()), so new Set([1,2]) after one next() and delete(1) can report isSubsetOf(new Set([2])) === false even though only 2 remains. Please avoid making the collection stay in iteration mode for abandoned iterators unless all later operations account for the persistent tombstones.
Useful? React with 👍 / 👎.
…terators
- Change delete/clear gate from `iteration_depth > 0` to `tombstones is Some(_)`
so that compaction decisions are driven by whether tombstone mode is active,
not by a depth counter that leaks when for..of loops are abandoned (break/return).
- make_set_iterator now enters tombstone mode (tombstones=Some({})) eagerly at
iterator creation instead of lazily on first next() call. No ACTIVE sentinel slot.
Remove SET_ITERATOR_ACTIVE_SYMBOL_ID constant and its per-iterator slot.
- set_iterator_next exhaustion compacts via compact_set_tombstones only when
iteration_depth==0 (no forEach is currently walking the same set). If a forEach
callback triggered the for..of, the forEach owns compaction on its own exit.
- forEach uses tombstones_pre_existed to decide who owns compaction: if tombstones
was already Some (inherited from outer for..of), forEach leaves them intact.
iteration_depth still counts active forEach calls to protect cursor stability
when a nested for..of exhausts inside a forEach callback.
- Fix all tombstone-blind bulk methods: union, intersection, difference,
isSubsetOf, isSupersetOf, isDisjointFrom, symmetricDifference now use
set_data_effective_size() for the size comparison and skip/ignore tombstoned
slots in scan loops and membership lookups via set_data_contains_live().
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5636e00864
ℹ️ 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".
| if set_data.tombstones is None { | ||
| set_data.tombstones = Some(@set.Set::default()) |
There was a problem hiding this comment.
With this patch, merely calling s.values() puts the Set into tombstone mode, so a later delete or clear leaves removed entries in values until the iterator is exhausted. Normal s.size reads do not use the updated prototype getter: both Set size fast paths in interpreter/runtime/property.mbt still return set_data.values.length() (checked at lines 460 and 1181), so const s = new Set([1,2]); const it = s.values(); s.delete(1); s.size reports 2 instead of 1. Fresh evidence versus the earlier abandoned-iterator concern is that this new iterator-creation path still enables tombstones for ordinary property reads without updating those runtime fast paths.
Useful? React with 👍 / 👎.
…exhaustion - Add SetData::effective_size() method (values.length() - tombstoned count) and use it in both size fast paths in property.mbt (lines previously returned values.length() raw, reporting 2 for new Set([1,2]) after values()+delete(1)). - Remove compaction from set_iterator_next exhaustion path. Compacting at exhaustion when multiple iterators share the same Set would shift values[] indices, corrupting other iterators' stored numeric cursor positions. Example: two iterators over [1,2,3]; delete(1) tombstones index 0; when the first iterator exhausts and compacts, the second (cursor=1) would skip to values[1]=3, missing element 2. Tombstone cleanup is now the exclusive responsibility of forEach when it exits tombstone mode (tombstones_pre_existed=false). Tombstones persist after iterator exhaustion — size is bounded by deleted-entry count (≤ values.length). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Four commits fixing iterator prototype brand checks from issue #207.
Commit 1: SetIteratorPrototype.next — brand check + done-flag (§23.2.5.2.1)
Initial fix: switched from
NativeCallabletomake_method_funcwith class_name brand check, addeddone_ref. Superseded by commit 3 (slot-based design).Commit 2: StringIteratorPrototype.next — brand check, class-name only (§22.1.5.2.1)
Object.create(stringIterator).next()→ TypeError. Superseded by commit 4 (slot-based design).Commit 3: SetIteratorPrototype.next — receiver internal slots
The commit 1 brand check was still insufficient:
%SetIteratorPrototype%hasclass_name: "Set Iterator", soiter.next.call(Object.getPrototypeOf(iter))passed the guard and advanced the closure-capturediter.Migration to Map-iterator-style internal slots:
SET_ITERATOR_{NEXT_INDEX,TARGET,KIND}_SYMBOL_IDconstants (IDs -133/-134/-135)set_iterator_next(this_val)reads[[SetNextIndex]],[[IteratedSet]],[[SetIterationKind]]fromthis_valslots — missing slots → TypeErrormake_set_iterator(realm_state, set_data, kind)stores state as hidden symbol slots[[IteratedSet]] == Undefined(§step 12) replacesdone_refSetIteratorPrototype/next: 6/20 → 20/20Commit 4: StringIteratorPrototype.next — receiver internal slots
Same root cause as commit 3:
%StringIteratorPrototype%hasclass_name: "String Iterator", soa.next.call(%StringIteratorPrototype%)passed the class-name guard and advanceda's closure state. Anda.next.call(b)readsa's closure-capturedindex_ref/charsregardless ofb's state.Migration:
STRING_ITERATOR_{NEXT_INDEX,STRING}_SYMBOL_IDconstants (IDs -136/-137)string_iterator_next(this_val)reads[[StringIteratorNextIndex]]and[[IteratedString]]from receiver slots — missing STRING slot → TypeError (catches the prototype)next()lives on%StringIteratorPrototype%unsafe_substring()for O(1) character slicing with correct surrogate-pair handling (previously O(n)to_array()per iterator construction)StringIteratorPrototype/next(incl. RegExpStringIterator): 40/40Test results
SetIteratorPrototype/next: 6/20 → 20/20SetIteratorPrototype(full): 22/22 ✓StringIteratorPrototype/next(incl. RegExpStringIterator): 40/40 ✓moon test: 2183/2183 ✓Closes part of #207 (slice 1 — iterator prototype brand checks + Set mutable iteration).
Test plan
make test262-filter FILTER="SetIteratorPrototype"→ 22/22make test262-filter FILTER="StringIteratorPrototype/next"→ 40/40iter.next.call(Object.getPrototypeOf(iter))→ TypeError for both Set and String iterators ✓moon test→ all 2183 pass🤖 Generated with Claude Code
Summary by CodeRabbit