Bug 234327 - Implement Array.prototype.groupBy and Array.prototype.groupByToMap
Summary: Implement Array.prototype.groupBy and Array.prototype.groupByToMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-14 17:47 PST by Devin Rousso
Modified: 2021-12-16 08:12 PST (History)
12 users (show)

See Also:


Attachments
[Patch] WIP (10.03 KB, patch)
2021-12-15 10:10 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (54.12 KB, patch)
2021-12-15 15:24 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.07 KB, patch)
2021-12-15 19:01 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (25.51 KB, patch)
2021-12-15 19:54 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (25.45 KB, patch)
2021-12-15 20:03 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (25.63 KB, patch)
2021-12-15 20:13 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (28.33 KB, patch)
2021-12-15 22:58 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-12-14 17:47:00 PST
`Array.prototype.groupBy` and `Array.prototype.groupByToMap` just advanced to TC39 stage 3

Proposal: <https://github.com/tc39/proposal-array-find-from-last>
Spec: <https://tc39.es/proposal-array-find-from-last/index.html>

```
const array = [1, 2, 3, 4, 5];

array.groupBy(n => n % 2 ? "odd" : "even") // { odd: [1, 3, 5], even: [2, 4] }
array.groupByToMap(n => n % 2 ? "odd" : "even") // Map [["odd", [1, 3, 5]], ["even", [2, 4]]
```
Comment 1 Devin Rousso 2021-12-15 10:10:35 PST
Created attachment 447253 [details]
[Patch] WIP

I am putting this up so that I can let EWS take a stab at it while I write some specific tests :)
Comment 2 Devin Rousso 2021-12-15 15:24:05 PST
Created attachment 447289 [details]
Patch
Comment 3 Yusuke Suzuki 2021-12-15 15:39:19 PST
Comment on attachment 447289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447289&action=review

r=me

> Source/JavaScriptCore/builtins/ArrayPrototype.js:192
> +        var key = callback.@call(thisArg, value, i, array);

Can you include a test, returning "__proto__" for key string?
Like,

Array.prototype.ok = 42; // enumerable.
var result = [......].groupByToMap((value, index, array) {
    return "__proto__";
});
result.has("ok"); // false
Comment 4 Devin Rousso 2021-12-15 19:01:10 PST
Created attachment 447312 [details]
Patch
Comment 5 Alexey Shvayka 2021-12-15 19:13:06 PST
Comment on attachment 447289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447289&action=review

> Source/JavaScriptCore/builtins/ArrayPrototype.js:193
> +        var group = (groups[key] ??= []);

You've probably already noticed this, but I comment either way: ToPropertyKey that is performed by `groups[key]` is not in the spec (unlike Array.prototype.groupBy), and rightfully so because Maps can have keys beyond strings & symbols.
Considering step 6.d, I would suggest `var groups = new @Map` since it's how Maps sanitise keys as well.
Also, it would be nice to ensure we have test cases with throwing toString() / valueOf() and -0 / 0 keys.

The spec I'm looking at is https://tc39.es/proposal-array-grouping.
Comment 6 Devin Rousso 2021-12-15 19:54:39 PST
Created attachment 447320 [details]
Patch
Comment 7 Alexey Shvayka 2021-12-15 20:01:02 PST
Comment on attachment 447320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447320&action=review

> Source/JavaScriptCore/builtins/ArrayPrototype.js:200
> +        if (key === -0)

This is extra before Map.prototype.set, which performs -0 => +0 coercion at step 5 (https://tc39.es/ecma262/#sec-map.prototype.set).
I'm pretty sure we can just pass `key` directly to Map's @get / @set.
Comment 8 Devin Rousso 2021-12-15 20:03:19 PST
Created attachment 447321 [details]
Patch
Comment 9 Alexey Shvayka 2021-12-15 20:04:42 PST
Comment on attachment 447321 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447321&action=review

> Source/JavaScriptCore/builtins/ArrayPrototype.js:173
> +        var group = groups[key];

Aren't we performing ToPropertyKey twice here (the second time @ L176)?
A test with counter would be hugely appreciated.
Comment 10 Devin Rousso 2021-12-15 20:13:25 PST
Created attachment 447323 [details]
Patch
Comment 11 Devin Rousso 2021-12-15 22:58:05 PST
Created attachment 447330 [details]
Patch
Comment 12 EWS 2021-12-16 08:11:24 PST
Committed r287136 (245320@main): <https://commits.webkit.org/245320@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447330 [details].
Comment 13 Radar WebKit Bug Importer 2021-12-16 08:12:18 PST
<rdar://problem/86576923>