Support set-like operands for Set methods#429
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSet operation methods ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SetOp as Set.prototype.method
participant GetSetRecord as GetSetRecord\n(validation)
participant KeysIter as Keys Iterator
participant ResultSet as Result Set
Client->>SetOp: call Set.prototype.method(other)
SetOp->>GetSetRecord: validate other (object, size, has, keys)
GetSetRecord->>GetSetRecord: coerce/check size\nensure non-negative
GetSetRecord->>GetSetRecord: ensure has is callable
GetSetRecord->>GetSetRecord: ensure keys is callable
GetSetRecord-->>SetOp: return SetRecord(size, has, keys)
alt iterate using smaller size's keys()
SetOp->>KeysIter: obtain iterator from SetRecord.keys()
KeysIter-->>SetOp: iterator
loop iterate keys
SetOp->>SetOp: call counterpart.has(key)\ncheck membership/add to ResultSet
SetOp->>ResultSet: add key / update size
end
else iterate own items calling counterpart.has(value)
loop iterate own items
SetOp->>GetSetRecord: call SetRecord.has(value)
SetOp->>ResultSet: add value if absent
end
end
SetOp-->>Client: return ResultSet
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
new feature
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 114 improved · 🔴 16 regressed · 256 unchanged · avg +0.9% arraybuffer.js — Interp: 🟢 4, 10 unch. · avg +1.6% · Bytecode: 🔴 12, 2 unch. · avg -7.6%
arrays.js — Interp: 🟢 3, 16 unch. · avg +1.4% · Bytecode: 🔴 19 · avg -11.0%
async-await.js — Interp: 🟢 1, 5 unch. · avg +1.7% · Bytecode: 🔴 5, 1 unch. · avg -9.7%
base64.js — Interp: 🟢 1, 9 unch. · avg +0.8% · Bytecode: 🔴 8, 2 unch. · avg -12.9%
classes.js — Interp: 🟢 9, 22 unch. · avg +1.5% · Bytecode: 🔴 21, 10 unch. · avg -4.9%
closures.js — Interp: 🟢 4, 7 unch. · avg +1.5% · Bytecode: 🔴 11 · avg -7.8%
collections.js — Interp: 🟢 5, 7 unch. · avg +1.7% · Bytecode: 🔴 11, 1 unch. · avg -10.0%
csv.js — Interp: 🟢 2, 11 unch. · avg +1.4% · Bytecode: 🔴 13 · avg -8.6%
destructuring.js — Interp: 🔴 2, 20 unch. · avg -0.4% · Bytecode: 🔴 21, 1 unch. · avg -8.1%
fibonacci.js — Interp: 🟢 2, 6 unch. · avg +0.7% · Bytecode: 🔴 8 · avg -11.5%
float16array.js — Interp: 🟢 9, 🔴 4, 19 unch. · avg +1.0% · Bytecode: 🟢 4, 🔴 24, 4 unch. · avg +3.5%
for-of.js — Interp: 🟢 1, 6 unch. · avg +0.4% · Bytecode: 🔴 7 · avg -11.1%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 31, 🔴 1, 10 unch. · avg +3.7% · Bytecode: 🟢 1, 🔴 32, 9 unch. · avg -5.7%
json.js — Interp: 🟢 2, 18 unch. · avg +0.6% · Bytecode: 🔴 20 · avg -13.0%
jsx.jsx — Interp: 🟢 4, 🔴 1, 16 unch. · avg +1.0% · Bytecode: 🔴 19, 2 unch. · avg -8.2%
modules.js — Interp: 🟢 1, 8 unch. · avg +1.1% · Bytecode: 🔴 9 · avg -10.4%
numbers.js — Interp: 🟢 1, 10 unch. · avg +0.3% · Bytecode: 🔴 10, 1 unch. · avg -9.0%
objects.js — Interp: 🔴 1, 6 unch. · avg -1.3% · Bytecode: 🔴 7 · avg -9.0%
promises.js — Interp: 🟢 2, 🔴 1, 9 unch. · avg +0.7% · Bytecode: 🔴 9, 3 unch. · avg -7.6%
regexp.js — Interp: 🟢 4, 7 unch. · avg +2.4% · Bytecode: 🔴 11 · avg -8.0%
strings.js — Interp: 🟢 4, 🔴 3, 12 unch. · avg -4.5% · Bytecode: 🟢 2, 🔴 15, 2 unch. · avg +2.2%
tsv.js — Interp: 🟢 6, 3 unch. · avg +4.8% · Bytecode: 🔴 9 · avg -8.7%
typed-arrays.js — Interp: 🟢 11, 🔴 1, 10 unch. · avg +1.7% · Bytecode: 🟢 11, 🔴 6, 5 unch. · avg +1.1%
uint8array-encoding.js — Interp: 🟢 7, 🔴 2, 9 unch. · avg -4.3% · Bytecode: 🟢 5, 🔴 9, 4 unch. · avg +5.8%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/built-ins/Set/prototype/isDisjointFrom.js (1)
22-61: Make the size-based branch test observable.The new “other size is smaller” case still passes if the implementation uses
has()instead of switching tokeys(). If you want this test to guard the branch behavior, makehas()fail or assert call counts so thekeys()path is exercised.Suggested test hardening
test("uses set-like keys when other size is smaller", () => { const setLike = { size: 1, - has(value) { - return value === 3 || value === 4; - }, + has() { + throw new Error("unexpected has() call"); + }, keys() { return [3, 4].values(); }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Set/prototype/isDisjointFrom.js` around lines 22 - 61, The test for the "other size is smaller" branch is not observable because implementations that always call has() still pass; update the test for Set.prototype.isDisjointFrom so it forces the algorithm to use keys() when the other.size is smaller—e.g., make the set-like object's has() throw or increment a counter and assert it is not called (or assert keys() is called) so the keys()-based path is actually exercised; target the set-like object used in that test and the isDisjointFrom invocation so the behavior is clearly verified.tests/built-ins/Set/prototype/difference.js (1)
23-66: Make the size-based branch test observable.The new branch-specific case still passes if the implementation falls back to the
has()-based path. If the goal is to lock down thekeys()branch, makehas()fail or otherwise assert call counts so the optimized path is actually exercised.Suggested test hardening
test("uses set-like keys when other size is smaller", () => { const setLike = { size: 1, - has(value) { - return value === 2 || value === 4; - }, + has() { + throw new Error("unexpected has() call"); + }, keys() { return [2, 4, 4].values(); }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Set/prototype/difference.js` around lines 23 - 66, The "uses set-like keys when other size is smaller" test isn't observable because an implementation can ignore keys() and use has(); change the test's setLike so has() will fail if called (e.g., throw or increment a spy and assert zero calls) and keep keys() returning the expected iterator, then assert the result still contains 1 and 3 and size 2; this forces execution of the keys()-driven branch in Set.prototype.difference and proves the size-based optimization is exercised.tests/built-ins/Set/prototype/intersection.js (1)
38-52: Make the “keys path” test branch-discriminating.Right now the test can still pass if
intersectionincorrectly useshas()in this branch. Makehas()intentionally disagree withkeys()so only the keys-iterator path succeeds.Suggested test hardening
test("uses set-like keys when other size is smaller", () => { const setLike = { size: 1, has(value) { - return value === 2 || value === 4; + return false; }, keys() { return [2, 4, 4].values(); }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Set/prototype/intersection.js` around lines 38 - 52, The test "uses set-like keys when other size is smaller" should force the codepath to use the keys() iterator rather than has(): update the setLike object so has() disagrees with keys() (e.g. make has(value) return true only for 2 while keys() yields [2, 4, 4]) so an implementation that wrongly uses has() will fail but a correct intersection(Set.prototype.intersection) that iterates keys() will pass; adjust the setLike.has and/or keys implementations inside that test to create this intentional disagreement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.SetValue.pas`:
- Around line 164-189: AdvanceNext and DirectNext currently treat non-object
results from iterator.next() as "done" but must instead throw a TypeError like
Return does; update both AdvanceNext and DirectNext to validate the value
returned by the invoked next() call and if it is not a TGocciaObjectValue (or is
TGocciaUndefinedLiteralValue) call ThrowTypeError with the same or analogous
error message used in Return (the TypeError path at lines ~171–172) so
non-object next() results raise an error per the spec; locate the calls that
invoke NextMethod in AdvanceNext and DirectNext and add the same object-check +
ThrowTypeError logic used in the Return method.
---
Nitpick comments:
In `@tests/built-ins/Set/prototype/difference.js`:
- Around line 23-66: The "uses set-like keys when other size is smaller" test
isn't observable because an implementation can ignore keys() and use has();
change the test's setLike so has() will fail if called (e.g., throw or increment
a spy and assert zero calls) and keep keys() returning the expected iterator,
then assert the result still contains 1 and 3 and size 2; this forces execution
of the keys()-driven branch in Set.prototype.difference and proves the
size-based optimization is exercised.
In `@tests/built-ins/Set/prototype/intersection.js`:
- Around line 38-52: The test "uses set-like keys when other size is smaller"
should force the codepath to use the keys() iterator rather than has(): update
the setLike object so has() disagrees with keys() (e.g. make has(value) return
true only for 2 while keys() yields [2, 4, 4]) so an implementation that wrongly
uses has() will fail but a correct intersection(Set.prototype.intersection) that
iterates keys() will pass; adjust the setLike.has and/or keys implementations
inside that test to create this intentional disagreement.
In `@tests/built-ins/Set/prototype/isDisjointFrom.js`:
- Around line 22-61: The test for the "other size is smaller" branch is not
observable because implementations that always call has() still pass; update the
test for Set.prototype.isDisjointFrom so it forces the algorithm to use keys()
when the other.size is smaller—e.g., make the set-like object's has() throw or
increment a counter and assert it is not called (or assert keys() is called) so
the keys()-based path is actually exercised; target the set-like object used in
that test and the isDisjointFrom invocation so the behavior is clearly verified.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9501bf69-7425-444d-8e91-9db6df97e705
📒 Files selected for processing (12)
docs/built-ins.mddocs/value-system.mdsource/units/Goccia.Error.Messages.passource/units/Goccia.Error.Suggestions.passource/units/Goccia.Values.SetValue.pastests/built-ins/Set/prototype/difference.jstests/built-ins/Set/prototype/intersection.jstests/built-ins/Set/prototype/isDisjointFrom.jstests/built-ins/Set/prototype/isSubsetOf.jstests/built-ins/Set/prototype/isSupersetOf.jstests/built-ins/Set/prototype/symmetricDifference.jstests/built-ins/Set/prototype/union.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/built-ins/Set/prototype/intersection.js (1)
38-46: Optional: makehas()throw to hard-assert the keys-based branch.The test already checks output shape, but making
has()throw would prove this path does not consulthas()whenother.sizeis smaller.Suggested test hardening
const setLike = { size: 1, - has(value) { - return value === 2; - }, + has() { + throw new Error("has() must not be called in this branch"); + }, keys() { return [2, 4, 4].values(); }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Set/prototype/intersection.js` around lines 38 - 46, Modify the "uses set-like keys when other size is smaller" test to make the setLike.has method throw (e.g., throw new Error("has should not be called")) so the test will fail if the intersection implementation erroneously calls has; keep setLike.keys() returning [2,4,4].values() and leave the rest of the test assertions unchanged so the test hard-asserts the keys-based branch is used.source/units/Goccia.Values.Iterator.Generic.pas (1)
70-96: Remove unreachable fallback branches after the new TypeError guards.After Line 70 and Line 127 throw on non-object
NextResult, the laterelsepaths for non-object results cannot execute. Flattening to a single object path will make both methods easier to maintain.Also applies to: 127-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.Iterator.Generic.pas` around lines 70 - 96, The code currently guards non-object NextResult with ThrowTypeError but still contains unreachable fallback else branches; remove the redundant non-object branches and flatten the logic to assume NextResult is TGocciaObjectValue after the ThrowTypeError check. Specifically, in the methods handling NextResult (references: NextResult, TGocciaObjectValue, ThrowTypeError) eliminate the final "else" branch that sets FDone and returns an undefined result, and consolidate the remaining logic to fetch PROP_DONE and PROP_VALUE (DoneVal, ValueVal, PROP_DONE, PROP_VALUE, TGocciaUndefinedLiteralValue), set FDone accordingly, and return CreateIteratorResult with the appropriate done flag; do the same refactor for the analogous block around lines 127–156.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/units/Goccia.Values.Iterator.Generic.pas`:
- Around line 70-96: The code currently guards non-object NextResult with
ThrowTypeError but still contains unreachable fallback else branches; remove the
redundant non-object branches and flatten the logic to assume NextResult is
TGocciaObjectValue after the ThrowTypeError check. Specifically, in the methods
handling NextResult (references: NextResult, TGocciaObjectValue, ThrowTypeError)
eliminate the final "else" branch that sets FDone and returns an undefined
result, and consolidate the remaining logic to fetch PROP_DONE and PROP_VALUE
(DoneVal, ValueVal, PROP_DONE, PROP_VALUE, TGocciaUndefinedLiteralValue), set
FDone accordingly, and return CreateIteratorResult with the appropriate done
flag; do the same refactor for the analogous block around lines 127–156.
In `@tests/built-ins/Set/prototype/intersection.js`:
- Around line 38-46: Modify the "uses set-like keys when other size is smaller"
test to make the setLike.has method throw (e.g., throw new Error("has should not
be called")) so the test will fail if the intersection implementation
erroneously calls has; keep setLike.keys() returning [2,4,4].values() and leave
the rest of the test assertions unchanged so the test hard-asserts the
keys-based branch is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f5afb94-503d-458c-b411-4aef1f4b1a24
📒 Files selected for processing (6)
source/units/Goccia.Values.Iterator.Generic.pastests/built-ins/Iterator/from.jstests/built-ins/Set/prototype/difference.jstests/built-ins/Set/prototype/intersection.jstests/built-ins/Set/prototype/isDisjointFrom.jstests/built-ins/Set/prototype/union.js
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Set/prototype/isDisjointFrom.js
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/built-ins/Set/prototype/difference.js
- tests/built-ins/Set/prototype/union.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #422.