757676 - (harmony:defaults) implement harmony default parameters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

See the URL.

Comment 1

[:Benjamin Peterson]

Attachment: draft implementation

Largest thing missing is decompilation, but I hate to do that before the bytecode format is approved. :)

Comment 2

[Jason Orendorff [:jorendorff]]

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]

Attachment: complete implementation

Comment 4

[Jason Orendorff [:jorendorff]]

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]]

>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]]

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]

Attachment: address review comments, ban yield

Comment 8

[:Benjamin Peterson]

Attachment: JaegerMonkey support

Comment 9

[:Benjamin Peterson]

Try shows nothing but a intermittent Windows failure, so the first patch can go in.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/699a613bf616

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/699a613bf616

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 [Laid off!]]

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]

Attachment: JaegerMonkey support ready for checkin

Comment 15

[satyr]

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]

(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]

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]

(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]

(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]]

(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 21

[Tom S]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8247bd0ca037

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8247bd0ca037

Comment 23

[aaronfrost]

(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]

(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.

Comment 25

[Florian Scholz (Open Web Docs)]

Already written:
https://developer.mozilla.org/en-US/Firefox/Releases/15#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters