Last Comment Bug 762363 - (harmony:spreadcall) implement the spread operator in calls
(harmony:spreadcall)
: implement the spread operator in calls
Status: RESOLVED FIXED
[js:t]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement with 2 votes (vote)
: mozilla27
Assigned To: Tooru Fujisawa [:arai]
:
:
Mentors:
http://wiki.ecmascript.org/doku.php?i...
Depends on:
Blocks: es6
  Show dependency treegraph
 
Reported: 2012-06-06 21:31 PDT by :Benjamin Peterson
Modified: 2013-09-24 10:15 PDT (History)
19 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Draft implementation for spreadcall: Prototype [A] (13.36 KB, patch)
2013-05-30 06:11 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [B] (14.69 KB, patch)
2013-05-30 06:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [C] (18.97 KB, patch)
2013-05-30 09:15 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [D] (38.48 KB, patch)
2013-06-02 10:37 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [E] (33.62 KB, patch)
2013-06-03 14:56 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [E]: fixed (33.58 KB, patch)
2013-06-04 11:15 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [E]: fixed2 (35.12 KB, patch)
2013-06-06 11:48 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [E]: fixed3 (35.01 KB, patch)
2013-06-17 08:39 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Draft implementation for spreadcall: Prototype [F] (40.60 KB, patch)
2013-06-18 10:30 PDT, Tooru Fujisawa [:arai]
jorendorff: review+
Details | Diff | Splinter Review
address review comments (55.19 KB, patch)
2013-06-21 09:28 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
address review comments (59.18 KB, patch)
2013-07-02 08:51 PDT, Tooru Fujisawa [:arai]
jdemooij: review+
Details | Diff | Splinter Review
address review comments (58.64 KB, patch)
2013-07-02 17:00 PDT, Tooru Fujisawa [:arai]
jdemooij: review+
Details | Diff | Splinter Review
fix TIMEOUT (60.26 KB, patch)
2013-07-04 18:46 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
remove over-redundant tests to decrease test time (58.45 KB, patch)
2013-07-05 11:04 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
updated for latest trunk (58.47 KB, patch)
2013-07-12 11:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
updated for latest trunk (57.83 KB, patch)
2013-07-16 10:33 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
updated for latest trunk (57.83 KB, patch)
2013-07-24 05:33 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
updated for latest trunk (57.79 KB, patch)
2013-08-13 12:59 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
updated for latest trunk (57.12 KB, patch)
2013-08-28 20:56 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
updated for latest trunk (56.98 KB, patch)
2013-09-02 05:08 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
updated for latest trunk (53.45 KB, patch)
2013-09-04 07:07 PDT, Tooru Fujisawa [:arai]
jorendorff: review+
Details | Diff | Splinter Review
address review comments (53.01 KB, patch)
2013-09-13 05:37 PDT, Tooru Fujisawa [:arai]
jorendorff: review+
Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-06-06 21:31:33 PDT
Bug 574130 adds support for the spread operator in array initializers. It also needs to be supported in calls.
Comment 1 Tooru Fujisawa [:arai] 2013-05-30 06:11:42 PDT
Created attachment 755903 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [A]

I created 2 prototypes.

---- Prototype [A] ----

[js/src/frontend/Parser.cpp]
If argument have spread operator, generate PNK_SPREAD node for it, as spreadarray does.
Then, if argument list contains at lease one spread operator, such like:
  f(...x)
  f(x, ...[y])
generate tree of following expression:
  f(...[...x])
  f(...[x, ...[y]])
First spread operator is just a flag for BytecodeEmitter.

To do this, I use "Vector<Node, 0, SystemAllocPolicy> args" to store each argument in loop:
  args.append(argNode);
and call handler.addList later:
  for (size_t i = 0, len = args.length(); i < len; i ++) {
      handler.addList(listNode, args[i]);
  }
even if arguments does not contain spread operator.
It needs extra memory allocation and slower than original version (see Question 2).


[js/src/frontend/BytecodeEmitter.cpp]
If function call has single argument which is PNK_SPREAD, emit following byte codes:
  callgname "f"
  undefined
  notearg

  // emit byte code for operand of spread operator
  newarray 1
    zero
    getgname "x"
    initelem_inc
    getgname "y"
    spread   // actual spread operation is done here
    pop
  endinit

  // do not emit "notearg" opcode because the operand is not real argument

  call ARGC_SPREAD

Here ARGC_SPREAD is UINT16_MAX.
Due to this, ARGC_LIMIT is changed from UINT16_LIMIT to UINT16_MAX.

Difference to normal function call is following:
  1) Do not emit "notearg" opcode for operand of spread operator
  2) argc is ARGC_SPREAD, instead of 1


[js/src/jsinterp.cpp]
If call's argc is ARGC_SPREAD, perform following:
  1) Pop array object from stack
  2) Extract elements of array to stack
  3) (Perform "notearg" operation for each argument if needed)
  4) Create CallArgs with array.length() and extracted stack
Comment 2 Tooru Fujisawa [:arai] 2013-05-30 06:12:46 PDT
Created attachment 755904 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [B]

---- Prototype [B] ----

[js/src/frontend/Parser.cpp]
Do not generate extra array node, so generated tree is same as input:
  f(...x)
  f(x, ...[y, z])
To check existence of spread operator in BytecodeEmitter.cpp in O(1), set PNX_SPECIALARGUMENTLIST flag to the argument list node when it contains spread operator.

[js/src/frontend/BytecodeEmitter.cpp]
If function call has PNX_SPECIALARGUMENTLIST, emit following byte codes
  callgname "f"
  undefined
  notearg

  newarray 1
  zero

  getgname "x" // emit byte code for 1st argument
  initelem_inc // emit "initelem_inc" if argument has no spread operator

  getgname "y" // emit byte code for 2nd argument
  spread       // emit "spread" if argument has spread operator

  pop
  endinit
  // do not emit "notearg" opcode too

  call ARGC_SPREAD
So, emitted byte codes are same as [A].

Other changes are also same as [A].

---- Limitation ----

1) "callFunction" is not supported, because it needs 1st(fun) and 2nd(this) arguments statically

2) Function call can have up to 65534 arguments, not 65535 as original (see Question 1).

---- Performance Tests ----

1) Performance regression of normal function call
   Test code:
     let start = elapsed(); for (let i = 0; i < 1000; i ++) { eval("z => f(10, 20, 30)"); } elapsed() - start;
   Result: (average of 20 runs)
     original: 223549 [us]
     [A]:      234356 [us] (4% slower than original)
     [B]:      226501 [us] (1% slower than original)

2) Performance comparison between 2 prototypes
   Test code:
     let start = elapsed(); for (let i = 0; i < 100; i ++) { eval("((...x) => x)(10, ...[20, 30])"); } elapsed() - start;
   Result: (average of 20 runs)
     [A]: 373182 [us]
     [B]: 372896 [us] (0.1% faster than [A], not a significant difference...)

---- Questions ----

1) I changed ARGC_LIMIT from UINT16_LIMIT(=65536) to UINT16_MAX(=65535), to indicate spreadcall by ARGC_SPREAD which is UINT16_MAX.
   Due to this change, following test fails:
     /js/src/jit-test/tests/basic/bug849777.js
   (passing 65535 arguments)
   Should I find another way?
   Such like ...
     a) Create dedicated opcodes (spreadcall, spreadnew, spreadfuncall, spreadfunapply, spreadeval)
     b) Push some flag on stack before call

2) If ParseHandler::Node supports iterate and remove operation, Prototype [A] does not need Vector and can be faster and simple.
   (Of course SyntaxParseHandler cannot provide such function, we also need a function to detect iterate and remove are supported or not)
   How do you think?

3) "notearg" opcode does nothing in jsinterp.cpp, maybe it's used by IonMonkey. Do I have to do something with this?

4) Following file contains test for "wrap" function.
     /js/src/frontend/jit-test/tests/basic/spread-array-wrap.js
   I'm not sure what this test is. Is it also needed for spreadcall?
Comment 3 Tooru Fujisawa [:arai] 2013-05-30 09:15:03 PDT
Created attachment 756016 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [C]

I created one more prototype (maybe better than [A] and [B])

---- Prototype [C] ----

Based on Prototype [B].

Use JSVAL_TYPE_MAGIC as flag of spreadcall, and remove ARGC_SPREAD.
So ARGC_LIMIT is set back to UINT16_LIMIT.

[js/src/frontend/BytecodeEmitter.cpp]
If function call has PNX_SPECIALARGUMENTLIST, emit following byte codes
  callgname "f"
  undefined
  notearg

  spreadarg    // emit "spreadarg" opcode which pushes magic value on stack

  newarray 1
  zero

  getgname "x" // emit byte code for 1st argument
  initelem_inc // emit "initelem_inc" if argument has no spread operator

  getgname "y" // emit byte code for 2nd argument
  spread       // emit "spread" if argument has spread operator

  pop
  endinit
  // do not emit "notearg" opcode too

  call 2

[js/src/jsinterp.cpp]
If call's argc == 2, and 1st argument is magic value:
  1) Pop both arguments
  2) Extract elements of array to stack
  3) (Perform "notearg" operation for each argument if needed)
  4) Create CallArgs with array.length() and extracted stack

Because "why" of magic value is ignored in non-DEBUG mode, I use magic value as 1st argument, to distinguish spreadcall from funapply's optimization (which uses magic value for 2nd argument).

---- Performance Tests ----

1) Performance regression of normal function call
   Test code:
     let start = elapsed(); for (let i = 0; i < 1000; i ++) { eval("z => f(10, 20, 30)"); } elapsed() - start;
   Result: (average of 20 runs)
     original: 223549 [us]
     [A]:      234356 [us] (4% slower than original)
     [B]:      226501 [us] (1% slower than original)
     [C]:      225189 [us] (1% slower than original)

2) Performance comparison between 3 prototypes
   Test code:
     let start = elapsed(); for (let i = 0; i < 100; i ++) { eval("((...x) => x)(10, ...[20, 30])"); } elapsed() - start;
     [A]: 373182 [us]
     [B]: 372896 [us]
     [C]: 370259 [us] (0.7% faster than [A] and [B])
Comment 4 Tooru Fujisawa [:arai] 2013-06-02 10:37:16 PDT
Created attachment 757150 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [D]

Sorry for repeated post.

Here is dedicated opcode version.

[js/src/Parser.cpp]
change opcode of node to spreadcall, when spread operator is found in its argument list.

[js/src/frontend/BytecodeEmitter.cpp]
If opcode is spreadcall, emit following byte codes
  callgname "f"
  undefined
  notearg

  newarray 1
  zero

  getgname "x" // emit byte code for 1st argument
  initelem_inc // emit "initelem_inc" if argument has no spread operator

  getgname "y" // emit byte code for 2nd argument
  spread       // emit "spread" if argument has spread operator

  pop
  endinit
  // do not emit "notearg" opcode too

  spreadcall 1

[js/src/jsinterp.cpp]
If opcode is spreadcall:
  1) Pop argument
  2) Extract elements of array to stack
  3) (Perform "notearg" operation for each argument if needed)
  4) Create CallArgs with array.length() and extracted stack

It seems that this is most simple solution.
Comment 5 Jason Orendorff [:jorendorff] 2013-06-03 07:33:12 PDT
The parse tree should reflect the syntax in the simplest possible way. I think this rules out approach A.

Let's use new opcodes for new functionality.  In addition to jsinterp.cpp there are other places that "know" what an opcode does, particularly the JITs. Adding special cases or magic values could confuse that existing code.

Prototype D seems simplest.

Suggestions:

  - Only add JSOP_SPREADCALL and JSOP_SPREADNEW,
not JSOP_SPREADEVAL or JSOP_SPREADFUNCALL
    or JSOP_SPREADFUNAPPLY
Comment 6 Jason Orendorff [:jorendorff] 2013-06-03 07:41:28 PDT
Sorry, accidentally submitted that comment. Here is what I meant to say at the end:

Prototype D seems simplest. Is there any reason not to do that?

Please only add JSOP_SPREADCALL, JSOP_SPREADEVAL, and JSOP_SPREADNEW,
not JSOP_SPREADFUNCALL, and JSOP_SPREADFUNAPPLY.

Instead of those last two, emit JSOP_SPREADCALL. In the interpreter, JSOP_FUNCALL is identical to JSOP_CALL, and JSOP_FUNAPPLY is very nearly identical (only some extra record-keeping for the JIT).
Comment 7 Jason Orendorff [:jorendorff] 2013-06-03 07:47:00 PDT
Comment on attachment 756016 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [C]

Clearing request flags for now. Please post a new patch and request review or feedback on that.

This looks like great work. Thank you!

One more note: spread-call-decompile.js isn't a very good test. We don't decompile functions for function.toString() anymore. In fact even the decompileFunction() shell builtin does not actually decompile bytecode. I think we need a new shell builtin to test the minimal decompiler we now have... someday.
Comment 8 :Benjamin Peterson 2013-06-03 07:53:11 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #7) 
> One more note: spread-call-decompile.js isn't a very good test. We don't
> decompile functions for function.toString() anymore. In fact even the
> decompileFunction() shell builtin does not actually decompile bytecode. I
> think we need a new shell builtin to test the minimal decompiler we now
> have... someday.

Yes, decompileFunction exists to test the JSAPI function JS_DecompileFunction, which also no longer does what it says. (It can die with JSD, btw.)

If you do in fact change the expression decompiler, please add cases to js/src/jit-test/tests/basic/expression-autopsy.js. The great thing about the expression decompiler, though, is that it generally not updating will cause it to fallback rather than create security holes. :)
Comment 9 Tooru Fujisawa [:arai] 2013-06-03 14:56:57 PDT
Created attachment 757651 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [E]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Please only add JSOP_SPREADCALL, JSOP_SPREADEVAL, and JSOP_SPREADNEW,
> not JSOP_SPREADFUNCALL, and JSOP_SPREADFUNAPPLY.
> 
> Instead of those last two, emit JSOP_SPREADCALL. In the interpreter,
> JSOP_FUNCALL is identical to JSOP_CALL, and JSOP_FUNAPPLY is very nearly
> identical (only some extra record-keeping for the JIT).
Okay, I removed JSOP_SPREADFUNCALL and JSOP_SPREADFUNAPPLY.
Convert JSOP_FUNAPPLY and JSOP_FUNCALL to JSOP_SPREADCALL if argument list contains spread operator, in js/src/frontend/Parser.cpp.

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> One more note: spread-call-decompile.js isn't a very good test. We don't
> decompile functions for function.toString() anymore. In fact even the
> decompileFunction() shell builtin does not actually decompile bytecode. I
> think we need a new shell builtin to test the minimal decompiler we now
> have... someday.
I also removed spread-call-decompile.js.

(In reply to :Benjamin Peterson from comment #8)
> If you do in fact change the expression decompiler, please add cases to
> js/src/jit-test/tests/basic/expression-autopsy.js. The great thing about the
> expression decompiler, though, is that it generally not updating will cause
> it to fallback rather than create security holes. :)
I added decompile test to js/src/jit-test/tests/basic/expression-autopsy.js.
For "ieval(...[])", decompiler should return "ieval(...)" just like JSOP_CALL does.

Then, I mis-understood the behavior of decompiler function.
In Prototype D, I added check for JSOP_SPREADCALL in DecompileArgumentFromStack in js/src/jsopcode.cpp, but that was wrong.
>    if ((JSOp(*current) != JSOP_CALL && JSOp(*current) != JSOP_SPREADCALL) ||
>        static_cast<unsigned>(formalIndex) >= GET_ARGC(current))
>        return true;
With JSOP_SPREADCALL, number of arguments on stack and result of "GET_ARGC(current)" is different, GET_ARGC(current) always returns 1 because actual arguments are stored in Array object. So we cannot decompile argument from stack in the same way as JSOP_CALL.
I removed the change, and choose "fallback" for argument.
Comment 10 Tooru Fujisawa [:arai] 2013-06-04 11:15:33 PDT
Created attachment 758052 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [E]: fixed

Sorry, I forgot to remove comment for JSOP_SPREADFUNCALL.
Comment 11 Tooru Fujisawa [:arai] 2013-06-06 11:48:08 PDT
Created attachment 759316 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [E]: fixed2

Updated patch for two Bugs.

1. Bug 876429 (Sorry, I overlooked this)

Destructuring assignment with spread call, such like:
  (function () { [a (...[])] = b; })
fails with following message:
  Assertion failure: bce->stackDepth >= 0, at ***/js/src/frontend/BytecodeEmitter.cpp:193

I've added JSOP_SPREADCALL and JSOP_SPREADEVAL cases into EmitDestructuringLHS in js/src/frontend/BytecodeEmitter.cpp,
and added test for it: js/src/jit-test/tests/basic/spread-call-setcall.js

2. Bug 778948

Just moved changes from js/src/jsinterp.cpp to js/src/vm/Interpreter.cpp.
Comment 12 Tooru Fujisawa [:arai] 2013-06-17 08:39:45 PDT
Created attachment 763584 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [E]: fixed3

updated patch for latest tree
Comment 13 Tooru Fujisawa [:arai] 2013-06-18 10:30:02 PDT
Created attachment 764264 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [F]

Excuse me.

I noticed that I must check length of arguments before pushing them on stack by GetElements in Interpreter.cpp.
Arguments of spreadcall can be extremely long, and passing arguments longer than StackSpace::ARGS_LENGTH_MAX causes crash.
Then, found that js_fun_apply in jsfun.cpp does it in simple way.

I've merged JSOP_SPREADCALL/JSOP_SPREADNEW/JSOP_SPREADEVAL into single simple block, and added Range Check there.

Added 2 tests for Range Check:
  js/src/jit-test/tests/basic/spread-call-maxarg.js
    range check for argument length in single call
  js/src/jit-test/tests/basic/spread-call-recursion.js
    range check for stack size in recursive call

Added a test for checking "this" and "arguments.callee" values:
  js/src/jit-test/tests/basic/spread-call-this.js

There is no change in Parser and BytecodeEmitter from Prototype E.
Comment 14 Jason Orendorff [:jorendorff] 2013-06-18 18:55:58 PDT
Comment on attachment 764264 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [F]

Review of attachment 764264 [details] [diff] [review]:
-----------------------------------------------------------------

Posting partial review. I'll have to finish this up tomorrow. Looking good so far, just a few comments.

::: js/src/frontend/Parser.cpp
@@ +6254,5 @@
> +        if (tokenStream.matchToken(TOK_TRIPLEDOT, TSF_OPERAND)) {
> +            if (tokenStream.matchToken(TOK_RP, TSF_OPERAND)) {
> +                report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);
> +                return null();
> +            }

It shouldn't be necessary to check for TOK_RP here. assignExpr() would reject that anyway. So please remove the last 4 lines of code above.

@@ +6378,3 @@
>              return null();
> +        if (isSpread)
> +            handler.setOp(lhs, JSOP_SPREADNEW);

Please rephrase it:

    if (tokenStream.matchToken(TOK_LP)) {
        bool isSpread = false;
        if (!argumentList(lhs, &isSpread))
            return null();
        if (isSpread)
            handler.setOp(lhs, JSOP_SPREADNEW);
    }

@@ +6436,1 @@
>                      handler.setOp(nextMember, JSOP_EVAL);

Please go ahead and remove all the handler.setOp calls and just call it once at the end.

@@ +6472,5 @@
> +                    case JSOP_EVAL:
> +                        op = JSOP_SPREADEVAL;
> +                        break;
> +                    default:
> +                        break;

Can the default case occur?

Could this whole switch statement be replaced with:
    op = (op == JSOP_EVAL ? JSOP_SPREADEVAL : JSOP_SPREADCALL);
?

::: js/src/jit-test/tests/basic/spread-call-invalid-syntax.js
@@ +1,4 @@
> +load(libdir + "asserts.js");
> +
> +var offenders = [
> +    "((...x) => x)(1 ... n)",

In each of these please replace the function with an identifier:
  "f(1 ... n)"
to make it clear what's being tested.

@@ +3,5 @@
> +var offenders = [
> +    "((...x) => x)(1 ... n)",
> +    "((...x) => x)(...x for (x in y))",
> +    "((...x) => x)(...)",
> +    "((...x) => x)(...,)"

Good tests. Please add:
  f(... ...[])
  f(...x,)
  f(x, ...)
  f(...x, x for (x in y))

::: js/src/jit-test/tests/basic/spread-call-new.js
@@ +6,5 @@
> +g.prototype = {
> +  get value() {
> +    return this._value;
> +  }
> +};

The lines about g.prototype don't help, I don't think; simply

    function g(...x) {
        this.value = x;
    }

    assertEqArray(new g(...[]).value, []);


It might be good to add, inside g:

        assertEq(Object.getPrototypeOf(this), g.prototype);

::: js/src/jit-test/tests/basic/spread-call-setcall.js
@@ +8,5 @@
> +
> +function check(expr) {
> +  let failed = true;
> +  try {
> +    new Function(expr)();

Put the `new Function(expr)` part outside the try block, to test that this syntax parses successfully, but throws at run time.

(TC39 could ban this, since the spread syntax is new. But let's assume it will not be banned, for now.)

@@ +14,5 @@
> +  } catch (e) {
> +    assertEq(e.message, "invalid assignment left-hand side");
> +  }
> +  if (!failed)
> +    throw new Error("didn't fail");

All this is very similar to a builtin we have:

    load(libdir + "asserts.js");

    function check(expr) {
        assertThrowsInstanceOf(Function(expr), ReferenceError);
    }

Checking error messages in tests usually doesn't catch many errors, compared to how often such tests have to be updated. However, whatever you think is appropriate to test here is fine with me.

::: js/src/jit-test/tests/basic/spread-call-this.js
@@ +9,5 @@
> +g1(...[]);
> +
> +let g2 = x => {
> +  assertEq(this, global);
> +  assertEq(arguments.callee, undefined);

I think this test would fail in the browser.

To test the behavior of arguments in arrow functions, the right thing to do is declare a global
    var arguments = 0;
and then inside, the arrow function, do:
    assertEq(arguments, 0);

Note that the JS shell has a global variable named 'arguments' which happens to be an array. This surprises everyone eventually. I just filed bug 884516 to get rid of it.

@@ +96,5 @@
> +      assertEq(this, global);
> +      assertEq(arguments.callee, g1);
> +    };
> +    g1(...[]);
> +    

Nit: please delete the spaces on the blank line here.

::: js/src/jit-test/tests/basic/spread-call.js
@@ +3,5 @@
> +function check(f) {
> +  assertEqArray(f(...[1, 2, 3]), [1, 2, 3]);
> +  assertEqArray(f(1, ...[2, 3, 4], 5), [1, 2, 3, 4, 5]);
> +  assertEqArray(f(1, ...[], 2), [1, 2]);
> +  assertEqArray(f(1, ...[2, 3], 4, ...[5, 6]), [1, 2, 3, 4, 5, 6]);

Also:

- test that f(...[undefined]) returns [undefined] and not an empty array.
- test using spread syntax to call a function without rest arguments
- test using spread syntax to call a function with argument defaults
- test other iterable objects besides arrays:
    assertEqArray(f(...Set([1, 2])), [1, 2]);
- test what happens when the object isn't iterable:
    assertThrowsInstanceOf(() => Math.sin(...{}), TypeError);
- test assignment expressions in spread operands:
    f(...a=b)

@@ +9,5 @@
> +// According to the draft spec, null and undefined are to be treated as empty
> +// arrays. However, they are not iterable. If the spec is not changed to be in
> +// terms of iterables, these tests should be fixed.
> +//assertEqArray(f(1, ...null, 2), [1, 2]);
> +//assertEqArray(f(1, ...undefined, 2), [1, 2]);

Keep the comment, but assert that all of these throw:

    assertThrowsInstanceOf(() => f(...null), TypeError);

and so on.

Change the existing spread-array tests too.

::: js/src/jsinferinlines.h
@@ +307,5 @@
>      // CALL, FUNCALL, FUNAPPLY, EVAL (Standard callsites)
>      // NEW (IonMonkey-only callsite)
>      // GETPROP, CALLPROP, and LENGTH. (Inlined Getters)
>      // SETPROP, SETNAME, SETGNAME (Inlined Setters)
> +    return op == JSOP_CALL || op == JSOP_SPREADCALL ||

Update the comment, not just the code.

::: js/src/jsopcode.cpp
@@ +154,5 @@
>          return NumBlockSlots(script, pc) + 1;
>        default:
>          /* stack: fun, this, [argc arguments] */
> +        JS_ASSERT(op == JSOP_NEW || op == JSOP_SPREADNEW ||
> +                  op == JSOP_CALL ||  op == JSOP_SPREADCALL || 

Style nit: remove the trailing whitespace on this last quoted line.
Comment 15 Jason Orendorff [:jorendorff] 2013-06-19 19:39:44 PDT
Comment on attachment 764264 [details] [diff] [review]
Draft implementation for spreadcall: Prototype [F]

Review of attachment 764264 [details] [diff] [review]:
-----------------------------------------------------------------

OK, here are the rest of my comments. It looks great. I love it. I can't wait to land it. Please request review from me again when you have an updated patch. Thanks!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5015,5 @@
> +    bool spread = false;
> +    if (pn->isOp(JSOP_SPREADCALL) || pn->isOp(JSOP_SPREADNEW) ||
> +        pn->isOp(JSOP_SPREADEVAL)) {
> +        spread = true;
> +    }

Write it like this:

    bool spread = pn->isOp(JSOP_SPREADCALL) ||
                  pn->isOp(JSOP_SPREADNEW) ||
                  pn->isOp(JSOP_SPREADEVAL);

Or alternatively, assuming you make the suggested changes in jsopcode.tbl:

    bool spread = JOF_OPTYPE(pn->getOp()) == JOF_BYTE;

@@ +5025,5 @@
>          {
>              /*
>               * Special-casing of callFunction to emit bytecode that directly
>               * invokes the callee with the correct |this| object and arguments.
>               * callFunction(fun, thisArg, ...args) thus becomes:

Oh, wow, the notation in this comment looks exactly like spread-call syntax, but it's not! That will be very confusing. Please change it to say:

         * callFunction(fun, thisArg, arg0, arg1) thus becomes:

and so on.

@@ +5127,5 @@
> +            int32_t nspread = 0;
> +            for (ParseNode *pn3 = pn2->pn_next; pn3; pn3 = pn3->pn_next) {
> +                if (pn3->isKind(PNK_SPREAD))
> +                    nspread++;
> +            }

All this code should be shared with EmitArray. Please common it up.

Incidentally, the code at the beginning of EmitArray that only runs if  pn->isKind(PNK_ARRAYCOMP) really does not belong in that function at all. Please feel free to move it into a separate EmitArrayComp function.

@@ +5162,5 @@
>          bce->emittingForInit = oldEmittingForInit;
>      }
>  
>      if (Emit3(cx, bce, pn->getOp(), ARGC_HI(argc), ARGC_LO(argc)) < 0)
>          return false;

I don't think it makes sense to emit argc for JSOP_SPREAD* ops, so call Emit1 instead of Emit3 if (spread).

::: js/src/js.msg
@@ +401,5 @@
>  MSG_DEF(JSMSG_PAR_ARRAY_SCATTER_BAD_TARGET, 348, 1, JSEXN_ERR, "target for index {0} is not an integer")
>  MSG_DEF(JSMSG_SELFHOSTED_UNBOUND_NAME,349, 0, JSEXN_TYPEERR, "self-hosted code may not contain unbound name lookups")
>  MSG_DEF(JSMSG_DEPRECATED_SOURCE_MAP,  350, 0, JSEXN_SYNTAXERR, "Using //@ to indicate source map URL pragmas is deprecated. Use //# instead")
> +MSG_DEF(JSMSG_TOO_MANY_CON_SPREADARGS,351, 0, JSEXN_RANGEERR, "too many constructor arguments")
> +MSG_DEF(JSMSG_TOO_MANY_FUN_SPREADARGS,352, 0, JSEXN_RANGEERR, "too many function arguments")

It's OK to reuse JSMSG_TOO_MANY_CON_ARGS and JSMSG_TOO_MANY_FUN_ARGS. Please don't define new error messages that are the same as the existing ones.

::: js/src/jsinfer.cpp
@@ +2583,2 @@
>          return false;
> +    }

Style nit: remove the curly braces from this if-statement since each condition and each branch fits on a single line.

::: js/src/jsopcode.tbl
@@ +94,5 @@
> +OPDEF(JSOP_SPREADCALL,      41, "spreadcall",      NULL,  3, -1,  1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET)
> +/* spreadcall variant of JSOP_NEW */
> +OPDEF(JSOP_SPREADNEW,       42, "spreadnew",       NULL,  3, -1,  1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET)
> +/* spreadcall variant of JSOP_EVAL */
> +OPDEF(JSOP_SPREADEVAL,      43, "spreadeval",      NULL,  3, -1,  1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET)

These opcodes always pop 3 values off the stack, then push 1 value. So change the -1 in the "use" column to 3 on these lines.

I mentioned before that I don't think it makes sense for these to have an argument. Here, that means changing JOF_UINT16 to JOF_BYTE and changing the length from 3 to 1. (Of course it also means making corresponding changes wherever these opcodes are used.)

::: js/src/vm/Interpreter.cpp
@@ +2152,5 @@
>  
> +BEGIN_CASE(JSOP_SPREADNEW)
> +BEGIN_CASE(JSOP_SPREADCALL)
> +    if (regs.fp()->hasPushedSPSFrame())
> +        cx->runtime()->spsProfiler.updatePC(script, regs.pc);

I wonder why this isn't set for JSOP_EVAL. I'll ask bhackett.

@@ +2155,5 @@
> +    if (regs.fp()->hasPushedSPSFrame())
> +        cx->runtime()->spsProfiler.updatePC(script, regs.pc);
> +    /* FALL THROUGH */
> +
> +BEGIN_CASE(JSOP_SPREADEVAL)

(Note that GET_UINT16(regs.pc) does not appear anywhere in here.)

@@ +2158,5 @@
> +
> +BEGIN_CASE(JSOP_SPREADEVAL)
> +{
> +    JS_ASSERT(regs.stackDepth() >= 3);
> +    RootedObject aobj(cx, &regs.sp[-1].toObject());

Instead:
    RootedObject &aobj = rootObject0;
    aobj = &regs.sp[-1].toObject();

This saves some bookkeeping that must be done whenever a RootedObject is created or destroyed.

@@ +2163,5 @@
> +    JS_ASSERT(aobj->isArray());
> +
> +    uint32_t length;
> +    if (!GetLengthProperty(cx, aobj, &length))
> +        goto error;

Instead:
    uint32_t length = obj->getArrayLength();

(Remove the JS_ASSERT(aobj->isArray()); above, since getArrayLength already asserts that.)

@@ +2170,5 @@
> +        if (*regs.pc == JSOP_SPREADNEW) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TOO_MANY_CON_SPREADARGS);
> +        } else {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TOO_MANY_FUN_SPREADARGS);
> +        }

Please write it like this:

    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
                         op == JSOP_SPREADNEW ? JSMSG_TOO_MANY_CON_ARGS
                                              : JSMSG_TOO_MANY_FUN_ARGS);

@@ +2176,5 @@
> +    }
> +
> +    InvokeArgsGuard args;
> +    if (!cx->stack.pushInvokeArgs(cx, length, &args))
> +        return false;

goto error, not return false.

@@ +2184,5 @@
> +
> +    if (!GetElements(cx, aobj, length, args.array()))
> +        goto error;
> +
> +    if (*regs.pc == JSOP_SPREADNEW) {

Replace `*regs.pc` with `op` here and in the other two places.

@@ +2190,5 @@
> +            goto error;
> +    } else if (*regs.pc == JSOP_SPREADCALL) {
> +        if (!Invoke(cx, args))
> +            goto error;
> +    } else if (*regs.pc == JSOP_SPREADEVAL) {

Instead of a third if, write:

    } else {
        JS_ASSERT(op == JSOP_SPREADEVAL);
        ...

@@ +2202,5 @@
> +    }
> +
> +    regs.sp -= 2;
> +    regs.sp[-1] = args.rval();
> +    TypeScript::Monitor(cx, script, regs.pc, regs.sp[-1]);

The type monitoring stuff is another thing I have to ask Brian about.
Comment 16 Tooru Fujisawa [:arai] 2013-06-20 11:55:05 PDT
Thank you for your comment!

I'll update the patch in a few days.

>It's OK to reuse JSMSG_TOO_MANY_CON_ARGS and JSMSG_TOO_MANY_FUN_ARGS.
It seems that those are SyntaxError.
I think we have to throw RangeError, because it's thrown at run time.
Or perhaps, is it more appropriate to make those into RangeError?
Comment 17 Jason Orendorff [:jorendorff] 2013-06-20 14:28:24 PDT
(In reply to Tooru Fujisawa [:arai] from comment #16)
> >It's OK to reuse JSMSG_TOO_MANY_CON_ARGS and JSMSG_TOO_MANY_FUN_ARGS.
> It seems that those are SyntaxError.
> I think we have to throw RangeError, because it's thrown at run time.
> Or perhaps, is it more appropriate to make those into RangeError?

Oh, I see!

We're inconsistent about this; sometimes we throw InternalError, sometimes RangeError.

RangeError will be fine. Thanks.
Comment 18 Tooru Fujisawa [:arai] 2013-06-21 09:28:50 PDT
Created attachment 765945 [details] [diff] [review]
address review comments

Changed bytecode length.
With changing it, I removed JSOP_SPREADCALL case in foldValue in js/src/jsanalyze.cpp.
GET_ARGC is used in this function, so I tried to figure out how it is used, but it is never called.

"spsProfiler.updatePC" and "TypeScript::Monitor" are stay same.

Added basic tests for spreadeval:
  js/src/jit-test/tests/basic/spread-call-eval.js

>- test that f(...[undefined]) returns [undefined] and not an empty array.
>- test using spread syntax to call a function without rest arguments
>- test using spread syntax to call a function with argument defaults
>- test other iterable objects besides arrays:
>    assertEqArray(f(...Set([1, 2])), [1, 2]);
>- test assignment expressions in spread operands:
>    f(...a=b)

Added them, and merged common tests of spreadcall, spreadnew and spreadcall for funcall into:
  js/src/jit-test/tests/basic/spread-call.js

tests for spreadeval and spreadcall for funapply cannot be merged, so added them in each file:
  js/src/jit-test/tests/basic/spread-call-eval.js
  js/src/jit-test/tests/basic/spread-call-funapply.js

>- test other iterable objects besides arrays:

Tested following:
  Iterator
  Set
  Map
  String
  typed array (Int32Array)
  Object which has iterator method
  Generator (JS 1.8's Generator, not Harmony's one)

>- test what happens when the object isn't iterable:
>    assertThrowsInstanceOf(() => Math.sin(...{}), TypeError);

Added in:
  js/src/jit-test/tests/basic/spread-call-not-iterable.js

Tested following:
  Boolean
  Date
  Function
  Number
  Object (with various error)
  RegExp
  Error

>Change the existing spread-array tests too.

Changed:
  js/src/jit-test/tests/basic/spread-array.js
  js/src/jit-test/tests/basic/spread-array-invalid-syntax.js
Comment 19 Jason Orendorff [:jorendorff] 2013-07-01 16:46:07 PDT
Comment on attachment 765945 [details] [diff] [review]
address review comments

Review of attachment 765945 [details] [diff] [review]:
-----------------------------------------------------------------

Arai, this patch doesn't apply cleanly anymore, due to rapid changes in the JS engine. Can you update it?

- The new way to write:   aobj->getArrayLength()   is:   aobj->as<ArrayObject>().length()

- Some code you were patching in jsscriptinlines.h was moved into js::CurrentScriptFileLineOrigin in jsscript.cpp.

- InvokeArgsGuard is now called InvokeArgs, and the API has changed; see bug 881902 for details.

As soon as you upload the new patch, please set r?jdemooij@mozilla.com for a closer review on the JIT code.

This looks great to me.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6015,1 @@
>  #endif

Yep. This is much nicer than what we had!
Comment 20 Jason Orendorff [:jorendorff] 2013-07-01 17:05:11 PDT
Arai, another change: bug 852762 made it so that arrow functions have 'arguments' after all. spread-call-this.js must be updated.

Your tests discovered bug 889158. I'm not entirely sure how I will fix that yet.
Comment 21 Tooru Fujisawa [:arai] 2013-07-02 08:51:04 PDT
Created attachment 770208 [details] [diff] [review]
address review comments

>Arai, another change: bug 852762 made it so that arrow functions have 'arguments' after all. spread-call-this.js must be updated.
>Your tests discovered bug 889158. I'm not entirely sure how I will fix that yet.
I changed assertions of arrow function's arguments.callee in spread-call-this.js, and commented them out for now.

Then, I separated test for "this" and arguments.callee in strict mode and non-strict mode:
  js/src/jit-test/tests/basic/spread-call-this.js
  js/src/jit-test/tests/basic/spread-call-this-strict.js
Comment 22 Jan de Mooij [:jandem] 2013-07-02 11:12:58 PDT
Comment on attachment 770208 [details] [diff] [review]
address review comments

Review of attachment 770208 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! r=me with comments below addressed.

::: js/src/jsinferinlines.h
@@ +304,5 @@
>  IsInlinableCall(jsbytecode *pc)
>  {
>      JSOp op = JSOp(*pc);
>  
> +    // CALL, SPREADCALL, FUNCALL, FUNAPPLY, EVAL, SPREADEVAL (Standard callsites)

Can you undo the changes to this function? It looks like this function is only used by some Ion asserts nowadays and since we don't JIT-compile these ops yet it's best to be conservative.

::: js/src/jsopcode.cpp
@@ +2036,5 @@
>       */
>  
> +    if (*pc == JSOP_CALL)
> +        pc += JSOP_CALL_LENGTH;
> +    else if (*pc != JSOP_SPREADCALL)

==, not !=

::: js/src/jsopcode.tbl
@@ +90,5 @@
>  OPDEF(JSOP_TYPEOF,    39, js_typeof_str,NULL,         1,  1,  1, JOF_BYTE|JOF_DETECTING)
>  OPDEF(JSOP_VOID,      40, js_void_str,  NULL,         1,  1,  1, JOF_BYTE)
>  
> +/* spreadcall variant of JSOP_CALL */
> +OPDEF(JSOP_SPREADCALL,41, "spreadcall", NULL,         1,  3,  1, JOF_BYTE|JOF_INVOKE|JOF_TYPESET)

Also update XDR_BYTECODE_VERSION in vm/Xdr.h (just increment the RHS, 148 -> 149 etc)
Comment 23 Tooru Fujisawa [:arai] 2013-07-02 17:00:40 PDT
Created attachment 770531 [details] [diff] [review]
address review comments

Thanks for your comment!
Comment 24 Jan de Mooij [:jandem] 2013-07-03 04:24:00 PDT
Comment on attachment 770531 [details] [diff] [review]
address review comments

Review of attachment 770531 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks.
Comment 25 Jan de Mooij [:jandem] 2013-07-03 12:21:53 PDT
Pushed to Try, as requested on IRC:

https://tbpl.mozilla.org/?tree=Try&rev=147b824a80ec
Comment 26 Tooru Fujisawa [:arai] 2013-07-03 17:11:10 PDT
Sorry, spread-call-recursion.js fails with TIMEOUT.

I noticed that limit of the number of recursion was changed in bug 881902, and now it does not depend on the number of argument.
So, this test is now meaningless, just time consuming and raise TIMEOUT in some case.
I'd like to remove this test, and try again.

I'll attach a fixed patch after all tests done.
Comment 27 Tooru Fujisawa [:arai] 2013-07-04 18:46:58 PDT
Created attachment 771521 [details] [diff] [review]
fix TIMEOUT

I decreased the number of arguments in spread-call-recursion.js,
now it simply tests over-recursion with spreadcall.
Call with many arguments does not decrease the maximum number of recursion,
so a few arguments are enough.

Then, I separated spread-call-maxarg.js to following 5 files:
  spread-call-maxarg.js
  spread-call-maxarg-eval.js
  spread-call-maxarg-funapply.js
  spread-call-maxarg-funcall.js
  spread-call-maxarg-new.js
because those tests takes too long time and sometimes it fails with TIMEOUT.

Also, I replaced ARGS_LENGTH_MAX with getMaxArgs().
Comment 28 Tooru Fujisawa [:arai] 2013-07-05 11:04:15 PDT
Created attachment 771661 [details] [diff] [review]
remove over-redundant tests to decrease test time

Discussed on IRC,
and note the difference between current draft spec (section 11.2.5) and current implementation,
and the problem about the time taken to run spread-call-maxarg.js.

Current implementation uses JSOP_SPREAD opcode to spread arguments, which is added in harmony:spreadarray.
I think it's better to use same operation for same notation.

Then, it does not call Get(spreadObj, "length") and Get(spreadObj, ToString(n)).
This is also the difference of spreadarray between current draft spec (section 11.1.4.1) and current implementation,
as mentioned in js/src/jit-test/tests/basic/spread-array.js

According to current draft spec, we can get the length of spreadObj by calling Get(spreadObj, "length")
and check the length of arguments before doing actual iteration.

Current implementation uses Iterator.
We cannot get length of arguments before actual iteration.

spread-call-maxarg.js takes so long time to spreading too long arguments  array (500001 elements, for now),
but it can be significantly faster if we use dedicated opcode which checks the length before actual iteration, instead of JSOP_SPREAD.

I'm not sure it is better or not to change to follow current draft spec
(either only spreadcall or both spreadarray and spreadcall),
if draft spec could be changed to follow current implementation,
as also mentioned in spread-array.js.

I think it's not a nice idea to change current implementation only for checking the length of arguments.
This does not improve performance in normal operation. only in a error check, only for decreasing the time taken to test.

Anyway, current test cases may be over-redundant for current implementation,
and can be reduced to following 3 calls (there are 8 calls in Attachment #771521 [details] [diff]):
  f(...a);     // for JSOP_SPREADCALL, where f = function() {}
  new f(...a); // for JSOP_SPREADNEW
  eval(...a);  // for JSOP_SPREADEVAL
each call takes about 10 seconds with debug build on my machine (Core 2 Duo 2.0GHz, Mac OS X) with any option.
It takes about 1 minutes and 40 seconds for above 3 calls with --tbpl option.

So, I remove following:
  spread-call-maxarg-eval.js
  spread-call-maxarg-funapply.js
  spread-call-maxarg-funcall.js
  spread-call-maxarg-new.js
and merge them again to:
  spread-call-maxarg.js
I think single file is enough for those.

no other changes from Attachment #771521 [details] [diff].
Comment 29 Tooru Fujisawa [:arai] 2013-07-12 11:12:26 PDT
Created attachment 774795 [details] [diff] [review]
updated for latest trunk

just updated patch for latest trunk.
Comment 30 Jason Orendorff [:jorendorff] 2013-07-12 17:09:43 PDT
OK. I will try to land this Monday.

I am still trying to get bug 717379 to run green on the Try server. This is the next thing after that.
Comment 31 Tooru Fujisawa [:arai] 2013-07-16 10:33:18 PDT
Created attachment 776503 [details] [diff] [review]
updated for latest trunk

Just updated patch for latest trunk.
Comment 32 Tooru Fujisawa [:arai] 2013-07-24 05:33:38 PDT
Created attachment 780351 [details] [diff] [review]
updated for latest trunk

update XDR_BYTECODE_VERSION.
Comment 33 Tooru Fujisawa [:arai] 2013-08-13 12:59:39 PDT
Created attachment 789774 [details] [diff] [review]
updated for latest trunk

Use TokenStream::Operand, increment XDR_BYTECODE_VERSION,
and reflect some changes around the my patch.
Comment 34 Brandon Benvie [:benvie] 2013-08-22 12:13:52 PDT
I think you need to add the r? for jorendorff back.
Comment 35 Tooru Fujisawa [:arai] 2013-08-28 20:56:05 PDT
Created attachment 797040 [details] [diff] [review]
updated for latest trunk

I'm sorry to trouble you.
Here is updated patch.
Remove some code for removed function in jsanalyze.cpp,
and increment XDR_BYTECODE_VERSION.
Comment 36 Tooru Fujisawa [:arai] 2013-09-02 05:08:57 PDT
Created attachment 798498 [details] [diff] [review]
updated for latest trunk

Nothing has changed but reflect changes (namespace {) in jsinfer.cpp.
Comment 37 Tooru Fujisawa [:arai] 2013-09-04 07:07:27 PDT
Created attachment 799484 [details] [diff] [review]
updated for latest trunk

Removed patch in jsinfer.cpp.
Comment 38 Jason Orendorff [:jorendorff] 2013-09-12 15:32:56 PDT
Comment on attachment 799484 [details] [diff] [review]
updated for latest trunk

Review of attachment 799484 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is almost done! Please address the minor comments below and upload a new patch r?me.

In BytecodeEmitter.cpp, EmitTree:
>+ #if JS_HAS_GENERATORS
>+       case PNK_ARRAYCOMP:
>+        ok = EmitArrayComp(cx, bce, pn);
>+        break;
>+ #endif

No #if here. PNK_ARRAYCOMP is always on.

In spread-call-eval.js:
>+try {
>+  eval(...["("]); // line 19
>+} catch (e) {
>+  assertEq(e.lineNumber, 19);
>+}

I learned a trick for doing this kind of thing:

    var line0 = Error().lineNumber;
    try {               // line0 + 1
        eval(...["("]); // line0 + 2
    } catch (e) {
        assertEq(e.lineNumber, line0 + 2);
    }

Less fragile. You can use it or not, your choice.

>+assertThrowsInstanceOf(() => eval("a + b", ...null), TypeError);
>+assertThrowsInstanceOf(() =>eval("a + b", ...undefined), TypeError);

Insert a space after => on the second quoted line above.

In spread-call-funapply.js:
>+  assertThrowsInstanceOf(() => f.apply(null, ...null, [1, 2, 3]), TypeError);
>+  assertThrowsInstanceOf(() =>f.apply(null, ...undefined, [1, 2, 3]), TypeError);

Same here.

In spread-call-new.js:
>+function g(a, b, c) {
>+  this.value = [a, b, c];
>+  assertEq(Object.getPrototypeOf(this), g.prototype);
>+  assertEq(arguments.callee, g);

Check arguments.length too.

In spread-call-this-strict.js:
>+let g1 = function() {
>+  assertEq(this, undefined);
>+};
>+g1([]);

None of the function calls in this file are using spread-call syntax. Did you mean `g1(...[])`?

>+  f2: x => {
>+    assertEq(this, global);
>+    let g1 = function() {
>+      assertEq(this, undefined);
>+    };
>+    g1([]);
[...]
>+  f4: x => {
>+    assertEq(this, global);
>+    let g1 = function() {
>+      assertEq(this, undefined);
>+    };
>+    g1([]);
[...]

f2 and f4 look identical. Please common them up if possible.

Same thing in spread-call-this.js.

In js.msg:
>-MSG_DEF(JSMSG_TOO_MANY_CON_ARGS,       44, 0, JSEXN_SYNTAXERR, "too many constructor arguments")
>-MSG_DEF(JSMSG_TOO_MANY_FUN_ARGS,       45, 0, JSEXN_SYNTAXERR, "too many function arguments")
>+MSG_DEF(JSMSG_TOO_MANY_CON_ARGS,       44, 0, JSEXN_RANGEERR, "too many constructor arguments")
>+MSG_DEF(JSMSG_TOO_MANY_FUN_ARGS,       45, 0, JSEXN_RANGEERR, "too many function arguments")

We discussed this on IRC right after my first review. You were right.
They should be two different types. I'm sorry for the noise. Please
change it back.

In jsanalyze.cpp, ScriptAnalysis::analyzeBytecode:
>           /* Additional opcodes which can be both compiled both normally and inline. */
>           case JSOP_ARGUMENTS:
>           case JSOP_CALL:
>+          case JSOP_SPREADCALL:
>           case JSOP_NEW:
>+          case JSOP_SPREADNEW:
>           case JSOP_FUNCALL:

Wait, these can't be JIT-compiled normally or inline. Right?

In jsscript.h, PCToLineNumber:
>  * linear scan to compute line number when the caller guarnatees that the
>- * script compilation occurs at a JSOP_EVAL.
>+ * script compilation occurs at a JSOP_EVAL/JSOP_SPREADEVAL.

While you're editing this comment, could you please fix the spelling of
"guarantees" on the previous line?
Comment 39 Tooru Fujisawa [:arai] 2013-09-13 05:37:37 PDT
Created attachment 804429 [details] [diff] [review]
address review comments

Here is fixed patch.

(In reply to Jason Orendorff [:jorendorff] from comment #38)
> In BytecodeEmitter.cpp, EmitTree:
> >+ #if JS_HAS_GENERATORS
> >+       case PNK_ARRAYCOMP:
> >+        ok = EmitArrayComp(cx, bce, pn);
> >+        break;
> >+ #endif
> 
> No #if here. PNK_ARRAYCOMP is always on.
Okay, removed.

> I learned a trick for doing this kind of thing:
> 
>     var line0 = Error().lineNumber;
>     try {               // line0 + 1
>         eval(...["("]); // line0 + 2
>     } catch (e) {
>         assertEq(e.lineNumber, line0 + 2);
>     }
> 
> Less fragile. You can use it or not, your choice.
Wow, nice idea!
Of course I use it :)

> In spread-call-new.js:
> >+function g(a, b, c) {
> >+  this.value = [a, b, c];
> >+  assertEq(Object.getPrototypeOf(this), g.prototype);
> >+  assertEq(arguments.callee, g);
> 
> Check arguments.length too.
Created spread-call-length.js for checking argument.length for
both JSOP_SPREADCALL and JSOP_SPREADNEW.

> In spread-call-this-strict.js:
> >+let g1 = function() {
> >+  assertEq(this, undefined);
> >+};
> >+g1([]);
> 
> None of the function calls in this file are using spread-call syntax. Did
> you mean `g1(...[])`?
Oops! I've fixed.

> >+  f2: x => {
> >+    assertEq(this, global);
> >+    let g1 = function() {
> >+      assertEq(this, undefined);
> >+    };
> >+    g1([]);
> [...]
> >+  f4: x => {
> >+    assertEq(this, global);
> >+    let g1 = function() {
> >+      assertEq(this, undefined);
> >+    };
> >+    g1([]);
> [...]
> 
> f2 and f4 look identical. Please common them up if possible.
> 
> Same thing in spread-call-this.js.
I also removed g4.

> In js.msg:
> >-MSG_DEF(JSMSG_TOO_MANY_CON_ARGS,       44, 0, JSEXN_SYNTAXERR, "too many constructor arguments")
> >-MSG_DEF(JSMSG_TOO_MANY_FUN_ARGS,       45, 0, JSEXN_SYNTAXERR, "too many function arguments")
> >+MSG_DEF(JSMSG_TOO_MANY_CON_ARGS,       44, 0, JSEXN_RANGEERR, "too many constructor arguments")
> >+MSG_DEF(JSMSG_TOO_MANY_FUN_ARGS,       45, 0, JSEXN_RANGEERR, "too many function arguments")
> 
> We discussed this on IRC right after my first review. You were right.
> They should be two different types. I'm sorry for the noise. Please
> change it back.
I added following 2 messages again.
>+MSG_DEF(JSMSG_TOO_MANY_CON_SPREADARGS, 362, 0, JSEXN_RANGEERR, "too many constructor arguments")
>+MSG_DEF(JSMSG_TOO_MANY_FUN_SPREADARGS, 363, 0, JSEXN_RANGEERR, "too many function arguments")

> In jsanalyze.cpp, ScriptAnalysis::analyzeBytecode:
> >           /* Additional opcodes which can be both compiled both normally and inline. */
> >           case JSOP_ARGUMENTS:
> >           case JSOP_CALL:
> >+          case JSOP_SPREADCALL:
> >           case JSOP_NEW:
> >+          case JSOP_SPREADNEW:
> >           case JSOP_FUNCALL:
> 
> Wait, these can't be JIT-compiled normally or inline. Right?
Oh, you are right.
I removed them.

> In jsscript.h, PCToLineNumber:
> >  * linear scan to compute line number when the caller guarnatees that the
> >- * script compilation occurs at a JSOP_EVAL.
> >+ * script compilation occurs at a JSOP_EVAL/JSOP_SPREADEVAL.
> 
>While you're editing this comment, could you please fix the spelling of
>"guarantees" on the previous line?
Sure.
Comment 40 Jason Orendorff [:jorendorff] 2013-09-13 06:47:01 PDT
I'll get to this today!
Comment 41 Jason Orendorff [:jorendorff] 2013-09-13 08:23:05 PDT
Try Server: https://tbpl.mozilla.org/?tree=Try&rev=d5978c3afc72
Comment 42 Jason Orendorff [:jorendorff] 2013-09-13 14:11:43 PDT
It seems to cause crashes in test/build/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_webappsActor.js

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=29458f6c07e5

But it passes for me locally. :-\
Comment 43 Tooru Fujisawa [:arai] 2013-09-14 08:36:30 PDT
I'm trying to build on Mac, however I have trouble with building,
failed to link with ICU, and it seems to take more time to reproduce and start debugging.
(I've built and tested on Linux these days...)

By the way, the xpcshell.ini for test_webappsActor.js was changed in Bug 905881,
and test_webappsActor.js will be skipped on desktop, in latest mozilla-central.
So, this test will not run in next try.

>--- a/toolkit/devtools/apps/tests/unit/xpcshell.ini
>+++ b/toolkit/devtools/apps/tests/unit/xpcshell.ini
>@@ -1,8 +1,9 @@
> [DEFAULT]
> head = head_apps.js
> tail = tail_apps.js
> 
> [test_webappsActor.js]
>+skip-if = (os == "win" || "linux" || "mac")
> [test_appInstall.js]
> # Persistent failures.
> skip-if = true
Comment 44 Tooru Fujisawa [:arai] 2013-09-14 12:25:03 PDT
Oops, I found that firefox-26.0a1.en-US.mac64.tests.zip contains xpcshell,
I could not find it in firefox-26.0a1.en-US.mac64.dmg, and tried to build it.

I've downloaded firefox-26.0a1.en-US.mac64.tests.zip and firefox-26.0a1.en-US.mac64.dmg from
  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jorendorff@mozilla.com-d5978c3afc72/try-macosx64-debug/

then, copy xpcshell, components and plugins to FirefoxNightlyDebug.app/Contents/MacOS/

  mkdir tests
  cd tests
  unzip firefox-26.0a1.en-US.mac64.tests.zip
  cd bin
  cp xpcshell ../FirefoxNightlyDebug.app/Contents/MacOS/
  cp -R components ../FirefoxNightlyDebug.app/Contents/MacOS/
  cp -R plugins ../FirefoxNightlyDebug.app/Contents/MacOS/
  cd ../xpcshell

and ran following test:

  python -u ./runxpcshelltests.py \
  --test-plugin-path=../FirefoxNightlyDebug.app/Contents/MacOS/plugins \
  --manifest=./tests/all-test-dirs.list \
  ../FirefoxNightlyDebug.app/Contents/MacOS/xpcshell

it passes all tests, including test_webappsActor.js... :O
I've tested on on Mac OS X 10.7.5.

Hmm, I wonder how I can reproduce the crash...
Comment 45 Tooru Fujisawa [:arai] 2013-09-15 11:11:58 PDT
If I run test with another build (I used 7cc165920e0f) before running test with d5978c3afc72,
test_webappsActor.js fails with d5978c3afc72, it shows different error, but this may be related.

> INFO | Using at most 8 threads.
> TEST-INFO | skipping /Users/arai/_Downloads/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_appInstall.js | skip-if: true
> TEST-INFO | /Users/arai/_Downloads/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_webappsActor.js | running test ...
> TEST-INFO | test_webappsActor.js | Test failed or timed out, will retry.
> Retrying tests that failed when run in parallel.
> TEST-INFO | /Users/arai/_Downloads/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_webappsActor.js | running test ...
> TEST-UNEXPECTED-FAIL | /Users/arai/_Downloads/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_webappsActor.js | test failed (with xpcshell return code: 1), see following log:
> >>>>>>>
> TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1)
> WARNING: No disk space watcher component available!: file ../../../dom/indexedDB/IndexedDatabaseManager.cpp, line 207
> TEST-INFO | (xpcshell/head.js) | test run_next_test 0 pending (2)
> TEST-INFO | (xpcshell/head.js) | test MAIN run_test finished (2)
> TEST-INFO | (xpcshell/head.js) | running event loop
> TEST-INFO | /Users/arai/_Downloads/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_webappsActor.js | Starting testLaunchInexistantApp
> TEST-INFO | (xpcshell/head.js) | test testLaunchInexistantApp pending (2)
> ###!!! ASSERTION: Existing entry in disk StartupCache.: 'zipItem == nullptr', file ../../startupcache/StartupCache.cpp, line 369
> DumpCompleteHeap+0x00011600 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0123A290]
> DumpCompleteHeap+0x00012ED2 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0123BB62]
> NS_InvokeByIndex+0x0000021D [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01DF125D]
> NS_NewBackstagePass(BackstagePass**)+0x00014A78 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01203E48]
> NS_NewBackstagePass(BackstagePass**)+0x00012DD2 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x012021A2]
> NS_NewBackstagePass(BackstagePass**)+0x0001D493 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0120C863]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x000400E7 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F9517]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00039C1B [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F304B]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00036A72 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025EFEA2]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00030261 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025E9691]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00039C2B [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F305B]
> js::IsInRequest(JSContext*)+0x00006063 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x027A6F43]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x000400E7 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F9517]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00039C1B [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F304B]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00036A72 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025EFEA2]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00030261 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025E9691]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00039C2B [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F305B]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x0003A2D4 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F3704]
> js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&)+0x000000E4 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0283A074]
> js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&)+0x000001DE [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0289B55E]
> js::AppendUnique(JSContext*, JS::AutoIdVector&, JS::AutoIdVector&)+0x0000242E [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0284557E]
> js::AppendUnique(JSContext*, JS::AutoIdVector&, JS::AutoIdVector&)+0x00004F1A [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0284806A]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x000400E7 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F9517]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00039C1B [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F304B]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00036A72 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025EFEA2]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00030261 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025E9691]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x00039C2B [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F305B]
> JS_DefineDebuggerObject(JSContext*, JSObject*)+0x0003A2D4 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x025F3704]
> JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*)+0x000003CB [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x02750AAB]
> NS_NewBackstagePass(BackstagePass**)+0x0000ABA9 [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x011F9F79]
> NS_NewBackstagePass(BackstagePass**)+0x00004A7F [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x011F3E4F]
> nsXPTCStubBase::Stub249()+0x000002EC [/Users/arai/_Downloads/tests/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01DF255C]
> ###!!! ASSERTION: Existing entry in disk StartupCache.: 'zipItem == nullptr', file ../../startupcache/StartupCache.cpp, line 369
> Hit MOZ_CRASH() at ../../../memory/mozalloc/mozalloc_abort.cpp:30
> <<<<<<<
> PROCESS-CRASH | /Users/arai/_Downloads/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_webappsActor.js | application crashed [Unknown top frame]
> Crash dump filename: /var/folders/d_/7wbprchj0095yjkm38shkx6r0000gn/T/tmpzWnz_t/A0609769-81A1-4AD1-84C9-98B3EE55F4DE.dmp
> No symbols path given, can't process dump.
> MINIDUMP_STACKWALK not set, can't process dump.
> INFO | Result summary:
> INFO | Passed: 0
> INFO | Failed: 1
> INFO | Todo: 0
> INFO | Retried: 1

After running 7cc165920e0f, startupCache with XDR_BYTECODE_VERSION = 0xb973c0de - 151 was created in:
  ~/Library/Caches/TemporaryItems/startupCache/startupCache.8.little

When I remove startupCache file, test with d5978c3afc72 passes and startupCache with XDR_BYTECODE_VERSION = 0xb973c0de - 152 was created.
Comment 46 Steve Fink [:sfink] [:s:] 2013-09-17 08:46:18 PDT
I think you can ignore the webapps failure. From reading that bug, it seems like there's some sort of packaging or installation issue. At any rate, a current try push looks good: 

https://tbpl.mozilla.org/?tree=Try&rev=e369f2ae95d2
Comment 47 Tooru Fujisawa [:arai] 2013-09-17 09:15:23 PDT
Thank you sfink for checking!
Yeah, the failure will be fixed in Bug 916874.

I should have written the progress here.

The failure is caused by bug in head_apps.js for test_webappsActor.js,
startupCache uses non-temporary file and older cache is used.
Currently the test is skipped, so the failure will not happen in try server.
Comment 48 Jason Orendorff [:jorendorff] 2013-09-17 10:49:27 PDT
Oh, sorry I didn't comment earlier. I already pushed this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d780eba18377
Comment 49 Ryan VanderMeulen [:RyanVM] 2013-09-17 17:41:51 PDT
https://hg.mozilla.org/mozilla-central/rev/d780eba18377
Comment 50 Tooru Fujisawa [:arai] 2013-09-18 10:27:15 PDT
Updated following pages:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Spread_operator
https://developer.mozilla.org/en-US/docs/Web/JavaScript/ECMAScript_6_support_in_Mozilla
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/27

"TC39 Consensus" field in ECMAScript_6_support_in_Mozilla is empty because I'm not sure about that.
Is it okay?
Comment 51 Jason Orendorff [:jorendorff] 2013-09-18 11:20:52 PDT
I updated the wiki page.

This feature has consensus.

The wiki now says "Not compliant with the current specification, but the bug is in the specification, not the Firefox implementation" with a link to https://bugs.ecmascript.org/show_bug.cgi?id=1113 .
Comment 52 Rick Waldron [:rwaldron] 2013-09-18 14:21:47 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #51)
> I updated the wiki page.
> 
> This feature has consensus.
> 
> The wiki now says "Not compliant with the current specification, but the bug
> is in the specification, not the Firefox implementation" with a link to
> https://bugs.ecmascript.org/show_bug.cgi?id=1113 .

Thanks for bumping that.

I've removed the section about converting array-likes to arrays from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Spread_operator#Converting_any_array_like

Note You need to log in before you can comment on or make changes to this bug.