Summary: | [ES6] Add support for default parameters | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, caitp, commit-queue, fpizlo, ggaren, mark.lam, m.goleb+bugzilla, mhahnenb, mmirman, msaboff, oliver, rniwa, saam, ysuzuki | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
URL: | http://wiki.ecmascript.org/doku.php?id=harmony:parameter_default_values | ||||||||||||||
Bug Depends on: | 141174, 142944 | ||||||||||||||
Bug Blocks: | 80559 | ||||||||||||||
Attachments: |
|
Description
Erik Arvidsson
2010-04-30 15:52:16 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. Created attachment 248635 [details]
WIP
It begins.
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 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 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");
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) (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. 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 (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. (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. 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. Created attachment 249648 [details]
WIP
rebased patch.
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.
Created attachment 257418 [details]
patch
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.
landed in: https://trac.webkit.org/changeset/187351 |