* 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
<rdar://problem/24050640>
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.
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.
Created attachment 270726 [details] Patch
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
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
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
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
Created attachment 270734 [details] Patch
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
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
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
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()
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.
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.
Created attachment 271498 [details] Patch
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.
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.
Created attachment 271509 [details] Patch for landing
Comment on attachment 271509 [details] Patch for landing Clearing flags on attachment: 271509 Committed r196675: <http://trac.webkit.org/changeset/196675>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 154344
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.
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.
Created attachment 271588 [details] Patch for landing
Comment on attachment 271588 [details] Patch for landing Clearing flags on attachment: 271588 Committed r196734: <http://trac.webkit.org/changeset/196734>