feat(stdlib): implement ES2024 Set methods (closes #236)#356
Conversation
Add the 7 ES2024 Set prototype methods: - union, intersection, difference, symmetricDifference - isSubsetOf, isSupersetOf, isDisjointFrom Implements GetSetRecord (§7.4.2) as a shared helper that validates the argument is an object, normalises size via floor() (not parseInt or Math.floor), and checks has/keys are callable. Each method applies the spec-correct branching strategy based on size comparison: - has-path (thisSize ≤ otherSize): iterate this, call other.has() - keys-path (thisSize > otherSize): iterate other.keys(), check this Has-path loops use manual index loops (`while i < data.values.length()`) instead of MoonBit's `for x in arr` for live-data semantics: the `has()` callback may mutate data.values mid-iteration, and MoonBit's for-each snapshots the length, causing out-of-bounds access when the array shrinks. Fixes in intersection: keys-path now checks alreadyInResult before adding to prevent duplicates; has-path checks element is still in thisData after other.has() may have deleted it (§24.2.3.2 step 5.a.ii.2). isSupersetOf and isDisjointFrom close the other.keys() iterator before returning false on early exit. Remove the "set-methods" entry from skip_features in test262_skip_metadata.json. Test262 result: 692/714 Set prototype tests pass (96.9%, up from 96.1%). Remaining 22 failures: 14 require BigInt support (no-BigInt engine limitation), 4 are pre-existing forEach/keys/values bugs, 4 are pre-existing iterator-mutation tests outside set-methods scope. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 24 minutes and 17 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 We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (3)
✨ 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: faf225de72
ℹ️ 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".
| let loc = @token.Loc::default() | ||
| let other = if args.length() > 0 { args[0] } else { Undefined } | ||
| let other_rec = get_set_record(interp, other, loc) | ||
| let result_values : Array[Value] = [] |
There was a problem hiding this comment.
The has-path for difference should start with a copy of the receiver's SetData and remove entries whose other.has call is truthy. Starting from an empty result and walking live data.values lets other.has mutations change the answer, e.g. adding 2 while checking a single-element set causes difference to include the newly added 2, although it was not in the original receiver snapshot.
Useful? React with 👍 / 👎.
Benchmark ResultsRun: https://github.com/dowdiness/js_engine/actions/runs/27531445160
Stage summary
Focused bytecode base-vs-head comparison
Base-vs-head comparison
Mean-time chart (log scale)
Closure-conversion comparison
|
- Set.prototype.forEach: restore manual index loop (the diff changed it to `for value in data.values` which snapshots the initial length; if the callback deletes an earlier element, the array shrinks past the snapshot length and the final iteration reads freed memory — same "illegal cast" bug fixed in isSubsetOf/isDisjointFrom) - get_set_record: reject +Infinity and -Infinity sizes with RangeError per §7.4.2 step 6b; floor(+Infinity)=+Infinity was slipping past the < 0.0 guard and being accepted as a valid size 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: 93a0fc218d
ℹ️ 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".
- get_set_record: use ToIntegerOrInfinity (truncation, not floor); allow +∞ as valid size - union/symmetricDifference: call other.keys() before copying this.[[SetData]] (§24.2.3.1 step 4 / §24.2.3.4 step 4) - intersection has-path: snapshot thisData before iterating; drop incorrect post-has re-check (§24.2.3.2 step 5) - difference has-path: snapshot thisData before calling other.has() (§24.2.3.3 step 5) - isSubsetOf/isDisjointFrom has-paths: snapshot thisData; skip elements deleted by has() - isSupersetOf/isDisjointFrom keys-paths: propagate iterator.return() errors (remove swallowing catch) 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: 4c02eeb975
ℹ️ 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".
…ator close - Set.prototype.forEach: don't advance index when callback deletes the current element (it compacts data.values; the successor shifts into the same slot) - has-paths (intersection/difference/isSubsetOf/isDisjointFrom): replace snapshot with live index loop — spec iterates live [[SetData]], so elements added by other.has() must be visited; keep deletion-safe advancement for both paths - intersection has-path/difference has-path: add SetDataHas dedup guard (§24.2.3.2/3) - all keys-paths (union/intersection/difference/symmetricDifference/isSupersetOf/ isDisjointFrom): wrap iterator_result_value in try-catch + iterator_close per IteratorStepValue §7.4.9 — if the value getter throws, close the iterator first 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: 43c6e32b69
ℹ️ 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 i < data.values.length() && | ||
| same_value_zero(data.values[i], value) { | ||
| i = i + 1 |
There was a problem hiding this comment.
This cursor check treats an equal value at the same compacted index as the original entry, but when the callback does s.delete(value); s.add(value) on a single-element Set, the re-added value occupies data.values[i]. The spec says values deleted after being visited and re-added before completion are visited again, so new Set([1]).forEach(...) should call the callback twice in that scenario; this increments past the re-added entry and returns after one call.
Useful? React with 👍 / 👎.
1 participant
Summary
union,intersection,difference,symmetricDifference,isSubsetOf,isSupersetOf,isDisjointFrom(closes Implement ES2024 Set methods #236)GetSetRecordhelper per §7.4.2 (validates receiver is object, checkshas/keyscallable, normalises size viafloor, rejects NaN/negative/infinite sizes)for x in arrsnapshots initial length; ahas()callback that mutates the Set would cause out-of-bounds readsSet.prototype.forEachwhich had the same snapshot-length hazard introduced by an earlier refactor in this branchTest plan
🤖 Generated with Claude Code