Array.prototype.{every, filter, find, findIndex, forEach, map, some}: Use fresh primitive wrapper f…

Array.prototype.{every, filter, find, findIndex, forEach, map, some}: Use fresh primitive wrapper for calls.

When the receiver is a primitive value, it's cast to an Object before entering the loop. Instead, it should be cast to an Object for each function call while in the loop.

BUG=v8:3536
LOG=Y
R=arv@chromium.org, svenpanne@chromium.org, wingo@igalia.com

Committed: https://code.google.com/p/v8/source/detail?r=24620

[Diego Pino, 2014-09-10 11:22:12]

dpino@igalia.com changed reviewers:
+ aperez@igalia.com, ceyusa@gmail.com, gemont@igalia.com, wingo@igalia.com

[aperez, 2014-09-10 12:25:48]

On 2014/09/10 11:22:12, Diego Pino wrote:
> mailto:dpino@igalia.com changed reviewers:
> + mailto:aperez@igalia.com, mailto:ceyusa@gmail.com, mailto:gemont@igalia.com,
mailto:wingo@igalia.com

There are some cases in which the same kind of modification should be applied.
For
example you have missed it in

[aperez, 2014-09-10 12:28:12]

On 2014/09/10 12:25:48, aperez wrote:
> On 2014/09/10 11:22:12, Diego Pino wrote:
> > mailto:dpino@igalia.com changed reviewers:
> > + mailto:aperez@igalia.com, mailto:ceyusa@gmail.com,
mailto:gemont@igalia.com,
> mailto:wingo@igalia.com
> 
> There are some cases in which the same kind of modification should be applied.
> For example you have missed it in

(Ouch, pressed “Send” button way too early.)

For example you have missed it in “ArrayForEach”, in “src/collections.js”.
Please
double check that the patch modifies all the relevant locations. Grepping for
“%_CallFunction” should help.

[aperez, 2014-09-10 12:30:25]

On 2014/09/10 12:28:12, aperez wrote:
> On 2014/09/10 12:25:48, aperez wrote:
> > On 2014/09/10 11:22:12, Diego Pino wrote:
> > > mailto:dpino@igalia.com changed reviewers:
> > > + mailto:aperez@igalia.com, mailto:ceyusa@gmail.com,
> mailto:gemont@igalia.com,
> > mailto:wingo@igalia.com
> > 
> > There are some cases in which the same kind of modification should be
applied.
> > For example you have missed it in
> 
> (Ouch, pressed “Send” button way too early.)
> 
> For example you have missed it in “ArrayForEach”, in “src/collections.js”.

I mean: ** MapForEach **

[Diego Pino, 2014-09-10 16:22:09]

dpino@igalia.com changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2014-09-12 16:29:17]

https://codereview.chromium.org/553413002/diff/20001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/array.js#newcode1139
src/array.js:1139: if (!IS_NULL_OR_UNDEFINED(receiver) &&
Can this test be moved out of the loop?

https://codereview.chromium.org/553413002/diff/20001/src/array.js#newcode1175
src/array.js:1175: if (!IS_NULL_OR_UNDEFINED(receiver) &&
Maybe we can introduce a helper function/macro for this?

SHOULD_CREATE_WRAPPER(receiver, f)

https://codereview.chromium.org/553413002/diff/20001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/collection.js#newcod...
src/collection.js:118: if (!IS_NULL_OR_UNDEFINED(receiver)) {
Why is this one different? Can you verify with the spec and add a test that
hilights the difference if the spec requires a different semantic?

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/array-itera...
File test/mjsunit/array-iteration.js (right):

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/array-itera...
test/mjsunit/array-iteration.js:72: a = new Array();
nit

a = [];

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/array-itera...
test/mjsunit/array-iteration.js:73: [1,2].filter(function() { a.push(this) },
"");
nit, space after comma

[mathias, 2014-09-13 04:08:34]

Why do we need the `!IS_SPEC_OBJECT(receiver) && %IsSloppyModeFunction(f)`
checks in the first place? I can’t find it in the spec.

If we don’t end up needing it, we could just use `var new_thisArg =
TO_OBJECT_INLINE(receiver);`.

[arv (Not doing code reviews), 2014-09-14 16:31:09]

On 2014/09/13 04:08:34, mathias wrote:
> Why do we need the `!IS_SPEC_OBJECT(receiver) && %IsSloppyModeFunction(f)`
> checks in the first place? I can’t find it in the spec.
> 
> If we don’t end up needing it, we could just use `var new_thisArg =
> TO_OBJECT_INLINE(receiver);`.

Beacause %_CallFunction does not handle the wrapping of primitives for sloppy
functions.

[wingo, 2014-09-15 09:12:21]

https://codereview.chromium.org/553413002/diff/20001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/collection.js#newcod...
src/collection.js:268: if (!IS_NULL_OR_UNDEFINED(receiver)) {
Same comment here as Erik's comment above.

https://codereview.chromium.org/553413002/diff/20001/src/harmony-array.js
File src/harmony-array.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/harmony-array.js#new...
src/harmony-array.js:36: var new_thisArg = thisArg;
Call it "receiver", or something that doesn't have this_awfulStyle :)  Same
comments about factoring out the test to some kind of macro.

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/array-itera...
File test/mjsunit/array-iteration.js (right):

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/array-itera...
test/mjsunit/array-iteration.js:76: // Do not create a new object in each
function call when receiver is a primitive value.
"when receiver is not a primitive value" here, and below

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-find.js (right):

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-find.js:246: // Do not create a new object in each
function call when receiver is a primitive value.
80 column limit.

[Diego Pino, 2014-09-16 10:08:29]

dpino@igalia.com changed reviewers:
- aperez@igalia.com, ceyusa@gmail.com, gemont@igalia.com

[Diego Pino, 2014-09-16 10:45:46]

https://codereview.chromium.org/553413002/diff/20001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/array.js#newcode1139
src/array.js:1139: if (!IS_NULL_OR_UNDEFINED(receiver) &&
On 2014/09/12 16:29:17, arv wrote:
> Can this test be moved out of the loop?

In strict mode, GetDefaultReceiver can return 'undefined'. ToObject throws an
exception if the argument is undefined, so I need to avoid that case.

https://codereview.chromium.org/553413002/diff/20001/src/array.js#newcode1175
src/array.js:1175: if (!IS_NULL_OR_UNDEFINED(receiver) &&
On 2014/09/12 16:29:17, arv wrote:
> Maybe we can introduce a helper function/macro for this?
> 
> SHOULD_CREATE_WRAPPER(receiver, f)

Acknowledged.

https://codereview.chromium.org/553413002/diff/20001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/collection.js#newcod...
src/collection.js:118: if (!IS_NULL_OR_UNDEFINED(receiver)) {
On 2014/09/12 16:29:17, arv wrote:
> Why is this one different? Can you verify with the spec and add a test that
> hilights the difference if the spec requires a different semantic?

I tried to preserve the semantics of the old code. There was no
!IS_SPEC_OBJECT(receiver) && %IsSloppyModeFunction(f) checking, so I didn't add
it.

The reason of IsSloppyModeFunction(f) is that according to the ECMA-262, Annex C
(The strict mode): "If this is evaluated within strict mode code, then the this
value is not coerced to an object". Currently, this is not being coerced to an
object (that's what the patch fixes :)) but it shouldn't do it if the function
is in strict mode, so MapForEach and SetForEach need this checking too.

https://codereview.chromium.org/553413002/diff/20001/src/harmony-array.js
File src/harmony-array.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/harmony-array.js#new...
src/harmony-array.js:36: var new_thisArg = thisArg;
On 2014/09/15 09:12:21, wingo wrote:
> Call it "receiver", or something that doesn't have this_awfulStyle :)  Same
> comments about factoring out the test to some kind of macro.

Acknowledged.

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/array-itera...
File test/mjsunit/array-iteration.js (right):

https://codereview.chromium.org/553413002/diff/20001/test/mjsunit/array-itera...
test/mjsunit/array-iteration.js:76: // Do not create a new object in each
function call when receiver is a primitive value.
On 2014/09/15 09:12:21, wingo wrote:
> "when receiver is not a primitive value" here, and below

Acknowledged.

[wingo, 2014-09-16 12:04:04]

https://codereview.chromium.org/553413002/diff/20001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/553413002/diff/20001/src/array.js#newcode1139
src/array.js:1139: if (!IS_NULL_OR_UNDEFINED(receiver) &&
On 2014/09/16 10:45:46, Diego Pino wrote:
> On 2014/09/12 16:29:17, arv wrote:
> > Can this test be moved out of the loop?
> 
> In strict mode, GetDefaultReceiver can return 'undefined'. ToObject throws an
> exception if the argument is undefined, so I need to avoid that case.

I think Erik was referring to the test, not the body of the test.

e.g.

var needs_wrapper = ...;

for () {
  var new_receiver = needs_wrapper ? ToObject(receiver) : receiver;
  ...
}

[wingo, 2014-09-16 12:06:20]

On 2014/09/16 10:45:46, Diego Pino wrote:
> The reason of IsSloppyModeFunction(f) is that according to the ECMA-262, Annex
C
> (The strict mode): 

Best to refer to the latest draft for these things, with a section link;
http://people.mozilla.org/~jorendorff/es6-draft.html.

[wingo, 2014-09-23 18:39:02]

Looking good, just a few comments

https://codereview.chromium.org/553413002/diff/40001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/553413002/diff/40001/src/array.js#newcode1413
src/array.js:1413: var element = array[i];
what happened to the ToObject that was here in the previous patch?

https://codereview.chromium.org/553413002/diff/40001/src/array.js#newcode1450
src/array.js:1450: var element = array[i];
same

https://codereview.chromium.org/553413002/diff/40001/src/harmony-array.js
File src/harmony-array.js (right):

https://codereview.chromium.org/553413002/diff/40001/src/harmony-array.js#new...
src/harmony-array.js:14: function ArrayFind(predicate /* receiver */) {  //
length == 1
So in the last patch I wanted to avoid new_thisArg (mixed underscores and
camelcase).  I suggested a fix to replace new_thisArg by receiver, though
perhaps it was unclear.  Now in this patch you changed the thisArg name to
receiver, which probably isn't the right thing, as the spec calls it thisArg. 
Please change it back; the only thing that really needs renaming was your
new_thisArg.  You could call it receiver or newThisArg or anything that keeps
the style :)

https://codereview.chromium.org/553413002/diff/40001/test/mjsunit/array-itera...
File test/mjsunit/array-iteration.js (right):

https://codereview.chromium.org/553413002/diff/40001/test/mjsunit/array-itera...
test/mjsunit/array-iteration.js:71: // When evaluating 'this', primitive values
should be coerced to an object.
previous comment was more correct -- the wrapper happens when determining the
receiver for the function call, not when evaluating this within the function.

https://codereview.chromium.org/553413002/diff/40001/test/mjsunit/array-itera...
test/mjsunit/array-iteration.js:85: [1, 2].filter(function() { 'use strict';
a.push(this); }, "");
great test.

[Diego Pino, 2014-09-24 10:13:01]

https://codereview.chromium.org/553413002/diff/40001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/553413002/diff/40001/src/array.js#newcode1413
src/array.js:1413: var element = array[i];
On 2014/09/23 18:39:01, wingo wrote:
> what happened to the ToObject that was here in the previous patch?

I think it doesn't apply in this case or at least I couldn't code a test case.

receiver is initialized from callback, and callback must be a function. So, it's
not possible that receiver will be a primitive value.

https://codereview.chromium.org/553413002/diff/40001/src/harmony-array.js
File src/harmony-array.js (right):

https://codereview.chromium.org/553413002/diff/40001/src/harmony-array.js#new...
src/harmony-array.js:14: function ArrayFind(predicate /* receiver */) {  //
length == 1
On 2014/09/23 18:39:01, wingo wrote:
> So in the last patch I wanted to avoid new_thisArg (mixed underscores and
> camelcase).  I suggested a fix to replace new_thisArg by receiver, though
> perhaps it was unclear.  Now in this patch you changed the thisArg name to
> receiver, which probably isn't the right thing, as the spec calls it thisArg. 
> Please change it back; the only thing that really needs renaming was your
> new_thisArg.  You could call it receiver or newThisArg or anything that keeps
> the style :)

OK, understood.

It's worth noticing that array functions 'filter', 'forEach', 'map', 'same' and
'every', call this parameter 'receiver' although the spec also names it
'thisArg'. Maybe at some point is worth to do a refactoring and use the same
name for all functions. For the moment I agree with not changing the name of
this parameter.

https://codereview.chromium.org/553413002/diff/40001/test/mjsunit/array-itera...
File test/mjsunit/array-iteration.js (right):

https://codereview.chromium.org/553413002/diff/40001/test/mjsunit/array-itera...
test/mjsunit/array-iteration.js:71: // When evaluating 'this', primitive values
should be coerced to an object.
On 2014/09/23 18:39:01, wingo wrote:
> previous comment was more correct -- the wrapper happens when determining the
> receiver for the function call, not when evaluating this within the function.

Acknowledged.

[wingo, 2014-09-30 16:01:36]

Some comments.

https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js
File src/harmony-array.js (right):

https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js#new...
src/harmony-array.js:30: thisArg = %GetDefaultthisArg(predicate) || thisArg;
Typo.  Did these tests pass for you? :)

https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js#new...
src/harmony-array.js:32: var needs_wrapper = SHOULD_CREATE_WRAPPER(predicate,
thisArg);
Here we are adding a test; SHOULD_CREATE_WRAPPER looks like this:

 macro SHOULD_CREATE_WRAPPER(functionName, receiver) =
(!IS_NULL_OR_UNDEFINED(rec
     eiver) && !IS_SPEC_OBJECT(receiver) &&
%IsSloppyModeFunction(functionName));

so we're adding the duplicate IS_NULL_OR_UNDEFINED check, and if the thisArg was
initialized in the previous if to the global object then we also go to the
!IS_SPEC_OBJECT case.  Probably this needs_wrapper initialization should be in
an else{}.

https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js#new...
src/harmony-array.js:67: var needs_wrapper = SHOULD_CREATE_WRAPPER(predicate,
thisArg);
same here and in similar places in the patch

https://codereview.chromium.org/553413002/diff/60001/test/mjsunit/es6/collect...
File test/mjsunit/es6/collections.js (right):

https://codereview.chromium.org/553413002/diff/60001/test/mjsunit/es6/collect...
test/mjsunit/es6/collections.js:698: // Create a new object in each function
call when receiver is a primitive value.
80 characters here and in other places in the tests

[Diego Pino, 2014-09-30 16:35:15]

Uploaded new patch. PTAL.

On 2014/09/30 16:01:36, wingo wrote:
> Some comments.
> 
> https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js
> File src/harmony-array.js (right):
> 
>
https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js#new...
> src/harmony-array.js:30: thisArg = %GetDefaultthisArg(predicate) || thisArg;
> Typo.  Did these tests pass for you? :)
> 

Actually not :) Now I doubled checked test were passing and launched a git cl
try.

>
https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js#new...
> src/harmony-array.js:32: var needs_wrapper = SHOULD_CREATE_WRAPPER(predicate,
> thisArg);
> Here we are adding a test; SHOULD_CREATE_WRAPPER looks like this:
> 
>  macro SHOULD_CREATE_WRAPPER(functionName, receiver) =
> (!IS_NULL_OR_UNDEFINED(rec
>      eiver) && !IS_SPEC_OBJECT(receiver) &&
> %IsSloppyModeFunction(functionName));
> 
> so we're adding the duplicate IS_NULL_OR_UNDEFINED check, and if the thisArg
was
> initialized in the previous if to the global object then we also go to the
> !IS_SPEC_OBJECT case.  Probably this needs_wrapper initialization should be in
> an else{}.
> 
>
https://codereview.chromium.org/553413002/diff/60001/src/harmony-array.js#new...
> src/harmony-array.js:67: var needs_wrapper = SHOULD_CREATE_WRAPPER(predicate,
> thisArg);
> same here and in similar places in the patch
> 

Right, the result of GetDefaultReceiver is either undefined (strict mode) or an
object, in neither case will be necessary to create an object (needs_wrapper
will be false).

>
https://codereview.chromium.org/553413002/diff/60001/test/mjsunit/es6/collect...
> File test/mjsunit/es6/collections.js (right):
> 
>
https://codereview.chromium.org/553413002/diff/60001/test/mjsunit/es6/collect...
> test/mjsunit/es6/collections.js:698: // Create a new object in each function
> call when receiver is a primitive value.
> 80 characters here and in other places in the tests

Acknowledge.

[wingo, 2014-09-30 16:41:12]

One nit -- can you recheck that tests pass after fixing?

https://codereview.chromium.org/553413002/diff/80001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/553413002/diff/80001/src/macros.py#newcode170
src/macros.py:170: macro SHOULD_CREATE_WRAPPER(functionName, receiver) =
(!IS_NULL_OR_UNDEFINED(receiver) && !IS_SPEC_OBJECT(receiver) &&
%IsSloppyModeFunction(functionName));
At this point IS_NULL_OR_UNDEFINED is unneeded here, no?

[Diego Pino, 2014-09-30 16:45:18]

On 2014/09/30 16:41:12, wingo wrote:
> One nit -- can you recheck that tests pass after fixing?
> 
> https://codereview.chromium.org/553413002/diff/80001/src/macros.py
> File src/macros.py (right):
> 
> https://codereview.chromium.org/553413002/diff/80001/src/macros.py#newcode170
> src/macros.py:170: macro SHOULD_CREATE_WRAPPER(functionName, receiver) =
> (!IS_NULL_OR_UNDEFINED(receiver) && !IS_SPEC_OBJECT(receiver) &&
> %IsSloppyModeFunction(functionName));
> At this point IS_NULL_OR_UNDEFINED is unneeded here, no?

Fixed.

[wingo, 2014-09-30 16:46:06]

LGTM, please add an OWNER

[Diego Pino, 2014-09-30 16:49:46]

dpino@igalia.com changed reviewers:
+ svenpanne@chromium.org

[Diego Pino, 2014-09-30 16:49:46]

Could you please take a look? Thanks :)

[Diego Pino, 2014-10-12 19:18:59]

Sven Panne: "git cl owner" suggested me you as an owner. The issue is already
reviewed by non-owners. Could you please take a look? Thanks :)

[Sven Panne, 2014-10-14 10:28:51]

On 2014/10/12 19:18:59, Diego Pino wrote:
> Sven Panne: "git cl owner" suggested me you as an owner. The issue is already
> reviewed by non-owners. Could you please take a look? Thanks :)

@Erik: Is the CL OK in its current state?

[arv (Not doing code reviews), 2014-10-14 15:55:33]

LGTM with nits

https://codereview.chromium.org/553413002/diff/100001/test/mjsunit/array-iter...
File test/mjsunit/array-iteration.js (right):

https://codereview.chromium.org/553413002/diff/100001/test/mjsunit/array-iter...
test/mjsunit/array-iteration.js:80: assertFalse(a[0] !== a[1]);
assertEquals

https://codereview.chromium.org/553413002/diff/100001/test/mjsunit/array-iter...
test/mjsunit/array-iteration.js:85: assertTrue(a[0] === "" && a[0] === a[1]);
assertEquals(a[0], a[1]);
assertEquals(a[0], "");

https://codereview.chromium.org/553413002/diff/100001/test/mjsunit/array-iter...
test/mjsunit/array-iteration.js:137: assertFalse(a[0] !== a[1]);
same here and elsewhere.

[Sven Panne, 2014-10-15 06:11:52]

LGTM (rubberstamped) when nits are addressed

[Diego Pino, 2014-10-15 07:48:27]

Nits fixed in last patch.

[Sven Panne, 2014-10-15 09:04:23]

Landing it for you...

[Sven Panne, 2014-10-15 09:11:47]

Committed patchset #7 (id:130001) manually as 24620 (presubmit successful).