Bug 757676 (harmony:defaults)

implement harmony default parameters

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla15
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2][DocArea=JS], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
See the URL.
(Assignee)

Updated

5 years ago
Alias: harmony:defaults
Blocks: 694100
Whiteboard: [js:p2]
(Assignee)

Updated

5 years ago
Assignee: general → bpeterson
(Assignee)

Comment 1

5 years ago
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. :)
Attachment #626599 - Flags: feedback?(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.
Attachment #626599 - Flags: feedback?(jorendorff)
(Assignee)

Comment 3

5 years ago
Created attachment 626959 [details] [diff] [review]
complete implementation
Attachment #626599 - Attachment is obsolete: true
Attachment #626959 - Flags: review?(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);
>assertThrowsEq(f, "x");

That should have been assertThrowsValue.

Incidentally, all these tests pass in benjamin's implementation, except the one about yield.
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();
    }
Attachment #626959 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 627405 [details] [diff] [review]
address review comments, ban yield
Attachment #626959 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 627410 [details] [diff] [review]
JaegerMonkey support
Attachment #627410 - Flags: review?(dvander)
(Assignee)

Comment 9

5 years ago
Try shows nothing but a intermittent Windows failure, so the first patch can go in.
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/integration/mozilla-inbound/rev/699a613bf616
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [js:p2] → [js:p2] [leave open]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/699a613bf616

Updated

5 years ago
Keywords: dev-doc-needed
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 :)
Attachment #627410 - Flags: review?(dvander) → review?(bhackett1024)
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.
Attachment #627410 - Flags: review?(bhackett1024) → review+
Depends on: 759904
(Assignee)

Comment 14

5 years ago
Created attachment 628553 [details] [diff] [review]
JaegerMonkey support ready for checkin
Attachment #627410 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [js:p2] [leave open] → [js:p2]
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Comment 15

5 years ago
How is FD hoisting supposed to work with defaults? In Scrachpad, I get:

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

which seems unintuitive.
(Assignee)

Comment 16

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 18

5 years ago
(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

5 years ago
(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
(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();
    }

Updated

5 years ago
Blocks: 760401
http://hg.mozilla.org/integration/mozilla-inbound/rev/8247bd0ca037
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8247bd0ca037
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 764508
Depends on: 762324
Depends on: 769072

Comment 23

5 years ago
(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?
(Assignee)

Comment 24

5 years ago
(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.
Whiteboard: [js:p2] → [js:p2][DocArea=JS]
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.