Bug 234327 – Implement Array.prototype.groupBy and Array.prototype.groupByToMap

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Devin Rousso]

`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]]
```

[Devin Rousso]

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 :)

[Devin Rousso]

Created attachment 447289 [details]
Patch

[Yusuke Suzuki]

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

[Devin Rousso]

Created attachment 447312 [details]
Patch

[Alexey Shvayka]

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.

[Devin Rousso]

Created attachment 447320 [details]
Patch

[Alexey Shvayka]

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.

[Devin Rousso]

Created attachment 447321 [details]
Patch

[Alexey Shvayka]

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.

[Devin Rousso]

Created attachment 447323 [details]
Patch

[Devin Rousso]

Created attachment 447330 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86576923>