Implement ES6 Array.of

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: evilpie, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla25
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=evilpies], URL)

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Summary: Implement Array.from and Array.of → Implement ES6 Array.from and Array.of
(Reporter)

Comment 1

4 years ago
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.
Whiteboard: [mentor=evilpies]
I would like to take this up.
You got it :)
Assignee: general → sankha93
(Reporter)

Comment 4

4 years ago
Sankha are you looking into this?
Flags: needinfo?(sankha93)
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.
Attachment #753958 - Flags: feedback?(evilpies)
Flags: needinfo?(sankha93)
Created attachment 753963 [details] [diff] [review]
WIP Patch

Please ignore the previous one. I had forgotten to update jsarray.cpp.
Attachment #753958 - Attachment is obsolete: true
Attachment #753958 - Flags: feedback?(evilpies)
Attachment #753963 - Flags: feedback?(evilpies)
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
(Reporter)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
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.
Assignee: sankha93 → jorendorff
Attachment #766382 - Flags: review?(evilpies)
(Assignee)

Comment 10

4 years ago
Created attachment 766383 [details] [diff] [review]
Part 2 - Array.of, v1
Attachment #766383 - Flags: review?(evilpies)
(Assignee)

Comment 11

4 years ago
Created attachment 766389 [details] [diff] [review]
Part 2 - Array.of, v2

Same as v1, but a few more tests.
Attachment #766383 - Attachment is obsolete: true
Attachment #766383 - Flags: review?(evilpies)
Attachment #766389 - Flags: review?(evilpies)
(Assignee)

Comment 12

4 years ago
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!
(Assignee)

Comment 13

4 years ago
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.
(Reporter)

Updated

4 years ago
Attachment #753963 - Flags: feedback?(evilpies)
(Reporter)

Comment 14

4 years ago
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?
Attachment #766382 - Flags: review?(evilpies) → review+
(Reporter)

Comment 15

4 years ago
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
Attachment #766389 - Flags: review?(evilpies) → review+
(Assignee)

Comment 16

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/98dccff45810
https://hg.mozilla.org/mozilla-central/rev/d0c3168c3c47
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 18

4 years ago
I went with assertDeepEq, btw.
Keywords: dev-doc-needed
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)`.
(Reporter)

Comment 20

4 years ago
You are right, the Array.from patches never landed and we need to update them to the latest spec. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
relnote-firefox: --- → ?

Updated

4 years ago
relnote-firefox: ? → 25+

Updated

4 years ago
Depends on: 903755
(Assignee)

Comment 21

4 years ago
In a new bug please?
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
relnote-firefox: 25+ → ---
Resolution: --- → FIXED
(In reply to Jason Orendorff [:jorendorff] from comment #21)
> In a new bug please?

bug 904723
Summary: Implement ES6 Array.from and Array.of → Implement ES6 Array.of
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
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.
(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 :)
> (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
(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
Perfect! Thanks a lot.

Sebastian
Florian reviewed the page. Thanks all and especially rick who wrote it!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.