574132 - (harmony:restparams) Prototype Harmony's rest parameters in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

See the bug URL. We want tracing, jaegering, decompilation, as usual. Death to the arguments object! ;-)

/be

Comment 1

[Dave Herman [:dherman]]

Attachment: lexing and parsing

Hacked up the lexer and parser changes on the train this morning. Just some first steps to build on, for whoever.

Dave

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 454081 [details] [diff] [review]
lexing and parsing

>+MSG_DEF(JSMSG_MISPLACED_REST_PARAM,   251, 0, JSEXN_SYNTAXERR, "rest-parameter must be be last formal parameter")

s/be be/be the/

>+++ b/js/src/jsparse.cpp
>@@ -1490,16 +1490,18 @@ DefineArg(JSParseNode *pn, JSAtom *atom,
>         if (!argsbody)
>             return false;
>         argsbody->pn_type = TOK_ARGSBODY;
>         argsbody->pn_op = JSOP_NOP;
>         argsbody->makeEmpty();
>         pn->pn_body = argsbody;
>     }
>     argsbody->append(argpn);
>+    if (tc->flags & TCF_FUN_REST_PARAMETER)
>+        argsbody->pn_xflags |= PNX_SPREAD;

Is PNX_SPREAD really needed, given TCF_FUN_REST_PARAMETER? The tcflags are in the funbox pointed at by the function node, so it seems redundant.

>+            if (JS_UNLIKELY(rest)) {

We have some kind of agreement to avoid adding JS_{,UN}LIKELY without profile data showing a win. Suspect PGO if not default compiler analysis will suffice here, if the cost of a mispredict isn't in the noise (branch target is near enough that it's in the pipe => minor penalty on mispredict). These macros impair readability.

>@@ -8153,17 +8173,19 @@ Parser::primaryExpr(TokenKind tt, JSBool
> 
>       case TOK_NAME:
>         pn = NameNode::create(tokenStream.currentToken().t_atom, tc);
>         if (!pn)
>             return NULL;
>         JS_ASSERT(tokenStream.currentToken().t_op == JSOP_NAME);
>         pn->pn_op = JSOP_NAME;
> 
>-        if ((tc->flags & (TCF_IN_FUNCTION | TCF_FUN_PARAM_ARGUMENTS)) == TCF_IN_FUNCTION &&
>+        /* Rest-parameters opt out of special |arguments|. */
>+        if ((tc->flags & (TCF_IN_FUNCTION | TCF_FUN_PARAM_ARGUMENTS | TCF_FUN_REST_PARAMETER)) ==
>+            TCF_IN_FUNCTION &&

Nit: == on second line of condition, we put && and || at end of line but try to put other ops (save assignment ops) on overflow lines.

>+++ b/js/src/jsscan.cpp
>@@ -1279,16 +1279,19 @@ TokenStream::getTokenInternal()
>       case '{':  tt = TOK_LC; break;
>       case '}':  tt = TOK_RC; break;
>       case '(':  tt = TOK_LP; break;
>       case ')':  tt = TOK_RP; break;
>       case ',':  tt = TOK_COMMA; break;
>       case '?':  tt = TOK_HOOK; break;
> 
>       case '.':
>+        if (matchChar(c) && matchChar(c))
>+            tt = TOK_ELLIPSIS;
>+        else
> #if JS_HAS_XML_SUPPORT
>         if (matchChar(c))
>             tt = TOK_DBLDOT;
>         else
> #endif
>             tt = TOK_DOT;

Bug: ".." will lex as TOK_DOT because of the two new matchChar(c) calls you added, only the first succeeds, leaving the else if (matchChar(c) for E4X's descendant operator failing (as did the second matchChar(c), the one on the right of && in the first if's condition).

Need to nest if conditions and split matchChar(c) cases up to cover each transitions in the state machine exactly once.

Great to see motion on this! Death to arguments, I <3 the idea of arguments becoming non-predefined when a rest param is used.

/be

Comment 3

[Dave Herman [:dherman]]

> Is PNX_SPREAD really needed, given TCF_FUN_REST_PARAMETER? The tcflags are in
> the funbox pointed at by the function node, so it seems redundant.

Ah, thanks, I didn't know about that. (It might still be needed for bug 574130, though.)

> Need to nest if conditions and split matchChar(c) cases up to cover each
> transitions in the state machine exactly once.

Got it. I'll update the patch.

Thanks,
Dave

Comment 4

[Dave Herman [:dherman]]

Attachment: lexing and parsing

Addressed Brendan's mini-review.

Dave

Comment 5

[Dave Herman [:dherman]]

This will still need logic to suppress binding |arguments| in heavyweight functions. This is the current behavior and wrong:

js> arguments = ["outer"]
["outer"]
js> function foo(...rest) { return eval("arguments[0]") }
js> function bar(...rest) { with ({}) { return arguments[0] } }
js> foo("inner")
"inner"
js> bar("inner")
"inner"

Dave

Comment 6

[:Benjamin Peterson]

Attachment: patch implementing the current standard

Comment 7

[:Benjamin Peterson]

Attachment: implementation

Comment 8

[Jason Orendorff [:jorendorff]]

Exciting! I'll look at this today.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 624266 [details] [diff] [review]
implementation

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

Looking great. The main thing now is the JIT stuff. You can work with luke on that.

An additional thing to test is

  var g = newGlobal('new-compartment');
  g.eval("function f(...rest) { return rest; }");
  var a = g.f(1, 2, 3);
  assertEq(a instanceof g.Array, true);

I think it'd be good to test that 'function f(...[x]) {}' and 'function f(...{x:x}) {}' are SyntaxErrors.

::: js/src/frontend/Parser.cpp
@@ +1309,5 @@
>                {
> +                if (processedRest) {
> +                    reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_PARAMETER_AFTER_REST);
> +                    return false;
> +                }

It seems like this could go at the top, before calling tokenStream.getToken(). At that point we've seen "... rest ," which is already wrong.

I think this means you can get rid of the local bool processedRest and just use hasRest.

::: js/src/jit-test/tests/arguments/rest-parameter.js
@@ +6,5 @@
> +assertEq((function () {}).hasRest, false);
> +assertEq(check.length, 1);
> +check([]);
> +check(['a', 'b'], 'a', 'b');
> +check(['a', 'b', 'c', 'd'], 'a', 'b', 'c', 'd');

Style nit: Make a separate file for each paragraph of about 10 lines of text, as long as you can do it without code duplication.

@@ +12,5 @@
> +function f(a, b, c, ...rest) {
> +    assertEq(a, 1);
> +    assertEq(b, undefined);
> +    assertEq(c, undefined);
> +    assertEq(rest.toString(), "");

You can assert that:
  Array.isArray(rest)
  rest.length === 0
  Object.getPrototypeOf(rest) === Array.prototype

@@ +17,5 @@
> +}
> +
> +f(1);
> +
> +g = (function (...rest) { return rest; })

Nit: semicolon here.

@@ +23,5 @@
> +
> +h = Function("a", "b", "c", "...rest", "return rest.toString();");
> +assertEq(h.hasRest, true);
> +assertEq(h.length, 3);
> +assertEq(h(1, 2, 3, 4, 5).toString(), "4,5");

This .toString() call is unnecessary since the function already returns rest.toString().

@@ +26,5 @@
> +assertEq(h.length, 3);
> +assertEq(h(1, 2, 3, 4, 5).toString(), "4,5");
> +
> +var offenders = [["..."], ["...rest"," x"], ["...rest", "...rest2"]];
> +for (var i = 0; i < offenders.length; i++) {

You could write:
  for (var arglist of offenders) {
Totally optional.

@@ +30,5 @@
> +for (var i = 0; i < offenders.length; i++) {
> +    try {
> +        eval("function x(" + offenders[i].join(", ") + ") {}");
> +        throw new Error("didn't throw SyntaxError");
> +    } catch (e if e instanceof SyntaxError) {}

Please load(libdir + "asserts.js"); at the top, and then:

  var ieval = eval;
  assertThrowsInstanceOf(function () { ieval("function x(" + offenders[i].join(", ") + ") {}"); }, SyntaxError);

Incidentally please add ["...rest", "[]"]. (Rest argument followed by a destructuring parameter with no bindings.)

@@ +34,5 @@
> +    } catch (e if e instanceof SyntaxError) {}
> +    try {
> +        Function.apply(Function, offenders[i].concat("return 0;"));
> +        throw new Error("didn't throw SyntaxError");
> +    } catch (e if e instanceof SyntaxError) {}

Nit: it would be more natural for the first argument here to be "this" or "null" rather than "Function".

  assertThrowsInstanceOf(function () { Function.apply(null, offenders[i].concat("return 0;")); }, SyntaxError);

And so on.

@@ +53,5 @@
> +} catch (e if e instanceof SyntaxError) {}
> +try {
> +    eval("function x(...rest) { eval('arguments'); }");
> +    throw new Error("didn't throw SyntaxError");
> +} catch (e if e instanceof SyntaxError) {}

Hmmmmmmm. This last one shouldn't throw a SyntaxError until it is called, right?

Also, note that eval-ing a function definition for x here would clobber the function definition on line 59, because in JS, unlike Python, function definitions aren't statements and they are all evaluated eagerly before the rest of the script runs.

::: js/src/js.msg
@@ +379,5 @@
>  MSG_DEF(JSMSG_QUERY_INNERMOST_WITHOUT_LINE_URL, 293, 0, JSEXN_TYPEERR, "findScripts query object has 'innermost' property without both 'url' and 'line' properties")
>  MSG_DEF(JSMSG_DEBUG_VARIABLE_NOT_FOUND, 294, 0, JSEXN_TYPEERR, "variable not found in environment")
> +MSG_DEF(JSMSG_PARAMETER_AFTER_REST,   295, 0, JSEXN_SYNTAXERR, "parameter after rest parameter")
> +MSG_DEF(JSMSG_NO_REST_NAME,           296, 0, JSEXN_SYNTAXERR, "no parameter name after ...")
> +MSG_DEF(JSMSG_ARGUMENTS_AND_REST,     297, 0, JSEXN_SYNTAXERR, "arguments may not be used in conjunction with a rest parameter")

This last message doesn't make it clear that we're talking about the arguments object. It sounds like we're saying the function's arguments can't be used.

Maybe "'arguments' object can't be used in a function with a rest parameter"?

::: js/src/jsapi.h
@@ +2194,1 @@
>  #define JSFUN_HEAVYWEIGHT       0x80    /* activation requires a Call object */

Nit: This seems out of order. Maybe put this new line just above the definition for 0x0200?

::: js/src/jsfun.cpp
@@ +336,2 @@
>          else
> +            v.setBoolean(fun->hasRest());

fn.hasRest seems reasonable enough, but it's not in the spec, right? We can chat with dherman about it (or maybe you've already done so).

@@ +1134,5 @@
>                   * TOK_ERROR, which was already reported.
>                   */
> +                if (hasRest) {
> +                    ReportCompileErrorNumber(cx, &ts, NULL, JSREPORT_ERROR,
> +                                             JSMSG_PARAMETER_AFTER_REST);

If tt is TOK_ERROR, then an error has already been reported on cx. So if that's the case, don't report again, just return false.

::: js/src/vm/Stack-inl.h
@@ -470,5 @@
>  
>      /* Include extra space to satisfy the method-jit stackLimit invariant. */
>      unsigned nvals = VALUES_PER_STACK_FRAME + script->nslots + StackSpace::STACK_JIT_EXTRA;
>  
> -    /* Maintain layout invariant: &formalArgs[0] == ((Value *)fp) - nformal. */

I think it's OK to keep this comment after the if (fun->hasRest()) block. In fact the invariant still holds, if instead of doing nformal-- you declare a new name for that.

@@ +486,5 @@
> +        unsigned ncopy = 2 + Min(nformal, args.length());
> +        PodCopy(dst, src, ncopy);
> +        if (args.length() < nformal)
> +            SetValueRangeToUndefined(firstUnused + 2 + args.length(), nformal - args.length());
> +        return reinterpret_cast<StackFrame *>(firstUnused + 2 + nformal + 1);

Very nice work.

We talked on IRC about possibly eliminating the copy here, but it's probably fine either way. It would be interesting to try measuring function call speed both ways.

More importantly, the methodjit inlines all this code into the function prologue, so we will need to teach the methodjit how to do a call to a hasRest function. This means all your tests should currently fail if you pass --jitflags=ma to the test runner or -m -a to the JS shell.

Comment 10

[:Benjamin Peterson]

Attachment: version 2 of implementation

- now implemented with opcode
- fixed some more cases of arguments detection
- removed hasRest function property for now

Comment 11

[:Benjamin Peterson]

Attachment: version 3

now with explosions on Function.arguments!

Comment 12

[:Benjamin Peterson]

Attachment: v4: makes the JSOP_REST opcode simpler

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 624965 [details] [diff] [review]
version 3

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

Almost done with v3, but I have to go. I'll finish this review today.

One more test idea: I mentioned on IRC, it'd be good to check that fn.apply and fn.call pass through extra arguments correctly.

Another thing to check is whether the debugger's eval-in-frame correctly rejects code that tries to refer to arguments. Like this:

var g = newGlobal('new-compartment');
g.eval("function f(...rest) { debugger; }");
var dbg = Debugger(g);
dbg.onDebuggerStatement = function (frame) {
    var result = frame.eval("arguments");
    assertEq("throw" in result, true);
    var result2 = frame.evalWithBindings("exc instanceof SyntaxError", {exc: result.throw});
    assertEq(result2.return, true);
};
g.f();

::: js/src/frontend/Parser.cpp
@@ +703,5 @@
>      }
>  
> +    BindingKind bindKind = tc->sc->bindings.lookup(context, arguments, NULL);
> +    switch (bindKind) {
> +    case NONE:

Style nit: indent case labels 2 spaces please.

@@ +719,5 @@
> +        if (bindKind != NONE && tc->sc->fun()->hasRest()) {
> +            reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_ARGUMENTS_AND_REST);
> +            return NULL;
> +        }
> +        /*

Style nit: blank line before the comment, please.

@@ +749,4 @@
>          }
> +        break;
> +    case ARGUMENT:
> +        break;

Heh! So stuff like this has to work:

function f1(...arguments) {}
function f2(arguments, ...rest) {}

Another test. This one I think is important, these corner cases will kill you if they aren't tested.

Yet another thing to test:

// 'eval' and 'arguments' are not valid rest-parameter names in strict mode.

"use strict";
assertThrowsInstanceOf(function () { eval("(function (...arguments) {})"); },
                       SyntaxError);
assertThrowsInstanceOf(function () { eval("(function (...eval) {})"); },
                       SyntaxError);

::: js/src/jit-test/tests/arguments/rest-disallow-arguments.js
@@ +19,5 @@
> +}
> +function h() {
> +    g.arguments;
> +}
> +g();

These days I try to throw a one-line comment in each test, but it is 100% optional.

// Functions with rest parameters can't use the arguments object.

I vote for a few blank lines between the tests here.

::: js/src/jit-test/tests/arguments/rest-invalid-syntax.js
@@ +1,5 @@
> +load(libdir + "asserts.js");
> +var ieval = eval;
> +var offenders = [["..."], ["...rest"," x"], ["...rest", "[x]"],
> +                 ["...rest", "...rest2"]];
> +for (var arglist in offenders.length) {

This line isn't right. The loop must be executing 0 times.

Use:  for (var arglist of offenders)   <--- note "of" rather than "in"

::: js/src/jit-test/tests/arguments/rest-nested-arguments.js
@@ +3,5 @@
> +        return arguments.length;
> +    }
> +    return nested;
> +}
> +assertEq(f()(1, 2, 3), 3);

Nice test case.

::: js/src/jsapi.h
@@ +2190,5 @@
>  
>  /* Function flags, internal use only, returned by JS_GetFunctionFlags. */
>  #define JSFUN_LAMBDA            0x08    /* expressed, not declared, function */
>  #define JSFUN_HEAVYWEIGHT       0x80    /* activation requires a Call object */
> +#define JSFUN_HAS_REST          0x0100  /* function has a rest (...) parameter */

Micro-nit: Move this down two more lines, next to JSFUN_CONSTRUCTOR.

::: js/src/jsfun.cpp
@@ +136,5 @@
>      if (JSID_IS_ATOM(id, cx->runtime->atomState.argumentsAtom)) {
> +        if (fun->hasRest()) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_FUNCTION_ARGUMENTS_AND_REST);
> +            return false;
> +        }

Good catch. Please add an assertion that fp->callee() in ArgumentsObject::create (vm/ArgumentsObject.cpp), to catch any other paths into here.

::: js/src/vm/Stack-inl.h
@@ +209,5 @@
> +{
> +    JS_ASSERT(fun()->hasRest());
> +    unsigned nformal = fun()->nargs - 1, nactual = numActualArgs();
> +    unsigned nrest = (nactual > nformal) ? nactual - nformal : 0;
> +    JSObject *rest = JS_NewArrayObject(cx, nrest, actualArgs() + nformal);

This is so much nicer. Love it.

Instead of JS_NewArrayObject, call NewDenseCopiedArray directly.

Comment 14

[:Benjamin Peterson]

Attachment: v5: addresses review comments

Comment 15

[Jason Orendorff [:jorendorff]]

Fuzzers take note: delicious new syntax.

Also note that you can take any current test, add a rest parameter to any or all of the functions, and the behavior shouldn't change except:
- the decompiler will of course reconstitute the rest param
- a Debugger could observe the rest param (but this will only happen in tests
  that use parameterNames)
- an function that uses "arguments" will probably just break,
  you can't use "arguments" in a hasRest function.

So every test that passes if you screw up those three features should also pass if you instead add rest parameters to all the functions.

Comment 16

[Mark S. Miller]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> - an function that uses "arguments" will probably just break,
>   you can't use "arguments" in a hasRest function.

What do you mean by "just break"? Does co-existence of rest params and "arguments" give an early error?

Comment 17

[:Benjamin Peterson]

(In reply to Mark S. Miller from comment #16)
> (In reply to Jason Orendorff [:jorendorff] from comment #15)
> > - an function that uses "arguments" will probably just break,
> >   you can't use "arguments" in a hasRest function.
> 
> What do you mean by "just break"? Does co-existence of rest params and
> "arguments" give an early error?

If "arguments" is in the function body (not as an argument), it will give a SyntaxError.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 625278 [details] [diff] [review]
v5: addresses review comments

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

Partial review. I have to quit for the day but we can pick it up on Monday. Sorry I didn't turn things around fast enough for us to land this this week. I wanted to, and this is good work, but it turned out to be a little more code than I could get through, balancing with other work.

I think you probably need to add a JSOP_REST case to the switch statement in ScriptAnalysis::analyzeTypesBytecode in jsinfer.cpp. The default case in that switch statement crashes the program. (!) You can get Brian Hackett to walk you through that, although just for now, this should work:

      case JSOP_REST:
        pushed[0].addType(cx, Type::UnknownType());  /* actually we know it's a dense array with no holes */
        break;

...though if the omission really is fatal, we'd be better off with a test case.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2622,5 @@
> +        bce->switchToProlog();
> +        if (Emit1(cx, bce, JSOP_REST) < 0)
> +            return false;
> +        uint16_t arg = bce->sc->fun()->nargs - 1;
> +        if (bce->sc->bindingsAccessedDynamically()) {

I think this needs to use logic similar to what's in BytecodeEmitter::isAliasedName instead. That is, the "Dynamically" in bindingsAccessedDynamically refers to accesses like eval(code), with, and so on; there are also possible statically-bound accesses, like:

    function f(...rest) {
        return function () { return rest; };
    }

I think we need to emit 'rest; setaliasedvar; pop' here, so if this is passing all your tests, we need another test.

@@ +2628,5 @@
> +            uint16_t binding = bce->sc->bindings.argToBinding(arg);
> +            if (!EmitAliasedVarOp(cx, JSOP_SETALIASEDVAR, binding, atom, bce))
> +                return false;
> +        } else if (!EmitUnaliasedVarOp(cx, JSOP_SETARG, arg, bce))
> +            return false;

Style nit: House style is that curly braces go around either each indented "suite"
of an if/elseif/else chain, or none of them, so please add curly braces here:

        } else if (!EmitUnaliasedVarOp(cx, JSOP_SETARG, arg, bce)) {
            return false;
        }

::: js/src/frontend/Parser.cpp
@@ +777,1 @@
>          }

If you ever really want to abort the process even in optimize builds, instead of abort() write MOZ_CRASH().

But that's not appropriate here; we need to figure out if bindings can contain arguments or not.

@@ +777,4 @@
>          }
>      }
> +    return true;
> + complain:

Style nit: blank line after this return; and two-space indent for the label.

Comment 19

[Jason Orendorff [:jorendorff]]

(In reply to Mark S. Miller from comment #16)
> (In reply to Jason Orendorff [:jorendorff] from comment #15)
> > - an function that uses "arguments" will probably just break,
> >   you can't use "arguments" in a hasRest function.
> 
> What do you mean by "just break"? Does co-existence of rest params and
> "arguments" give an early error?

Yes.

function f(...rest) { arguments; }  // SyntaxError

function f(...rest) { eval("arguments"); }  // SyntaxError when called

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 625278 [details] [diff] [review]
v5: addresses review comments

OK, r=me with the concerns from comment 18 addressed. One more nit:

>+inline JSObject *
>+StackFrame::getRestParameter(JSContext *cx)
>+{
>+    JS_ASSERT(fun()->hasRest());
>+    unsigned nformal = fun()->nargs - 1, nactual = numActualArgs();
>+    unsigned nrest = (nactual > nformal) ? nactual - nformal : 0;
>+    return NewDenseCopiedArray(cx, nrest, actualArgs() + nformal);
>+}

Nit: Since you can't use this to get the canonical rest-argument for the current stack frame, "get" is misleading. Rename it to createRestArray or something.

Comment 21

[:Benjamin Peterson]

Attachment: v6 - ready for checkin

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5130ffc92faf

Comment 23

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Backed out due to blowing up debug builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/258554de726c

https://tbpl.mozilla.org/php/getParsedLog.php?id=11931989&tree=Mozilla-Inbound

Comment 24

[:Benjamin Peterson]

Attachment: v7 fixed type inference

Comment 25

[:Benjamin Peterson]

Attachment: v8 - with the changes I meant to put in the last patch

Comment 26

[:Benjamin Peterson]

Attachment: v9 actually fix failures

Looks like I didn't run if the right --jitflags last time...

Comment 27

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd094709d5b9

Nice work, Benjamin.

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dd094709d5b9

Comment 29

[Jean-Yves Perrier [:teoli]]

Updated
https://developer.mozilla.org/en-US/docs/Firefox_15_for_developers and
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/rest_parameters (which was already done)

Comment 30

[Tom S [:evilpie]]

We should link this from Function and the JavaScript reference.

Comment 31

[Caitlin Potter [:caitp]]

So, reading the 10-14-2014 draft, I'm not seeing where "arguments" should result in a SyntaxError. 9.2.13 seems to be saying that, for (non-arrow) functions with rest parameters, a "strict" arguments object is created, and presumably bound to "arguments".

I am probably missing something, but I'm interested in the reasoning for this behaviour in SpiderMonkey, as it does not appear to match (my reading) of the text.

Comment 32

[Jeff Walden [:Waldo]]

(In reply to Caitlin Potter (:caitp) from comment #31)
> So, reading the 10-14-2014 draft, I'm not seeing where "arguments" should
> result in a SyntaxError. 9.2.13 seems to be saying that, for (non-arrow)
> functions with rest parameters, a "strict" arguments object is created, and
> presumably bound to "arguments".

Hm, that seems right.  It seems to me rest arguments should preclude an arguments object (mapped or unmapped) entirely.  Otherwise you have basically duplicated work to create the rest array, and also to create the array-like arguments object, when it's pretty likely only the rest array is going to be used.  (And the intent of rest arrays was to get rid of the arguments object, I thought -- *not* just to make it more stupid, by being less stupid.)  File a spec bug?  Adding a static semantics rule that can be used as a final condition in step 19a seems fairly simple.

Comment 33

[Caitlin Potter [:caitp]]

It's interesting, because the following case does not throw in this nightly (and I'm not seeing a test case incorporating this in the landed patch, not sure if it has been subsequently added):

```
function outer(...restParams) {
  var inner = () => arguments;

  return inner();
}
outer(1,2,3);
```

will yield an Arguments object containing the expected 1,2,3 items --- so the arguments object is created even though restParams are used, and makes things a bit inconsistent.

The problem with this is, we don't necessarily want to eagerly parse arrow function declarations, so it's hard to know if we will need to create the arguments object or not.

These behaviours seem a bit underspecified, so I've filed at https://bugs.ecmascript.org/show_bug.cgi?id=3386 if you're interested in adding to that.

Comment 34

[Caitlin Potter [:caitp]]

Actually hang on, I'm wrong about that --- the inner arrow function actually returns an empty arguments object. Which is an even more bizarre behaviour.

Comment 35

[Jeff Walden [:Waldo]]

(In reply to Caitlin Potter (:caitp) from comment #34)
> the inner arrow function actually returns an empty arguments object.

That's just bug 889158; I have a partial patch for it that I need to finish up.

Comment 36

[Caitlin Potter [:caitp]]

I see --- but even without that, the following still results in a syntax error:

```
(function a() {
  var x = (...rest) => { return arguments; }
  return x(1,2,3);
})(4,5,6)
```

And you'd expect that even with a rest parameter, it would be able to reference the outer arguments object, especially given that it can't have its own arguments object at all. Adding the static error case really complicates things a lot more than it maybe should.

Comment 37

[Tom S [:evilpie]]

We don't yet implement the lexical arguments binding for arrow functions, so this is kind of expected. I think we should maybe move this discussion to a new bug.

Comment 38

[Allen Wirfs-Brock]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #32)
> (In reply to Caitlin Potter (:caitp) from comment #31)
> > So, reading the 10-14-2014 draft, I'm not seeing where "arguments" should
> > result in a SyntaxError. 9.2.13 seems to be saying that, for (non-arrow)
> > functions with rest parameters, a "strict" arguments object is created, and
> > presumably bound to "arguments".
> 
> Hm, that seems right.  It seems to me rest arguments should preclude an
> arguments object (mapped or unmapped) entirely.  Otherwise you have
> basically duplicated work to create the rest array, and also to create the
> array-like arguments object, when it's pretty likely only the rest array is
> going to be used.  (And the intent of rest arrays was to get rid of the
> arguments object, I thought -- *not* just to make it more stupid, by being
> less stupid.)  File a spec bug?  Adding a static semantics rule that can be
> used as a final condition in step 19a seems fairly simple.

Nope, the ES6 spec. always provides an arguments object (except for arrow functions and cases where `arguments` is explicitly redeclared).  See http://people.mozilla.org/~jorendorff/es6-draft.html#sec-functiondeclarationinstantiation. 

The arguments object and a rest parameter are not generally the same or interchangeable. Of course implementation can (and probably should) perform analysis to determine whether the arguments object is not referenced and skip its creation in those cases.