Bug 162377 - test262: Function length should be number of parameters before parameters with default values
Summary: test262: Function length should be number of parameters before parameters wit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-22 01:11 PDT by Joseph Pecoraro
Modified: 2016-09-26 11:43 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (53.68 KB, patch)
2016-09-22 01:16 PDT, Joseph Pecoraro
saam: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Landing (54.19 KB, patch)
2016-09-22 11:49 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-09-22 01:11:32 PDT
Summary:
Function length should be number of parameters before parameters with default values

Spec:
https://tc39.github.io/ecma262/#sec-function-definitions-static-semantics-expectedargumentcount

> NOTE: The ExpectedArgumentCount of a FormalParameterList is the number of
> FormalParameters to the left of either the rest parameter or the first
> FormalParameter with an Initializer. A FormalParameter without an
> initializer is allowed after the first parameter with an initializer
> but such parameters are considered to be optional with undefined as
> their default value.

Example:
// foo.length should be 1
function foo(a,b=1,c) {};

Notes:
- Applies to all functions (getters/setters, generators, methods, arrow functions)
- setters require 1 param, but that can have a default value so this needs to be handled
- getters require 0 params, so are not affected

Covered by 32 tst262 tests with "*-dflt-length.js" in the name.
Comment 1 Joseph Pecoraro 2016-09-22 01:16:03 PDT
Created attachment 289535 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2016-09-22 01:20:19 PDT
Comment on attachment 289535 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=289535&action=review

> Source/JavaScriptCore/ChangeLog:19
> +        Alongside the parameterCount value, maintain a separate count,
> +        nonDefaultParameterCount, which will be the count before seeing
> +        a rest parameter or initialized parameter.

Of course after I wrote the patch "nonDefaultParameterCount" is a kind of poor name since (1) you can have non-default-parameters after the first default-parameter and (2) a rest parameter affects this but is not a default parameter.

    function (a,b=1,c) {} // length === 1

I'm open to better naming suggestions!

Here are some suggestions:

    parameterCountForLength
    parameterCountForFunctionLength
    parameterCountBeforeInitializedParameter
Comment 3 Saam Barati 2016-09-22 09:03:29 PDT
Comment on attachment 289535 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=289535&action=review

r=me

> JSTests/stress/es6-default-parameters.js:236
> +function named11(a=1,...b){};

Can you also add a test like:
(a=10, b, c=20, ...r) or something similar.

>> Source/JavaScriptCore/ChangeLog:19
>> +        a rest parameter or initialized parameter.
> 
> Of course after I wrote the patch "nonDefaultParameterCount" is a kind of poor name since (1) you can have non-default-parameters after the first default-parameter and (2) a rest parameter affects this but is not a default parameter.
> 
>     function (a,b=1,c) {} // length === 1
> 
> I'm open to better naming suggestions!
> 
> Here are some suggestions:
> 
>     parameterCountForLength
>     parameterCountForFunctionLength
>     parameterCountBeforeInitializedParameter

Why not just "functionLength"?
Comment 4 Joseph Pecoraro 2016-09-22 11:49:36 PDT
Created attachment 289589 [details]
[PATCH] For Landing
Comment 5 WebKit Commit Bot 2016-09-22 12:13:39 PDT
Comment on attachment 289589 [details]
[PATCH] For Landing

Clearing flags on attachment: 289589

Committed r206268: <http://trac.webkit.org/changeset/206268>