Implement ES6 Array.of()

Implement ES6 Array.of()

BUG=v8:3427
LOG=Y

[Diego Pino, 2014-07-07 16:37:39]

PTAL

[caitp (gmail), 2014-07-07 16:52:09]

Just one comment, it turns out ---
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdatapropertyo...
(which defers to
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdataproperty)
says that the new property descriptor for the created property should have
{[[Value]]: V, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]:
true}. So don't add the DONT_ENUM flag, I think, just `newArray[k] =
%_Arguments(k);` should be good enough, I think.

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

https://codereview.chromium.org/364853009/diff/1/src/harmony-array.js#newcode136
src/harmony-array.js:136: %AddProperty(A, k, %_Arguments(k), DONT_ENUM);
Should this not be enumerable? I don't see anything in CreateDataPropertyOrThrow
that says this shouldn't be enumerable

[Diego Pino, 2014-07-07 17:23:44]

On 2014/07/07 16:52:09, caitp wrote:
> Just one comment, it turns out ---
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdatapropertyo...
> (which defers to
> https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdataproperty)
> says that the new property descriptor for the created property should have
> {[[Value]]: V, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]:
> true}. So don't add the DONT_ENUM flag, I think, just `newArray[k] =
> %_Arguments(k);` should be good enough, I think.
> 
> https://codereview.chromium.org/364853009/diff/1/src/harmony-array.js
> File src/harmony-array.js (right):
> 
>
https://codereview.chromium.org/364853009/diff/1/src/harmony-array.js#newcode136
> src/harmony-array.js:136: %AddProperty(A, k, %_Arguments(k), DONT_ENUM);
> Should this not be enumerable? I don't see anything in
CreateDataPropertyOrThrow
> that says this shouldn't be enumerable

Thanks for the review.

Definitively the DONT_ENUM flag is not correct, thanks for pointing that out.

Now with regard to whether to do the assignment using AddProperty(A, k,
%_Arguments(k)) or A[k] = %_Arguments, the main different is that the former is
(if I'm correct) semantically the same as DefineProperty and the latter is a
SetProperty (or CreateDataProperty), which is what the spec says should be used.

I tried to keep the tests imported from Firefox correct. The array-of-4.js test
validates that the array assignment do not trigger a setter. Since AddProperty
do not trigger a setter, the test passes, and it fails if I use (A[k] =
%_Arguments), however I think it is what it should be used.

I'm a little bit lost here. If (A[k] = %_Arguments) is semantically the same
operation as CreateDataProperty I think the array-of-4.js test should be
removed.

[caitp (gmail), 2014-07-07 17:30:36]

On 2014/07/07 17:23:44, Diego Pino wrote:
> On 2014/07/07 16:52:09, caitp wrote:
> > Just one comment, it turns out ---
> >
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdatapropertyo...
> > (which defers to
> >
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdataproperty)
> > says that the new property descriptor for the created property should have
> > {[[Value]]: V, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]:
> > true}. So don't add the DONT_ENUM flag, I think, just `newArray[k] =
> > %_Arguments(k);` should be good enough, I think.
> > 
> > https://codereview.chromium.org/364853009/diff/1/src/harmony-array.js
> > File src/harmony-array.js (right):
> > 
> >
>
https://codereview.chromium.org/364853009/diff/1/src/harmony-array.js#newcode136
> > src/harmony-array.js:136: %AddProperty(A, k, %_Arguments(k), DONT_ENUM);
> > Should this not be enumerable? I don't see anything in
> CreateDataPropertyOrThrow
> > that says this shouldn't be enumerable
> 
> Thanks for the review.
> 
> Definitively the DONT_ENUM flag is not correct, thanks for pointing that out.
> 
> Now with regard to whether to do the assignment using AddProperty(A, k,
> %_Arguments(k)) or A[k] = %_Arguments, the main different is that the former
is
> (if I'm correct) semantically the same as DefineProperty and the latter is a
> SetProperty (or CreateDataProperty), which is what the spec says should be
used.
> 
> I tried to keep the tests imported from Firefox correct. The array-of-4.js
test
> validates that the array assignment do not trigger a setter. Since AddProperty
> do not trigger a setter, the test passes, and it fails if I use (A[k] =
> %_Arguments), however I think it is what it should be used.
> 
> I'm a little bit lost here. If (A[k] = %_Arguments) is semantically the same
> operation as CreateDataProperty I think the array-of-4.js test should be
> removed.

Sorry, I didn't mean that we should just use `A[k] = %_Arguments[k]`, I only
meant that simple assignment should give us the property descriptor that we
expect to have.

We still need to throw if the property can't be added, and so on, so the runtime
routines are still want we want to use.

[rossberg, 2014-07-08 08:39:17]

https://codereview.chromium.org/364853009/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/364853009/diff/1/src/runtime.cc#newcode7807
src/runtime.cc:7807: RUNTIME_FUNCTION(Runtime_IsConstructor) {
Drive-by-comment: Unfortunately, I don't think this function is quite correct.
For example, it does not handle function proxies.

[rossberg, 2014-07-08 09:59:09]

https://codereview.chromium.org/364853009/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/364853009/diff/1/src/runtime.cc#newcode7807
src/runtime.cc:7807: RUNTIME_FUNCTION(Runtime_IsConstructor) {
On 2014/07/08 08:39:17, rossberg wrote:
> Drive-by-comment: Unfortunately, I don't think this function is quite correct.
> For example, it does not handle function proxies.

And more importantly, for JSFunctions it is testing whether they _have_ a
constructor, not whether they _are_ one -- completely different thing
(GetConstructor will normally return Function.prototype in this case).

So, this whole function is completely wrong. Unfortunately, it's not so simple
to implement it correctly either. You can have a look at Runtime_NewObjectHelper
in runtime.cc to get an idea. But if we actually need such a predicate, then the
condition used there should be properly refactored. For now, I would prefer just
to fall back to IS_SPEC_FUNCTION.

[caitp (gmail), 2014-07-14 18:54:33]

So just a question, are we blocked on filling out the rest of the harmony
standard library additions until we have a proper IsConstructor() routine? I
feel like these could do to land and fix IsConstructor() later / give people a
chance to eat the dogfood and see what if anything is wrong with the current
implementations.

If we are blocked on that, I'll look at writing a separate CL to implement a
more "correct" IsConstructor()

[rossberg, 2014-07-16 09:52:15]

On 2014/07/14 18:54:33, caitp wrote:
> So just a question, are we blocked on filling out the rest of the harmony
> standard library additions until we have a proper IsConstructor() routine? I
> feel like these could do to land and fix IsConstructor() later / give people a
> chance to eat the dogfood and see what if anything is wrong with the current
> implementations.
> 
> If we are blocked on that, I'll look at writing a separate CL to implement a
> more "correct" IsConstructor()

No, no need to block on that. Just use IS_SPEC_FUNCTION for the time being, and
add a suitable TODO.

[Diego Pino, 2014-07-21 11:23:09]

On 2014/07/16 09:52:15, rossberg wrote:

> No, no need to block on that. Just use IS_SPEC_FUNCTION for the time being,
and
> add a suitable TODO.

Included a TODO comment (TODO: Implement IsConstructor (ES6 section 7.2.5)). If
everything is correct, I think the patch should be ready to land.

I run the trybots, but some are failing. It seems that the problem is that when
trying to run the tests, the function Array.of() is undefined. The tests have
the flag harmony-arrays set, and I can run them in my local machine (Linux
x86_64). Any hint with regard to the trybots appreciated.

[rossberg, 2014-07-21 16:07:07]

Can you please merge all the tests into one file?

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

https://codereview.chromium.org/364853009/diff/60001/src/harmony-array.js#new...
src/harmony-array.js:132: var A = new C(len);
Please don't make use of var scoping like that. Preferred:

  var A = IS_SPEC_FUNCTION(C) ? new C(len) : []

Also, please use more descriptive names than A and C.

https://codereview.chromium.org/364853009/diff/60001/src/harmony-array.js#new...
src/harmony-array.js:137: %AddProperty(A, k, %_Arguments(k));
Unfortunately, this runtime function has recently been removed (or rather,
specialised to AddNamedProperty). You probably have to implement your own
runtime function AddElement.

https://codereview.chromium.org/364853009/diff/60001/test/mjsunit/es6/array-o...
File test/mjsunit/es6/array-of-length-setter-2.js (right):

https://codereview.chromium.org/364853009/diff/60001/test/mjsunit/es6/array-o...
test/mjsunit/es6/array-of-length-setter-2.js:10: // Array.of does a strict
assignment to the new object's .length.
Hm, of is never called in this test?

[Diego Pino, 2014-07-23 11:47:29]

On 2014/07/21 16:07:07, rossberg wrote:
> Can you please merge all the tests into one file?
>

Done.
 
> https://codereview.chromium.org/364853009/diff/60001/src/harmony-array.js
> File src/harmony-array.js (right):
> 
>
https://codereview.chromium.org/364853009/diff/60001/src/harmony-array.js#new...
> src/harmony-array.js:132: var A = new C(len);
> Please don't make use of var scoping like that. Preferred:
> 
>   var A = IS_SPEC_FUNCTION(C) ? new C(len) : []
> 
> Also, please use more descriptive names than A and C.
> 

OK.

>
https://codereview.chromium.org/364853009/diff/60001/src/harmony-array.js#new...
> src/harmony-array.js:137: %AddProperty(A, k, %_Arguments(k));
> Unfortunately, this runtime function has recently been removed (or rather,
> specialised to AddNamedProperty). You probably have to implement your own
> runtime function AddElement.
>

I provided an implementation for AddElement. It sets an element into an array
object using the flag DEFINE_PROPERTY. The spec says that the way of doing the
array assignment is by using the CreateDataPropertyOrThrow operation (instead of
Put(O, Pk, value, true), which normally maps to an array assignment). Assigning
the elements to the new array this way, doesn't trigger a setter.
 
>
https://codereview.chromium.org/364853009/diff/60001/test/mjsunit/es6/array-o...
> File test/mjsunit/es6/array-of-length-setter-2.js (right):
> 
>
https://codereview.chromium.org/364853009/diff/60001/test/mjsunit/es6/array-o...
> test/mjsunit/es6/array-of-length-setter-2.js:10: // Array.of does a strict
> assignment to the new object's .length.
> Hm, of is never called in this test?

My fault, I slipped the of() call. Calling Empty.of() in this test should launch
an exception.

[rossberg, 2014-07-23 12:19:14]

LGTM, with one nit: please rename array-of-tests.js to just array-of.js, and
move it to the ../harmony directory (which contains tests for all Harmony
features not staged yet).

> I provided an implementation for AddElement. It sets an element into an array
> object using the flag DEFINE_PROPERTY. The spec says that the way of doing the
> array assignment is by using the CreateDataPropertyOrThrow operation (instead
of
> Put(O, Pk, value, true), which normally maps to an array assignment).
Assigning
> the elements to the new array this way, doesn't trigger a setter.

Yes, that should be the right thing.

[Diego Pino, 2014-07-29 15:30:42]

I think the patch is ready to land now. All try bots are green except
'v8_linux_layout_dbg' which seems to be flaky. The bot fails with and without
the patch. I don't know if there's something on my side I could do to fix it.

[caitp (gmail), 2014-08-05 02:09:07]

I'm not an owner, so take this with a grain of salt:

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

https://codereview.chromium.org/364853009/diff/260001/src/harmony-array.js#ne...
src/harmony-array.js:133: %AddElement(array, i, %_Arguments(i), NONE);
My review doesn't mean much here, but I'm not sure I see why we need
Runtime_AddElement:

1. It seems to be a wrapper for CreateDataProperty except that we limit the
attributes for the property to enumerable (or not), writable (or not), and
whatever we get from DONT_DELETE. Reading it, it doesn't look like we throw if
we can't successfully create the data property.

2. It's not totally clear to me what the authors of the spec actually want,
because it's written to be as obtuse as possible, apparently. But at a glance it
looks like what we actually want to do is try to define the property with the
default property descriptor flags, and throw if we can't. (This might be
entirely wrong, and I would be frankly surprised if anyone, including the
authors of the draft, can walk away from this writing and come back after 2
weeks correctly interpreting what they've written).

3. I don't really see how Runtime_AddElement really provides any of the abstract
methods described in the draft, and having that clearly explained in comments
would be really, really valuable. I can't look at this implementation, and then
look at the spec, and make the connection. I'm just not seeing it. It's not that
I don't trust that it's correct (since rossberg@ has already vetted it), but I'd
prefer it if the correctness of it to be explained in the tree, in comment form,
if not by matching the language in the draft.

I would be very happy if, at the very least, this runtime function, and how it
is accomplishing what is asked for in 22.1.2.3.8c, were explained clearly in
comments.

Also, nit: the indentation in ArrayOf is inconsistent with the rest of v8
(despite being consistent with Blink and Chromium). I don't care a whole lot,
but it is what it is.

[caitp (gmail), 2014-08-05 02:11:14]

For what it's worth, if AddElement is in some way a useful implementation of
CreateDataPropertyOrThrow, we'd have uses for it elsewhere, so the connection
should be more clearly explained.

[Diego Pino, 2014-08-08 12:22:50]

Thanks caitp for your comments, I think they are very valid and I also have
doubts. I comment below.

On 2014/08/05 02:09:07, caitp wrote:
> I'm not an owner, so take this with a grain of salt:
> 
> https://codereview.chromium.org/364853009/diff/260001/src/harmony-array.js
> File src/harmony-array.js (right):
> 
>
https://codereview.chromium.org/364853009/diff/260001/src/harmony-array.js#ne...
> src/harmony-array.js:133: %AddElement(array, i, %_Arguments(i), NONE);
> My review doesn't mean much here, but I'm not sure I see why we need
> Runtime_AddElement:
> 
> 1. It seems to be a wrapper for CreateDataProperty except that we limit the
> attributes for the property to enumerable (or not), writable (or not), and
> whatever we get from DONT_DELETE. Reading it, it doesn't look like we throw if
> we can't successfully create the data property.
> 

The CreateDataProperty spec says that the PropertyDescriptor should be
{writable: true, enumerable: true and cofigurable: true}. That translates to
attributes "None" in V8. I let AddElement receive customized attributes, and
pass "None" in the JavaScript call. I can change the implementation of
AddElement and set the attributes always to "None" so it's a more equivalent
implementation to CreateDataProperty.

My understanding is that DONT_DELETE maps to "configurable: false" according to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
 (configurable: true if and only if the type of this property descriptor may be
changed and if the property may be deleted from the corresponding object).

The spec says that if the property could not be successfully created a TypeError
exception is launched. More precisely, the spec describes the following steps:

3. Let success be CreateDataProperty(O, P, V).
4. ReturnIfAbrupt(success).
5. If success is false, then throw a TypeError exception.

I followed the implementation of similar methods. For instance, the spec for
DeletePropertyOrThrow describes these steps:

3. Let success be the result of calling the Delete internal method of O passing
P as the argument.
4. ReturnIfAbrupt(success).
5. If success is false, then throw a TypeError exception.

However, the implementation of Runtime_DeleteProperty  doesn't launch a
TypeError exception, it ends with a call to
"ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result,
JSReceiver::DeleteProperty(object, key, delete_mode));". 

My understanding is that wrapping the JSObject::SetElement call within
ASSIGN_RETURN_FAILURE_ON_EXCEPTION is enough to cover these steps, but maybe I'm
wrong.

> 2. It's not totally clear to me what the authors of the spec actually want,
> because it's written to be as obtuse as possible, apparently. But at a glance
it
> looks like what we actually want to do is try to define the property with the
> default property descriptor flags, and throw if we can't. (This might be
> entirely wrong, and I would be frankly surprised if anyone, including the
> authors of the draft, can walk away from this writing and come back after 2
> weeks correctly interpreting what they've written).
> 

I agree, apparently it seems that a property definition (DEFINE_PROPERTY flag in
V8) is required, in contrast to an array assignment which is generally 
described in the spec as "Put(O, Pk, value, true)" (see for instance
Array.prototype.fill). An array assignment in V8 is generally coded as
"JSObject::SetElement(js_object, index, value, attributes, strict_mode,
check_prototype, SET_PROPERTY)".

Before AddProperty was renamed and recoded to AddNamedProperty, AddProperty
could be used to define properties in an array using indexes. This is no longer
possible. So doing a recap: 

   * The spec requires to be able to define a property into an indexed array.
   * An indexed array assignment uses the SET_PROPERTY flag in V8 not the
DEFINE_PROPERTY flag.
   * It's necessary to provide a method that can define properties into an
indexed array, that means, an operation that does a JSObject::SetElement using
the DEFINE_PROPERTY flag. This method is AddElement.

This also has to do with one of the tests I imported from Firefox (Array.of does
not trigger prototype setters). If the implementation of ArrayOf used array
assignment the setter will be triggered. If the array is created defining
properties the setter is not triggered. 

> 3. I don't really see how Runtime_AddElement really provides any of the
abstract
> methods described in the draft, and having that clearly explained in comments
> would be really, really valuable. I can't look at this implementation, and
then
> look at the spec, and make the connection. I'm just not seeing it. It's not
that
> I don't trust that it's correct (since rossberg@ has already vetted it), but
I'd
> prefer it if the correctness of it to be explained in the tree, in comment
form,
> if not by matching the language in the draft.
> 

OK, maybe we could rename the method to match the one from the specification. I
think what I mentioned in 2) explains what's the rationale behind AddElement and
how it is related to CreateDataPropertyOrThrow.

> I would be very happy if, at the very least, this runtime function, and how it
> is accomplishing what is asked for in 22.1.2.3.8c, were explained clearly in
> comments.
> 

OK, I can add some comments in the header explaining how is related to
CreateDataPropertyOrThrow and for what cases is used (define a data property in
an indexed array).

> Also, nit: the indentation in ArrayOf is inconsistent with the rest of v8
> (despite being consistent with Blink and Chromium). I don't care a whole lot,
> but it is what it is.

Correct. Thanks for the pointer. I will change that.

[Diego Pino, 2014-08-08 12:29:10]

On 2014/08/05 02:11:14, caitp wrote:
> For what it's worth, if AddElement is in some way a useful implementation of
> CreateDataPropertyOrThrow, we'd have uses for it elsewhere, so the connection
> should be more clearly explained.

This is a valid concern and I also have my doubts. There are currently several
operations implemented in V8 which its spec description relies on
CreateDataPropertyOrThrow, so how it's possible that there's not an
implementation of this method available already?

Let's take the spec of Array.prototype.map for example. The spec describes the
following steps:

   * Let mappedValue be the result of calling the Call internal method of
callbackfn with T as thisArgument and a List containing kValue, k, and O as
argumentsList.
   * ReturnIfAbrupt(mappedValue).
   * Let status be CreateDataPropertyOrThrow (A, Pk, mappedValue).
   * ReturnIfAbrupt(status).

Which is translated to the following implementation in V8:

accumulator[i] = %_CallFunction(receiver, element, i, array, f);

That means, to an array assignment, and it happens the same in many other
functions (which in the spec rely on CreateDataPropertyOrThrow). So, should we
drop the implementation of AddElement and use a simple array assignment? Or in
contrast, should we change all the functions which rely on
CreateDataPropertyOrThrow and use AddElement instead of doing an array
assignment? 

The subtle difference between AddElement and a direct array assignment is that
the former defines a property an the letter sets an element. If we drop the
implementation of AddElement and use an array assignment we should consider the
"Array.of does not trigger prototype setters" test invalid.

[rossberg, 2014-08-08 12:53:38]

CreateDataPropertyOrThrow is new in the ES6 spec, which explains why it hasn't
occurred in V8 so far. (Likewise, DONT_DELETE predates ES5, which chose to
rename it to (not) configurable, along with renames for the other property
attributes.)

On the other hand, V8 internally distinguished named properties from indexed
properties (a.k.a. elements). We also have to fit in V8 API functionality like
interceptors, which sometimes is at odds with how the spec is factored. We hence
need different sets of methods internally, with somewhat different invariants.

It would definitely make sense to clean some of this up and get it more in sync
with the new spec. We are slowly doing some such clean-ups, but it's not trivial
at all. It needs to be designed carefully, and definitely is outside the scope
of this CL.

For the time being, CreateDataProperty should translate to AddProperty/Element.
Assignment definitely is incorrect (which might well be wrong in other places
currently -- please feel free to submit patches :) ).

[rossberg, 2014-08-08 12:54:31]

On 2014/08/08 12:53:38, rossberg wrote:
> For the time being, CreateDataProperty should translate to
AddProperty/Element.

(plus extra error checking where necessary, of course)

[Diego Pino, 2014-08-08 20:57:04]

Thanks for the clarifications.

So I'm going to leave the patch as it is and fix the indentation in
harmony-array.js plus add a header comment in AddElemnt. Patch already uploaded.

I still have issues with the trybots. It seems the trybots cannot apply the
patch because the application of the changes in tools/generate-runtime-tests.py
is failing. I did a "git cl rebase" in master before submiting the patch and
rebased the local branch of the patch against master too. I've been trying to
fix this issue for the last couple of hours with little success. I don't know
what I'm doing wrong. Any hint?

[Diego Pino, 2014-08-18 14:51:36]

The CQ bit was checked by dpino@igalia.com

[Diego Pino, 2014-08-18 14:51:36]

The CQ bit was unchecked by dpino@igalia.com

[Diego Pino, 2014-08-18 14:53:23]

All trybots are green now.

[Diego Pino, 2014-08-18 16:38:49]

The CQ bit was checked by dpino@igalia.com

[commit-bot: I haz the power, 2014-08-18 16:39:24]

CQ is trying da patch. Follow status at
 https://v8-status.appspot.com/cq/dpino@igalia.com/364853009/340001

[commit-bot: I haz the power, 2014-08-18 16:39:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-18 16:39:26]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rossberg, 2014-08-19 11:11:03]

Note that V8 does not have a commit queue, so the CQ bit isn't operational. I
will land for you. (Sorry for the delay, I was OOO last week.)