759498 - defaults bound to functions are broken

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

function f(a=42) {
    return a;
    function a() { return 19; }
}
f()

gives an assertion error at the moment.

Comment 1

[:Benjamin Peterson]

Attachment: fix the bug

Fixing this required somewhat major surgery on the implementation. Some things are a bit cleaner now, though. For simplicity, every function is required to have an ARGSBODY node now.

Comment 2

[Jason Orendorff [:jorendorff]]

The semantics suggested ("specified" would be too strong a word) in the draft, that I think best explain the behavior with functions, is:
 - bind actual arguments, bind everything else to undefined
 - for each omitted argument that has an initializer-expression,
   evaluate the expression and rebind
 - rebind the rest-param, if any
 - rebind all inner functions

The difference between that and what you've implemented here is observable, barely:

  function f(a=(b=8), b=4) {
      function b(){}
      return b;
  }

but ... I don't care, especially since the semantics will surely change again. And we need to fix the assertion. So I'm going to review this as-is.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 628248 [details] [diff] [review]
fix the bug

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

Whew. OK, I like making ARGSBODY always present and thus unifying the two copies of the PNX_DESTRUCT-handling code.

r=me with nits picked; let's get it in. We can mop up the carnage once we get some answers from dherman about how defaults are really supposed to work.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5869,5 @@
> +            if (!EmitVarOp(cx, arg, JSOP_SETARG, bce))
> +                return false;
> +        } else {
> +
> +            // Create a dummy JSEOP_SETLOCAL for the decompiler. Jump over it

JSEOP_SETLOCAL is a typo.

Assert at the beginning of this block what sort of thing arg is (it's an assignment node, we know exactly what it must be).

@@ +5925,5 @@
> +        // 3. Defaults
> +        ParseNode *pnchild = pnlast->pn_head;
> +        if (pnlast->pn_xflags & PNX_DESTRUCT) {
> +
> +            // Assign the destructuring arguments before defining any functions,

Style nit: No blank line before this comment at the beginning of a block.

@@ +5935,5 @@
> +            pnchild = pnchild->pn_next;
> +        }
> +        if (pnlast->pn_xflags & PNX_FUNCDEFS) {
> +
> +            // This block contains top-level function definitions. To ensure

Also here.

@@ +5951,5 @@
> +                        if (!EmitTree(cx, bce, pn2))
> +                            return false;
> +                    } else {
> +
> +                        //  JSOP_DEFFUN in a top-level block with function

And here; and just one space between '//' and 'JSOP_DEFFUN'.

::: js/src/frontend/ParseNode.h
@@ +587,5 @@
>              ParseNode   *head;          /* first node in list */
>              ParseNode   **tail;         /* ptr to ptr to last node in list */
>              uint32_t    count;          /* number of nodes in list */
> +            uint32_t    xflags:14,      /* extra flags, see below */
> +                        blockid:18;     /* see name variant below */

Why this change?

@@ +621,5 @@
>              UpvarCookie cookie;         /* upvar cookie with absolute frame
>                                             level (not relative skip), possibly
>                                             in current frame */
> +            uint32_t    dflags:14,      /* definition/use flags, see below */
> +                        blockid:18;     /* block number, for subset dominance

Same here.

::: js/src/frontend/Parser.cpp
@@ +863,5 @@
>       * If dn is arg, or in [var, const, let] and has an initializer, then we
>       * must rewrite it to be an assignment node, whose freshly allocated
>       * left-hand side becomes a use of pn.
>       */
> +    if (dn->isBindingForm() || dn->kind() == Definition::ARG) {

It looks like this is the only use of isBindingForm, so you can just change the predicate. (It might as well be called canHaveInitialiser(), if you care to rename it.)

@@ +1425,5 @@
>                          return false;
> +                    ParseNode *arg = tc->sc->funbox->node->pn_body->last();
> +                    arg->pn_dflags |= PND_DEFAULT;
> +                    arg->pn_expr = def_expr;
> +                    tc->sc->funbox->ndefaults++;

Can you get rid of the hasDefaults out-param? Not a big deal either way.

::: js/src/jit-test/tests/arguments/defaults-bound-to-function.js
@@ +9,5 @@
> +    return [a, b];
> +    function b() { return 42; }
> +}
> +var res = h();
> +assertEq(res[0], res[1]);

(Of course this behavior is not necessarily what TC39 will specify. It's a goofy corner case. We talked to dherman about this one -- and a bunch of others -- and basically he owes us answers.)

::: js/src/jsopcode.cpp
@@ +4840,5 @@
> +                    defaultsSwitch = false;
> +                    len = GET_JUMP_OFFSET(pc);
> +                    if (jp->fun->hasRest()) {
> +
> +                        // Jump over rest parameter things.

Style nit: No blank line again.

@@ +5340,5 @@
> +                // Ignore bytecode related to handling rest.
> +                pc += GetBytecodeLength(pc);
> +                if (*pc == JSOP_UNDEFINED) {
> +                    pc += GetBytecodeLength(pc);
> +                }

Style nit: no curly braces here.

Comment 4

[:Benjamin Peterson]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> Comment on attachment 628248 [details] [diff] [review]
> fix the bug
> 
> Review of attachment 628248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Whew. OK, I like making ARGSBODY always present and thus unifying the two
> copies of the PNX_DESTRUCT-handling code.
> 
> r=me with nits picked; let's get it in. We can mop up the carnage once we
> get some answers from dherman about how defaults are really supposed to work.
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +5869,5 @@
> > +            if (!EmitVarOp(cx, arg, JSOP_SETARG, bce))
> > +                return false;
> > +        } else {
> > +
> > +            // Create a dummy JSEOP_SETLOCAL for the decompiler. Jump over it
> 
> JSEOP_SETLOCAL is a typo.
> 
> Assert at the beginning of this block what sort of thing arg is (it's an
> assignment node, we know exactly what it must be).

This was already ascertained in the previous if statement.


> 
> @@ +5925,5 @@
> > +        // 3. Defaults
> > +        ParseNode *pnchild = pnlast->pn_head;
> > +        if (pnlast->pn_xflags & PNX_DESTRUCT) {
> > +
> > +            // Assign the destructuring arguments before defining any functions,
> 
> Style nit: No blank line before this comment at the beginning of a block.
> 
> @@ +5935,5 @@
> > +            pnchild = pnchild->pn_next;
> > +        }
> > +        if (pnlast->pn_xflags & PNX_FUNCDEFS) {
> > +
> > +            // This block contains top-level function definitions. To ensure
> 
> Also here.
> 
> @@ +5951,5 @@
> > +                        if (!EmitTree(cx, bce, pn2))
> > +                            return false;
> > +                    } else {
> > +
> > +                        //  JSOP_DEFFUN in a top-level block with function
> 
> And here; and just one space between '//' and 'JSOP_DEFFUN'.
> 
> ::: js/src/frontend/ParseNode.h
> @@ +587,5 @@
> >              ParseNode   *head;          /* first node in list */
> >              ParseNode   **tail;         /* ptr to ptr to last node in list */
> >              uint32_t    count;          /* number of nodes in list */
> > +            uint32_t    xflags:14,      /* extra flags, see below */
> > +                        blockid:18;     /* see name variant below */
> 
> Why this change?

I created a flag with fills the 13th bit.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Benjamin Peterson from comment #4)
> > Assert at the beginning of this block what sort of thing arg is (it's an
> > assignment node, we know exactly what it must be).
> 
> This was already ascertained in the previous if statement.

Oh, ok. I see it now. :)

> > ::: js/src/frontend/ParseNode.h
> > > +            uint32_t    xflags:14,      /* extra flags, see below */
> > > +                        blockid:18;     /* see name variant below */
> > 
> > Why this change?
> 
> I created a flag with fills the 13th bit.

The new flag, PND_DEFAULT, has a value of 0x400, which fits all right in 12 bits.

Comment 6

[:Benjamin Peterson]

Attachment: address review comments

Comment 7

[Tom S]

http://hg.mozilla.org/integration/mozilla-inbound/rev/70acf458a32d

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/70acf458a32d