Optimize access to lexically scoped variables

If we access a lexically scoped variable after the initializer, then we
know it's either initialized or at least undefined, so we don't need to
do the TDZ check anymore.

The ES tests ensure that we don't optimize too much and the newly
revived tst_v4misc test ensures that we do not generate the TDZ check
instruction for certain scenarios.

Change-Id: I6706d1feb22217f323124ee698ebadb70324693b
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

7 files changed, 103 insertions, 199 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 5d16494c1b..f39f941cc6 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -523,7 +523,7 @@ void Codegen::variableDeclarationList(VariableDeclarationList *ast)
 Codegen::Reference Codegen::targetForPatternElement(AST::PatternElement *p)
 {
     if (!p->bindingIdentifier.isNull())
-        return referenceForName(p->bindingIdentifier.toString(), true);
+        return referenceForName(p->bindingIdentifier.toString(), true, p->firstSourceLocation());
     if (!p->bindingTarget || p->destructuringPattern())
         return Codegen::Reference::fromStackSlot(this);
     Reference lhs = expression(p->bindingTarget);
@@ -2242,9 +2242,9 @@ bool Codegen::visit(FunctionExpression *ast)
     return false;
 }
 
-Codegen::Reference Codegen::referenceForName(const QString &name, bool isLhs)
+Codegen::Reference Codegen::referenceForName(const QString &name, bool isLhs, const SourceLocation &accessLocation)
 {
-    Context::ResolvedName resolved = _context->resolveName(name);
+    Context::ResolvedName resolved = _context->resolveName(name, accessLocation);
 
     if (resolved.type == Context::ResolvedName::Local || resolved.type == Context::ResolvedName::Stack
         || resolved.type == Context::ResolvedName::Import) {
@@ -2302,7 +2302,7 @@ bool Codegen::visit(IdentifierExpression *ast)
     if (hasError)
         return false;
 
-    _expr.setResult(referenceForName(ast->name.toString(), false));
+    _expr.setResult(referenceForName(ast->name.toString(), false, ast->firstSourceLocation()));
     return false;
 }
 
@@ -3111,7 +3111,7 @@ bool Codegen::visit(ForEachStatement *ast)
     Reference lhsValue = Reference::fromStackSlot(this);
 
     // 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.
+    // This block should define a temporal dead zone for those variables.
     {
         RegisterScope innerScope(this);
         ControlFlowBlock controlFlow(this, ast);
diff --git a/src/qml/compiler/qv4codegen_p.h b/src/qml/compiler/qv4codegen_p.h
index b7992bd8f2..d780df394b 100644
--- a/src/qml/compiler/qv4codegen_p.h
+++ b/src/qml/compiler/qv4codegen_p.h
@@ -685,7 +685,7 @@ public:
     void handleTryFinally(AST::TryStatement *ast);
 
 
-    Reference referenceForName(const QString &name, bool lhs);
+    Reference referenceForName(const QString &name, bool lhs, const QQmlJS::AST::SourceLocation &accessLocation = QQmlJS::AST::SourceLocation());
 
     QQmlRefPointer<QV4::CompiledData::CompilationUnit> generateCompilationUnit(bool generateUnitData = true);
     static QQmlRefPointer<QV4::CompiledData::CompilationUnit> createUnitForLoading();
diff --git a/src/qml/compiler/qv4compilercontext.cpp b/src/qml/compiler/qv4compilercontext.cpp
index ac0c2e0ecc..ba9b270601 100644
--- a/src/qml/compiler/qv4compilercontext.cpp
+++ b/src/qml/compiler/qv4compilercontext.cpp
@@ -71,7 +71,22 @@ Context *Module::newContext(Node *node, Context *parent, ContextType contextType
     return c;
 }
 
-bool Context::addLocalVar(const QString &name, Context::MemberType type, VariableScope scope, FunctionExpression *function)
+bool Context::Member::requiresTDZCheck(const SourceLocation &accessLocation, bool accessAcrossContextBoundaries) const
+{
+    if (!isLexicallyScoped())
+        return false;
+
+    if (accessAcrossContextBoundaries)
+        return true;
+
+    if (!accessLocation.isValid() || !endOfInitializerLocation.isValid())
+        return true;
+
+    return accessLocation.begin() < endOfInitializerLocation.end();
+}
+
+bool Context::addLocalVar(const QString &name, Context::MemberType type, VariableScope scope, FunctionExpression *function,
+                          const QQmlJS::AST::SourceLocation &endOfInitializer)
 {
     // ### can this happen?
     if (name.isEmpty())
@@ -96,17 +111,18 @@ bool Context::addLocalVar(const QString &name, Context::MemberType type, Variabl
 
     // hoist var declarations to the function level
     if (contextType == ContextType::Block && (scope == VariableScope::Var && type != MemberType::FunctionDefinition))
-        return parent->addLocalVar(name, type, scope, function);
+        return parent->addLocalVar(name, type, scope, function, endOfInitializer);
 
     Member m;
     m.type = type;
     m.function = function;
     m.scope = scope;
+    m.endOfInitializerLocation = endOfInitializer;
     members.insert(name, m);
     return true;
 }
 
-Context::ResolvedName Context::resolveName(const QString &name)
+Context::ResolvedName Context::resolveName(const QString &name, const QQmlJS::AST::SourceLocation &accessLocation)
 {
     int scope = 0;
     Context *c = this;
@@ -126,7 +142,7 @@ Context::ResolvedName Context::resolveName(const QString &name)
             result.scope = scope;
             result.index = m.index;
             result.isConst = (m.scope == VariableScope::Const);
-            result.requiresTDZCheck = m.isLexicallyScoped();
+            result.requiresTDZCheck = m.requiresTDZCheck(accessLocation, c != this);
             if (c->isStrict && (name == QLatin1String("arguments") || name == QLatin1String("eval")))
                 result.isArgOrEval = true;
             return result;
diff --git a/src/qml/compiler/qv4compilercontext_p.h b/src/qml/compiler/qv4compilercontext_p.h
index e7847e7072..3df0aa6b3a 100644
--- a/src/qml/compiler/qv4compilercontext_p.h
+++ b/src/qml/compiler/qv4compilercontext_p.h
@@ -167,8 +167,10 @@ struct Context {
         QQmlJS::AST::VariableScope scope = QQmlJS::AST::VariableScope::Var;
         mutable bool canEscape = false;
         QQmlJS::AST::FunctionExpression *function = nullptr;
+        QQmlJS::AST::SourceLocation endOfInitializerLocation;
 
         bool isLexicallyScoped() const { return this->scope != QQmlJS::AST::VariableScope::Var; }
+        bool requiresTDZCheck(const QQmlJS::AST::SourceLocation &accessLocation, bool accessAcrossContextBoundaries) const;
     };
     typedef QMap<QString, Member> MemberMap;
 
@@ -206,6 +208,7 @@ struct Context {
     bool isWithBlock = false;
     bool isCatchBlock = false;
     QString caughtVariable;
+    QQmlJS::AST::SourceLocation lastBlockInitializerLocation;
 
     enum UsesArgumentsObject {
         ArgumentsObjectUnknown,
@@ -313,7 +316,8 @@ struct Context {
         usedVariables.insert(name);
     }
 
-    bool addLocalVar(const QString &name, MemberType contextType, QQmlJS::AST::VariableScope scope, QQmlJS::AST::FunctionExpression *function = nullptr);
+    bool addLocalVar(const QString &name, MemberType contextType, QQmlJS::AST::VariableScope scope, QQmlJS::AST::FunctionExpression *function = nullptr,
+                     const QQmlJS::AST::SourceLocation &endOfInitializer = QQmlJS::AST::SourceLocation());
 
     struct ResolvedName {
         enum Type {
@@ -329,9 +333,10 @@ struct Context {
         bool requiresTDZCheck = false;
         int scope = -1;
         int index = -1;
+        QQmlJS::AST::SourceLocation endOfDeclarationLocation;
         bool isValid() const { return type != Unresolved; }
     };
-    ResolvedName resolveName(const QString &name);
+    ResolvedName resolveName(const QString &name, const QQmlJS::AST::SourceLocation &accessLocation);
     void emitBlockHeader(Compiler::Codegen *codegen);
     void emitBlockFooter(Compiler::Codegen *codegen);
 
diff --git a/src/qml/compiler/qv4compilerscanfunctions.cpp b/src/qml/compiler/qv4compilerscanfunctions.cpp
index 06335b3aa0..5d3a7a6d8c 100644
--- a/src/qml/compiler/qv4compilerscanfunctions.cpp
+++ b/src/qml/compiler/qv4compilerscanfunctions.cpp
@@ -312,6 +312,10 @@ bool ScanFunctions::visit(PatternElement *ast)
     QStringList names;
     ast->boundNames(&names);
 
+    QQmlJS::AST::SourceLocation lastInitializerLocation = ast->lastSourceLocation();
+    if (_context->lastBlockInitializerLocation.isValid())
+        lastInitializerLocation = _context->lastBlockInitializerLocation;
+
     for (const QString &name : qAsConst(names)) {
         if (_context->isStrict && (name == QLatin1String("eval") || name == QLatin1String("arguments")))
             _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Variable name may not be eval or arguments in strict mode"));
@@ -322,7 +326,8 @@ bool ScanFunctions::visit(PatternElement *ast)
             _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Missing initializer in const declaration"));
             return false;
         }
-        if (!_context->addLocalVar(name, ast->initializer ? Context::VariableDefinition : Context::VariableDeclaration, ast->scope)) {
+        if (!_context->addLocalVar(name, ast->initializer ? Context::VariableDefinition : Context::VariableDeclaration, ast->scope,
+                                   /*function*/nullptr, lastInitializerLocation)) {
             _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Identifier %1 has already been declared").arg(name));
             return false;
         }
@@ -485,6 +490,8 @@ void ScanFunctions::endVisit(ForStatement *)
 
 bool ScanFunctions::visit(ForEachStatement *ast) {
     enterEnvironment(ast, ContextType::Block, QStringLiteral("%Foreach"));
+    if (ast->expression)
+        _context->lastBlockInitializerLocation = ast->expression->lastSourceLocation();
     Node::accept(ast->lhs, this);
     Node::accept(ast->expression, this);
 
diff --git a/tests/auto/qml/qml.pro b/tests/auto/qml/qml.pro
index 1bd23573b0..4d9e5c23c5 100644
--- a/tests/auto/qml/qml.pro
+++ b/tests/auto/qml/qml.pro
@@ -73,7 +73,8 @@ PRIVATETESTS += \
     qv4mm \
     qv4identifiertable \
     ecmascripttests \
-    bindingdependencyapi
+    bindingdependencyapi \
+    v4misc
 
 qtHaveModule(widgets) {
     PUBLICTESTS += \
diff --git a/tests/auto/qml/v4misc/tst_v4misc.cpp b/tests/auto/qml/v4misc/tst_v4misc.cpp
index 55a1f65608..22ff9c43fc 100644
--- a/tests/auto/qml/v4misc/tst_v4misc.cpp
+++ b/tests/auto/qml/v4misc/tst_v4misc.cpp
@@ -26,207 +26,82 @@
 **
 ****************************************************************************/
 
-#include <qhashfunctions.h>
 #include <qtest.h>
-
-#define V4_AUTOTEST
-#include <private/qv4ssa_p.h>
+#include <private/qv4instr_moth_p.h>
+#include <private/qv4script_p.h>
 
 class tst_v4misc: public QObject
 {
     Q_OBJECT
 
 private slots:
-    void initTestCase();
-
-    void rangeSplitting_1();
-    void rangeSplitting_2();
-    void rangeSplitting_3();
-
-    void moveMapping_1();
-    void moveMapping_2();
+    void tdzOptimizations_data();
+    void tdzOptimizations();
 };
 
-using namespace QT_PREPEND_NAMESPACE(QV4::IR);
-
-void tst_v4misc::initTestCase()
-{
-    qSetGlobalQHashSeed(0);
-    QCOMPARE(qGlobalQHashSeed(), 0);
-}
-
-// split between two ranges
-void tst_v4misc::rangeSplitting_1()
+void tst_v4misc::tdzOptimizations_data()
 {
-    LifeTimeInterval interval;
-    interval.addRange(59, 59);
-    interval.addRange(61, 62);
-    interval.addRange(64, 64);
-    interval.addRange(69, 71);
-    interval.validate();
-    QCOMPARE(interval.end(), 71);
-
-    LifeTimeInterval newInterval = interval.split(66, 70);
-    interval.validate();
-    newInterval.validate();
-    QVERIFY(newInterval.isSplitFromInterval());
-
-    QCOMPARE(interval.ranges().size(), 3);
-    QCOMPARE(interval.ranges()[0].start, 59);
-    QCOMPARE(interval.ranges()[0].end, 59);
-    QCOMPARE(interval.ranges()[1].start, 61);
-    QCOMPARE(interval.ranges()[1].end, 62);
-    QCOMPARE(interval.ranges()[2].start, 64);
-    QCOMPARE(interval.ranges()[2].end, 64);
-    QCOMPARE(interval.end(), 70);
-
-    QCOMPARE(newInterval.ranges().size(), 1);
-    QCOMPARE(newInterval.ranges()[0].start, 70);
-    QCOMPARE(newInterval.ranges()[0].end, 71);
-    QCOMPARE(newInterval.end(), 71);
-}
+    QTest::addColumn<QString>("scriptToCompile");
 
-// split in the middle of a range
-void tst_v4misc::rangeSplitting_2()
-{
-    LifeTimeInterval interval;
-    interval.addRange(59, 59);
-    interval.addRange(61, 64);
-    interval.addRange(69, 71);
-    interval.validate();
-    QCOMPARE(interval.end(), 71);
-
-    LifeTimeInterval newInterval = interval.split(62, 64);
-    interval.validate();
-    newInterval.validate();
-    QVERIFY(newInterval.isSplitFromInterval());
-
-    QCOMPARE(interval.ranges().size(), 2);
-    QCOMPARE(interval.ranges()[0].start, 59);
-    QCOMPARE(interval.ranges()[0].end, 59);
-    QCOMPARE(interval.ranges()[1].start, 61);
-    QCOMPARE(interval.ranges()[1].end, 62);
-    QCOMPARE(interval.end(), 64);
-
-    QCOMPARE(newInterval.ranges().size(), 2);
-    QCOMPARE(newInterval.ranges()[0].start, 64);
-    QCOMPARE(newInterval.ranges()[0].end, 64);
-    QCOMPARE(newInterval.ranges()[1].start, 69);
-    QCOMPARE(newInterval.ranges()[1].end, 71);
-    QCOMPARE(newInterval.end(), 71);
+    QTest::newRow("access-after-let") << QString("let x; x = 10;");
+    QTest::newRow("access-after-const") << QString("const x = 10; print(x);");
+    QTest::newRow("access-after-let") << QString("for (let x of y) print(x);");
 }
 
-// split in the middle of a range, and let it never go back to active again
-void tst_v4misc::rangeSplitting_3()
+void tst_v4misc::tdzOptimizations()
 {
-    LifeTimeInterval interval;
-    interval.addRange(59, 59);
-    interval.addRange(61, 64);
-    interval.addRange(69, 71);
-    interval.validate();
-    QCOMPARE(interval.end(), 71);
-
-    LifeTimeInterval newInterval = interval.split(64, LifeTimeInterval::InvalidPosition);
-    interval.validate();
-    newInterval.validate();
-    QVERIFY(!newInterval.isValid());
-
-    QCOMPARE(interval.ranges().size(), 2);
-    QCOMPARE(interval.ranges()[0].start, 59);
-    QCOMPARE(interval.ranges()[0].end, 59);
-    QCOMPARE(interval.ranges()[1].start, 61);
-    QCOMPARE(interval.ranges()[1].end, 64);
-    QCOMPARE(interval.end(), 71);
-}
+    QFETCH(QString, scriptToCompile);
+
+    QV4::ExecutionEngine v4;
+    QV4::Script script(&v4, nullptr, scriptToCompile);
+    script.parse();
+    QVERIFY(!v4.hasException);
+
+    const auto function = script.compilationUnit->unitData()->functionAt(0);
+    const auto *code = function->code();
+    const auto len = function->codeSize;
+    const char *end = code + len;
+
+    const auto decodeInstruction = [&code]() {
+        QV4::Moth::Instr::Type type = QV4::Moth::Instr::Type(static_cast<uchar>(*code));
+    dispatch:
+        switch (type) {
+            case QV4::Moth::Instr::Type::Nop:
+                ++code;
+                type = QV4::Moth::Instr::Type(static_cast<uchar>(*code));
+                goto dispatch;
+            case QV4::Moth::Instr::Type::Nop_Wide: /* wide prefix */
+                ++code;
+                type = QV4::Moth::Instr::Type(0x100 | static_cast<uchar>(*code));
+                goto dispatch;
+
+#define CASE_AND_GOTO_INSTRUCTION(name, nargs, ...) \
+      case QV4::Moth::Instr::Type::name: \
+            MOTH_ADJUST_CODE(qint8, nargs); \
+            break;
+
+#define CASE_AND_GOTO_WIDE_INSTRUCTION(name, nargs, ...) \
+      case QV4::Moth::Instr::Type::name##_Wide: \
+            MOTH_ADJUST_CODE(int, nargs); \
+            type = QV4::Moth::Instr::Type::name; \
+            break;
+
+#define MOTH_DECODE_WITHOUT_ARGS(instr) \
+     INSTR_##instr(CASE_AND_GOTO) \
+     INSTR_##instr(CASE_AND_GOTO_WIDE)
+
+            FOR_EACH_MOTH_INSTR(MOTH_DECODE_WITHOUT_ARGS)
+        }
+        return type;
+    };
+
+    while (code < end) {
+        QV4::Moth::Instr::Type type = decodeInstruction();
+        QVERIFY(type != QV4::Moth::Instr::Type::DeadTemporalZoneCheck);
+    }
 
-void tst_v4misc::moveMapping_1()
-{
-    Temp fp2(DoubleType, Temp::PhysicalRegister, 2);
-    Temp fp3(DoubleType, Temp::PhysicalRegister, 3);
-    Temp fp4(DoubleType, Temp::PhysicalRegister, 4);
-    Temp fp5(DoubleType, Temp::PhysicalRegister, 5);
-
-    MoveMapping mapping;
-    mapping.add(&fp2, &fp3);
-    mapping.add(&fp2, &fp4);
-    mapping.add(&fp2, &fp5);
-    mapping.add(&fp3, &fp2);
-
-    mapping.order();
-//    mapping.dump();
-
-    QCOMPARE(mapping._moves.size(), 3);
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp2, &fp4, false)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp2, &fp5, false)));
-    QVERIFY(mapping._moves.last() == MoveMapping::Move(&fp2, &fp3, true) ||
-            mapping._moves.last() == MoveMapping::Move(&fp3, &fp2, true));
-}
-
-void tst_v4misc::moveMapping_2()
-{
-    Temp fp1(DoubleType, Temp::PhysicalRegister, 1);
-    Temp fp2(DoubleType, Temp::PhysicalRegister, 2);
-    Temp fp3(DoubleType, Temp::PhysicalRegister, 3);
-    Temp fp4(DoubleType, Temp::PhysicalRegister, 4);
-    Temp fp5(DoubleType, Temp::PhysicalRegister, 5);
-    Temp fp6(DoubleType, Temp::PhysicalRegister, 6);
-    Temp fp7(DoubleType, Temp::PhysicalRegister, 7);
-    Temp fp8(DoubleType, Temp::PhysicalRegister, 8);
-    Temp fp9(DoubleType, Temp::PhysicalRegister, 9);
-    Temp fp10(DoubleType, Temp::PhysicalRegister, 10);
-    Temp fp11(DoubleType, Temp::PhysicalRegister, 11);
-    Temp fp12(DoubleType, Temp::PhysicalRegister, 12);
-    Temp fp13(DoubleType, Temp::PhysicalRegister, 13);
-
-    MoveMapping mapping;
-    mapping.add(&fp2, &fp1);
-    mapping.add(&fp2, &fp3);
-    mapping.add(&fp3, &fp2);
-    mapping.add(&fp3, &fp4);
-
-    mapping.add(&fp9, &fp8);
-    mapping.add(&fp8, &fp7);
-    mapping.add(&fp7, &fp6);
-    mapping.add(&fp7, &fp5);
-
-    mapping.add(&fp10, &fp11);
-    mapping.add(&fp11, &fp12);
-    mapping.add(&fp12, &fp13);
-    mapping.add(&fp13, &fp10);
-
-    mapping.order();
-//    mapping.dump();
-
-    QCOMPARE(mapping._moves.size(), 10);
-
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp2, &fp1, false)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp3, &fp4, false)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp7, &fp6, false)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp7, &fp5, false)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp8, &fp7, false)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp9, &fp8, false)));
-
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp2, &fp3, true)) ||
-            mapping._moves.contains(MoveMapping::Move(&fp3, &fp2, true)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp10, &fp13, true)) ||
-            mapping._moves.contains(MoveMapping::Move(&fp13, &fp10, true)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp12, &fp13, true)) ||
-            mapping._moves.contains(MoveMapping::Move(&fp13, &fp12, true)));
-    QVERIFY(mapping._moves.contains(MoveMapping::Move(&fp12, &fp11, true)) ||
-            mapping._moves.contains(MoveMapping::Move(&fp11, &fp12, true)));
-
-    QVERIFY(!mapping._moves.at(0).needsSwap);
-    QVERIFY(!mapping._moves.at(1).needsSwap);
-    QVERIFY(!mapping._moves.at(2).needsSwap);
-    QVERIFY(!mapping._moves.at(3).needsSwap);
-    QVERIFY(!mapping._moves.at(4).needsSwap);
-    QVERIFY(!mapping._moves.at(5).needsSwap);
-    QVERIFY(mapping._moves.at(6).needsSwap);
-    QVERIFY(mapping._moves.at(7).needsSwap);
-    QVERIFY(mapping._moves.at(8).needsSwap);
-    QVERIFY(mapping._moves.at(9).needsSwap);
 }
 
-QTEST_MAIN(tst_v4misc)
+QTEST_MAIN(tst_v4misc);
 
 #include "tst_v4misc.moc"