Last Comment Bug 759498 - defaults bound to functions are broken
: defaults bound to functions are broken
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Benjamin Peterson
:
Mentors:
Depends on: 763313 767660 785305
Blocks: 760304
  Show dependency treegraph
 
Reported: 2012-05-29 13:57 PDT by :Benjamin Peterson
Modified: 2012-08-23 21:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix the bug (28.17 KB, patch)
2012-05-29 23:15 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Review
address review comments (29.23 KB, patch)
2012-06-01 16:46 PDT, :Benjamin Peterson
no flags Details | Diff | Review

Description :Benjamin Peterson 2012-05-29 13:57:55 PDT
function f(a=42) {
    return a;
    function a() { return 19; }
}
f()

gives an assertion error at the moment.
Comment 1 :Benjamin Peterson 2012-05-29 23:15:12 PDT
Created attachment 628248 [details] [diff] [review]
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] 2012-05-31 20:17:10 PDT
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] 2012-06-01 16:05:13 PDT
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 2012-06-01 16:28:38 PDT
(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] 2012-06-01 16:34:57 PDT
(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 2012-06-01 16:46:21 PDT
Created attachment 629394 [details] [diff] [review]
address review comments
Comment 8 Phil Ringnalda (:philor) 2012-06-03 12:14:53 PDT
https://hg.mozilla.org/mozilla-central/rev/70acf458a32d

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