Changeset 192586 in webkit


Ignore:
Timestamp:
Nov 18, 2015, 2:01:19 PM (10 years ago)
Author:
[email protected]
Message:

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.

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:
Location:
trunk/Source/JavaScriptCore
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r192561 r192586  
     12015-11-18  Saam barati  <[email protected]>
     2
     3        There is a bug when default parameter values are mixed with destructuring parameter values
     4        https://bugs.webkit.org/show_bug.cgi?id=151369
     5
     6        Reviewed by Geoffrey Garen and Mark Lam.
     7
     8        This patch changes our parser to no longer declare destructuring
     9        parameters as "var"s. This is a weird bug that just happened
     10        to work in a world without default parameter values. In a world with
     11        default parameter values this is just completely wrong. It would
     12        incorrectly transform this program:
     13        ```function foo(a = function() { b = 40; }, {b}) { a(); return b; }; foo(undefined, {b: 42}); // Should return 40```
     14        into
     15        ```function foo(a = function() { b = 40; }, {b}) { var b; a(); return b; }; foo(undefined, {b:42}); // Returns 42, not 40.```
     16        Which is wrong because we end up with two distinct bindings of "b" when
     17        there should only be one.
     18
     19        * parser/Parser.cpp:
     20        (JSC::Parser<LexerType>::parseVariableDeclarationList):
     21        (JSC::Parser<LexerType>::createBindingPattern):
     22        (JSC::Parser<LexerType>::parseDestructuringPattern):
     23        * parser/Parser.h:
     24        (JSC::Scope::declareParameter):
     25        (JSC::Scope::getUsedVariables):
     26        (JSC::Parser::strictMode):
     27        (JSC::Parser::isValidStrictMode):
     28        (JSC::Parser::declareParameter):
     29        (JSC::Parser::breakIsValid):
     30        (JSC::Scope::declareBoundParameter): Deleted.
     31        (JSC::Parser::declareBoundParameter): Deleted.
     32        * tests/stress/es6-default-parameters.js:
     33
    1342015-11-17  Benjamin Poulain  <[email protected]>
    235

  • trunk/Source/JavaScriptCore/parser/Parser.cpp

    r192436 r192586  
    668668
    669669template <typename LexerType>
    670 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)
     670template <class TreeBuilder> TreeDestructuringPattern Parser<LexerType>::createBindingPattern(TreeBuilder& context, DestructuringKind kind, ExportType exportType, const Identifier& name, JSToken token, AssignmentContext bindingContext, const Identifier** duplicateIdentifier)
    671671{
    672672    ASSERT(!name.isNull());
    … …  
    686686        }
    687687    } else if (kind == DestructureToParameters) {
    688         if (depth) {
    689             auto bindingResult = declareBoundParameter(&name);
    690             if (bindingResult == Scope::StrictBindingFailed && strictMode()) {
    691                 semanticFailIfTrue(isEvalOrArguments(&name), "Cannot destructure to a parameter name '", name.impl(), "' in strict mode");
    692                 if (m_lastFunctionName && name == *m_lastFunctionName)
    693                     semanticFail("Cannot destructure to '", name.impl(), "' as it shadows the name of a strict mode function");
    694                 semanticFailureDueToKeyword("bound parameter name");
    695                 if (hasDeclaredParameter(name))
    696                     semanticFail("Cannot destructure to '", name.impl(), "' as it has already been declared");
    697                 semanticFail("Cannot bind to a parameter named '", name.impl(), "' in strict mode");
    698             }
    699             if (bindingResult == Scope::BindingFailed) {
    700                 semanticFailureDueToKeyword("bound parameter name");
    701                 if (hasDeclaredParameter(name))
    702                     semanticFail("Cannot destructure to '", name.impl(), "' as it has already been declared");
    703                 semanticFail("Cannot destructure to a parameter named '", name.impl(), "'");
    704             }
    705         } else {
    706             DeclarationResultMask declarationResult = declareParameter(&name);
    707             if ((declarationResult & DeclarationResult::InvalidStrictMode) && strictMode()) {
    708                 semanticFailIfTrue(isEvalOrArguments(&name), "Cannot destructure to a parameter name '", name.impl(), "' in strict mode");
    709                 if (m_lastFunctionName && name == *m_lastFunctionName)
    710                     semanticFail("Cannot declare a parameter named '", name.impl(), "' as it shadows the name of a strict mode function");
    711                 semanticFailureDueToKeyword("parameter name");
    712                 if (hasDeclaredParameter(name))
    713                     semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode as it has already been declared");
    714                 semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode");
    715             }
    716             if (declarationResult & DeclarationResult::InvalidDuplicateDeclaration) {
    717                 // It's not always an error to define a duplicate parameter.
    718                 // It's only an error when there are default parameter values or destructuring parameters.
    719                 // We note this value now so we can check it later.
    720                 if (duplicateIdentifier)
    721                     *duplicateIdentifier = &name;
    722             }
     688        DeclarationResultMask declarationResult = declareParameter(&name);
     689        if ((declarationResult & DeclarationResult::InvalidStrictMode) && strictMode()) {
     690            semanticFailIfTrue(isEvalOrArguments(&name), "Cannot destructure to a parameter name '", name.impl(), "' in strict mode");
     691            if (m_lastFunctionName && name == *m_lastFunctionName)
     692                semanticFail("Cannot declare a parameter named '", name.impl(), "' as it shadows the name of a strict mode function");
     693            semanticFailureDueToKeyword("parameter name");
     694            if (hasDeclaredParameter(name))
     695                semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode as it has already been declared");
     696            semanticFail("Cannot declare a parameter named '", name.impl(), "' in strict mode");
     697        }
     698        if (declarationResult & DeclarationResult::InvalidDuplicateDeclaration) {
     699            // It's not always an error to define a duplicate parameter.
     700            // It's only an error when there are default parameter values or destructuring parameters.
     701            // We note this value now so we can check it later.
     702            if (duplicateIdentifier)
     703                *duplicateIdentifier = &name;
    723704        }
    724705    }
    … …  
    880861                    innerPattern = parseBindingOrAssignmentElement(context, kind, exportType, duplicateIdentifier, hasDestructuringPattern, bindingContext, depth + 1);
    881862                else
    882                     innerPattern = createBindingPattern(context, kind, exportType, *propertyName, depth + 1, identifierToken, bindingContext, duplicateIdentifier);
     863                    innerPattern = createBindingPattern(context, kind, exportType, *propertyName, identifierToken, bindingContext, duplicateIdentifier);
    883864            } else {
    884865                JSTokenType tokenType = m_token.m_type;
    … …  
    936917        }
    937918        failIfTrue(match(LET) && (kind == DestructureToLet || kind == DestructureToConst), "Can't use 'let' as an identifier name for a LexicalDeclaration");
    938         pattern = createBindingPattern(context, kind, exportType, *m_token.m_data.ident, depth, m_token, bindingContext, duplicateIdentifier);
     919        pattern = createBindingPattern(context, kind, exportType, *m_token.m_data.ident, m_token, bindingContext, duplicateIdentifier);
    939920        next();
    940921        break;

  • trunk/Source/JavaScriptCore/parser/Parser.h

    r192436 r192586  
    425425    }
    426426   
    427     enum BindingResult {
    428         BindingFailed,
    429         StrictBindingFailed,
    430         BindingSucceeded
    431     };
    432     BindingResult declareBoundParameter(const Identifier* ident)
    433     {
    434         bool isArgumentsIdent = isArguments(m_vm, ident);
    435         auto addResult = m_declaredVariables.add(ident->impl());
    436         addResult.iterator->value.setIsVar(); // Treat destructuring parameters as "var"s.
    437         bool isValidStrictMode = addResult.isNewEntry && !isEval(m_vm, ident) && !isArgumentsIdent;
    438         m_isValidStrictMode = m_isValidStrictMode && isValidStrictMode;
    439    
    440         if (isArgumentsIdent)
    441             m_shadowsArguments = true;
    442         if (!addResult.isNewEntry)
    443             return BindingFailed;
    444         return isValidStrictMode ? BindingSucceeded : StrictBindingFailed;
    445     }
    446 
    447427    void getUsedVariables(IdentifierSet& usedVariables)
    448428    {
    … …  
    10551035    bool isValidStrictMode() { return currentScope()->isValidStrictMode(); }
    10561036    DeclarationResultMask declareParameter(const Identifier* ident) { return currentScope()->declareParameter(ident); }
    1057     Scope::BindingResult declareBoundParameter(const Identifier* ident) { return currentScope()->declareBoundParameter(ident); }
    10581037    bool breakIsValid()
    10591038    {
    … …  
    11601139    template <class TreeBuilder> TreeSourceElements parseArrowFunctionSingleExpressionBodySourceElements(TreeBuilder&);
    11611140    template <class TreeBuilder> TreeExpression parseArrowFunctionExpression(TreeBuilder&);
    1162     template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern createBindingPattern(TreeBuilder&, DestructuringKind, ExportType, const Identifier&, int depth, JSToken, AssignmentContext, const Identifier** duplicateIdentifier);
     1141    template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern createBindingPattern(TreeBuilder&, DestructuringKind, ExportType, const Identifier&, JSToken, AssignmentContext, const Identifier** duplicateIdentifier);
    11631142    template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern createAssignmentElement(TreeBuilder&, TreeExpression&, const JSTextPosition&, const JSTextPosition&);
    11641143    template <class TreeBuilder> NEVER_INLINE TreeDestructuringPattern parseBindingOrAssignmentElement(TreeBuilder& context, DestructuringKind, ExportType, const Identifier** duplicateIdentifier, bool* hasDestructuringPattern, AssignmentContext bindingContext, int depth);

  • trunk/Source/JavaScriptCore/tests/stress/es6-default-parameters.js

    r187351 r192586  
    236236
    237237
     238// Test proper variable binding.
     239;(function() {
     240    function foo(a = function() { return b; }, {b}) {
     241        assert(a() === 34);
     242        assert(b === 34);
     243        b = 50;
     244        assert(a() === 50);
     245        assert(b === 50);
     246    }
     247    function bar(a = function(x) { b = x; }, {b}) {
     248        assert(b === 34);
     249        a(50);
     250        assert(b === 50);
     251    }
     252    function baz(f1 = function(x) { b = x; }, f2 = function() { return b; }, {b}) {
     253        var b;
     254        assert(b === 34);
     255        assert(f2() === 34);
     256        f1(50);
     257        assert(b === 34);
     258        assert(f2() === 50);
     259    }
     260    noInline(foo);
     261    noInline(bar);
     262    noInline(baz);
     263    for (let i = 0; i < 1000; i++) {
     264        foo(undefined, {b: 34});
     265        bar(undefined, {b: 34});
     266        baz(undefined, undefined, {b: 34});
     267    }
     268})();
     269
    238270
    239271// Syntax errors.
Note: See TracChangeset for help on using the changeset viewer.