Bug 38409 – [ES6] Add support for default parameters

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Erik Arvidsson]

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) {}

[Oliver Hunt]

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.

[Saam Barati]

Created attachment 248635 [details]
WIP

It begins.

[Saam Barati]

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

[Oliver Hunt]

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

[Saam Barati]

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");

[Oliver Hunt]

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)

[Saam Barati]

(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.

[Oliver Hunt]

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

[Saam Barati]

(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.

[Ryosuke Niwa]

(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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 249648 [details]
WIP

rebased patch.

[Saam Barati]

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.

[Saam Barati]

Created attachment 257418 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/187351