Last Comment Bug 866849 - Implement ES6 Array.of
: Implement ES6 Array.of
Status: RESOLVED FIXED
[mentor=evilpies]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla25
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
https://people.mozilla.com/~jorendorf...
Depends on: 903755
Blocks: es6
  Show dependency treegraph
 
Reported: 2013-04-29 11:56 PDT by Tom Schuster [:evilpie]
Modified: 2013-11-01 01:56 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP Patch (3.19 KB, patch)
2013-05-24 13:53 PDT, Sankha Narayan Guria [:sankha]
no flags Details | Diff | Review
WIP Patch (5.20 KB, patch)
2013-05-24 13:59 PDT, Sankha Narayan Guria [:sankha]
no flags Details | Diff | Review
v1 (8.29 KB, patch)
2013-06-22 22:36 PDT, Jason Orendorff [:jorendorff]
evilpies: review+
Details | Diff | Review
Part 2 - Array.of, v1 (10.79 KB, patch)
2013-06-22 22:40 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
Part 2 - Array.of, v2 (12.58 KB, patch)
2013-06-23 01:15 PDT, Jason Orendorff [:jorendorff]
evilpies: review+
Details | Diff | Review
Part 3 - Array.from, WIP 1 (9.99 KB, patch)
2013-06-23 02:02 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review

Description Tom Schuster [:evilpie] 2013-04-29 11:56:57 PDT

    
Comment 1 Tom Schuster [:evilpie] 2013-05-17 10:42:14 PDT
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] 2013-05-17 11:58:57 PDT
I would like to take this up.
Comment 3 Till Schneidereit [:till] 2013-05-17 12:00:15 PDT
You got it :)
Comment 4 Tom Schuster [:evilpie] 2013-05-24 11:46:21 PDT
Sankha are you looking into this?
Comment 5 Sankha Narayan Guria [:sankha] 2013-05-24 13:53:56 PDT
Created attachment 753958 [details] [diff] [review]
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] 2013-05-24 13:59:31 PDT
Created attachment 753963 [details] [diff] [review]
WIP Patch

Please ignore the previous one. I had forgotten to update jsarray.cpp.
Comment 7 Brandon Benvie [:benvie] 2013-05-24 15:37:39 PDT
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 Schuster [:evilpie] 2013-05-27 09:27:39 PDT
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] 2013-06-22 22:36:52 PDT
Created attachment 766382 [details] [diff] [review]
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] 2013-06-22 22:40:36 PDT
Created attachment 766383 [details] [diff] [review]
Part 2 - Array.of, v1
Comment 11 Jason Orendorff [:jorendorff] 2013-06-23 01:15:25 PDT
Created attachment 766389 [details] [diff] [review]
Part 2 - Array.of, v2

Same as v1, but a few more tests.
Comment 12 Jason Orendorff [:jorendorff] 2013-06-23 02:02:01 PDT
Created attachment 766394 [details] [diff] [review]
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] 2013-06-23 15:31:05 PDT
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 Schuster [:evilpie] 2013-06-26 08:02:12 PDT
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 Schuster [:evilpie] 2013-06-26 08:10:33 PDT
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] 2013-07-01 09:01:53 PDT
(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 18 Jason Orendorff [:jorendorff] 2013-07-10 14:31:37 PDT
I went with assertDeepEq, btw.
Comment 19 Brandon Benvie [:benvie] 2013-07-16 14:03:02 PDT
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 Schuster [:evilpie] 2013-07-16 16:14:49 PDT
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] 2013-08-13 10:37:56 PDT
In a new bug please?
Comment 22 Brandon Benvie [:benvie] 2013-08-13 11:05:18 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #21)
> In a new bug please?

bug 904723
Comment 23 Sebastian Zartner [:sebo] 2013-10-30 01:49:19 PDT
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] 2013-10-30 01:55:24 PDT
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] 2013-10-30 15:29:32 PDT
(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] 2013-10-30 15:35:21 PDT
> (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] 2013-10-30 15:49:15 PDT
(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] 2013-10-30 16:15:03 PDT
Perfect! Thanks a lot.

Sebastian
Comment 29 Jean-Yves Perrier [:teoli] 2013-11-01 01:56:39 PDT
Florian reviewed the page. Thanks all and especially rick who wrote it!

Note You need to log in before you can comment on or make changes to this bug.