Support iterable inputs in Map.groupBy#317
Merged
Merged
Conversation
- Accept arrays, sets, maps, strings, and custom iterables - Add coverage for non-iterables, empty iterables, and callback indices
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughMap.groupBy was extended to accept any iterable (arrays, Sets, Maps, strings, custom iterables) by adding iterator-protocol dispatch paths, refactoring grouping logic into a helper, and expanding tests to cover diverse inputs, edge cases, and non-iterable errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MapGroupBy as Map.groupBy
participant Dispatch as DispatchLogic
participant Iterator as Iterator
participant Callback
participant Result as ResultMap
Caller->>MapGroupBy: call(source, callbackfn)
MapGroupBy->>MapGroupBy: validate callbackfn
MapGroupBy->>Dispatch: determine source type
alt Array source
Dispatch->>MapGroupBy: fast array path (indexing)
MapGroupBy->>Callback: Invoke(value, index)
else String source
Dispatch->>Iterator: use string iterator
Iterator->>Callback: Invoke(char, index)
else Object with Symbol.iterator
Dispatch->>Iterator: get & call Symbol.iterator
Iterator->>Callback: Invoke(value, index)
else Non-iterable
Dispatch-->>MapGroupBy: throw TypeError
end
Callback-->>Dispatch: return groupKey
Dispatch->>Result: find/create group for groupKey
Result->>Result: append value to group
MapGroupBy-->>Caller: return ResultMap
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/built-ins/Map/groupBy.js`:
- Around line 83-87: The test for Map.groupBy's non-iterable inputs is too loose
because it uses toThrow(); update the three assertions inside the "throws
TypeError for non-iterable" test to assert the specific error type (e.g.,
expect(() => Map.groupBy(42, () => "a")).toThrow(TypeError) or use toThrowError
with the expected message) so failures only pass when a TypeError is thrown by
Map.groupBy for non-iterable inputs; change the three expect lines that call
Map.groupBy with 42, true, and null accordingly.
In `@units/Goccia.Builtins.GlobalMap.pas`:
- Around line 77-93: The loop currently compares keys using
ToStringLiteral.Value which conflates distinct keys; replace that string-based
comparison with a proper Map-key equality check (e.g. call the key object's
equality method or compare the key reference/value directly) when iterating
ResultMap.Entries to find a matching key for GroupKey, and when a match is found
use the existing entry.Key (not the string) as the map key for the bucket;
update the code around ResultMap.Entries[K].Key, GroupKey and ResultMap.SetEntry
to use the equality check (e.g. ResultMap.Entries[K].Key.Equals(GroupKey) or an
appropriate reference/value comparison) and remove the ToStringLiteral.Value
comparisons so distinct keys (1 vs "1" or different objects) remain separate.
- Around line 109-123: The array fast-path in the global Map implementation
creates ResultMap (TGocciaMapValue) but does not protect it from GC before
calling AddToGroup on array elements; mirror the iterator branches by
rooting/protecting ResultMap (and any temp group arrays) prior to the while-loop
that calls AddToGroup and unrooting after the loop so partially-built maps
cannot be reclaimed if the callback triggers GC; locate the
ResultMap/TGocciaMapValue creation and the SourceArray/AddToGroup loop and apply
the same temp-rooting API used elsewhere in this unit to push ResultMap onto the
GC root stack before the loop and pop it afterwards.
🪄 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: 64f5dd03-6f69-4f43-a3a5-07e686bd4097
📒 Files selected for processing (2)
tests/built-ins/Map/groupBy.jsunits/Goccia.Builtins.GlobalMap.pas
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 79 improved · 🔴 54 regressed · 231 unchanged · avg -0.6% arraybuffer.js — Interp: 🟢 3, 🔴 1, 10 unch. · avg -0.4% · Bytecode: 🟢 12, 🔴 1, 1 unch. · avg +6.2%
arrays.js — Interp: 🟢 2, 🔴 1, 16 unch. · avg +0.9% · Bytecode: 🟢 17, 2 unch. · avg +11.1%
async-await.js — Interp: 🟢 1, 5 unch. · avg +0.7% · Bytecode: 🟢 4, 2 unch. · avg +6.9%
base64.js — Interp: 🟢 1, 9 unch. · avg +0.7% · Bytecode: 🟢 8, 2 unch. · avg +6.5%
classes.js — Interp: 🟢 5, 26 unch. · avg +0.8% · Bytecode: 🟢 13, 🔴 1, 17 unch. · avg +4.5%
closures.js — Interp: 🟢 5, 6 unch. · avg +2.1% · Bytecode: 🟢 11 · avg +10.2%
collections.js — Interp: 🟢 2, 🔴 1, 9 unch. · avg -0.5% · Bytecode: 🟢 12 · avg +14.7%
destructuring.js — Interp: 🟢 8, 14 unch. · avg +1.8% · Bytecode: 🟢 21, 1 unch. · avg +10.9%
fibonacci.js — Interp: 8 unch. · avg +0.7% · Bytecode: 🟢 8 · avg +11.3%
float16array.js — Interp: 🟢 7, 🔴 1, 24 unch. · avg +1.2% · Bytecode: 🟢 27, 🔴 4, 1 unch. · avg +7.4%
for-of.js — Interp: 🟢 3, 4 unch. · avg +2.9% · Bytecode: 🟢 7 · avg +11.4%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 5, 🔴 3, 34 unch. · avg +0.2% · Bytecode: 🟢 36, 6 unch. · avg +7.6%
json.js — Interp: 🟢 3, 🔴 5, 12 unch. · avg -0.8% · Bytecode: 🟢 17, 3 unch. · avg +7.5%
jsx.jsx — Interp: 🟢 3, 🔴 3, 15 unch. · avg +0.4% · Bytecode: 🟢 21 · avg +12.6%
modules.js — Interp: 🟢 1, 🔴 1, 7 unch. · avg -0.9% · Bytecode: 🟢 7, 2 unch. · avg +10.6%
numbers.js — Interp: 🟢 7, 4 unch. · avg +3.2% · Bytecode: 🟢 10, 🔴 1 · avg +12.5%
objects.js — Interp: 🟢 2, 5 unch. · avg +1.3% · Bytecode: 🟢 5, 2 unch. · avg +4.3%
promises.js — Interp: 🟢 1, 🔴 3, 8 unch. · avg -2.2% · Bytecode: 🟢 12 · avg +13.0%
regexp.js — Interp: 🟢 11 · avg +6.4% · Bytecode: 🟢 11 · avg +8.3%
strings.js — Interp: 🟢 7, 🔴 6, 6 unch. · avg -9.3% · Bytecode: 🟢 18, 1 unch. · avg +26.5%
typed-arrays.js — Interp: 🟢 1, 🔴 16, 5 unch. · avg -1.2% · Bytecode: 🟢 15, 🔴 4, 3 unch. · avg +4.9%
uint8array-encoding.js — Interp: 🟢 1, 🔴 13, 4 unch. · avg -14.7% · Bytecode: 🟢 8, 🔴 7, 3 unch. · avg -5.3%
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. |
…ise error assertions - Use Map.FindEntry (SameValueZero) instead of ToStringLiteral comparison for group key lookup, preserving key identity (e.g. 1 vs "1") - Temp-root ResultMap on the array fast path to prevent GC reclamation when the callback triggers collection - Assert TypeError specifically in non-iterable tests instead of bare toThrow() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Builtins.GlobalMap.pas`:
- Around line 150-175: The call to IteratorMethod.Call (via
CallArgs/IteratorObj) can run user code before ResultMap is registered with the
GC, risking collection; move the temp-root registration for ResultMap
(TGarbageCollector.Instance.AddTempRoot(ResultMap)) to occur before invoking
TGocciaFunctionBase(IteratorMethod).Call, so ResultMap is rooted prior to any
user-callable execution, and keep the removal/cleanup in the existing
try/finally that currently surrounds GC roots; ensure you still create/free
CallArgs and preserve logic that inspects IteratorObj (TGocciaIteratorValue /
TGocciaObjectValue) and add Iterator to temp roots after determining it.
🪄 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: 2026c8a2-e0b7-45d8-9292-96626ea6cc4b
📒 Files selected for processing (2)
tests/built-ins/Map/groupBy.jsunits/Goccia.Builtins.GlobalMap.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/built-ins/Map/groupBy.js
…path The IteratorMethod.Call invocation executes user code, which may trigger GC before ResultMap was rooted. Move AddTempRoot(ResultMap) to wrap the entire object-iterator block, nesting the Iterator root inside it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Map.groupByto accept iterable sources, including arrays, sets, maps, strings, and custom iterables.Fixes #310