Last Comment Bug 757676 - (harmony:defaults) implement harmony default parameters
(harmony:defaults)
: implement harmony default parameters
Status: RESOLVED FIXED
[js:p2][DocArea=JS]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Benjamin Peterson
:
Mentors:
http://wiki.ecmascript.org/doku.php?i...
Depends on: 759904 762324 764508 769072
Blocks: es6 760401
  Show dependency treegraph
 
Reported: 2012-05-22 17:30 PDT by :Benjamin Peterson
Modified: 2014-09-22 07:05 PDT (History)
13 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
draft implementation (16.98 KB, patch)
2012-05-23 15:11 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
complete implementation (27.62 KB, patch)
2012-05-24 14:14 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review
address review comments, ban yield (33.56 KB, patch)
2012-05-25 16:44 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
JaegerMonkey support (2.38 KB, patch)
2012-05-25 17:14 PDT, :Benjamin Peterson
bhackett1024: review+
Details | Diff | Splinter Review
JaegerMonkey support ready for checkin (2.38 KB, patch)
2012-05-30 18:05 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-05-22 17:30:13 PDT
See the URL.
Comment 1 :Benjamin Peterson 2012-05-23 15:11:23 PDT
Created attachment 626599 [details] [diff] [review]
draft implementation

Largest thing missing is decompilation, but I hate to do that before the bytecode format is approved. :)
Comment 2 Jason Orendorff [:jorendorff] 2012-05-23 16:21:47 PDT
Comment on attachment 626599 [details] [diff] [review]
draft implementation

Nice tests. It would be good to also have a test that calls a function with default-arguments and passes too many arguments, so that nothing defaults and some arguments even get thrown away.

Micro-nit: Looks like all the tests are missing a newline at the end of the last line.

I mentioned a few of these things on IRC:

- I'm not sure it's ok to emit arbitrary bytecode in a prolog. Ask around at the office. Unfortunately, depending on spec details I haven't looked at, it might not be OK to do JSOP_REST in the prolog and then the default arguments in the body, because of cases like:

    function f(x=y, ...y) { return x; }  // x is populated before y (?)
    assertEq(f(), undefined);            // not an empty array

- if JSFunction::ndefaults changes the allocation size, we should look for some way to avoid storing it.

- It might be nice to avoid JSOP_DEFAULTSNEEDED, and thus the need for fun->ndefaults. We could instead have an opcode that just pushes fp->numActualArgs().

- You can expect decompilation and destructuring to be a pain.
Comment 3 :Benjamin Peterson 2012-05-24 14:14:34 PDT
Created attachment 626959 [details] [diff] [review]
complete implementation
Comment 4 Jason Orendorff [:jorendorff] 2012-05-25 09:45:46 PDT
More things to test:

// Default arguments are not evaluated until the function is called.

var g = function (x=FAIL()) {};  // don't throw
var n = 0;
function f(a=n++) {}
assertEq(n, 0);


// Default arguments are evaluated each time the function is called.

function f(a=[]) { return a; }
assertEq(f() !== f(), true);

function g(a=function () {}) { return a; }
assertEq(g() !== g(), true);

function h(a=Date) { return a; }
assertEq(h(), Date);
Date = 0;
assertEq(h(), 0);


// Exceptions in default argument expressions are propagated.

load(libdir + "asserts.js");

function die() { throw "x"; }
var ok = true;
function f(a = die()) { ok = false; }
assertThrowsEq(f, "x");


// Default argument expression scoping

var x = 'global';
function f(a=x) {  // local variable x
    var x = 'local';
    return a;
}
assertEq(f(), undefined);

function g(f=function () { return ++x; }) {  // closes on local variable x
    var x = 0;
    return f;
}
var gf = g();
assertEq(gf(), 1);
assertEq(gf(), 2);
gf = g();
assertEq(gf(), 1);

function h(f=function (s) { return eval(s); }) {  // closes on local scope
    var x = 'hlocal';
    return f;
}
var hf = h();
assertEq(hf('x'), 'hlocal');
assertEq(hf('f'), hf);
assertEq(hg('var x = 3; x'), 3);

function j(expr, v=eval(expr)) {
    return v;
}
assertEq(j("expr"), "expr");
assertEq(j("v"), undefined);
assertEq(j("Array"), Array);
assertEq(j("arguments").length, 1);


// (for defaults-invalid-syntax) Yield isn't allowed here.

assertBlah(... function f(a = yield 0) {} ...blahblah SyntaxError);


// The arguments object can coexist with defaults.
function f(a=1, b=2, c=3) { return arguments; }
var args = f();
assertEq(args.length, 0);
assertEq("0" in args, false);
args = f(5, 6);
assertEq(args.length, 2);
assertEq(args[1], 6);
args = f(9, 8, 7, 6, 5);
assertEq(args.length, 5);
assertEq(args[4], 5);
Comment 5 Jason Orendorff [:jorendorff] 2012-05-25 09:48:17 PDT
>assertThrowsEq(f, "x");

That should have been assertThrowsValue.

Incidentally, all these tests pass in benjamin's implementation, except the one about yield.
Comment 6 Jason Orendorff [:jorendorff] 2012-05-25 11:52:45 PDT
Comment on attachment 626959 [details] [diff] [review]
complete implementation

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

Looks great. r=me with these comments addressed.

Whenever changing bytecode format, bump the version number in js/src/vm/Xdr.h:
> static const uint32_t XDR_BYTECODE_VERSION = uint32_t(0xb973c0de - 115);

As the comment says, "increment the subtrahend" lol, i.e. from 115 to 116.


More things to test:

// Default argument expressions are evaluated after destructuring arguments.

function f([a, b], A=a, B=b) {
    assertEq(A, a);
    assertEq(B, b);
}
f([0, 1]);


// fn.toString() retains parens around a default argument expression that uses the comma operator

function f(a=(0, c=1)) {}
assertEq(f.length, 1);
var g = eval("(" + f + ")");
assertEq(g.length, 1);

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5860,5 @@
>  }
>  
> +static bool
> +EmitDefaults(JSContext *cx, BytecodeEmitter *bce, ParseNode *pn)
> +{

For the benefit of future hackers reading this code, assert here that pn->isKind(PNK_ARGSBODY).

@@ +5864,5 @@
> +{
> +    ParseNode *arg;
> +    ParseNode *pnlast = pn->last();
> +    unsigned ndefaults = 0;
> +    for (arg = pn->pn_head; arg != pnlast; arg = arg->pn_next)

Style gibbering: FWIW, I would write 'for (ParseNode *arg = ...' here and declare a separate variable later. A lot of this front-end code has a C-like style for no better reason than that it used to be C.

@@ +5872,5 @@
> +    unsigned nformal = fun->nargs - fun->hasRest();
> +    EMIT_UINT16_IMM_OP(JSOP_ACTUALSFILLED, nformal - ndefaults);
> +    ptrdiff_t top = bce->offset();
> +    size_t tableSize = (size_t)(JUMP_OFFSET_LEN * (3 + ndefaults));
> +    int noteIndex = NewSrcNote3(cx, bce, SRC_SWITCH, 0, 0);

I don't need think this source note is necessary. (Source notes exist for the decompiler's benefit, and the decompiler does not check for source notes on this TABLESWITCH instruction.)

@@ +5883,5 @@
> +    SET_JUMP_OFFSET(pc, nformal - ndefaults);
> +    pc += JUMP_OFFSET_LEN;
> +    SET_JUMP_OFFSET(pc, nformal - 1);
> +    pc += JUMP_OFFSET_LEN;
> +    /* Fill body of switch, which sets defaults where needed. */

Nit: blank line before comment (except at the beginning of a block).

@@ +5945,5 @@
> +                   Defaults and rest need special handling. The rest parameter
> +                   needs to be undefined while defaults are being processed. To
> +                   do this, we create the rest argument and let it sit on the
> +                   stack while processing defaults. The rest parameter's slot is
> +                   set to undefined for the course of default processing.

Style nits:

Blank line before this comment; or just put the comment before the assertion.

Either use // or put a star at the beginning of each line of a multiline /**/.

::: js/src/frontend/ParseNode.h
@@ +387,5 @@
>   *                          pn_left: property id, pn_right: value
>   *                          var {x} = object destructuring shorthand shares
>   *                          PN_NAME node for x on left and right of PNK_COLON
>   *                          node in PNK_RC's list, has PNX_DESTRUCT flag
> + * PNK_DEFAULTARG unary     holds the default value of a argument

"an argument"

This comment isn't sufficient to explain what a PNK_DEFAULTARG node looks like or where it's used. However later I'm going to suggest removing this and making do with the existing PNK_NAME stuff, the way we do for variable declarations with initializers (note the comment about PNK_VAR later in this file:

> * PNK_VAR,     list        pn_head: list of PNK_NAME or PNK_ASSIGN nodes
> * PNK_CONST                         each name node has [...]
> *                                     pn_used: false
> *                                     pn_atom: variable name
> *                                     pn_expr: initializer or null
> [...]

)

::: js/src/frontend/Parser.cpp
@@ +1428,5 @@
> +                        freeTree(def_expr);
> +                        return false;
> +                    }
> +                    def->pn_kid = def_expr;
> +                    tc->sc->funbox->node->pn_body->append(def);

Instead of adding another node here, consider using the pn_expr field of the arg node (perhaps making DefineArg return it). This is what we use for variable declarations

    var a=1, b=2;  // PNK_VAR with two PNK_NAME children, each with expr() subnodes

At least, I think that is what we do. If this is as straightforward as it seems, it will make the emitter code a little shorter and sweeter. (If it's not straightforward, don't do it.)

::: js/src/jsapi.h
@@ +2197,2 @@
>                                             without creating a this object */
> +#define JSFUN_HAS_DEFAULTS      0x0400

Needs a comment.

@@ +2199,2 @@
>  
>  #define JSFUN_FLAGS_MASK      0x07f8    /* overlay JSFUN_* attributes --

Does this constant need to be bumped to 0x0ff8?

::: js/src/jsopcode.cpp
@@ +5530,5 @@
>  
>  #if JS_HAS_DESTRUCTURING
>          ss.printer = NULL;
>          jp->script = script;
> +        jsbytecode *dspc = pc;

This should not be defined inside the #if JS_HAS_DESTRUCTURING block.

@@ +5540,5 @@
> +        uint16_t defstart = 0;
> +        unsigned nformal = fun->nargs - fun->hasRest();
> +
> +        if (fun->hasDefaults()) {
> +#define SKIP(pc, op) JS_ASSERT(*pc == op); pc += js_CodeSpec[op].length;

Consider instead moving the LOCAL_ASSERT definition up here and using that.

(Because changing the bytecode emitter without updating the decompiler is a common mistake, because decompiler performance does not matter, and because the decompiler is scary code containing lots of potentially security-sensitive bugs, we use a special kind of assert that is checked even in release builds. One might be forgiven for thinking this is deeply crazy and we just need to delete the decompiler. We will, eventually...)

@@ +5553,5 @@
> +            SKIP(defpc, JSOP_ACTUALSFILLED);
> +            JS_ASSERT(*defpc == JSOP_TABLESWITCH);
> +            defbegin = defpc;
> +            deflen = GET_JUMP_OFFSET(defpc);
> +            /* Update the pointer to destructuring bytecode. */

Style nit: blank line before each comment here.

@@ +5554,5 @@
> +            JS_ASSERT(*defpc == JSOP_TABLESWITCH);
> +            defbegin = defpc;
> +            deflen = GET_JUMP_OFFSET(defpc);
> +            /* Update the pointer to destructuring bytecode. */
> +            dspc = defpc + deflen;

This assignment to dspc is not in a JS_HAS_DESTRUCTURING block.

@@ +5627,5 @@
> +                jsbytecode *casestart = defbegin + TABLE_OFF(i - defstart);
> +                jsbytecode *caseend = defbegin + ((i < nformal - 1) ? TABLE_OFF(i - defstart + 1) : deflen);
> +#undef TABLE_OFF
> +                jsbytecode *exprstop = casestart;
> +                /* Find where the expression ends. */

Nit: blank line before comment.

@@ +5634,5 @@
> +                    if ((*exprstop == JSOP_SETARG &&
> +                         exprstop == caseend - js_CodeSpec[JSOP_SETARG].length - poplen) ||
> +                        (*exprstop == JSOP_SETALIASEDVAR &&
> +                         exprstop == caseend - js_CodeSpec[JSOP_SETALIASEDVAR].length - poplen))
> +                        break;

I'm not sure, but it seems like simply decompiling the whole case would produce an appropriate assignment-expression. (The result would have extra spaces, compared to what you're doing, but we don't care.) It might be easier to add destructuring that way, too.

Style nit: curly braces for the for-loop and for the if-statement here. We only omit the braces when both the controlling if/for and the substatement are one line each:

    if (x)     // simple
        y();   // also simple

but

    for (;;) {  // substatement is two lines, gets braces
        if (x)
            y();
    }

and

    // controlling "if" is multiple lines, gets braces
    if (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ||
        yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy)
    {
        z();
    }
Comment 7 :Benjamin Peterson 2012-05-25 16:44:17 PDT
Created attachment 627405 [details] [diff] [review]
address review comments, ban yield
Comment 8 :Benjamin Peterson 2012-05-25 17:14:23 PDT
Created attachment 627410 [details] [diff] [review]
JaegerMonkey support
Comment 9 :Benjamin Peterson 2012-05-26 01:57:18 PDT
Try shows nothing but a intermittent Windows failure, so the first patch can go in.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-05-26 06:41:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/699a613bf616
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-05-26 15:28:45 PDT
https://hg.mozilla.org/mozilla-central/rev/699a613bf616
Comment 12 David Anderson [:dvander] 2012-05-29 11:47:32 PDT
Comment on attachment 627410 [details] [diff] [review]
JaegerMonkey support

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

bhackett is better than me for reviewing mjit changes now :)
Comment 13 Brian Hackett (:bhackett) 2012-05-30 07:17:10 PDT
Comment on attachment 627410 [details] [diff] [review]
JaegerMonkey support

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

::: js/src/methodjit/Compiler.cpp
@@ +2257,5 @@
> +            masm.load32(Address(JSFrameReg, StackFrame::offsetOfNumActual()), nactual);
> +
> +            // Best would be a single instruction where available (like
> +            // cmovge on x86), but there's no way get that yet, so jump.
> +            Jump j = masm.branchPtr(Assembler::LessThan, nactual, Imm32(defstart));

This should be branch32.
Comment 14 :Benjamin Peterson 2012-05-30 18:05:43 PDT
Created attachment 628553 [details] [diff] [review]
JaegerMonkey support ready for checkin
Comment 15 satyr 2012-05-31 11:07:24 PDT
How is FD hoisting supposed to work with defaults? In Scrachpad, I get:

    (function (a = f) {
      function f() {}
      return a
    })()
    /*
    undefined
    */

which seems unintuitive.
Comment 16 :Benjamin Peterson 2012-05-31 11:12:29 PDT
(In reply to satyr from comment #15)
> How is FD hoisting supposed to work with defaults? In Scrachpad, I get:
> 
>     (function (a = f) {
>       function f() {}
>       return a
>     })()
>     /*
>     undefined
>     */
> 
> which seems unintuitive.

That's currently being dealt with in bug 759498.
Comment 17 satyr 2012-05-31 11:38:14 PDT
Also, why is:

    function f([a]=4) {}

supposed to fail? The Wiki spec has

  FormalParameter ::= Identifier Initialiser?
                    | Pattern    Initialiser?  # <--

which should be useful as well.


(In reply to Benjamin Peterson from comment #16)
> That's currently being dealt with in bug 759498.

Cool.
Comment 18 :Benjamin Peterson 2012-05-31 11:44:09 PDT
(In reply to satyr from comment #17)
> Also, why is:
> 
>     function f([a]=4) {}
> 
> supposed to fail? The Wiki spec has
> 
>   FormalParameter ::= Identifier Initialiser?
>                     | Pattern    Initialiser?  # <--
> 
> which should be useful as well.

Destructuring is supposed to have it's own defaults like function f([a=4]) {}
Comment 19 satyr 2012-05-31 12:06:03 PDT
(In reply to Benjamin Peterson from comment #18)
> Destructuring is supposed to have it's own defaults like function f([a=4]) {}

That's nice too, but doesn't help when that argument is omitted:

    function f([a = 4]) {}
    f() //=> TypeError: arguments[0] is undefined

We need:

    function f([a] = [4]) { return a }
    f() //=> 4

or maybe:

    function f(...[a = 4]) { return a }
    f() //=> 4
Comment 20 Jason Orendorff [:jorendorff] 2012-05-31 12:31:05 PDT
(In reply to satyr from comment #15)
> How is FD hoisting supposed to work with defaults? [...]

The proposal itself isn't precise enough about the semantics. So the answer is we honestly don't know yet. We're working with TC39 to get this and a bunch of other little issues resolved now. Here's a list:

- Does a function's .length property include arguments with defaults?
  (function (a=1) {}).length
- Can arguments using destructuring have defaults?
- What is the value of a rest-argument when default expressions are being evaluated?
    function f(a=alert(rest), ...rest){}
- Is a yield expression allowed as a default value?
    function* f(a=(yield 0)) {}
- In the code below, is g() called?
  Or would it be called only when iter.next() is called?
    function* f(a=g()) { yield 1; }
    var iter = f();
- What is the value of f() here:
    function f(a=42) { return a; function a() {} }
- And here:
    function f(a=b, b=4) { return a; function b() {}}
- And here (since b is no longer undefined by the time we get to it):
    function f(a=(b=3), b=4) { return b; }
- Are here (are local variables in scope?):
    var x = "global";
    function f(a=function () { return x; }) {
        var x = "local";
        return a();
    }
Comment 22 Phil Ringnalda (:philor) 2012-06-03 12:14:19 PDT
https://hg.mozilla.org/mozilla-central/rev/8247bd0ca037
Comment 23 aaronfrost 2012-08-10 00:06:01 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #20)
> We're working with TC39 to get this and a
> bunch of other little issues resolved now. Here's a list:

Did each of these questions receive an answer? If so, what were those answers and have those been implemented?
Comment 24 :Benjamin Peterson 2012-08-10 10:31:26 PDT
(In reply to aaronfrost from comment #23)
> (In reply to Jason Orendorff [:jorendorff] from comment #20)
> > We're working with TC39 to get this and a
> > bunch of other little issues resolved now. Here's a list:
> 
> Did each of these questions receive an answer? If so, what were those
> answers and have those been implemented?

Not all of them are settled but a few follow-up bugs like bug 781422 have been filed.

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