Use a PatternElement for VariableDeclarations

Required to get proper destructuring working.

Change-Id: I99fc20a9f1bace1fe3981d88ce5466f9c8d98245
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

10 files changed, 67 insertions, 100 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 15faefe66c..0323012686 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -374,17 +374,17 @@ void Codegen::statementList(StatementList *ast)
     requiresReturnValue = _requiresReturnValue;
 }
 
-void Codegen::variableDeclaration(VariableDeclaration *ast)
+void Codegen::variableDeclaration(PatternElement *ast)
 {
     RegisterScope scope(this);
 
-    if (!ast->expression)
+    if (!ast->initializer)
         return;
-    Reference rhs = expression(ast->expression);
+    Reference rhs = expression(ast->initializer);
     if (hasError)
         return;
 
-    Reference lhs = referenceForName(ast->name.toString(), true);
+    Reference lhs = referenceForName(ast->bindingIdentifier, true);
     //### if lhs is a temp, this won't generate a temp-to-temp move. Same for when rhs is a const
     rhs.loadInAccumulator();
     lhs.storeConsumeAccumulator();
@@ -627,12 +627,6 @@ bool Codegen::visit(UiQualifiedPragmaId *)
     return false;
 }
 
-bool Codegen::visit(VariableDeclaration *)
-{
-    Q_UNREACHABLE();
-    return false;
-}
-
 bool Codegen::visit(VariableDeclarationList *)
 {
     Q_UNREACHABLE();
@@ -2350,7 +2344,7 @@ int Codegen::defineFunction(const QString &name, AST::Node *ast,
     if (!_context->parent || _context->usesArgumentsObject == Context::ArgumentsObjectUnknown)
         _context->usesArgumentsObject = Context::ArgumentsObjectNotUsed;
     if (_context->usesArgumentsObject == Context::ArgumentsObjectUsed)
-        _context->addLocalVar(QStringLiteral("arguments"), Context::VariableDeclaration, AST::VariableDeclaration::FunctionScope);
+        _context->addLocalVar(QStringLiteral("arguments"), Context::VariableDeclaration, AST::VariableScope::Var);
 
     bool allVarsEscape = _context->hasWith || _context->hasTry || _context->hasDirectEval;
     if (_context->compilationMode == QmlBinding // we don't really need this for bindings, but we do for signal handlers, and we don't know if the code is a signal handler or not.
@@ -2787,7 +2781,7 @@ bool Codegen::visit(LocalForEachStatement *ast)
 
     BytecodeGenerator::Label body = bytecodeGenerator->label();
 
-    Reference it = referenceForName(ast->declaration->name.toString(), true).asLValue();
+    Reference it = referenceForName(ast->declaration->bindingIdentifier, true).asLValue();
 
     nextIterObj.loadInAccumulator();
     it.storeConsumeAccumulator();
diff --git a/src/qml/compiler/qv4codegen_p.h b/src/qml/compiler/qv4codegen_p.h
index df7096f455..ed43620e3a 100644
--- a/src/qml/compiler/qv4codegen_p.h
+++ b/src/qml/compiler/qv4codegen_p.h
@@ -508,7 +508,7 @@ protected:
 
     void program(AST::Program *ast);
     void statementList(AST::StatementList *ast);
-    void variableDeclaration(AST::VariableDeclaration *ast);
+    void variableDeclaration(AST::PatternElement *ast);
     void variableDeclarationList(AST::VariableDeclarationList *ast);
 
     void initializeAndDestructureBindingElement(AST::PatternElement *e, const Reference &baseRef);
@@ -545,7 +545,6 @@ protected:
     bool visit(AST::UiProgram *ast) override;
     bool visit(AST::UiQualifiedId *ast) override;
     bool visit(AST::UiQualifiedPragmaId *ast) override;
-    bool visit(AST::VariableDeclaration *ast) override;
     bool visit(AST::VariableDeclarationList *ast) override;
 
     bool visit(AST::PatternElement *ast) override;
diff --git a/src/qml/compiler/qv4compilercontext_p.h b/src/qml/compiler/qv4compilercontext_p.h
index 9ce0d5fa1e..e53fb26e4a 100644
--- a/src/qml/compiler/qv4compilercontext_p.h
+++ b/src/qml/compiler/qv4compilercontext_p.h
@@ -118,11 +118,11 @@ struct Context {
     struct Member {
         MemberType type = UndefinedMember;
         int index = -1;
-        QQmlJS::AST::VariableDeclaration::VariableScope scope = QQmlJS::AST::VariableDeclaration::FunctionScope;
+        QQmlJS::AST::VariableScope scope = QQmlJS::AST::VariableScope::Var;
         mutable bool canEscape = false;
         QQmlJS::AST::FunctionExpression *function = nullptr;
 
-        bool isLexicallyScoped() const { return this->scope != QQmlJS::AST::VariableDeclaration::FunctionScope; }
+        bool isLexicallyScoped() const { return this->scope != QQmlJS::AST::VariableScope::Var; }
     };
     typedef QMap<QString, Member> MemberMap;
 
@@ -259,18 +259,18 @@ struct Context {
         usedVariables.insert(name);
     }
 
-    bool addLocalVar(const QString &name, MemberType type, QQmlJS::AST::VariableDeclaration::VariableScope scope, QQmlJS::AST::FunctionExpression *function = nullptr)
+    bool addLocalVar(const QString &name, MemberType type, QQmlJS::AST::VariableScope scope, QQmlJS::AST::FunctionExpression *function = nullptr)
     {
         if (name.isEmpty())
             return true;
 
         if (type != FunctionDefinition) {
             if (formals->containsName(name))
-                return (scope == QQmlJS::AST::VariableDeclaration::FunctionScope);
+                return (scope == QQmlJS::AST::VariableScope::Var);
         }
         MemberMap::iterator it = members.find(name);
         if (it != members.end()) {
-            if (scope != QQmlJS::AST::VariableDeclaration::FunctionScope || (*it).scope != QQmlJS::AST::VariableDeclaration::FunctionScope)
+            if (scope != QQmlJS::AST::VariableScope::Var || (*it).scope != QQmlJS::AST::VariableScope::Var)
                 return false;
             if ((*it).type <= type) {
                 (*it).type = type;
diff --git a/src/qml/compiler/qv4compilerscanfunctions.cpp b/src/qml/compiler/qv4compilerscanfunctions.cpp
index 217c077dc0..3f98d7fd67 100644
--- a/src/qml/compiler/qv4compilerscanfunctions.cpp
+++ b/src/qml/compiler/qv4compilerscanfunctions.cpp
@@ -191,20 +191,22 @@ bool ScanFunctions::visit(ArrayPattern *ast)
     return true;
 }
 
-bool ScanFunctions::visit(VariableDeclaration *ast)
+bool ScanFunctions::visit(PatternElement *ast)
 {
-    if (_context->isStrict && (ast->name == QLatin1String("eval") || ast->name == QLatin1String("arguments")))
+    if (!ast->isVariableDeclaration())
+        return true;
+
+    if (_context->isStrict && (ast->bindingIdentifier == QLatin1String("eval") || ast->bindingIdentifier == QLatin1String("arguments")))
         _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Variable name may not be eval or arguments in strict mode"));
-    checkName(ast->name, ast->identifierToken);
-    if (ast->name == QLatin1String("arguments"))
+    checkName(QStringRef(&ast->bindingIdentifier), ast->identifierToken);
+    if (ast->bindingIdentifier == QLatin1String("arguments"))
         _context->usesArgumentsObject = Context::ArgumentsObjectNotUsed;
-    if (ast->scope == AST::VariableDeclaration::VariableScope::ReadOnlyBlockScope && !ast->expression) {
+    if (ast->scope == VariableScope::Const && !ast->initializer) {
         _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Missing initializer in const declaration"));
         return false;
     }
-    QString name = ast->name.toString();
-    if (!_context->addLocalVar(ast->name.toString(), ast->expression ? Context::VariableDefinition : Context::VariableDeclaration, ast->scope)) {
-        _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Identifier %1 has already been declared").arg(name));
+    if (!_context->addLocalVar(ast->bindingIdentifier, ast->initializer ? Context::VariableDefinition : Context::VariableDeclaration, ast->scope)) {
+        _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Identifier %1 has already been declared").arg(ast->bindingIdentifier));
         return false;
     }
     return true;
@@ -396,11 +398,11 @@ bool ScanFunctions::enterFunction(Node *ast, const QString &name, FormalParamete
         outerContext->hasNestedFunctions = true;
         // The identifier of a function expression cannot be referenced from the enclosing environment.
         if (enterName) {
-            if (!outerContext->addLocalVar(name, Context::FunctionDefinition, VariableDeclaration::FunctionScope, expr)) {
+            if (!outerContext->addLocalVar(name, Context::FunctionDefinition, VariableScope::Var, expr)) {
                 _cg->throwSyntaxError(ast->firstSourceLocation(), QStringLiteral("Identifier %1 has already been declared").arg(name));
                 return false;
             }
-            outerContext->addLocalVar(name, Context::FunctionDefinition, VariableDeclaration::FunctionScope, expr);
+            outerContext->addLocalVar(name, Context::FunctionDefinition, VariableScope::Var, expr);
         }
         if (name == QLatin1String("arguments"))
             outerContext->usesArgumentsObject = Context::ArgumentsObjectNotUsed;
@@ -417,7 +419,7 @@ bool ScanFunctions::enterFunction(Node *ast, const QString &name, FormalParamete
 
 
     if (!name.isEmpty() && !formals->containsName(name))
-        _context->addLocalVar(name, Context::ThisFunctionName, QQmlJS::AST::VariableDeclaration::FunctionScope);
+        _context->addLocalVar(name, Context::ThisFunctionName, VariableScope::Var);
     _context->formals = formals;
 
     if (body && !_context->isStrict)
@@ -444,7 +446,7 @@ bool ScanFunctions::enterFunction(Node *ast, const QString &name, FormalParamete
             }
         }
         if (!_context->arguments.contains(arg))
-            _context->addLocalVar(arg, Context::VariableDefinition, QQmlJS::AST::VariableDeclaration::FunctionScope);
+            _context->addLocalVar(arg, Context::VariableDefinition, VariableScope::Var);
     }
     return true;
 }
diff --git a/src/qml/compiler/qv4compilerscanfunctions_p.h b/src/qml/compiler/qv4compilerscanfunctions_p.h
index b479b65e6c..5ec9310b8d 100644
--- a/src/qml/compiler/qv4compilerscanfunctions_p.h
+++ b/src/qml/compiler/qv4compilerscanfunctions_p.h
@@ -105,7 +105,7 @@ protected:
     bool visit(AST::CallExpression *ast) override;
     bool visit(AST::NewMemberExpression *ast) override;
     bool visit(AST::ArrayPattern *ast) override;
-    bool visit(AST::VariableDeclaration *ast) override;
+    bool visit(AST::PatternElement *ast) override;
     bool visit(AST::IdentifierExpression *ast) override;
     bool visit(AST::ExpressionStatement *ast) override;
     bool visit(AST::FunctionExpression *ast) override;
diff --git a/src/qml/parser/qqmljs.g b/src/qml/parser/qqmljs.g
index 607ca8665c..5200779924 100644
--- a/src/qml/parser/qqmljs.g
+++ b/src/qml/parser/qqmljs.g
@@ -261,6 +261,7 @@ public:
     union Value {
       int ival;
       double dval;
+      AST::VariableScope scope;
       AST::ArgumentList *ArgumentList;
       AST::CaseBlock *CaseBlock;
       AST::CaseClause *CaseClause;
@@ -278,7 +279,6 @@ public:
       AST::Statement *Statement;
       AST::StatementList *StatementList;
       AST::Block *Block;
-      AST::VariableDeclaration *VariableDeclaration;
       AST::VariableDeclarationList *VariableDeclarationList;
       AST::Pattern *Pattern;
       AST::PatternElement *PatternElement;
@@ -2737,20 +2737,20 @@ StatementListOpt: StatementList;
 LetOrConst: T_LET;
 /.
     case $rule_number: {
-        sym(1).ival = AST::VariableDeclaration::BlockScope;
+        sym(1).scope = AST::VariableScope::Let;
     } break;
 ./
 LetOrConst: T_CONST;
 /.
     case $rule_number: {
-        sym(1).ival = AST::VariableDeclaration::ReadOnlyBlockScope;
+        sym(1).scope = AST::VariableScope::Const;
     } break;
 ./
 
 Var: T_VAR;
 /.
     case $rule_number: {
-        sym(1).ival = AST::VariableDeclaration::FunctionScope;
+        sym(1).scope = AST::VariableScope::Var;
     } break;
 ./
 
@@ -2763,8 +2763,7 @@ VarDeclaration: Var VariableDeclarationList;
 VarDeclaration_In: Var VariableDeclarationList_In;
 /.
     case $rule_number: {
-        AST::VariableDeclaration::VariableScope s = AST::VariableDeclaration::VariableScope(sym(1).ival);
-        AST::VariableStatement *node = new (pool) AST::VariableStatement(sym(2).VariableDeclarationList->finish(s));
+        AST::VariableStatement *node = new (pool) AST::VariableStatement(sym(2).VariableDeclarationList->finish(sym(1).scope));
         node->declarationKindToken = loc(1);
         sym(1).Node = node;
     } break;
@@ -2782,7 +2781,7 @@ VariableDeclarationList: VariableDeclaration;
 VariableDeclarationList_In: VariableDeclaration_In;
 /.
     case $rule_number: {
-  sym(1).Node = new (pool) AST::VariableDeclarationList(sym(1).VariableDeclaration);
+  sym(1).Node = new (pool) AST::VariableDeclarationList(sym(1).PatternElement);
     } break;
 ./
 
@@ -2795,7 +2794,7 @@ VariableDeclarationList: VariableDeclarationList T_COMMA VariableDeclaration;
 VariableDeclarationList_In: VariableDeclarationList_In T_COMMA VariableDeclaration_In;
 /.
     case $rule_number: {
-        AST::VariableDeclarationList *node = new (pool) AST::VariableDeclarationList(sym(1).VariableDeclarationList, sym(3).VariableDeclaration);
+        AST::VariableDeclarationList *node = new (pool) AST::VariableDeclarationList(sym(1).VariableDeclarationList, sym(3).PatternElement);
         node->commaToken = loc(2);
         sym(1).Node = node;
     } break;
@@ -2810,7 +2809,7 @@ VariableDeclaration: BindingIdentifier InitializerOpt;
 VariableDeclaration_In: BindingIdentifier InitializerOpt_In;
 /.
     case $rule_number: {
-        AST::VariableDeclaration *node = new (pool) AST::VariableDeclaration(stringRef(1), sym(2).Expression);
+        auto *node = new (pool) AST::PatternElement(stringRef(1), sym(2).Expression);
         node->identifierToken = loc(1);
         sym(1).Node = node;
     } break;
@@ -2823,7 +2822,14 @@ LexicalBinding_In: BindingPattern Initializer_In;
 VariableDeclaration: BindingPattern Initializer;
 /.  case $rule_number: Q_FALLTHROUGH(); ./
 VariableDeclaration_In: BindingPattern Initializer_In;
-/.  case $rule_number: { UNIMPLEMENTED; } ./
+/.
+    case $rule_number: {
+    UNIMPLEMENTED;
+        auto *node = new (pool) AST::PatternElement(sym(1).Pattern, sym(2).Expression);
+        node->identifierToken = loc(1);
+        sym(1).Node = node;
+    } break;
+./
 
 BindingPattern:  T_LBRACE ObjectBindingPattern T_RBRACE;
 /.
@@ -3125,7 +3131,7 @@ IterationStatement: T_FOR T_LPAREN LeftHandSideExpression T_IN Expression_In T_R
 IterationStatement: T_FOR T_LPAREN ForDeclaration T_IN Expression_In T_RPAREN Statement;
 /.
     case $rule_number: {
-        AST::LocalForEachStatement *node = new (pool) AST::LocalForEachStatement(sym(3).VariableDeclaration, sym(5).Expression, sym(7).Statement);
+        AST::LocalForEachStatement *node = new (pool) AST::LocalForEachStatement(sym(3).PatternElement, sym(5).Expression, sym(7).Statement);
         node->forToken = loc(1);
         node->lparenToken = loc(2);
         node->varToken = loc(3);
@@ -3146,9 +3152,9 @@ ForDeclaration: LetOrConst ForBinding;
 ForDeclaration: Var ForBinding;
 /.
     case $rule_number: {
-        AST::VariableDeclaration *node = new (pool) AST::VariableDeclaration(stringRef(2), nullptr);
-        node->scope = AST::VariableDeclaration::VariableScope(sym(1).ival);
+        auto *node = new (pool) AST::PatternElement(stringRef(2), nullptr);
         node->identifierToken = loc(2);
+        node->scope = sym(1).scope;
         sym(1).Node = node;
     } break;
 ./
diff --git a/src/qml/parser/qqmljsast.cpp b/src/qml/parser/qqmljsast.cpp
index 3aa6f83934..a9fd2e9278 100644
--- a/src/qml/parser/qqmljsast.cpp
+++ b/src/qml/parser/qqmljsast.cpp
@@ -483,15 +483,6 @@ void VariableDeclarationList::accept0(Visitor *visitor)
     visitor->endVisit(this);
 }
 
-void VariableDeclaration::accept0(Visitor *visitor)
-{
-    if (visitor->visit(this)) {
-        accept(expression, visitor);
-    }
-
-    visitor->endVisit(this);
-}
-
 void EmptyStatement::accept0(Visitor *visitor)
 {
     if (visitor->visit(this)) {
diff --git a/src/qml/parser/qqmljsast_p.h b/src/qml/parser/qqmljsast_p.h
index 5c0ef707d5..cc44c99c5d 100644
--- a/src/qml/parser/qqmljsast_p.h
+++ b/src/qml/parser/qqmljsast_p.h
@@ -112,6 +112,13 @@ namespace QQmlJS {
 
 namespace AST {
 
+enum class VariableScope {
+    NoScope,
+    Var,
+    Let,
+    Const
+};
+
 template <typename T1, typename T2>
 T1 cast(T2 *ast)
 {
@@ -671,6 +678,9 @@ public:
     PatternElementList *elementList() const { ArrayPattern *a = cast<ArrayPattern *>(bindingPattern); return a ? a->elements : nullptr; }
     PatternPropertyList *propertyList() const { ObjectPattern *o = cast<ObjectPattern *>(bindingPattern); return o ? o->properties : nullptr;  }
 
+    bool isVariableDeclaration() const { return scope != VariableScope::NoScope; }
+    bool isLexicallyScoped() const { return scope == VariableScope::Let || scope == VariableScope::Const; }
+
     virtual void boundNames(QStringList *names);
 
 // attributes
@@ -679,6 +689,8 @@ public:
     Pattern *bindingPattern = nullptr;
     ExpressionNode *initializer = nullptr;
     Type type = Literal;
+    // when used in a VariableDeclarationList
+    VariableScope scope = VariableScope::NoScope;
 };
 
 class QML_PARSER_EXPORT PatternElementList : public Node
@@ -1408,50 +1420,17 @@ public:
     StatementList *next;
 };
 
-class QML_PARSER_EXPORT VariableDeclaration: public Node
-{
-public:
-    QQMLJS_DECLARE_AST_NODE(VariableDeclaration)
-
-    enum VariableScope {
-        UnknownScope,
-        FunctionScope,
-        BlockScope, // let
-        ReadOnlyBlockScope // const
-    };
-
-    VariableDeclaration(const QStringRef &n, ExpressionNode *e)
-        : name(n), expression(e)
-    { kind = K; }
-
-    bool isLexicallyScoped() const { return scope != FunctionScope; }
-
-    void accept0(Visitor *visitor) override;
-
-    SourceLocation firstSourceLocation() const override
-    { return identifierToken; }
-
-    SourceLocation lastSourceLocation() const override
-    { return expression ? expression->lastSourceLocation() : identifierToken; }
-
-// attributes
-    QStringRef name;
-    ExpressionNode *expression;
-    SourceLocation identifierToken;
-    VariableScope scope = UnknownScope;
-};
-
 class QML_PARSER_EXPORT VariableDeclarationList: public Node
 {
 public:
     QQMLJS_DECLARE_AST_NODE(VariableDeclarationList)
 
-    VariableDeclarationList(VariableDeclaration *decl):
-        declaration (decl), next (this)
-        { kind = K; }
+    VariableDeclarationList(PatternElement *decl)
+        : declaration(decl), next(this)
+    { kind = K; }
 
-    VariableDeclarationList(VariableDeclarationList *previous, VariableDeclaration *decl):
-        declaration (decl)
+    VariableDeclarationList(VariableDeclarationList *previous, PatternElement *decl)
+        : declaration(decl)
     {
         kind = K;
         next = previous->next;
@@ -1470,7 +1449,7 @@ public:
         return declaration->lastSourceLocation();
     }
 
-    inline VariableDeclarationList *finish(VariableDeclaration::VariableScope s)
+    inline VariableDeclarationList *finish(VariableScope s)
     {
         VariableDeclarationList *front = next;
         next = nullptr;
@@ -1482,7 +1461,7 @@ public:
     }
 
 // attributes
-    VariableDeclaration *declaration;
+    PatternElement *declaration;
     VariableDeclarationList *next;
     SourceLocation commaToken;
 };
@@ -1724,7 +1703,7 @@ class QML_PARSER_EXPORT LocalForEachStatement: public Statement
 public:
     QQMLJS_DECLARE_AST_NODE(LocalForEachStatement)
 
-    LocalForEachStatement(VariableDeclaration *v, ExpressionNode *e, Statement *stmt):
+    LocalForEachStatement(PatternElement *v, ExpressionNode *e, Statement *stmt):
         declaration (v), expression (e), statement (stmt)
         { kind = K; }
 
@@ -1737,7 +1716,7 @@ public:
     { return statement->lastSourceLocation(); }
 
 // attributes
-    VariableDeclaration *declaration;
+    PatternElement *declaration;
     ExpressionNode *expression;
     Statement *statement;
     SourceLocation forToken;
diff --git a/src/qml/parser/qqmljsastfwd_p.h b/src/qml/parser/qqmljsastfwd_p.h
index 590f4480a9..063c43c524 100644
--- a/src/qml/parser/qqmljsastfwd_p.h
+++ b/src/qml/parser/qqmljsastfwd_p.h
@@ -133,7 +133,6 @@ class Block;
 class StatementList;
 class VariableStatement;
 class VariableDeclarationList;
-class VariableDeclaration;
 class EmptyStatement;
 class ExpressionStatement;
 class IfStatement;
diff --git a/src/qml/parser/qqmljsastvisitor_p.h b/src/qml/parser/qqmljsastvisitor_p.h
index 184fcecd1c..b3b54364c4 100644
--- a/src/qml/parser/qqmljsastvisitor_p.h
+++ b/src/qml/parser/qqmljsastvisitor_p.h
@@ -248,9 +248,6 @@ public:
     virtual bool visit(VariableDeclarationList *) { return true; }
     virtual void endVisit(VariableDeclarationList *) {}
 
-    virtual bool visit(VariableDeclaration *) { return true; }
-    virtual void endVisit(VariableDeclaration *) {}
-
     virtual bool visit(EmptyStatement *) { return true; }
     virtual void endVisit(EmptyStatement *) {}