777060 - Allow parameters without defaults after default parameters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

function f(x=1, y) { return [x, y]; }

This is currently a SyntaxError, but it should be allowed.

f() should return [1, undefined].

Comment 1

[:Benjamin Peterson]

I suppose we can just make them implicitly default to undefined.

Comment 2

[Dave Herman [:dherman]]

> I suppose we can just make them implicitly default to undefined.

Correct. That's exactly what the semantics should be.

Dave

Comment 3

[Jason Orendorff [:jorendorff]]

There remains the question of whether f(2) should return [2, undefined] or [1, 2].

Dave, has TC39 decided not to include this in ES6?

Comment 4

[Dave Herman [:dherman]]

Not yet resolved. We should probably keep it as a syntax error until the semantics has been determined.

Dave

Comment 5

[Brendan Eich [:brendan]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> There remains the question of whether f(2) should return [2, undefined] or
> [1, 2].
> 
> Dave, has TC39 decided not to include this in ES6?

There's no universe in which [1, 2] would be returned given function f from comment 0 called via f(2). Cc'ing Allen to confirm.

/be

Comment 6

[:Benjamin Peterson]

Attachment: do it

The actual implementation is very simple; basically just remove the error checking. Most of this is shuffling around to implement the |length| property on Function instances.

Comment 7

[Allen Wirfs-Brock]

(In reply to Brendan Eich [:brendan] from comment #5)
> (In reply to Jason Orendorff [:jorendorff] from comment #3)
> > There remains the question of whether f(2) should return [2, undefined] or
> > [1, 2].
> > 
> > Dave, has TC39 decided not to include this in ES6?
> 
> There's no universe in which [1, 2] would be returned given function f from
> comment 0 called via f(2). Cc'ing Allen to confirm.
> 
> /be

That's correct, the correct result according to the ES6 draft is [1, undefined].

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 785487 [details] [diff] [review]
do it

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

::: js/src/frontend/Parser.cpp
@@ +1651,5 @@
> +                        hasDefaults = true;
> +
> +                        // The Function.length property is the number of formals
> +                        // before the first default argument.
> +                        funbox->length = pc->numArgs() - 1;

This is a lot nicer than it was before. More direct.

::: js/src/frontend/SharedContext.h
@@ +302,5 @@
>      void setDefinitelyNeedsArgsObj()       { JS_ASSERT(funCxFlags.argumentsHasLocalBinding);
>                                               funCxFlags.definitelyNeedsArgsObj   = true; }
>  
> +    bool hasDefaults() const {
> +        return length != function()->nargs;

nargs includes rest params, right? So shouldn't this be:
    return length != function()->nargs - function()->hasRest();
? If there's not a test that triggers the bug, please add one.

While I was looking at this, I noticed that jsfun.h says:

    uint16_t        nargs;        /* maximum number of specified arguments,
                                     reflected as f.length/f.arity */

Please fix the comment there.

Comment 9

[:Benjamin Peterson]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> ::: js/src/frontend/SharedContext.h
> @@ +302,5 @@
> >      void setDefinitelyNeedsArgsObj()       { JS_ASSERT(funCxFlags.argumentsHasLocalBinding);
> >                                               funCxFlags.definitelyNeedsArgsObj   = true; }
> >  
> > +    bool hasDefaults() const {
> > +        return length != function()->nargs;
> 
> nargs includes rest params, right? So shouldn't this be:
>     return length != function()->nargs - function()->hasRest();
> ? If there's not a test that triggers the bug, please add one.

Very good catch! It is incorrect, but right now its usage doesn't trigger a semantic bug; it just causes some unnecessary bytecode to be generated. I will fix.

> 
> While I was looking at this, I noticed that jsfun.h says:
> 
>     uint16_t        nargs;        /* maximum number of specified arguments,
>                                      reflected as f.length/f.arity */
> 
> Please fix the comment there.

Done.

Comment 10

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2442b877654f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2442b877654f

Comment 12

[Florian Scholz (Open Web Docs)]

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