Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (192602 => 192603)
--- trunk/Source/_javascript_Core/ChangeLog 2015-11-19 01:24:02 UTC (rev 192602)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-11-19 01:26:36 UTC (rev 192603)
@@ -1,3 +1,38 @@
+2015-11-18 Saam barati <sbar...@apple.com>
+
+ There is a bug when default parameter values are mixed with destructuring parameter values
+ https://bugs.webkit.org/show_bug.cgi?id=151369
+
+ Reviewed by Geoffrey Garen and Mark Lam.
+
+ Relanding this after a rollout.
+
+ This patch changes our parser to no longer declare destructuring
+ parameters as "var"s. This is a weird bug that just happened
+ to work in a world without default parameter values. In a world with
+ default parameter values this is just completely wrong. It would
+ incorrectly transform this program:
+ ```function foo(a = function() { b = 40; }, {b}) { a(); return b; }; foo(undefined, {b: 42}); // Should return 40```
+ into
+ ```function foo(a = function() { b = 40; }, {b}) { var b; a(); return b; }; foo(undefined, {b:42}); // Returns 42, not 40.```
+ Which is wrong because we end up with two distinct bindings of "b" when
+ there should only be one.
+
+ * parser/Parser.cpp:
+ (JSC::Parser<LexerType>::parseVariableDeclarationList):
+ (JSC::Parser<LexerType>::createBindingPattern):
+ (JSC::Parser<LexerType>::parseDestructuringPattern):
+ * parser/Parser.h:
+ (JSC::Scope::declareParameter):
+ (JSC::Scope::getUsedVariables):
+ (JSC::Parser::strictMode):
+ (JSC::Parser::isValidStrictMode):
+ (JSC::Parser::declareParameter):
+ (JSC::Parser::breakIsValid):
+ (JSC::Scope::declareBoundParameter): Deleted.
+ (JSC::Parser::declareBoundParameter): Deleted.
+ * tests/stress/es6-default-parameters.js:
+
2015-11-18 Mark Lam <mark....@apple.com>
Snippefy op_mul for the baseline JIT.
Modified: trunk/Source/_javascript_Core/parser/Parser.cpp (192602 => 192603)
--- trunk/Source/_javascript_Core/parser/Parser.cpp 2015-11-19 01:24:02 UTC (rev 192602)
+++ trunk/Source/_javascript_Core/parser/Parser.cpp 2015-11-19 01:26:36 UTC (rev 192603)
@@ -667,7 +667,7 @@
}
template <typename LexerType>
-template <class TreeBuilder> TreeDestructuringPattern Parser<LexerType>::createBindingPattern(TreeBuilder& context, DestructuringKind kind, ExportType exportType, const Identifier& name, int depth, JSToken token, AssignmentContext bindingContext, const Identifier** duplicateIdentifier)
+template <class TreeBuilder> TreeDestructuringPattern Parser<LexerType>::createBindingPattern(TreeBuilder& context, DestructuringKind kind, ExportType exportType, const Identifier& name, JSToken token, AssignmentContext bindingContext, const Identifier** duplicateIdentifier)
{
ASSERT(!name.isNull());
@@ -685,42 +685,23 @@
failIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Cannot declare a lexical variable twice: '", name.impl(), "'");
}
} else if (kind == DestructureToParameters) {
- if (depth) {
- auto bindingResult = declareBoundParameter(&name);
- if (bindingResult == Scope::StrictBindingFailed && strictMode()) {
- semanticFailIfTrue(isEvalOrArguments(&name), "Cannot destructure to a parameter name '", name.impl(), "' in strict mode");
- if (m_lastFunctionName && name == *m_lastFunctionName)
- semanticFail("Cannot destructure to '", name.impl(), "' as it shadows the name of a strict mode function");
- semanticFailureDueToKeyword("bound parameter name");
- if (hasDeclaredParameter(name))
- semanticFail("Cannot destructure to '", name.impl(), "' as it has already been declared");
- semanticFail("Cannot bind to a parameter named '", name.impl(), "' in strict mode");
- }
- if (bindingResult == Scope::BindingFailed) {
- semanticFailureDueToKeyword("bound parameter name");
- if (hasDeclaredParameter(name))
- semanticFail("Cannot destructure to '", name.impl(), "' as it has already been declared");
- semanticFail("Cannot destructure to a parameter named '", name.impl(), "'");
- }
- } else {
- DeclarationResultMask declarationResult = declareParameter(&name);
- if ((declarationResult & DeclarationResult::InvalidStrictMode) && strictMode()) {
- semanticFailIfTrue(isEvalOrArguments(&name), "Cannot destructure to a parameter name '", name.impl(), "' in strict mode");
- if (m_lastFunctionName && name == *m_lastFunctionName)
- semanticFail("Cannot declare a parameter named '", name.impl(), "' as it shadows the name of a strict mode function");
- semanticFailureDueToKeyword("parameter name");
- if (hasDeclaredParameter(name))
- semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode as it has already been declared");
- semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode");
- }
- if (declarationResult & DeclarationResult::InvalidDuplicateDeclaration) {
- // It's not always an error to define a duplicate parameter.
- // It's only an error when there are default parameter values or destructuring parameters.
- // We note this value now so we can check it later.
- if (duplicateIdentifier)
- *duplicateIdentifier = &name;
- }
+ DeclarationResultMask declarationResult = declareParameter(&name);
+ if ((declarationResult & DeclarationResult::InvalidStrictMode) && strictMode()) {
+ semanticFailIfTrue(isEvalOrArguments(&name), "Cannot destructure to a parameter name '", name.impl(), "' in strict mode");
+ if (m_lastFunctionName && name == *m_lastFunctionName)
+ semanticFail("Cannot declare a parameter named '", name.impl(), "' as it shadows the name of a strict mode function");
+ semanticFailureDueToKeyword("parameter name");
+ if (hasDeclaredParameter(name))
+ semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode as it has already been declared");
+ semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode");
}
+ if (declarationResult & DeclarationResult::InvalidDuplicateDeclaration) {
+ // It's not always an error to define a duplicate parameter.
+ // It's only an error when there are default parameter values or destructuring parameters.
+ // We note this value now so we can check it later.
+ if (duplicateIdentifier)
+ *duplicateIdentifier = &name;
+ }
}
if (exportType == ExportType::Exported) {
@@ -847,7 +828,7 @@
if (consume(COLON))
innerPattern = parseDestructuringPattern(context, kind, exportType, duplicateIdentifier, hasDestructuringPattern, bindingContext, depth + 1);
else
- innerPattern = createBindingPattern(context, kind, exportType, *propertyName, depth + 1, identifierToken, bindingContext, duplicateIdentifier);
+ innerPattern = createBindingPattern(context, kind, exportType, *propertyName, identifierToken, bindingContext, duplicateIdentifier);
} else {
JSTokenType tokenType = m_token.m_type;
switch (m_token.m_type) {
@@ -903,7 +884,7 @@
failWithMessage("Expected a parameter pattern or a ')' in parameter list");
}
failIfTrue(match(LET) && (kind == DestructureToLet || kind == DestructureToConst), "Can't use 'let' as an identifier name for a LexicalDeclaration");
- pattern = createBindingPattern(context, kind, exportType, *m_token.m_data.ident, depth, m_token, bindingContext, duplicateIdentifier);
+ pattern = createBindingPattern(context, kind, exportType, *m_token.m_data.ident, m_token, bindingContext, duplicateIdentifier);
next();
break;
}
Modified: trunk/Source/_javascript_Core/parser/Parser.h (192602 => 192603)
--- trunk/Source/_javascript_Core/parser/Parser.h 2015-11-19 01:24:02 UTC (rev 192602)
+++ trunk/Source/_javascript_Core/parser/Parser.h 2015-11-19 01:26:36 UTC (rev 192603)
@@ -424,26 +424,6 @@
return result;
}
- enum BindingResult {
- BindingFailed,
- StrictBindingFailed,
- BindingSucceeded
- };
- BindingResult declareBoundParameter(const Identifier* ident)
- {
- bool isArgumentsIdent = isArguments(m_vm, ident);
- auto addResult = m_declaredVariables.add(ident->impl());
- addResult.iterator->value.setIsVar(); // Treat destructuring parameters as "var"s.
- bool isValidStrictMode = addResult.isNewEntry && !isEval(m_vm, ident) && !isArgumentsIdent;
- m_isValidStrictMode = m_isValidStrictMode && isValidStrictMode;
-
- if (isArgumentsIdent)
- m_shadowsArguments = true;
- if (!addResult.isNewEntry)
- return BindingFailed;
- return isValidStrictMode ? BindingSucceeded : StrictBindingFailed;
- }
-
void getUsedVariables(IdentifierSet& usedVariables)
{
usedVariables.swap(m_usedVariables);
@@ -1054,7 +1034,6 @@
bool strictMode() { return currentScope()->strictMode(); }
bool isValidStrictMode() { return currentScope()->isValidStrictMode(); }
DeclarationResultMask declareParameter(const Identifier* ident) { return currentScope()->declareParameter(ident); }
- Scope::BindingResult declareBoundParameter(const Identifier* ident) { return currentScope()->declareBoundParameter(ident); }
bool breakIsValid()
{
ScopeRef current = currentScope();
@@ -1159,7 +1138,7 @@
template <class TreeBuilder> TreeExpression parseVariableDeclarationList(TreeBuilder&, int& declarations, TreeDestructuringPattern& lastPattern, TreeExpression& lastInitializer, JSTextPosition& identStart, JSTextPosition& initStart, JSTextPosition& initEnd, VarDeclarationListContext, DeclarationType, ExportType, bool& forLoopConstDoesNotHaveInitializer);
template <class TreeBuilder> TreeSourceElements parseArrowFunctionSingleExpressionBodySourceElements(TreeBuilder&);
template <class TreeBuilder> TreeExpression parseArrowFunctionExpression(TreeBuilder&);
- template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern createBindingPattern(TreeBuilder&, DestructuringKind, ExportType, const Identifier&, int depth, JSToken, AssignmentContext, const Identifier** duplicateIdentifier);
+ template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern createBindingPattern(TreeBuilder&, DestructuringKind, ExportType, const Identifier&, JSToken, AssignmentContext, const Identifier** duplicateIdentifier);
template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern parseDestructuringPattern(TreeBuilder&, DestructuringKind, ExportType, const Identifier** duplicateIdentifier = nullptr, bool* hasDestructuringPattern = nullptr, AssignmentContext = AssignmentContext::DeclarationStatement, int depth = 0);
template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern tryParseDestructuringPatternExpression(TreeBuilder&, AssignmentContext);
template <class TreeBuilder> NEVER_INLINE TreeExpression parseDefaultValueForDestructuringPattern(TreeBuilder&);
Modified: trunk/Source/_javascript_Core/tests/stress/es6-default-parameters.js (192602 => 192603)
--- trunk/Source/_javascript_Core/tests/stress/es6-default-parameters.js 2015-11-19 01:24:02 UTC (rev 192602)
+++ trunk/Source/_javascript_Core/tests/stress/es6-default-parameters.js 2015-11-19 01:26:36 UTC (rev 192603)
@@ -235,7 +235,39 @@
})();
+// Test proper variable binding.
+;(function() {
+ function foo(a = function() { return b; }, {b}) {
+ assert(a() === 34);
+ assert(b === 34);
+ b = 50;
+ assert(a() === 50);
+ assert(b === 50);
+ }
+ function bar(a = function(x) { b = x; }, {b}) {
+ assert(b === 34);
+ a(50);
+ assert(b === 50);
+ }
+ function baz(f1 = function(x) { b = x; }, f2 = function() { return b; }, {b}) {
+ var b;
+ assert(b === 34);
+ assert(f2() === 34);
+ f1(50);
+ assert(b === 34);
+ assert(f2() === 50);
+ }
+ noInline(foo);
+ noInline(bar);
+ noInline(baz);
+ for (let i = 0; i < 1000; i++) {
+ foo(undefined, {b: 34});
+ bar(undefined, {b: 34});
+ baz(undefined, undefined, {b: 34});
+ }
+})();
+
// Syntax errors.
shouldThrowSyntaxError("function b(a = 20, a = 40) {}");
shouldThrowSyntaxError("function b(aaaaa = 20,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v, aaaaa = 40) {}");
