Closed Bug 1022962 Opened 6 years ago Closed 6 years ago

Default parameters should be evaluated before function declarations

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(2 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

(Separated from 1018628)

Evaluate following code in Firefox 29:

> function f(a=(b=1), b=2, c=a) {
>   function a() {}
>   return [a, b, c];
> }
> f()



Actual results:

|f()| returns |[function a() {}, 2, function a() {}]|.



Expected results:

|f()| should returns |[function a() {}, 1, 1]|.

|function a() {}| is evaluated before default parameters, so parameter |a| is always defined as |function a() {}|, and default parameter for |a| is never evaluated.

Patch is almost ready, I'll post after try run.
Blocks: 1018628, es6, 884372
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 1022967
This patch emits bytecode for default parameters before function declarations.

As I noted in Bug 1018628, this patch will introduce following incompatibility.

1. Default parameter is evaluated even if function body contains a function with same name as parameter:
> function f(a=alert(1)) {
>   function a() {}
> }
> f();
After this patch is applied, alert will be shown (was not shown before) in above code.

2. Default parameter cannot refer functions inside function body
> function g(a=b) {
>   function b() {}
>   return a;
> }
> g();
After this patch is applied, |g()| returns |undefined| (was |function b() {}| before).
There is an exception about this, closure can refer those functions,
e.g. |function g(a=()=>b) ...|, this should be addressed in Bug 1022967.

Fortunatelly, there is no regression in Firefox code.
I hope there is also no impact in Add-ons.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=58bf2595f74b
Attachment #8437485 - Flags: review?(jorendorff)
Fantastic. I will review this tomorrow morning.
Comment on attachment 8437485 [details] [diff] [review]
Evaluate default parameters before function declarations.

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

Please post patches with at least 8 lines of context. (`hg diff -U8` is one way to generate that kind of patch.)

Clearing r? flag for now. This patch looks exactly correct to me except for the bits that seem extraneous in BytecodeEmitter.cpp.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6157,5 @@
> +            def_expr = arg->pn_right;
> +        } else {
> +            name = arg;
> +            def_expr = name->expr();
> +        }

All the changes before line 6200 don't seem to belong in this patch. (They also don't seem correct, but maybe I just don't understand what they're doing...)

::: js/src/jit-test/tests/arguments/defaults-bound-to-function.js
@@ +16,5 @@
>      function b() {}
>  }
> +assertThrowsInstanceOf(function i(b=FAIL) {
> +  function b() {}
> +}, ReferenceError);

You can just say

assertThrowsInstanceOf(i, ReferenceError);

if you want.
Attachment #8437485 - Flags: review?(jorendorff)
Thank you for reviewing, and letting me know about U8 option!

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> All the changes before line 6200 don't seem to belong in this patch. (They
> also don't seem correct, but maybe I just don't understand what they're
> doing...)

That is the change for following case.
  function f(a=alert(1)) {
    function a() {}
  }
Here, |a=alert(1)| is parsed to |(ASSIGN a (CALL [alert 1]))|.

If the function body does not contain |function a() {}|,
|a=alert(1)| is parsed to NAME node |a|, with PND_DEFAULT flag, and |(CALL [alert 1])| in its pn_expr.

In former case, ASSIGN node is created by makeDefIntoUse function,
which is called from checkFunctionDefinition in Parser.cpp.
>            /*
>             * Body-level function statements are effectively variable
>             * declarations where the initialization is hoisted to the
>             * beginning of the block. This means that any other variable
>             * declaration with the same name is really just an assignment to
>             * the function's binding (which is mutable), so turn any existing
>             * declaration into a use.
>             */
>            if (bodyLevel && !makeDefIntoUse(dn, pn, funName))
>                return false;

Then, without this patch, EmitDefaults does not emit bytecode for ASSIGN node,
because it means |function a() {}| is already defined before default parameters,
and the default value is never used (I guess this is a kind of optimization).

But if we move function declarations after default parameter,
we should emit bytecode for ASSIGN node.

Please correct me if I'm wrong.
Flags: needinfo?(jorendorff)
Comment on attachment 8437485 [details] [diff] [review]
Evaluate default parameters before function declarations.

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

Setting the review flag again. Clearly I need to read more closely.
Attachment #8437485 - Flags: review?(jorendorff)
Comment on attachment 8437485 [details] [diff] [review]
Evaluate default parameters before function declarations.

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

OK. This is correct. Now my objection here is a little different. If argument defaults now behave the same whether there's a function with the same name or not, then we definitely don't need two different AST structures. Please get rid of the PNK_ASSIGN idiom!
Attachment #8437485 - Flags: review?(jorendorff)
Flags: needinfo?(jorendorff)
Is it okay to having 2 Definition node for same variable?
With this patch, argument is not turned into use, so variable |a| will have Definition node, one for default argument and other for function declaration, in following code:

  function (a=10) {
    function a() {}
  }

It seems working without single different behavior in reflect, but I'm not sure this is correct or not.

In Reflect, default value was overwritten by function declaration with same name.
With this patch, correct default value is output.

>  assertDecl("function foo(a=(function () {})) { function a() {} }",
>             funDecl(ident("foo"), [ident("a")], blockStmt([funDecl(ident("a"), [], blockStmt([]))]),
> -                   [funExpr(ident("a"), [], blockStmt([]))]));
> +                   [funExpr(null, [], blockStmt([]))]));

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=1ec1bfa48fa6
Attachment #8437485 - Attachment is obsolete: true
Attachment #8447712 - Flags: review?(jorendorff)
(In reply to Tooru Fujisawa [:arai] from comment #7)
> Created attachment 8447712 [details] [diff] [review]
> Evaluate default parameters before function declarations.
> 
> Is it okay to having 2 Definition node for same variable?

Hmm. Well, no, it's not exactly OK.

Certainly it wouldn't be OK for ParseContext<FullParseHandler>::define() to be called for both the argument and the function. An assertion would fail if that were happening.

It would also be bad if the frontend gave the function its own slot.

It would also be bad if anything else (such as a PNK_NAME node) ended up being treated as a "use" of the function node rather than the argument node.

Right now, the bytecode emitted for such a case is like this:

js> dis(function (a) { function a(){} })
flags: LAMBDA
loc     op
-----   --
main:
00000:  lambda function a(){}
00005:  setarg 0
00008:  pop
00009:  retrval

Make sure this doesn't change with your patch. In particular make sure we're emitting 'setarg 0' and not a 'setlocal' instruction or something else.

Also check the case where a nested function closes over a. In that case you should get a setaliasedarg instruction.

If you find anything unexpected, it should be fairly easy to write a test that proves the new code is wrong. And that would be a good test to have.

I think it's just barely OK that the PNK_FUNCTION node has pn_defn=1, as long as it is never stored in pc->decls().

> With this patch, argument is not turned into use, so variable |a| will have
> Definition node, one for default argument and other for function
> declaration, in following code:
> 
>   function (a=10) {
>     function a() {}
>   }

OK. Well... If you look at the dis() output and it all looks correct, that's fine (although I'd like to understand how the function node ever gets bound to the correct slot; can you explain it?)

And if not, I think the fix would be to manually bind the PNK_FUNCTION node to the argument's slot. In checkFunctionDefinition(), change this

            if (bodyLevel && !makeDefIntoUse(dn, pn, funName))
                return false;

to be something like this:

            if (bodyLevel) {
                if (dn->kind() == ARG) {
                    // The exception to the above comment is when the function
                    // has the same name as an argument. Then the argument node
                    // remains a definition. But change the function node pn so
                    // that it knows where the argument is located.
                    pn->setOp(JSOP_GETARG);
                    pn->pn_cookie.set(tokenStream, 0, dn->pn_cookie.slot());
                    pn->pn_dflags |= PND_BOUND;
                    dn->markAsAssigned();
                } else {
                    if (!makeDefIntoUse(dn, pn, funName))
                        return false;
                }
            }

I think the 0 I'm passing to pn_cookie.set there is correct, but not totally sure.

> With this patch, correct default value is output.
> 
> >  assertDecl("function foo(a=(function () {})) { function a() {} }",
> >             funDecl(ident("foo"), [ident("a")], blockStmt([funDecl(ident("a"), [], blockStmt([]))]),
> > -                   [funExpr(ident("a"), [], blockStmt([]))]));
> > +                   [funExpr(null, [], blockStmt([]))]));

Looks good.
Comment on attachment 8447712 [details] [diff] [review]
Evaluate default parameters before function declarations.

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

Clearing the review flag here for now; but you can set it r?me again if the dis() output looks good and you can understand why the change in makeDefIntoUse is sufficient.
Attachment #8447712 - Flags: review?(jorendorff)
Okay, I use the fix you wrote (bind function node manually) in comment #8.

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> Right now, the bytecode emitted for such a case is like this:
> 
> js> dis(function (a) { function a(){} })
> flags: LAMBDA
> loc     op
> -----   --
> main:
> 00000:  lambda function a(){}
> 00005:  setarg 0
> 00008:  pop
> 00009:  retrval
> 
> Make sure this doesn't change with your patch. In particular make sure we're
> emitting 'setarg 0' and not a 'setlocal' instruction or something else.
> 
> Also check the case where a nested function closes over a. In that case you
> should get a setaliasedarg instruction.
> 
> If you find anything unexpected, it should be fairly easy to write a test
> that proves the new code is wrong. And that would be a good test to have.

With the previous patch, the output of dis() was same as above, and there was no different bytecode for any case (except for emitting default parameter).

But, the previous patch failed with function re-declaration after assignment, for example:
  function f(a) {
    function a() {
      return 42;
    }
    a = ()=>43;
    function a() {
      return 44;
    }
  }
It fails to change all uses to new def node, in makeDefIntoUse function, because there is def node (argument node) in the dn_uses link.

For more related tests, I've added alias-function-closed.js and alias-function-not-closed.js in this patch.

> > With this patch, argument is not turned into use, so variable |a| will have
> > Definition node, one for default argument and other for function
> > declaration, in following code:
> > 
> >   function (a=10) {
> >     function a() {}
> >   }
> 
> OK. Well... If you look at the dis() output and it all looks correct, that's
> fine (although I'd like to understand how the function node ever gets bound
> to the correct slot; can you explain it?)

Sorry if I misunderstand about slot, I think it's because of that pc->updateDecl(atom, pn) was called for function node, so the function node's cookie is set to the same value of the argument node's cookie, and the value is used because pn_defn=1.

> And if not, I think the fix would be to manually bind the PNK_FUNCTION node
> to the argument's slot. In checkFunctionDefinition(), change this
> 
>             if (bodyLevel && !makeDefIntoUse(dn, pn, funName))
>                 return false;
> 
> to be something like this:
> 
>             if (bodyLevel) {
>                 if (dn->kind() == ARG) {
>                     // The exception to the above comment is when the
> function
>                     // has the same name as an argument. Then the argument
> node
>                     // remains a definition. But change the function node pn
> so
>                     // that it knows where the argument is located.
>                     pn->setOp(JSOP_GETARG);
>                     pn->pn_cookie.set(tokenStream, 0, dn->pn_cookie.slot());
>                     pn->pn_dflags |= PND_BOUND;
>                     dn->markAsAssigned();
>                 } else {
>                     if (!makeDefIntoUse(dn, pn, funName))
>                         return false;
>                 }
>             }
> 
> I think the 0 I'm passing to pn_cookie.set there is correct, but not totally
> sure.

Thanks, yes, it should be better to use this way, it's simpler :)
Then, it seems that pn_defn=1 is required, because function node cannot have pn_lexdef, so I use following code, instead of calling pn_cookie.set(...):
+                    pn->setDefn(true);
+                    pn->pn_cookie = dn->pn_cookie;

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=2603ee5871f3
Attachment #8447712 - Attachment is obsolete: true
Attachment #8454258 - Flags: review?(jorendorff)
Comment on attachment 8454258 [details] [diff] [review]
Evaluate default parameters before function declarations.

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

Thanks for your persistence. Let's land this!

::: js/src/jit-test/tests/arguments/alias-function-closed.js
@@ +1,1 @@
> +function f1(a, aIs,        // without default parameter

Good tests. Thanks for writing these.

Generally speaking, it's a *lot* easier to review a lot of small tests than a few long ones. And later on, when something breaks, it's easier to debug smaller tests. But I'm thrilled at these, regardless. Thank you.
Attachment #8454258 - Flags: review?(jorendorff) → review+
Thank you!

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> Generally speaking, it's a *lot* easier to review a lot of small tests than
> a few long ones. And later on, when something breaks, it's easier to debug
> smaller tests.

Okay, thank you for letting me know about debugging, I'll do so in next time :)
Keywords: checkin-needed
Keywords: checkin-needed
Xdr.h conflicted.
Increment XDR_BYTECODE_VERSION, which conflicted with bug 1037871.
Carrying forward jorendorff's review.
Attachment #8454258 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8456342 [details] [diff] [review]
Evaluate default parameters before function declarations.

For the sheriffs to land this, it has to have the r+ flag set. You can just do that yourself when carrying an r+.

Green try run in comment 10.
Attachment #8456342 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b3918dd99a0e
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
This patch only adds a test.

Sorry, I found another (and simpler) testcase which will be broken by this change, that was written as an example in MDN's default parameters page (the page is already fixed).
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/default_parameters

  function withDefaults(a, b = 5, c = b, d = go(), e = this, f = arguments, g = this.value){
    function go(){return ":P"}
    return [a,b,c,d,e,f,g];
  }

Here, go() is called inside the default parameter, which is not yet defined.

In principle, this is tested by the test for duplicated name of parameter and function, but it should be better to have individual test.

try is running: https://tbpl.mozilla.org/?tree=Try&rev=9346f180b7d1
(The test is passed locally on Mac OS X 10.9.4 64bit, so I guess this is also passed on try server soon)
Attachment #8457521 - Flags: review?(jorendorff)
Comment on attachment 8457521 [details] [diff] [review]
Part 2: Add test for function call inside default parameter.

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

::: js/src/jit-test/tests/arguments/defaults-call-function.js
@@ +3,5 @@
> +function f1(a=g()) {
> +  function g() {
> +  }
> +}
> +assertThrowsInstanceOf(f1, TypeError);

I know we throw a TypeError right now, but I think ES6 actually requires a ReferenceError. It's OK to check this in anyway, but add a comment indicating that it's a known bug.
Attachment #8457521 - Flags: review?(jorendorff) → review+
Thank you!
Added comment in the test.
The problem will be fixed in Bug 1022967.

Try is running: https://tbpl.mozilla.org/?tree=Try&rev=eb24b85e7d05
Previous try run was cancelled.
Attachment #8457521 - Attachment is obsolete: true
Attachment #8457619 - Flags: review?(jorendorff)
Attachment #8457619 - Flags: review?(jorendorff) → review+
Following documents are updated:
  https://developer.mozilla.org/en-US/Firefox/Releases/33
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/default_parameters

TODO:
  * Site Compatibility for Firefox 33
(maybe this page should be created after Firefox 33 Aurora is released)
Attachment #8456342 - Flags: checkin+
Keywords: checkin-needed
Updated "Site Compatibility for Firefox 33" page:
  https://developer.mozilla.org/en-US/Firefox/Releases/33/Site_Compatibility
Thanks for the MDN doc updates, :arai!
(see comment 21 and comment 25)
No longer blocks: 884372
You need to log in before you can comment on or make changes to this bug.