Bug 38409 - [ES6] Add support for default parameters
Summary: [ES6] Add support for default parameters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL: http://wiki.ecmascript.org/doku.php?i...
Keywords:
Depends on: 141174 142944
Blocks: 80559
  Show dependency treegraph
 
Reported: 2010-04-30 15:52 PDT by Erik Arvidsson
Modified: 2015-07-24 11:43 PDT (History)
14 users (show)

See Also:


Attachments
WIP (12.69 KB, patch)
2015-03-13 19:36 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (31.54 KB, patch)
2015-03-20 16:20 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (36.01 KB, patch)
2015-03-27 23:49 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (31.49 KB, patch)
2015-07-23 15:37 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (58.34 KB, patch)
2015-07-23 18:55 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2010-04-30 15:52:16 PDT
Default parameters is one of the approved proposals for the next version of ECMAScript (Harmony).

var x;
function f(y = x) {
  return y
}

x = 1;
assertEquals(1, f());
x = 2;
assertEquals(2, f());

Default parameters may not be followed by non default parameters so the following is an error

function g(x = y, z) {}
Comment 1 Oliver Hunt 2010-05-01 12:08:02 PDT
We'll hold off on this until there's less flux in the spec.

I believe strict mode has more value than this as well.
Comment 2 Saam Barati 2015-03-13 19:36:36 PDT
Created attachment 248635 [details]
WIP

It begins.
Comment 3 Saam Barati 2015-03-16 00:09:12 PDT
Read the spec on this.

We need to implement algorithm in point 28 in this section:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-functiondeclarationinstantiation
Comment 4 Oliver Hunt 2015-03-16 09:40:13 PDT
More fun cases to consider:

function f(x, y=x) { .... }
function f(x=y, y) { .... }
function f(x, y=function() {return x}) { .... }
function f(x=function() { return y}, y) { .... }
function f(x=f, f) { .... }
function f(f, x=f) { .... }
(function f(x=f, f) { .... })
(function f(f, x=f) { .... })

etc, etc
Comment 5 Saam Barati 2015-03-20 16:20:58 PDT
Created attachment 249144 [details]
WIP

I'm still not sure if I like the approach of saving the SourceCode of a function's parameters
for reparsing later. Maybe a better solution will be to have the SourceCode of a function
contain the text of both its parameters and its body. This would probably require changing 
the places where a function's SourceCode is assumed only to be its body.

Some things that don't work yet in this patch:
function scoping(f = function() { return local;}) {
    assert(f() === undefined);
    var local = 10;
    assert(f() === undefined);
}
scoping();

function augmentsArguments(x = 20) {
    assert(x === 20);
    assert(arguments[0] === x);

    arguments[0] = 10;
    assert(x === 10);
    assert(arguments[0] === x);

    x = 15
    assert(x === 15);
    assert(arguments[0] === x);

    function augment() { x = 40 }
    augment()
    assert(x === 40);
    assert(arguments[0] === x);
    arguments = 20;
}
augmentsArguments();
augmentsArguments(20);

some things that do work:
function basic(x, y=x) {
    assert(y === x, "basics don't work.")
}
basic(20);
basic("hello");
basic({foo: 20});

function basicError(x = y, y) { }
shouldThrow(basicError)

function basicError2(x = x) { }
shouldThrow(basicError2)

;(function(){
    var scopeVariable = {hello: "world"};
    function basicScope(x = scopeVariable) {
        assert(x === scopeVariable);
    }
    basicScope();
})();

function basicFunctionCaptureInDefault(theArg = 20, y = function() {return theArg}) {
    assert(theArg === y(), "y should return x.");
    theArg = {};
    assert(theArg === y(), "y should return x.");
}
basicFunctionCaptureInDefault()

function basicCaptured(x = 20, y = x) {
    assert(x === y, "y should equal x");
    function mutate() { x = "mutation"; }
    mutate()
    assert(x !== y, "y should not equal x");
}
basicCaptured()

// FIXME: not sure if this should work or not. Currently, it does.
function tricky(globalX = (globalX = "x"), y = (globalX = 20)) {
    assert(globalX === 20);
    assert(globalX === y);
}
tricky();
assert(globalX === "x");
Comment 6 Oliver Hunt 2015-03-20 16:36:55 PDT
I believe the spec is fairly explicit about when the parameters come into scope, and when they cease being in a TDZ.

You should also have tests for

function f(x=5) ...

f(undefined)

function f(x=y, y=5)
f(undefined, undefined)
Comment 7 Saam Barati 2015-03-20 17:55:46 PDT
(In reply to comment #6)
> I believe the spec is fairly explicit about when the parameters come into
> scope, and when they cease being in a TDZ.
Right. I'll read through the spec more closely this weekend
in regards to this and make explicit tests. 

I think to get this working:
function scoping(f = function() { return local;}) {
    assert(f() === undefined);
    var local = 10;
    assert(f() === undefined);
}
scoping();

We'll need a stronger notion
of scope that would also be used to implement 'let's block scoping.
Comment 8 Oliver Hunt 2015-03-21 08:35:54 PDT
I believe a lot of these scoping semantics would be easier to do if we first supported block scoping (eg let). Then we should already have the necessary internal models of lexical scoping and tdz
Comment 9 Saam Barati 2015-03-21 13:41:00 PDT
(In reply to comment #8)
> I believe a lot of these scoping semantics would be easier to do if we first
> supported block scoping (eg let). Then we should already have the necessary
> internal models of lexical scoping and tdz

Right. Agreed.

Also, I totally gave the wrong section of the spec earlier. After some closer reading today, point 28 of:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-functiondeclarationinstantiation
Specifies that if the formal parameters list contains default expressions, then a separate environment must be created for these expressions to be evaluated in.
This environment should be separate from the var declarations inside a function.

If we support block scoping, then this should be super easy to do. I'm going
to open up a bug to implement 'let' block scoping.

Section 14.1.18 specifies the actual evaluation of the default expressions inside
the formal parameters list (and formal parameter list evaluation in general):
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions-runtime-semantics-iteratorbindinginitialization

I haven't got the chance to read through that in detail yet but will today
or tomorrow.
Comment 10 Ryosuke Niwa 2015-03-21 14:00:03 PDT
(In reply to comment #8)
> I believe a lot of these scoping semantics would be easier to do if we first
> supported block scoping (eg let). Then we should already have the necessary
> internal models of lexical scoping and tdz

I've already added op_check_tdz in http://trac.webkit.org/changeset/181466 (althrough it currently doesn't take care of heap access in DFG) so we just need lexical scoping, which is presumably a parser/bytecodegenerator level feature.
Comment 11 Saam Barati 2015-03-21 14:37:25 PDT
A(In reply to comment #10)
> (In reply to comment #8)
> > I believe a lot of these scoping semantics would be easier to do if we first
> > supported block scoping (eg let). Then we should already have the necessary
> > internal models of lexical scoping and tdz
> 
> I've already added op_check_tdz in http://trac.webkit.org/changeset/181466
> (althrough it currently doesn't take care of heap access in DFG) so we just
> need lexical scoping, which is presumably a parser/bytecodegenerator level
> feature.

Right. I think we should be able to make the changes
in the parser/AST/BytecodeGenerator. I'll open a bug
for this and read the spec more closely and investigate
what exactly we need to do. I'm happy to work on the
feature unless somebody else wants to take it. 
Also, if I take it, would love some feedback on how
ya'll think it should be implemented.
Comment 12 Saam Barati 2015-03-27 23:49:41 PDT
Created attachment 249648 [details]
WIP

rebased patch.
Comment 13 Saam Barati 2015-07-23 15:37:29 PDT
Created attachment 257394 [details]
WIP

Almost done.
I need to make the parser throw errors for duplicate parameter definitions in the presence of default parameter values.
I also need to write more tests.
Comment 14 Saam Barati 2015-07-23 18:55:47 PDT
Created attachment 257418 [details]
patch
Comment 15 WebKit Commit Bot 2015-07-23 18:57:58 PDT
Attachment 257418 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:722:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:214:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:558:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:624:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:1451:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Saam Barati 2015-07-24 11:43:39 PDT
landed in:
https://trac.webkit.org/changeset/187351