Last Comment Bug 777061 - Function .length property should not count rest parameters or parameters with default values
: Function .length property should not count rest parameters or parameters with...
Status: RESOLVED FIXED
[js:p2]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla18
Assigned To: :Benjamin Peterson
: general
Mentors:
Depends on: 793151
Blocks: es6
  Show dependency treegraph
 
Reported: 2012-07-24 13:50 PDT by Jason Orendorff [:jorendorff]
Modified: 2013-01-03 02:24 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't count defaults in length (7.17 KB, patch)
2012-09-14 09:00 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-07-24 13:50:21 PDT
<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] 2012-07-24 13:52:13 PDT
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) 2012-07-24 13:59:09 PDT
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 [:bz] 2012-07-24 14:24:57 PDT
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) 2012-07-24 14:36:04 PDT
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 2012-09-14 09:00:58 PDT
Created attachment 661243 [details] [diff] [review]
don't count defaults in length
Comment 6 Jason Orendorff [:jorendorff] 2012-09-19 06:33:23 PDT
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] 2012-09-19 06:35:47 PDT
sfink, see comment 6.
Comment 8 Boris Zbarsky [:bz] 2012-09-19 06:36:39 PDT
Cameron, should WebIDL change accordingly?
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-09-19 07:29:30 PDT
Aryeh, does idlharness test for this?
Comment 11 Boris Zbarsky [:bz] 2012-09-19 08:37:10 PDT
Cameron, what about constructors (section 4.4.1 still says "maximum argument list length of the constructors")?
Comment 12 Cameron McCormack (:heycam) 2012-09-19 08:52:41 PDT
Oh, I got named constructors but not that one.  Fixed now.
Comment 13 Steve Fink [:sfink] [:s:] 2012-09-19 10:26:32 PDT
(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 2012-09-19 10:50:52 PDT
Steve is correct. The padding on JSScript keeps it properly aligned to 8 bytes on 32-bit platforms.
Comment 15 :Benjamin Peterson 2012-09-19 11:00:00 PDT
(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:] 2012-09-19 13:21:39 PDT
(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 2012-09-19 13:45:27 PDT
(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 19 :Benjamin Peterson 2012-09-19 13:54:39 PDT
(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 (⌚ UTC+1/+2) 2012-09-20 04:52:23 PDT
https://hg.mozilla.org/mozilla-central/rev/7fa378330331
Comment 21 :Aryeh Gregor (away until August 15) 2012-09-20 04:58:01 PDT
(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 (⌚ UTC+1/+2) 2012-09-20 05:02:10 PDT
(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 24 Kohei Yoshino [:kohei] 2013-01-03 02:24:27 PST
Just added: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_18

Note You need to log in before you can comment on or make changes to this bug.