777061 - Function .length property should not count rest parameters or parameters with default values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

<dherman> quick bit of info: I can confirm that the .length property should be the longest prefix of formal parameters with no default
<dherman> so (function(x, y, z=1, w){}).length === 2

Currently that's a SyntaxError but that's bug 777060; here's a test revealing the bug to be fixed here:

function f(x=1) {}
assertEq(f.length, 0);  // fails, f.length is 1

Comment 1

[Dave Herman [:dherman]]

Note that WebIDL currently appears to have a different policy for .length but I will discuss with Cameron to see if that part of WebIDL can be changed.

Dave

Comment 2

[Cameron McCormack (:heycam)]

I'm open to changing.  I think the Web IDL spec is what you want except that it also counts a final "..." argument, for example

  void f(int... x);

has a .length of 1.  I think 0 makes more sense there.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Actually, per WebIDL if I have this IDL:

  void foo(long x, optional long y = 5, optional long z);

then foo.length will be 3, unless I'm missing something.

Comment 4

[Cameron McCormack (:heycam)]

Right, sorry I misread.  Web IDL counts all optional arguments too.  Either way, I'm happy to change to match JS here.

Comment 5

[:Benjamin Peterson]

Attachment: don't count defaults in length

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 661243 [details] [diff] [review]
don't count defaults in length

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

It's kind of hard to believe we have a 32-bit PADDING field just lying around like that. What the heck? sfink, do you know what that was about?

Apart from that, my only comment is that maybe it'd be a little plainer to store a "length" field (since that's the only reason we're keeping it) rather than "ndefaults".

Comment 7

[Jason Orendorff [:jorendorff]]

sfink, see comment 6.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Cameron, should WebIDL change accordingly?

Comment 9

[Cameron McCormack (:heycam)]

Done: http://dev.w3.org/cvsweb/2006/webapi/WebIDL/Overview.xml.diff?r1=1.565;r2=1.566;f=h

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Aryeh, does idlharness test for this?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Cameron, what about constructors (section 4.4.1 still says "maximum argument list length of the constructors")?

Comment 12

[Cameron McCormack (:heycam)]

Oh, I got named constructors but not that one.  Fixed now.

Comment 13

[Steve Fink [:sfink] [:s:]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Comment on attachment 661243 [details] [diff] [review]
> don't count defaults in length
> 
> Review of attachment 661243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's kind of hard to believe we have a 32-bit PADDING field just lying
> around like that. What the heck? sfink, do you know what that was about?

No clue. Came in with bug 761723. The usual problem is that JSScript wants to be an even multiple of sizeof(Cell), and the size of some JSScript fields varies between 32- and 64-bit platforms. benjamin?

Comment 14

[:Benjamin Peterson]

Steve is correct. The padding on JSScript keeps it properly aligned to 8 bytes on 32-bit platforms.

Comment 15

[:Benjamin Peterson]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Apart from that, my only comment is that maybe it'd be a little plainer to
> store a "length" field (since that's the only reason we're keeping it)
> rather than "ndefaults".

I'm afraid someone will confuse it with "length" of the script (bytecode length or something).

Comment 16

[Steve Fink [:sfink] [:s:]]

(In reply to Benjamin Peterson [:benjamin] from comment #14)
> Steve is correct. The padding on JSScript keeps it properly aligned to 8
> bytes on 32-bit platforms.

But it's unconditional. Is it necessary on 64-bit platforms? I just removed it, and my 64-bit build still seems happy.

Comment 17

[:Benjamin Peterson]

(In reply to Steve Fink [:sfink] from comment #16)
> (In reply to Benjamin Peterson [:benjamin] from comment #14)
> > Steve is correct. The padding on JSScript keeps it properly aligned to 8
> > bytes on 32-bit platforms.
> 
> But it's unconditional. Is it necessary on 64-bit platforms? I just removed
> it, and my 64-bit build still seems happy.

If it's 3 (mod 8) bytes on 64-bit, the compiler will align it to the word.

Comment 18

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa378330331

Comment 19

[:Benjamin Peterson]

(In reply to Benjamin Peterson [:benjamin] from comment #17)
> (In reply to Steve Fink [:sfink] from comment #16)
> > (In reply to Benjamin Peterson [:benjamin] from comment #14)
> > > Steve is correct. The padding on JSScript keeps it properly aligned to 8
> > > bytes on 32-bit platforms.
> > 
> > But it's unconditional. Is it necessary on 64-bit platforms? I just removed
> > it, and my 64-bit build still seems happy.
> 
> If it's 3 (mod 8) bytes on 64-bit, the compiler will align it to the word.

What I meant to say, is some members after it force it to have the same alignment whether it's there or not.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7fa378330331

Comment 21

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ms2ger from comment #10)
> Aryeh, does idlharness test for this?

Fixed to match new spec:

http://dvcs.w3.org/hg/resources/rev/d39b9dcfa572

It currently doesn't handle operation overloading, because I was focusing on testing the DOM spec's interfaces and that doesn't have any overloading last I checked.  Nor does it test variadic arguments, since the only functions in DOM with variadic arguments were unimplemented anyway at the time I wrote it (are they still?).

Does the patch for this bug also fix the fact that (e.g.) CSSStyleDeclaration.prototype.setProperty.length is 3 rather than 2, or should that get its own bug?

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Aryeh Gregor from comment #21)
> (In reply to :Ms2ger from comment #10)
> > Aryeh, does idlharness test for this?
> 
> Fixed to match new spec:
> 
> http://dvcs.w3.org/hg/resources/rev/d39b9dcfa572

Thanks!

> It currently doesn't handle operation overloading, because I was focusing on
> testing the DOM spec's interfaces and that doesn't have any overloading last
> I checked.  Nor does it test variadic arguments, since the only functions in
> DOM with variadic arguments were unimplemented anyway at the time I wrote it
> (are they still?).

They still are, yes.

> Does the patch for this bug also fix the fact that (e.g.)
> CSSStyleDeclaration.prototype.setProperty.length is 3 rather than 2, or
> should that get its own bug?

That needs its own bug.

Comment 23

[Tom S [:evilpie]]

Updated https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Function/length.

Comment 24

[Kohei Yoshino [:kohei]]

Just added: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_18