866849 - Implement ES6 Array.of

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

We should implement this via self-hosting. You would want to look at array.js. Take a look at bug 715181, if you are interested in how this would look like.

Comment 2

[Sankha Narayan Guria [:sankha]]

I would like to take this up.

Comment 3

[Till Schneidereit [:till]]

You got it :)

Comment 4

[Tom S [:evilpie]]

Sankha are you looking into this?

Comment 5

[Sankha Narayan Guria [:sankha]]

Attachment: WIP Patch

I just wrote the base function. Can you just see if they are alright? I'll be writing the tests next.

Comment 6

[Sankha Narayan Guria [:sankha]]

Attachment: WIP Patch

Please ignore the previous one. I had forgotten to update jsarray.cpp.

Comment 7

[Brandon Benvie [:benvie]]

Comment on attachment 753963 [details] [diff] [review]
WIP Patch

Review of attachment 753963 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Array.js
@@ +149,5 @@
> +
> +    // Steps 4-5
> +    var A;
> +    if (isConstructor(C)) {
> +        var newObj = C(len);

Should be `new C(len)`

@@ +221,5 @@
> +    var putStatus = Put(A, "length", len, true);
> +    return A;
> +}
> +
> +function ArrayStaticOf(list, arrayLike, mapfn = undefined, thisArg = undefined) {

ArrayStaticFrom

Comment 8

[Tom S [:evilpie]]

That doesn't really work right? Because of DefinePropertyOrThrow and Put. Now that I think about, I probably told you some of the things I found on IRC already. You should maybe do this after you set up all the infrastructure in Bug 866847.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v1

Stealing this bug. If we can simplify the implementation by throwing this C++ code away and writing it in JS, *great*! But let's try and ship something.

This first patch may appear to have nothing to do with anything. It defines an assertEqual function that can be used in the test suite; it's sort of like the arraysEqual function in array-compare.js, except that

 1. you can use it on any object, not just arrays;

 2. assertEqual is to assert∘arraysEqual as deep copy is to shallow copy:
    assertEqual([{}], [{}]) passes because {} "equals" {}, whereas
    arraysEqual([{}], [{}]) returns false because {} !== {};

 3. it's otherwise a lot more thorough, checking property attributes,
    extensibility, getters and setters, etc.; and

 4. it generates nice error messages. (has to because of deep equivalence)

Etymology: Scheme uses a predicate named eq? to test identity equivalence and one named equal? to test structural equivalence. That's where assertEq's name comes from.

I'll use this function in tests later on.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Array.of, v1

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Array.of, v2

Same as v1, but a few more tests.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Array.from, WIP 1

This isn't quite finished. I need to change the type of the loop variable from uint32_t to double. Good night!

Comment 13

[Jason Orendorff [:jorendorff]]

Two more changes to Array.from appear to have consensus, according to the TC39 meeting notes, but are not yet in the spec:

- Array.from will accept an iterable argument, not just an arraylike. It will
  check whether the argument is iterable first, and fall back on arraylike.

https://github.com/rwldrn/tc39-notes/blob/master/es6/2012-11/nov-29.md#revisit-nov-27-resolution-on-iterables-in-spread

- Array.from will accept two extra optional arguments:
      Array.from(iterableOrArraylike, mapFn, thisArg)

https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-01/jan-30.md#revising-the-array-subclassing-kind-issue

I'm going to ask a few questions about these changes on es-discuss.

Comment 14

[Tom S [:evilpie]]

Comment on attachment 766382 [details] [diff] [review]
v1

Review of attachment 766382 [details] [diff] [review]:
-----------------------------------------------------------------

This is quite neat! I however still feel that semantics are somewhat surprising. Maybe it's not the best name for this function.

::: js/src/jit-test/lib/asserts.js
@@ +106,5 @@
> +        newError = Error,
> +        has = call.bind(Map.prototype.has),
> +        get = call.bind(Map.prototype.get),
> +        set = call.bind(Map.prototype.set),
> +        toString = call.bind(Object.prototype.toString),

I would name all functions like Map_has, Object_toString. I think that fits with that we have been doing for self-hosting.

@@ +117,5 @@
> +        join = call.bind(Array.prototype.join),
> +        _uneval = uneval;
> +
> +    let isPrimitive = function isPrimitive(v) {
> +        return v === null ||

I thought about switching this to be switch(typeof), but that makes the object and undefined case very confusing. We always have to make a strict compare with undefined, because of document.all. You should add that as a comment here.

@@ +133,5 @@
> +            throw new Error(exc.message + (msg ? " " + msg : ""));
> +        }
> +    };
> +
> +    let assertSameClass = function assertSameClass(a, b, msg) {

why sometimes arrow and sometimes not?

Comment 15

[Tom S [:evilpie]]

Comment on attachment 766389 [details] [diff] [review]
Part 2 - Array.of, v2

Review of attachment 766389 [details] [diff] [review]:
-----------------------------------------------------------------

The functions really makes these tests look reasonable.

::: js/src/jsarray.cpp
@@ +2686,5 @@
>  }
>  
> +// ES6 9.2.5 IsConstructor
> +static bool
> +IsConstructor(const Value &v)

This should probably be in jsfun.h

Comment 16

[Jason Orendorff [:jorendorff]]

(In reply to Tom Schuster [:evilpie] from comment #14)
> This is quite neat! I however still feel that semantics are somewhat
> surprising. Maybe it's not the best name for this function.

You didn't like the Scheme analogy? :-)

I admit it's not exactly self-explanatory. I'll ask around for a new name, but I'll let that naming decision race freely with landing this.

> I would name all functions like Map_has, Object_toString. I think that fits
> with that we have been doing for self-hosting.

Done.

> I thought about switching this to be switch(typeof), but that makes the
> object and undefined case very confusing. We always have to make a strict
> compare with undefined, because of document.all. You should add that as a
> comment here.

Yow! I had forgotten about typeof document.all. Commented.

> why sometimes arrow and sometimes not?

Uhrrr.... because I wrote this without proper caffienation? Switched to arrows throughout, except assertEqual itself which remains a plain function, to match the others in this file.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98dccff45810
https://hg.mozilla.org/mozilla-central/rev/d0c3168c3c47

Comment 18

[Jason Orendorff [:jorendorff]]

I went with assertDeepEq, btw.

Comment 19

[Brandon Benvie [:benvie]]

I see this was resolved fixed but it looks like Array.from was never reviewed and landed. Also in the meantime the new ES6 spec came out that does include Array.from with the signature `Array.from(arrayLike, mapfn=undefined, thisArg=undefined)`.

Comment 20

[Tom S [:evilpie]]

You are right, the Array.from patches never landed and we need to update them to the latest spec. Reopening.

Comment 21

[Jason Orendorff [:jorendorff]]

In a new bug please?

Comment 22

[Brandon Benvie [:benvie]]

(In reply to Jason Orendorff [:jorendorff] from comment #21)
> In a new bug please?

bug 904723

Comment 23

[Sebastian Zartner [:sebo]]

Firefox 25 is out now and this still lacks some documentation. I expect this to be described here:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/of

Sebastian

Comment 24

[Jean-Yves Perrier [:teoli]]

Sebastian, it is a wiki, just do it. There is no guarantee when a doc will be written, we have not enough resources to do it all.

Comment 25

[Rick Waldron [:rwaldron]]

(In reply to Jean-Yves Perrier [:teoli] from comment #24)
> Sebastian, it is a wiki, just do it. There is no guarantee when a doc will
> be written, we have not enough resources to do it all.

I don't mind doing the docs. I wrote the spec proposal so I have a pretty good understanding :)

Comment 26

[Sebastian Zartner [:sebo]]

> (In reply to Jean-Yves Perrier [:teoli] from comment #24)
> Sebastian, it is a wiki, just do it.

Well, I didn't know what it does. So I wanted to have a look at the wiki page...

> we have not enough resources to do it all.

I know this problem very well.

Sebastian(In reply to Rick Waldron [:rwaldron] from comment #25)
> I don't mind doing the docs. I wrote the spec proposal so I have a pretty
> good understanding :)

That would be fantastic.

Sebastian

Comment 27

[Rick Waldron [:rwaldron]]

(In reply to Sebastian Zartner from comment #26)
> > (In reply to Jean-Yves Perrier [:teoli] from comment #24)
> > Sebastian, it is a wiki, just do it.
> 
> Well, I didn't know what it does. So I wanted to have a look at the wiki
> page...
> 
> > we have not enough resources to do it all.
> 
> I know this problem very well.
> 
> Sebastian(In reply to Rick Waldron [:rwaldron] from comment #25)
> > I don't mind doing the docs. I wrote the spec proposal so I have a pretty
> > good understanding :)
> 
> That would be fantastic.
> 
> Sebastian

Done!

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/of

Comment 28

[Sebastian Zartner [:sebo]]

Perfect! Thanks a lot.

Sebastian

Comment 29

[Jean-Yves Perrier [:teoli]]

Florian reviewed the page. Thanks all and especially rick who wrote it!