Improve for-in and for-of support

Create a Block scope per iteration as defined in the ES spec. So
closures created inside the loop will remember the iteration variable
at that loop iteration.

Add support for destructuring of the left hand side expression or
declaration.

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

9 files changed, 131 insertions, 84 deletions

diff --git a/src/qml/compiler/qv4bytecodegenerator_p.h b/src/qml/compiler/qv4bytecodegenerator_p.h
index 286ceed57d..0033acf3c7 100644
--- a/src/qml/compiler/qv4bytecodegenerator_p.h
+++ b/src/qml/compiler/qv4bytecodegenerator_p.h
@@ -241,6 +241,7 @@ public:
     int newRegister();
     int newRegisterArray(int n);
     int registerCount() const { return regCount; }
+    int currentRegister() const { return currentReg; }
 
     void finalize(Compiler::Context *context);
 
diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 31c0764570..a3a7f24bdf 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -141,6 +141,12 @@ int Codegen::leaveContext()
     return functionIndex;
 }
 
+Context *Codegen::enterBlock(Node *node)
+{
+    enterContext(node);
+    return _context;
+}
+
 Codegen::Reference Codegen::unop(UnaryOperation op, const Reference &expr)
 {
     if (hasError)
@@ -388,7 +394,7 @@ void Codegen::variableDeclarationList(VariableDeclarationList *ast)
     }
 }
 
-void Codegen::initializeAndDestructureBindingElement(AST::PatternElement *e, const Codegen::Reference &baseRef)
+void Codegen::initializeAndDestructureBindingElement(AST::PatternElement *e, const Reference &baseRef)
 {
     RegisterScope scope(this);
     Reference varToStore = e->bindingIdentifier.isNull() ? Reference::fromStackSlot(this, bytecodeGenerator->newRegister()) : referenceForName(e->bindingIdentifier, true);
@@ -494,6 +500,17 @@ void Codegen::destructureElementList(const Codegen::Reference &array, PatternEle
     }
 }
 
+void Codegen::destructurePattern(Pattern *p, const Reference &rhs)
+{
+    RegisterScope scope(this);
+    if (auto *o = AST::cast<ObjectPattern *>(p))
+        destructurePropertyList(rhs, o->properties);
+    else if (auto *a = AST::cast<ArrayPattern *>(p))
+        destructureElementList(rhs, a->elements);
+    else
+        Q_UNREACHABLE();
+}
+
 
 bool Codegen::visit(ArgumentList *)
 {
@@ -837,12 +854,7 @@ bool Codegen::visit(BinaryExpression *ast)
         if (AST::Pattern *p = ast->left->patternCast()) {
             RegisterScope scope(this);
             Reference right = expression(ast->right).storeOnStack();
-            if (auto *o = AST::cast<ObjectPattern *>(p))
-                destructurePropertyList(right, o->properties);
-            else if (auto *a = AST::cast<ArrayPattern *>(p))
-                destructureElementList(right, a->elements);
-            else
-                Q_UNREACHABLE();
+            destructurePattern(p, right);
             if (!_expr.accept(nx)) {
                 right.loadInAccumulator();
                 _expr.setResult(Reference::fromAccumulator(this));
@@ -2469,14 +2481,8 @@ bool Codegen::visit(Block *ast)
 
     RegisterScope scope(this);
 
-    enterContext(ast);
-    _module->blocks.append(_context);
-    _context->blockIndex = _module->blocks.count() - 1;
-    {
-        ControlFlowBlock controlFlow(this, _context);
-        statementList(ast->statements);
-    }
-    leaveContext();
+    ControlFlowBlock controlFlow(this, ast);
+    statementList(ast->statements);
     return false;
 }
 
@@ -2600,44 +2606,24 @@ bool Codegen::visit(ForEachStatement *ast)
     RegisterScope scope(this);
 
     Reference iterator = Reference::fromStackSlot(this);
-    Reference expr = expression(ast->expression);
-    if (hasError)
-        return true;
+    Reference lhsValue = Reference::fromStackSlot(this);
 
-    expr.loadInAccumulator();
-    Instruction::GetIterator iteratorObjInstr;
-    iteratorObjInstr.iterator = (ast->type == ForEachType::Of) ? 1 : 0;
-    bytecodeGenerator->addInstruction(iteratorObjInstr);
-    iterator.storeConsumeAccumulator();
-
-    Reference lhs;
-    if (ExpressionNode *e = ast->lhs->expressionCast()) {
-        lhs = expression(e);
-    } else if (PatternElement *p = AST::cast<PatternElement *>(ast->lhs)) {
-        variableDeclaration(p);
-        lhs = referenceForName(p->bindingIdentifier, true).asLValue();
-    } else {
-        Q_UNREACHABLE();
-    }
+    // There should be a temporal block, so that variables declared in lhs shadow outside vars.
+    // This block should define a temporal dead zone for those variables, which is not yet implemented.
+    {
+        RegisterScope innerScope(this);
+        ControlFlowBlock controlFlow(this, ast);
+        Reference expr = expression(ast->expression);
+        if (hasError)
+            return true;
 
-    if (hasError)
-        return false;
-    if (!lhs.isLValue()) {
-        throwSyntaxError(ast->forToken, QLatin1String("Destructuring not supported with for-in and for-of loops."));
-        return false;
+        expr.loadInAccumulator();
+        Instruction::GetIterator iteratorObjInstr;
+        iteratorObjInstr.iterator = (ast->type == ForEachType::Of) ? 1 : 0;
+        bytecodeGenerator->addInstruction(iteratorObjInstr);
+        iterator.storeConsumeAccumulator();
     }
 
-    lhs = lhs.asLValue();
-
-    foreachBody(lhs, ast->statement, ast->forToken, iterator);
-    return false;
-}
-
-void Codegen::foreachBody(const Reference &lhs, Statement *statements, const SourceLocation &forToken, const Reference &iterator)
-{
-    RegisterScope scope(this);
-    Reference nextIterObj = Reference::fromStackSlot(this);
-
     BytecodeGenerator::Label in = bytecodeGenerator->newLabel();
     BytecodeGenerator::Label end = bytecodeGenerator->newLabel();
 
@@ -2647,25 +2633,52 @@ void Codegen::foreachBody(const Reference &lhs, Statement *statements, const Sou
 
     BytecodeGenerator::Label body = bytecodeGenerator->label();
 
-    Reference::fromMember(nextIterObj, QStringLiteral("value")).loadInAccumulator();
-    lhs.storeConsumeAccumulator();
+    // each iteration gets it's own context, as per spec
+    {
+        RegisterScope innerScope(this);
+        ControlFlowBlock controlFlow(this, ast);
+
+        if (ExpressionNode *e = ast->lhs->expressionCast()) {
+            if (AST::Pattern *p = e->patternCast()) {
+                RegisterScope scope(this);
+                Reference right = Reference::fromMember(lhsValue, QStringLiteral("value")).storeOnStack();
+                destructurePattern(p, right);
+            } else {
+                Reference lhs = expression(e);
+                if (hasError)
+                    goto error;
+                lhs = lhs.asLValue();
+                Reference::fromMember(lhsValue, QStringLiteral("value")).loadInAccumulator();
+                lhs.storeConsumeAccumulator();
+            }
+        } else if (PatternElement *p = AST::cast<PatternElement *>(ast->lhs)) {
+            Reference::fromMember(lhsValue, QStringLiteral("value")).loadInAccumulator();
+            initializeAndDestructureBindingElement(p, Reference::fromAccumulator(this));
+            if (hasError)
+                goto error;
+        } else {
+            Q_UNREACHABLE();
+        }
+
+        statement(ast->statement);
+        setJumpOutLocation(bytecodeGenerator, ast->statement, ast->forToken);
 
-    statement(statements);
-    setJumpOutLocation(bytecodeGenerator, statements, forToken);
+    }
 
+  error:
     in.link();
-
     Reference next = Reference::fromMember(iterator, QStringLiteral("next"));
     next.loadInAccumulator();
     next = next.asLValue();
     Codegen::Arguments args{0, 0};
     handleCall(next, args);
-    nextIterObj.storeConsumeAccumulator();
-    Reference done = Reference::fromMember(nextIterObj, QStringLiteral("done"));
+    lhsValue.storeConsumeAccumulator();
+    Reference done = Reference::fromMember(lhsValue, QStringLiteral("done"));
     done.loadInAccumulator();
     bytecodeGenerator->jumpFalse().link(body);
 
     end.link();
+    return false;
 }
 
 bool Codegen::visit(ForStatement *ast)
diff --git a/src/qml/compiler/qv4codegen_p.h b/src/qml/compiler/qv4codegen_p.h
index 18ccf04dc8..49752270a0 100644
--- a/src/qml/compiler/qv4codegen_p.h
+++ b/src/qml/compiler/qv4codegen_p.h
@@ -466,7 +466,10 @@ protected:
 
     void enterContext(AST::Node *node);
     int leaveContext();
-
+public:
+    Context *enterBlock(AST::Node *node);
+    int leaveBlock() { return leaveContext(); }
+protected:
     void leaveLoop();
 
     enum UnaryOperation {
@@ -513,9 +516,10 @@ protected:
     void variableDeclaration(AST::PatternElement *ast);
     void variableDeclarationList(AST::VariableDeclarationList *ast);
 
-    void initializeAndDestructureBindingElement(AST::PatternElement *e, const Reference &baseRef);
+    void initializeAndDestructureBindingElement(AST::PatternElement *e, const Reference &baseRef = Reference());
     void destructurePropertyList(const Reference &object, AST::PatternPropertyList *bindingList);
     void destructureElementList(const Reference &array, AST::PatternElementList *bindingList);
+    void destructurePattern(AST::Pattern *p, const Reference &rhs);
 
     // Hook provided to implement QML lookup semantics
     virtual Reference fallbackNameLookup(const QString &name);
@@ -644,8 +648,6 @@ public:
     void handleTryCatch(AST::TryStatement *ast);
     void handleTryFinally(AST::TryStatement *ast);
 
-    void foreachBody(const Reference &lhs, AST::Statement *body, const AST::SourceLocation &forToken, const Reference &nextIterObj);
-
 
     Reference referenceForName(const QString &name, bool lhs);
 
@@ -657,6 +659,8 @@ public:
 
     void loadClosure(int index);
 
+    Module *module() const { return _module; }
+
 protected:
     friend class ScanFunctions;
     friend struct ControlFlow;
diff --git a/src/qml/compiler/qv4compilercontext.cpp b/src/qml/compiler/qv4compilercontext.cpp
index 07e19811a8..bfe0d4b131 100644
--- a/src/qml/compiler/qv4compilercontext.cpp
+++ b/src/qml/compiler/qv4compilercontext.cpp
@@ -167,6 +167,11 @@ int Context::emitBlockHeader(Codegen *codegen)
     setupFunctionIndices(bytecodeGenerator);
     int contextReg = -1;
 
+    if (requiresExecutionContext && blockIndex < 0) {
+        codegen->module()->blocks.append(this);
+        blockIndex = codegen->module()->blocks.count() - 1;
+    }
+
     if (requiresExecutionContext && contextType == ContextType::Global) {
         Instruction::PushScriptContext scriptContext;
         scriptContext.index = blockIndex;
@@ -268,9 +273,15 @@ QT_WARNING_POP
 
 void Context::setupFunctionIndices(Moth::BytecodeGenerator *bytecodeGenerator)
 {
+    if (registerOffset != -1) {
+        // already computed, check for consistency
+        Q_ASSERT(registerOffset == bytecodeGenerator->currentRegister());
+        bytecodeGenerator->newRegisterArray(nRegisters);
+        return;
+    }
     Q_ASSERT(locals.size() == 0);
     Q_ASSERT(nRegisters == 0);
-    registerOffset = bytecodeGenerator->registerCount();
+    registerOffset = bytecodeGenerator->currentRegister();
 
     switch (contextType) {
     case ContextType::Block:
@@ -304,7 +315,7 @@ void Context::setupFunctionIndices(Moth::BytecodeGenerator *bytecodeGenerator)
         }
         break;
     }
-    nRegisters = bytecodeGenerator->registerCount() - registerOffset;
+    nRegisters = bytecodeGenerator->currentRegister() - registerOffset;
 }
 
 QT_END_NAMESPACE
diff --git a/src/qml/compiler/qv4compilercontrolflow_p.h b/src/qml/compiler/qv4compilercontrolflow_p.h
index db83af0022..89791330f9 100644
--- a/src/qml/compiler/qv4compilercontrolflow_p.h
+++ b/src/qml/compiler/qv4compilercontrolflow_p.h
@@ -331,10 +331,10 @@ struct ControlFlowWith : public ControlFlowUnwind
 
 struct ControlFlowBlock : public ControlFlowUnwind
 {
-    ControlFlowBlock(Codegen *cg, Context *block)
-        : ControlFlowUnwind(cg, Block),
-          block(block)
+    ControlFlowBlock(Codegen *cg, AST::Node *ast)
+        : ControlFlowUnwind(cg, Block)
     {
+        block = cg->enterBlock(ast);
         savedContextRegister = block->emitBlockHeader(cg);
 
         if (savedContextRegister != -1) {
@@ -354,6 +354,7 @@ struct ControlFlowBlock : public ControlFlowUnwind
 
         if (savedContextRegister != -1)
             emitUnwindHandler();
+        cg->leaveBlock();
     }
     virtual Handler getHandler(HandlerType type, const QString &label = QString()) {
         if (savedContextRegister == -1)
@@ -411,10 +412,7 @@ struct ControlFlowCatch : public ControlFlowUnwind
         // exceptions inside the try block go here
         exceptionLabel.link();
 
-        cg->enterContext(catchExpression);
-        Context *block = cg->currentContext();
-        cg->_module->blocks.append(block);
-        block->blockIndex = cg->_module->blocks.count() - 1;
+        Context *block = cg->enterBlock(catchExpression);
 
         int savedContextReg = block->emitBlockHeader(cg);
 
@@ -434,7 +432,7 @@ struct ControlFlowCatch : public ControlFlowUnwind
         catchUnwindLabel.link();
         block->emitBlockFooter(cg, savedContextReg);
 
-        cg->leaveContext();
+        cg->leaveBlock();
 
         // break/continue/return statements in try go here
         unwindLabel.link();
diff --git a/src/qml/compiler/qv4compilerscanfunctions.cpp b/src/qml/compiler/qv4compilerscanfunctions.cpp
index eadbc3ae17..ccf7e49e25 100644
--- a/src/qml/compiler/qv4compilerscanfunctions.cpp
+++ b/src/qml/compiler/qv4compilerscanfunctions.cpp
@@ -342,6 +342,8 @@ bool ScanFunctions::visit(LocalForStatement *ast) {
 }
 
 bool ScanFunctions::visit(ForEachStatement *ast) {
+    enterEnvironment(ast, ContextType::Block);
+    _context->name = QLatin1String("Foreach");
     Node::accept(ast->lhs, this);
     Node::accept(ast->expression, this);
 
@@ -351,6 +353,11 @@ bool ScanFunctions::visit(ForEachStatement *ast) {
     return false;
 }
 
+void ScanFunctions::endVisit(ForEachStatement *)
+{
+    leaveEnvironment();
+}
+
 bool ScanFunctions::visit(ThisExpression *)
 {
     _context->usesThis = true;
diff --git a/src/qml/compiler/qv4compilerscanfunctions_p.h b/src/qml/compiler/qv4compilerscanfunctions_p.h
index 7d29af4fa3..640ee86c0b 100644
--- a/src/qml/compiler/qv4compilerscanfunctions_p.h
+++ b/src/qml/compiler/qv4compilerscanfunctions_p.h
@@ -127,6 +127,8 @@ protected:
     bool visit(AST::ForStatement *ast) override;
     bool visit(AST::LocalForStatement *ast) override;
     bool visit(AST::ForEachStatement *ast) override;
+    void endVisit(AST::ForEachStatement *) override;
+
     bool visit(AST::ThisExpression *ast) override;
 
     bool visit(AST::Block *ast) override;
diff --git a/src/qml/parser/qqmljs.g b/src/qml/parser/qqmljs.g
index 5efc392efa..4b1dc437f4 100644
--- a/src/qml/parser/qqmljs.g
+++ b/src/qml/parser/qqmljs.g
@@ -3144,14 +3144,14 @@ IterationStatement: T_FOR T_LPAREN LeftHandSideExpression InOrOf Expression_In T
 /.
     case $rule_number: {
         // need to convert the LHS to an AssignmentPattern if it was an Array/ObjectLiteral
-//        if (AST::Pattern *p = sym(3).Expression->patternCast()) {
-//            AST::SourceLocation errorLoc;
-//            QString errorMsg;
-//            if (!p->convertLiteralToAssignmentPattern(pool, &errorLoc, &errorMsg)) {
-//                syntaxError(errorLoc, errorMsg);
-//                return false;
-//            }
-//        }
+        if (AST::Pattern *p = sym(3).Expression->patternCast()) {
+            AST::SourceLocation errorLoc;
+            QString errorMsg;
+            if (!p->convertLiteralToAssignmentPattern(pool, &errorLoc, &errorMsg)) {
+                syntaxError(errorLoc, errorMsg);
+                return false;
+            }
+        }
         AST::ForEachStatement *node = new (pool) AST::ForEachStatement(sym(3).Expression, sym(5).Expression, sym(7).Statement);
         node->forToken = loc(1);
         node->lparenToken = loc(2);
@@ -3175,9 +3175,9 @@ IterationStatement: T_FOR T_LPAREN ForDeclaration InOrOf Expression_In T_RPAREN
     } break;
 ./
 
-ForDeclaration: LetOrConst ForBinding;
+ForDeclaration: LetOrConst BindingIdentifier;
 /.  case $rule_number: Q_FALLTHROUGH(); ./
-ForDeclaration: Var ForBinding;
+ForDeclaration: Var BindingIdentifier;
 /.
     case $rule_number: {
         auto *node = new (pool) AST::PatternElement(stringRef(2), nullptr);
@@ -3187,10 +3187,17 @@ ForDeclaration: Var ForBinding;
     } break;
 ./
 
-ForBinding: BindingIdentifier;
-
-ForBinding: BindingPattern;
-/.  case $rule_number: UNIMPLEMENTED; ./
+ForDeclaration: LetOrConst BindingPattern;
+/.  case $rule_number: Q_FALLTHROUGH(); ./
+ForDeclaration: Var BindingPattern;
+/.
+    case $rule_number: {
+        auto *node = new (pool) AST::PatternElement(sym(2).Pattern, nullptr);
+        node->identifierToken = loc(2);
+        node->scope = sym(1).scope;
+        sym(1).Node = node;
+    } break;
+./
 
 ContinueStatement: T_CONTINUE T_AUTOMATIC_SEMICOLON;
 ContinueStatement: T_CONTINUE T_SEMICOLON;
diff --git a/src/qml/parser/qqmljsast_p.h b/src/qml/parser/qqmljsast_p.h
index 83fcb6559c..9ed7ba79e5 100644
--- a/src/qml/parser/qqmljsast_p.h
+++ b/src/qml/parser/qqmljsast_p.h
@@ -1725,6 +1725,10 @@ public:
     SourceLocation lastSourceLocation() const override
     { return statement->lastSourceLocation(); }
 
+    PatternElement *declaration() const {
+        return AST::cast<PatternElement *>(lhs);
+    }
+
 // attributes
     Node *lhs;
     ExpressionNode *expression;