[ES6] Array.from need to accept iterables

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dean Jackson]

The implementation of Array.from fails when passed a Set.

[Yusuke Suzuki]

Created attachment 251423 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 251423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251423&action=review

Cool! I have some comments, but this looks good to me!

> Source/JavaScriptCore/builtins/ArrayConstructor.js:66
> +        // Since for-of loop once more looks up the @@iterator property of a given iterable,
> +        // it could be observable if the user defines a getter for @@iterator.
> +        // To avoid this situation, we define a wrapper object that @@iterator just returns a given iterator.
> +        var wrapper = {
> +            [@symbolIterator]() {
> +                return iterator;
> +            }
> +        };
> +
> +        for (var value of wrapper) {

Could you drop the wrapper and use the iterator instance in the loop?

    for (var value of iterator) { ... }

> Source/JavaScriptCore/tests/stress/array-from-with-iterable.js:29
> +    shouldBe(set.has(array[i][0]), true);

This is testing the Map case, so this line should be map.has(...)!

[Yusuke Suzuki]

Comment on attachment 251423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251423&action=review

Thank you for your comment!

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:66
>> +        for (var value of wrapper) {
> 
> Could you drop the wrapper and use the iterator instance in the loop?
> 
>     for (var value of iterator) { ... }

We cannot do that because when executing `for (var value of iterator) { ... }`, `iterator[Symbol.itrator]()` is called (It's observable effect).
However, this behavior is not defined in the spec...
I tested it in array-from-with-iterator.js.

>> Source/JavaScriptCore/tests/stress/array-from-with-iterable.js:29
>> +    shouldBe(set.has(array[i][0]), true);
> 
> This is testing the Map case, so this line should be map.has(...)!

Oops! Thank you. I'll fix it soon.

[Joseph Pecoraro]

(In reply to comment #3)
> Comment on attachment 251423 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251423&action=review
> 
> Thank you for your comment!
> 
> >> Source/JavaScriptCore/builtins/ArrayConstructor.js:66
> >> +        for (var value of wrapper) {
> > 
> > Could you drop the wrapper and use the iterator instance in the loop?
> > 
> >     for (var value of iterator) { ... }
> 
> We cannot do that because when executing `for (var value of iterator) { ...
> }`, `iterator[Symbol.itrator]()` is called (It's observable effect).
> However, this behavior is not defined in the spec...
> I tested it in array-from-with-iterator.js.

Ahh, excellent point!

[Yusuke Suzuki]

Comment on attachment 251423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251423&action=review

>>>> Source/JavaScriptCore/builtins/ArrayConstructor.js:66
>>>> +        for (var value of wrapper) {
>>> 
>>> Could you drop the wrapper and use the iterator instance in the loop?
>>> 
>>>     for (var value of iterator) { ... }
>> 
>> We cannot do that because when executing `for (var value of iterator) { ... }`, `iterator[Symbol.itrator]()` is called (It's observable effect).
>> However, this behavior is not defined in the spec...
>> I tested it in array-from-with-iterator.js.
> 
> Ahh, excellent point!

Or is it better to write like the following? :)

while (true) {
    var next = iterator.next();
    if (!@isObject(next))
        throw @TypeError("...");

    if (next.done) {
        result.length = k;
        return result;
    }

    var nextValue = next.value;
    if (mapFn) {
        try {
            @putByValDirect(result, k, thisArg === undefined ? mapFn(value, k) : mapFn.@call(thisArg, value, k));
        } catch (error) {
            var returnMethod = iterator.return;
            if (returnMethod != null) {
                try {
                    returnMethod.@call(iterator);
                } catch (watedError) {
                }
            }
            throw error;
        }
    } else
        @putByValDirect(result, k, nextValue);
    k += 1
}

If so, I'll rewrite it with this. Maybe, builtins doesn't allow to use try-catch now, so I'll relax it when it's needed.

[Yusuke Suzuki]

Created attachment 251465 [details]
Patch

Fixed the tests for Map

[Joseph Pecoraro]

> Or is it better to write like the following? :)
> 
> [...snip...]

I think what you have with the wrapper is good. It keeps the iterator implementation is in one place (C++) instead of two (C++ and JavaScript).

[Yusuke Suzuki]

(In reply to comment #7)
> > Or is it better to write like the following? :)
> > 
> > [...snip...]
> 
> I think what you have with the wrapper is good. It keeps the iterator
> implementation is in one place (C++) instead of two (C++ and JavaScript).

Make sense :)

[Darin Adler]

Comment on attachment 251465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251465&action=review

> Source/JavaScriptCore/builtins/ArrayConstructor.js:49
> +            throw new @TypeError("Array.from requires that the @@iterator property of the first argument, when exists, be a function");

Should @@iterator be used in the text of an exception? I’m not sure that’s terminology that we can expect JavaScript programmers to understand.

> Source/JavaScriptCore/builtins/ArrayConstructor.js:91
> +    var len = lengthValue > 0 ? (lengthValue < maxSafeInteger ? lengthValue : maxSafeInteger) : 0;

We normally like to avoid abbreviations like "len" in WebKit code. Not sure how the JavaScriptCore folks feel about this.

[Yusuke Suzuki]

Comment on attachment 251465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251465&action=review

Thank you for your review.

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:49
>> +            throw new @TypeError("Array.from requires that the @@iterator property of the first argument, when exists, be a function");
> 
> Should @@iterator be used in the text of an exception? I’m not sure that’s terminology that we can expect JavaScript programmers to understand.

I've changed it to "Array.from requires that the property of the first argument, items[Symbol.iterator], when exists, be a function"

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:91
>> +    var len = lengthValue > 0 ? (lengthValue < maxSafeInteger ? lengthValue : maxSafeInteger) : 0;
> 
> We normally like to avoid abbreviations like "len" in WebKit code. Not sure how the JavaScriptCore folks feel about this.

This `len` is derived from the spec[1]. Howeevr, "len" seems not good.
I'll change it to arrayLikeLength.

[1]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.from

[Yusuke Suzuki]

Committed r183357: <http://trac.webkit.org/changeset/183357>