1739648 - Implement Array grouping proposal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Yulia Startsev [:yulia]]

This bug is for implementing the Array.prototype.groupBy and Array.prototype.groupByMap proposal.

Comment 1

[Sigurd Setså]

Attachment: Bug 1739648 - implement Array GroupBy; r=yulia

Comment 2

[Rolf Martin Glomsrud]

Attachment: Bug 1739648 - add Map constructor to self hosted GetBuiltinConstructor; r=yulia

Comment 3

[Rolf Martin Glomsrud]

Attachment: Bug 1739648 - implement groupByToMap; r=yulia

Depends on D132791

Comment 4

[Rolf Martin Glomsrud]

Attachment: Bug 1739648 - add groupByToMap test; r=yulia

Depends on D132792

Comment 5

[Rolf Martin Glomsrud]

Attachment: Bug 1739648 - implement groupBy r=arai

Depends on D133870

Comment 6

[Yulia Startsev [:yulia]]

Attachment: Bug 1739648 - enable on Nightly and add tests; r=arai

Depends on D133881

Comment 7

[Yulia Startsev [:yulia]]

Attachment: Bug 1739648 - address test failures due user controlled object.prototype; r=arai

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b555a7d312e1
add Map constructor to self hosted GetBuiltinConstructor; r=yulia
https://hg.mozilla.org/integration/autoland/rev/fd3c8641a135
implement groupByToMap; r=yulia
https://hg.mozilla.org/integration/autoland/rev/45d663b4c79e
add groupByToMap test; r=yulia
https://hg.mozilla.org/integration/autoland/rev/3f6f68e74c50
implement groupBy r=arai
https://hg.mozilla.org/integration/autoland/rev/9473997f9d35
enable on Nightly and add tests; r=arai

Comment 9

[Natalia Csoregi [:nataliaCs]]

Backed out for causing failures on test_xrayToJS.xhtml

• backout: https://hg.mozilla.org/integration/autoland/rev/71853bc0eae7b7ba5599c6538625b73f74e882c7

• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=b-l9ac_yTQ2t1xXBxgJ2vQ.0&revision=9473997f9d3545f9a101815cfa6ecfad49388946

• failure log: https://treeherder.mozilla.org/logviewer?job_id=363261724&repo=autoland&lineNumber=3896

[task 2022-01-06T17:39:44.805Z] 17:39:44     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xhtml | Message correct: Error: Not allowed to define accessor property on [Object] or [Array] XrayWrapper 
[task 2022-01-06T17:39:44.806Z] 17:39:44     INFO - Buffered messages finished
[task 2022-01-06T17:39:44.810Z] 17:39:44     INFO - TEST-UNEXPECTED-FAIL | js/xpconnect/tests/chrome/test_xrayToJS.xhtml | A property on the Array prototype has changed! You need a security audit from an XPConnect peer - got "[\"at\", \"concat\", \"constructor\", \"copyWithin\", \"entries\", \"every\", \"fill\", \"filter\", \"find\", \"findIndex\", \"flat\", \"flatMap\", \"forEach\", \"groupBy\", \"groupByToMap\", \"includes\", \"indexOf\", \"join\", \"keys\", \"lastIndexOf\", \"length\", \"map\", \"pop\", \"push\", \"reduce\", \"reduceRight\", \"reverse\", \"shift\", \"slice\", \"some\", \"sort\", \"splice\", \"toLocaleString\", \"toSource\", \"toString\", \"unshift\", \"values\"]", expected "[\"at\", \"concat\", \"constructor\", \"copyWithin\", \"entries\", \"every\", \"fill\", \"filter\", \"find\", \"findIndex\", \"flat\", \"flatMap\", \"forEach\", \"includes\", \"indexOf\", \"join\", \"keys\", \"lastIndexOf\", \"length\", \"map\", \"pop\", \"push\", \"reduce\", \"reduceRight\", \"reverse\", \"shift\", \"slice\", \"some\", \"sort\", \"splice\", \"toLocaleString\", \"toSource\", \"toString\", \"unshift\", \"values\"]"
[task 2022-01-06T17:39:44.810Z] 17:39:44     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:500:14
[task 2022-01-06T17:39:44.812Z] 17:39:44     INFO - testXray@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xhtml:440:7
[task 2022-01-06T17:39:44.813Z] 17:39:44     INFO - testArray@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xhtml:626:13
[task 2022-01-06T17:39:44.813Z] 17:39:44     INFO - go@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xhtml:163:5
[task 2022-01-06T17:39:44.814Z] 17:39:44     INFO - onload@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xhtml:1:1
[task 2022-01-06T17:39:44.816Z] 17:39:44     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xhtml | A symbol-keyed property on the Array prototype has been changed! You need a security audit from an XPConnect peer 

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e29042ec08b
add Map constructor to self hosted GetBuiltinConstructor; r=yulia
https://hg.mozilla.org/integration/autoland/rev/cacff53512a1
implement groupByToMap; r=yulia
https://hg.mozilla.org/integration/autoland/rev/1de34f637810
add groupByToMap test; r=yulia
https://hg.mozilla.org/integration/autoland/rev/b181e6c65058
implement groupBy r=arai
https://hg.mozilla.org/integration/autoland/rev/e36262888093
enable on Nightly and add tests; r=arai

Comment 11

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/8e29042ec08b
https://hg.mozilla.org/mozilla-central/rev/cacff53512a1
https://hg.mozilla.org/mozilla-central/rev/1de34f637810
https://hg.mozilla.org/mozilla-central/rev/b181e6c65058
https://hg.mozilla.org/mozilla-central/rev/e36262888093

Comment 12

[Yulia Startsev [:yulia]]

Attachment: Bug 1739648 - Add groupby and groupbytomap to xrayToJS only in nightly build

Comment 13

[Phabricator Automation]

Comment on attachment 9258477 [details]
Bug 1739648 - Add groupby and groupbytomap to xrayToJS only in nightly build

Revision D135604 was moved to bug 1749496. Setting attachment 9258477 [details] to obsolete.

Comment 14

[Karl Dubost💡 :karlcow]

Another regression. https://github.com/webcompat/web-bugs/issues/98458

Comment 15

[Hamish Willee]

FF98 nightly docs work for this can be tracked in https://github.com/mdn/content/issues/12579#issuecomment-1038626847

Note that I'm a little unclear why the groupByToMap() is useful. You can use object keys of course, as in the spec, https://github.com/tc39/proposal-array-grouping#proposal-array-grouping
However because Map uses same-value-zero equality. From this table you can see two objects that happen to have the same property names and values are not considered the same.

What that means is that if you want to use an object key you have to keep a hold of that forever, because otherwise you won't be able to get the group out of the map (since you can't recreate the same key). So why is this useful? I can see you might use it to create a group for undefined, which I guess could be handy.

Comment 16

[Yulia Startsev [:yulia]]

The underlying reason this was included as part of the proposal according to the champion is that will be useful with records and tuples. In other words, you can create composite keys without defining an object outside of the function scope.

There are use cases for objects as keys in a map, and it is expected that you will hold a reference to it if you want to access it. You can see this in action here: https://searchfox.org/mozilla-central/rev/94d7c959115c03ea1e9406d6105b36cabe63775d/browser/components/extensions/parent/ext-devtools.js#197,217 -- Objects change over time, so transforming an object into a string representation will result in the key and the actual object getting out of sync -- leading to a loss of access to the values without some synchronization point. Otherwise, you need to do some form of mapping it to some identifier.

When you are using an object as a key in a map, it is because you are mapping related information to that specific object. The same applies to groupByToMap.

Comment 17

[Hamish Willee]

Thanks Yulia.

According to the spec on the callback(s):

If a thisArg parameter is provided, it will be used as the this value for each invocation of callbackfn. If it is not provided, undefined is used instead.

But I if not set, on FF nightly this actually appears to be the window object. The test code (from Will Bamberg here) is:

const inventory = [
  { name: 'asparagus', type: 'vegetables', quantity: 9 },
  { name: 'bananas',  type: 'fruit', quantity: 5 },
  { name: 'goat', type: 'meat', quantity: 23 },
  { name: 'cherries', type: 'fruit', quantity: 12 },
  { name: 'fish', type: 'meat', quantity: 22 }
];

let restock  = { restock: true };
const sufficient = { restock: false };
const result = inventory.groupByToMap( ({ quantity }) => {
  console.log(`it is: ${this}`)
  return quantity < 6 ? restock : sufficient
});
console.log(result.get(restock));

The log result for each item is:

it is: [object Window]

Can you please explain whether we're doing something wrong, or the implementation has a problem?

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Hamish Willee from comment #17)

Can you please explain whether we're doing something wrong, or the implementation has a problem?

1. Arrow functions have a lexical this, so they use the this value of the surrounding code instead of what's passed to the function.
2. If you change it to a non-arrow function, you then also have to use strict-mode ("use strict") or else undefined is turned into the global object when you use this.

Comment 19

[Hamish Willee]

Thanks Jan - a bit late, and FMI only:

I understand that you are saying a function has a particular this by default.
In the case of an arrow function it gets the this for whatever context it is called in, and in the case of another function in non-strict mode it is the global object. In strict mode a non-arrow function has no predefined this so unless you assign one it will be undefined.

The problem is that the spec says none of this - it just says If it is not provided, undefined` is used instead. It is usual for a spec to be so imprecise? I mean should it say something like "if it is not provided the value will follow the normal conventions for the type of function in which it is called"?

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Hamish Willee from comment #19)

The problem is that the spec says none of this - it just says If it is not provided, undefined` is used instead. It is usual for a spec to be so imprecise? I mean should it say something like "if it is not provided the value will follow the normal conventions for the type of function in which it is called"?

The groupBy spec invokes the callback with a certain this-value: either thisArg or undefined.

If the callback is a JS function, we then end up in the [[Call]] operation defined here: https://tc39.es/ecma262/#sec-ecmascript-function-objects-call-thisargument-argumentslist Step 5 there calls OrdinaryCallBindThis, defined here: https://tc39.es/ecma262/#sec-ordinarycallbindthis

In OrdinaryCallBindThis, steps 1-2 handle the arrow function case (lexical this). Steps 5-6 perform the undefined => globalObject conversion for non-strict mode.

So the spec is precise, but invoking a JS function involves some extra machinery that's defined by the [[Call]] operation.

Comment 21

[Hamish Willee]

Thank you Jan. That makes things completely clear!

Comment 22

[hemanth]

Attachment: Bug 1739648: Renaming to group and groupToMap r?yulia

Comment 23

[Phabricator Automation]

Comment on attachment 9280297 [details]
Bug 1739648: Renaming to group and groupToMap r?yulia

Revision D148672 was moved to bug 1773471. Setting attachment 9280297 [details] to obsolete.

Comment 24

[Phabricator Automation]

Comment on attachment 9280297 [details]
Bug 1739648: Renaming to group and groupToMap r?yulia

Revision D148672 was moved to bug 1773471. Setting attachment 9280297 [details] to obsolete.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a9b5179c88c
Renaming to group and groupToMap r=yulia

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8a9b5179c88c