The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: Benjamin)

Tracking

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

Other Branch
mozilla18
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
<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
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
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.
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.
Right, sorry I misread.  Web IDL counts all optional arguments too.  Either way, I'm happy to change to match JS here.
(Assignee)

Updated

5 years ago
Blocks: 694100
Whiteboard: [js:p2]
(Assignee)

Comment 5

5 years ago
Created attachment 661243 [details] [diff] [review]
don't count defaults in length
Assignee: general → benjamin
Attachment #661243 - Flags: review?(jorendorff)
(Reporter)

Comment 6

5 years ago
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".
Attachment #661243 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 7

5 years ago
sfink, see comment 6.
Cameron, should WebIDL change accordingly?
Done: http://dev.w3.org/cvsweb/2006/webapi/WebIDL/Overview.xml.diff?r1=1.565;r2=1.566;f=h
Aryeh, does idlharness test for this?
Cameron, what about constructors (section 4.4.1 still says "maximum argument list length of the constructors")?
Oh, I got named constructors but not that one.  Fixed now.
(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?
(Assignee)

Comment 14

5 years ago
Steve is correct. The padding on JSScript keeps it properly aligned to 8 bytes on 32-bit platforms.
(Assignee)

Comment 15

5 years ago
(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).
(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.
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa378330331
(Assignee)

Comment 19

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/7fa378330331
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(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?
(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.
Depends on: 793151

Updated

5 years ago
Keywords: dev-doc-needed
Updated https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Function/length.
Keywords: dev-doc-needed → dev-doc-complete
Just added: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_18
You need to log in before you can comment on or make changes to this bug.