Bug 152721 – Spread operator should be allowed when not the first argument of parameter list

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

* SUMMARY
Spread operator should be allowed when not the first argument of parameter list.

* TEST
<script>
function foo() { return Array.from(arguments); }
foo(...[1,2]); // Good: [1, 2]
foo(0, ...[1,2]); // Expected: [0, 1, 2], Actual: SyntaxError: Unexpected token '...'
</script>

* NOTES
- Firefox and Chrome both behave as expected

[Radar WebKit Bug Importer]

<rdar://problem/24050640>

[Joseph Pecoraro]

Looks like Parser<LexerType>::parseArguments only checks for "..." at the start of the argument list, and not for each argument. Also, "SpreadMode" now seems unnecessary, since DontAllowSpread is never used.

[Joseph Pecoraro]

Created attachment 268588 [details]
Incomplete Patch

Passing on to Keith. This WIP changes the parser to parse argument lists allowing for multiple spread arguments:

> ArgumentList[Yield] :
>     AssignmentExpression[In, ?Yield]
>     ... AssignmentExpression[In, ?Yield]
>     ArgumentList[?Yield] , AssignmentExpression[In, ?Yield]
>     ArgumentList[?Yield] , ... AssignmentExpression[In, ?Yield]

But I didn't make any progress inside of BytecodeGenerator::emitCall, which will need to then convert the ArgumentList of expressions which may now include multiple Spread Expressions into a var args call.

[Keith Miller]

Created attachment 270726 [details]
Patch

[Build Bot]

Comment on attachment 270726 [details]
Patch

Attachment 270726 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/784733

New failing tests:
js/parser-syntax-check.html
js/basic-spread.html

[Build Bot]

Created attachment 270731 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 270726 [details]
Patch

Attachment 270726 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/784760

New failing tests:
js/parser-syntax-check.html
js/basic-spread.html

[Build Bot]

Created attachment 270732 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Keith Miller]

Created attachment 270734 [details]
Patch

[Build Bot]

Comment on attachment 270734 [details]
Patch

Attachment 270734 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/784946

New failing tests:
js/dfg-convert-this-dom-window.html
js/function-call-aliased.html

[Build Bot]

Created attachment 270738 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Comment on attachment 270734 [details]
Patch

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

Awesome!

> Source/JavaScriptCore/tests/stress/spread-calling.js:41
> +    testFunction(0, ..."12345", ...object3);

A few more interesting tests would be:

  - spreading an empty list
  - spreading the same array multiple times in the same argument list. foo(...arr, ...arr)
  - spreading `arguments` in a call. Interestingly Firefox doesn't allow it (yet?), but Safari / Chrome seem to.
  - spreading an iterable that throws an exception when generating values
    - but it looks like we don't have spread working with generic iterables yet because those s6 tests are still marked fail

[Saam Barati]

Comment on attachment 270734 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:3758
> +        TreeExpression spreadExpr = parseAssignmentExpression(context);

Is assignmentExpression correct here with respect to the spec?

> Source/JavaScriptCore/parser/Parser.cpp:3759
> +        failIfFalse(spreadExpr, "Cannot parse subject of a spread operation");

You might get a better error message here if you call propagateError()

[Saam Barati]

Comment on attachment 270734 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:3758
>> +        TreeExpression spreadExpr = parseAssignmentExpression(context);
> 
> Is assignmentExpression correct here with respect to the spec?

it seems like it should be. Might be worth having a test of an assignment here.

[Keith Miller]

Comment on attachment 270734 [details]
Patch

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

It looks like there is an issue with the Function.prototype.call builtin. I'll take a look at it later. Removing r=?

>>> Source/JavaScriptCore/parser/Parser.cpp:3758
>>> +        TreeExpression spreadExpr = parseAssignmentExpression(context);
>> 
>> Is assignmentExpression correct here with respect to the spec?
> 
> it seems like it should be. Might be worth having a test of an assignment here.

Yeah it should be. (https://tc39.github.io/ecma262/#sec-argument-lists-runtime-semantics-argumentlistevaluation)

>> Source/JavaScriptCore/parser/Parser.cpp:3759
>> +        failIfFalse(spreadExpr, "Cannot parse subject of a spread operation");
> 
> You might get a better error message here if you call propagateError()

Ok will fix

>> Source/JavaScriptCore/tests/stress/spread-calling.js:41
>> +    testFunction(0, ..."12345", ...object3);
> 
> A few more interesting tests would be:
> 
>   - spreading an empty list
>   - spreading the same array multiple times in the same argument list. foo(...arr, ...arr)
>   - spreading `arguments` in a call. Interestingly Firefox doesn't allow it (yet?), but Safari / Chrome seem to.
>   - spreading an iterable that throws an exception when generating values
>     - but it looks like we don't have spread working with generic iterables yet because those s6 tests are still marked fail

These seem like good tests. I will add.

generic iterables work. Those tests just fail because when the tests were imported the testing harness was not so they call functions that don't exist when we run them.

[Keith Miller]

Created attachment 271498 [details]
Patch

[Saam Barati]

Comment on attachment 271498 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/parser/Parser.cpp:3712
> +
> +

style: only one newline needed.

> Source/JavaScriptCore/parser/Parser.cpp:3714
> +    TreeExpression firstArg = parseArgument(context, argType);

you might get a better error message if you propagateError() here.

> Source/JavaScriptCore/parser/Parser.cpp:3728
> +
> +

ditto on style.

[Keith Miller]

Comment on attachment 271498 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:3712
>> +
> 
> style: only one newline needed.

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:3714
>> +    TreeExpression firstArg = parseArgument(context, argType);
> 
> you might get a better error message if you propagateError() here.

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:3728
>> +
> 
> ditto on style.

Fixed.

[Keith Miller]

Created attachment 271509 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 271509 [details]
Patch for landing

Clearing flags on attachment: 271509

Committed r196675: <http://trac.webkit.org/changeset/196675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 154344

[Keith Miller]

For some reason the first time I ran benchmarks I was only getting a minor slowdown in deltablue-varargs but now I'm seeing a 3x regression. Rolling out since I think I'm going to need to make some optimizations.

[Keith Miller]

After having an offline discussion with Geoff and Filip we reached the conclusion that these regression are not a big enough issue that we should stop landing the patch because of them. After ES6 is complete we will work on improving the performance of spread calls.

[Keith Miller]

Created attachment 271588 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 271588 [details]
Patch for landing

Clearing flags on attachment: 271588

Committed r196734: <http://trac.webkit.org/changeset/196734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.