Allow parameters without defaults after default parameters

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: Benjamin)

Tracking

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

Other Branch
mozilla26
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

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

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

f() should return [1, undefined].
(Assignee)

Comment 1

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

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

Dave
(Assignee)

Updated

7 years ago
Blocks: 694100
Whiteboard: [js:t]
Whiteboard: [js:t] → [js:p2]
(Reporter)

Comment 3

7 years ago
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?
Not yet resolved. We should probably keep it as a syntax error until the semantics has been determined.

Dave
Blocks: 884372
(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
(Assignee)

Comment 6

6 years ago
Posted patch do itSplinter Review
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.
Attachment #785487 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed

Comment 7

6 years ago
(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].
(Reporter)

Comment 8

6 years ago
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.
Attachment #785487 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 9

6 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/2442b877654f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Whiteboard: [js:p2] → [js:p2][DocArea=JS]
(Reporter)

Updated

3 years ago
No longer blocks: 884372
You need to log in before you can comment on or make changes to this bug.