ES6: Implement Array.from()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dean Jackson]

Implement the Array.from method from ES6.

[Radar WebKit Bug Importer]

<rdar://problem/19654521>

[Dean Jackson]

Created attachment 245658 [details]
Patch

[Build Bot]

Comment on attachment 245658 [details]
Patch

Attachment 245658 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6287579481636864

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 245673 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245658 [details]
Patch

Attachment 245658 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5152604308897792

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 245683 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Geoffrey Garen]

Comment on attachment 245658 [details]
Patch

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

> Source/JavaScriptCore/builtins/ArrayConstructor.js:29
> +        return typeof fn === 'function' || @Object.prototype.toString.@call(fn) === '[object Function]';

For isCallable, I would just use the typeof check.

@Object.prototype is problematic -- but luckily, we can just delete it.

> Source/JavaScriptCore/builtins/ArrayConstructor.js:32
> +    var toLength = function(value) {

Are we allowed to define our helper functions outside of from()? That would be more efficient. Otherwise, you'll allocate two function objects per call to from().

> Source/JavaScriptCore/builtins/ArrayConstructor.js:36
> +        if (@isNaN(numberValue)) {

You can just use "numberValue != numberValue" instead of @isNaN if you want.

[Dean Jackson]

Comment on attachment 245658 [details]
Patch

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

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:29
>> +        return typeof fn === 'function' || @Object.prototype.toString.@call(fn) === '[object Function]';
> 
> For isCallable, I would just use the typeof check.
> 
> @Object.prototype is problematic -- but luckily, we can just delete it.

Great.

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:32
>> +    var toLength = function(value) {
> 
> Are we allowed to define our helper functions outside of from()? That would be more efficient. Otherwise, you'll allocate two function objects per call to from().

I don't think we can call helpers outside the built-ins, because these functions are not really defined in a global scope. The script parses the file looking for top-level functions and squirts the JS code directly into the C++, then intercepts the call.

So I think I'll just inline the functions. There really isn't a need in both cases here.

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:36
>> +        if (@isNaN(numberValue)) {
> 
> You can just use "numberValue != numberValue" instead of @isNaN if you want.

Nice! That saves another global.

[Dean Jackson]

Created attachment 246924 [details]
Patch

[Filip Pizlo]

Comment on attachment 246924 [details]
Patch

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

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

Is this really the rule?  In all other respects, the max value of an array's length is (1 << 31) - 2.

[Filip Pizlo]

Comment on attachment 246924 [details]
Patch

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

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:59
>> +    var itemsLength = lengthValue > 0 ? (lengthValue < maxSafeInteger ? lengthValue : maxSafeInteger) : 0;
> 
> Is this really the rule?  In all other respects, the max value of an array's length is (1 << 31) - 2.

This is what the spec wants!

[Dean Jackson]

Committed r180370: <http://trac.webkit.org/changeset/180370>

[Geoffrey Garen]

Comment on attachment 246924 [details]
Patch

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

> LayoutTests/js/script-tests/array-from.js:69
> +var crazyPants = {

Best var evar.

[Joseph Pecoraro]

Comment on attachment 246924 [details]
Patch

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

> Source/JavaScriptCore/builtins/ArrayConstructor.js:49
> +    var numberValue = @Number(items.length);

If "arrayLike" was a Map/Set then we would want .size instead of .length. Perhaps that is why this doesn't work for Set.

I think there is a generic way to check if something is iterable and get an iterator via Symbol.iterator:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols

So this would work with any custom iterable object. That said, I don't know if we implement those portions.

[Alexey Proskuryakov]

The new test fails on all bots:

** The following JSC stress test failures have been introduced:
	jsc-layout-tests.yaml/js/script-tests/array-from.js.layout
	jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-dfg-eager-no-cjit
	jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl
	jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl-eager-no-cjit
	jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl-no-cjit
	jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-cjit
	jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint

https://build.webkit.org/builders/Apple%20Mavericks%20Release%20WK2%20(Tests)/builds/11730/steps/jscore-test/logs/stdio

Dean, are you available to take a look soon?

[Alexey Proskuryakov]

Re-opening to attach a prospective test fix.

[Alexey Proskuryakov]

Created attachment 246939 [details]
test fix

Removing document.querySelectorAll use. It is not appropriate in LayoutTests/js, these cannot rely on DOM.

This probably need to be re-added in a separate test.

[WebKit Commit Bot]

Comment on attachment 246939 [details]
test fix

Clearing flags on attachment: 246939

Committed r180394: <http://trac.webkit.org/changeset/180394>

[Michael Saboff]

(In reply to comment #18)
> Comment on attachment 246939 [details]
> test fix
> 
> Clearing flags on attachment: 246939
> 
> Committed r180394: <http://trac.webkit.org/changeset/180394>

There are still some failing tests on iOS 64 bit after 180394:

             Test                                                             r180388 r180394
jsc-layout-tests.yaml/js/script-tests/array-from.js.layout                    Failed  Passed
jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-dfg-eager-no-cjit  Failed  Passed
jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl                Failed  Passed
jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl-eager-no-cjit  Failed  Passed
jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl-no-cjit        Failed  Failed
jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-cjit            Failed  Failed
jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint           Failed  Failed

All of the failures look like this:

[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: DIFF FAILURE!
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: --- ../.tests/jsc-layout-tests.yaml/js/array-from-expected.txt 2015-02-22 14:50:14.000000000 -0800
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: +++ ../jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint.out 2015-02-22 15:16:04.000000000 -0800
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: @@ -58,11 +58,11 @@
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:  Modify length during construction
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:  -------
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: -PASS Array.from(crazyPants) is ['one', 'two', 'three', 'four']
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: +FAIL Array.from(crazyPants) should be one,two,three,four. Was one,two,three,four,ERROR: this should never be called.
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:  Modify length during mapping
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:  -------
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: -PASS Array.from(crazyPants, function (x) { crazyPants.length = x; return x; }) is ['one', 'two', 'three', 'four']
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint: +FAIL Array.from(crazyPants, function (x) { crazyPants.length = x; return x; }) should be one,two,three,four. Was one,two,three,four,ERROR: this should never be called.
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:  Construction using Set object
[2015-02-22 15:16:05] INFO: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint:  -------
[2015-02-22 15:16:05] ERROR: FAIL: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint

[Csaba Osztrogonác]

note: It is skipped by http://trac.webkit.org/changeset/180573.

Additionally I get exactly the same failure on Linux Aarch64, 
but it didn't fail in all cases:

FAIL: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-llint
FAIL: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-no-cjit
FAIL: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl-no-cjit
PASS: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout
PASS: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-dfg-eager-no-cjit
PASS: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl
PASS: jsc-layout-tests.yaml/js/script-tests/array-from.js.layout-ftl-eager-no-cjit

[Joseph Pecoraro]

Array.from has been working for a while. There is a separate issue about arm64 which has its own bug filed. Closing this.