Fix module dependency handling

The evaluation of a module can have side-effects by modifying the global
object or objects in it. Therefore even a seemingly empty import such as

    import "./foo.js"

needs to be listed in the module requests. It's also important that they
are evaluated in the order of declaration. Therefore we collect all
module requests separately - even those that don't have import variables
to process. This patch also ensures that the export and import
declarations are visited in the correct order, by unifying both AST
nodes to be hooked into the statement list.

The fact that we connect the module list items into a statement list is
solely an artifact of re-using defineFunction() which takes a
StatementList as body.

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

10 files changed, 54 insertions, 47 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index fc43031c5c..29af643081 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -160,6 +160,9 @@ void Codegen::generateFromModule(const QString &fileName,
             }
         }
         _module->importEntries = moduleContext->importEntries;
+
+        _module->moduleRequests = std::move(moduleContext->moduleRequests);
+        _module->moduleRequests.removeDuplicates();
     }
 
     std::sort(_module->localExportEntries.begin(), _module->localExportEntries.end(), ExportEntry::lessThan);
diff --git a/src/qml/compiler/qv4compileddata.cpp b/src/qml/compiler/qv4compileddata.cpp
index 10799f70da..52db3100fc 100644
--- a/src/qml/compiler/qv4compileddata.cpp
+++ b/src/qml/compiler/qv4compileddata.cpp
@@ -383,22 +383,9 @@ bool CompilationUnit::verifyChecksum(const DependentTypesHasher &dependencyHashe
 QStringList CompilationUnit::moduleRequests() const
 {
     QStringList requests;
-
-    for (uint i = 0; i < data->importEntryTableSize; ++i) {
-        const ImportEntry &entry = data->importEntryTable()[i];
-        requests << stringAt(entry.moduleRequest);
-    }
-
-    for (uint i = 0; i < data->indirectExportEntryTableSize; ++i) {
-        const ExportEntry &entry = data->indirectExportEntryTable()[i];
-        requests << stringAt(entry.moduleRequest);
-    }
-
-    for (uint i = 0; i < data->starExportEntryTableSize; ++i) {
-        const ExportEntry &entry = data->starExportEntryTable()[i];
-        requests << stringAt(entry.moduleRequest);
-    }
-
+    requests.reserve(data->moduleRequestTableSize);
+    for (uint i = 0; i < data->moduleRequestTableSize; ++i)
+        requests << stringAt(data->moduleRequestTable()[i]);
     return requests;
 }
 
diff --git a/src/qml/compiler/qv4compileddata_p.h b/src/qml/compiler/qv4compileddata_p.h
index 7cd9e83dbd..4b0e6422bb 100644
--- a/src/qml/compiler/qv4compileddata_p.h
+++ b/src/qml/compiler/qv4compileddata_p.h
@@ -849,6 +849,8 @@ struct Unit
     quint32_le offsetToStarExportEntryTable;
     quint32_le importEntryTableSize;
     quint32_le offsetToImportEntryTable;
+    quint32_le moduleRequestTableSize;
+    quint32_le offsetToModuleRequestTable;
     qint32_le indexOfRootFunction;
     quint32_le sourceFileIndex;
     quint32_le finalUrlIndex;
@@ -939,9 +941,11 @@ struct Unit
     const ExportEntry *localExportEntryTable() const { return reinterpret_cast<const ExportEntry *>(reinterpret_cast<const char *>(this) + offsetToLocalExportEntryTable); }
     const ExportEntry *indirectExportEntryTable() const { return reinterpret_cast<const ExportEntry *>(reinterpret_cast<const char *>(this) + offsetToIndirectExportEntryTable); }
     const ExportEntry *starExportEntryTable() const { return reinterpret_cast<const ExportEntry *>(reinterpret_cast<const char *>(this) + offsetToStarExportEntryTable); }
+
+    const quint32_le *moduleRequestTable() const { return reinterpret_cast<const quint32_le*>((reinterpret_cast<const char *>(this)) + offsetToModuleRequestTable); }
 };
 
-static_assert(sizeof(Unit) == 232, "Unit structure needs to have the expected size to be binary compatible on disk when generated by host compiler and loaded by target");
+static_assert(sizeof(Unit) == 240, "Unit structure needs to have the expected size to be binary compatible on disk when generated by host compiler and loaded by target");
 
 struct TypeReference
 {
diff --git a/src/qml/compiler/qv4compiler.cpp b/src/qml/compiler/qv4compiler.cpp
index d7c7563315..594ea32b51 100644
--- a/src/qml/compiler/qv4compiler.cpp
+++ b/src/qml/compiler/qv4compiler.cpp
@@ -260,6 +260,9 @@ QV4::CompiledData::Unit *QV4::Compiler::JSUnitGenerator::generateUnit(GeneratorO
             registerString(entry.importName);
             registerString(entry.localName);
         }
+
+        for (const QString &request: module->moduleRequests)
+            registerString(request);
     }
 
     Q_ALLOCA_VAR(quint32_le, blockClassAndFunctionOffsets, (module->functions.size() + module->classes.size() + module->blocks.size()) * sizeof(quint32_le));
@@ -353,6 +356,14 @@ QV4::CompiledData::Unit *QV4::Compiler::JSUnitGenerator::generateUnit(GeneratorO
         }
     }
 
+    {
+        quint32_le *moduleRequestEntryToWrite = reinterpret_cast<quint32_le *>(dataPtr + unit->offsetToModuleRequestTable);
+        for (const QString &moduleRequest: module->moduleRequests) {
+            *moduleRequestEntryToWrite = getStringId(moduleRequest);
+            moduleRequestEntryToWrite++;
+        }
+    }
+
     // write strings and string table
     if (option == GenerateWithStringTable)
         stringTable.serialize(unit);
@@ -610,6 +621,12 @@ QV4::CompiledData::Unit QV4::Compiler::JSUnitGenerator::generateHeader(QV4::Comp
     nextOffset += unit.importEntryTableSize * sizeof(CompiledData::ImportEntry);
     nextOffset = (nextOffset + 7) & ~quint32(0x7);
 
+
+    unit.moduleRequestTableSize = module->moduleRequests.count();
+    unit.offsetToModuleRequestTable = nextOffset;
+    nextOffset += unit.moduleRequestTableSize * sizeof(uint);
+    nextOffset = (nextOffset + 7) & ~quint32(0x7);
+
     quint32 functionSize = 0;
     for (int i = 0; i < module->functions.size(); ++i) {
         Context *f = module->functions.at(i);
diff --git a/src/qml/compiler/qv4compilercontext_p.h b/src/qml/compiler/qv4compilercontext_p.h
index 216d5ca56b..0f7f4c835d 100644
--- a/src/qml/compiler/qv4compilercontext_p.h
+++ b/src/qml/compiler/qv4compilercontext_p.h
@@ -140,6 +140,7 @@ struct Module {
     QVector<ExportEntry> indirectExportEntries;
     QVector<ExportEntry> starExportEntries;
     QVector<ImportEntry> importEntries;
+    QStringList moduleRequests;
 };
 
 
@@ -176,6 +177,7 @@ struct Context {
     QQmlJS::AST::FormalParameterList *formals = nullptr;
     QStringList arguments;
     QStringList locals;
+    QStringList moduleRequests;
     QVector<ImportEntry> importEntries;
     QVector<ExportEntry> exportEntries;
     bool hasDefaultExport = false;
diff --git a/src/qml/compiler/qv4compilerscanfunctions.cpp b/src/qml/compiler/qv4compilerscanfunctions.cpp
index f47643826f..6ca46fd362 100644
--- a/src/qml/compiler/qv4compilerscanfunctions.cpp
+++ b/src/qml/compiler/qv4compilerscanfunctions.cpp
@@ -164,8 +164,11 @@ void ScanFunctions::endVisit(ESModule *)
 bool ScanFunctions::visit(ExportDeclaration *declaration)
 {
     QString module;
-    if (declaration->fromClause)
+    if (declaration->fromClause) {
         module = declaration->fromClause->moduleSpecifier.toString();
+        if (!module.isEmpty())
+            _context->moduleRequests << module;
+    }
 
     if (declaration->exportAll) {
         Compiler::ExportEntry entry;
@@ -233,8 +236,14 @@ bool ScanFunctions::visit(ExportDeclaration *declaration)
 bool ScanFunctions::visit(ImportDeclaration *declaration)
 {
     QString module;
-    if (declaration->fromClause)
+    if (declaration->fromClause) {
         module = declaration->fromClause->moduleSpecifier.toString();
+        if (!module.isEmpty())
+            _context->moduleRequests << module;
+    }
+
+    if (!declaration->moduleSpecifier.isEmpty())
+        _context->moduleRequests << declaration->moduleSpecifier.toString();
 
     if (ImportClause *import = declaration->importClause) {
         if (!import->importedDefaultBinding.isEmpty()) {
diff --git a/src/qml/parser/qqmljsast.cpp b/src/qml/parser/qqmljsast.cpp
index 0b1b97a41a..ca4317a9a6 100644
--- a/src/qml/parser/qqmljsast.cpp
+++ b/src/qml/parser/qqmljsast.cpp
@@ -1162,26 +1162,10 @@ void ExportDeclaration::accept0(Visitor *visitor)
     visitor->endVisit(this);
 }
 
-void ModuleItemList::accept0(Visitor *visitor)
-{
-    if (visitor->visit(this)) {
-        for (ModuleItemList *it = this; it; it = it->next) {
-            if (it->item) {
-                // The statement list is concatenated together between module list
-                // items and stored in the ESModule, thus not visited from there.
-                if (it->item->kind == Kind_StatementList)
-                    continue;
-                // Export declaration are also injected into the statement list for
-                // processing of declarations an variable statements, so don't visit
-                // it here to avoid double visits.
-                if (it->item->kind == Kind_ExportDeclaration)
-                    continue;
-            }
-            accept(it->item, visitor);
-        }
-    }
-
-    visitor->endVisit(this);
+void ModuleItemList::accept0(Visitor *)
+{
+    // See ESModule::accept0
+    Q_UNREACHABLE();
 }
 
 StatementList *ModuleItemList::buildStatementList(MemoryPool *pool) const
@@ -1192,6 +1176,8 @@ StatementList *ModuleItemList::buildStatementList(MemoryPool *pool) const
         if (!listItem) {
             if (AST::ExportDeclaration *exportDecl = AST::cast<AST::ExportDeclaration*>(item->item))
                 listItem = new (pool) AST::StatementList(exportDecl);
+            else if (AST::ImportDeclaration *importDecl = AST::cast<AST::ImportDeclaration*>(item->item))
+                listItem = new (pool) AST::StatementList(importDecl);
             else
                 continue;
         }
@@ -1209,7 +1195,11 @@ StatementList *ModuleItemList::buildStatementList(MemoryPool *pool) const
 void ESModule::accept0(Visitor *visitor)
 {
     if (visitor->visit(this)) {
-        accept(body, visitor);
+        // We don't accept the ModuleItemList (body) as instead the statement list items
+        // as well as the import/export declarations are linked together in the statement
+        // list. That way they are processed in correct order and can be used with the codegen's
+        // defineFunction() that expects a statement list.
+        // accept(body, visitor);
         accept(statements, visitor);
     }
 
diff --git a/src/qml/parser/qqmljsast_p.h b/src/qml/parser/qqmljsast_p.h
index e344dd4a28..1429d784c6 100644
--- a/src/qml/parser/qqmljsast_p.h
+++ b/src/qml/parser/qqmljsast_p.h
@@ -2539,7 +2539,7 @@ public:
     QStringRef moduleSpecifier;
 };
 
-class QML_PARSER_EXPORT ImportDeclaration: public Node
+class QML_PARSER_EXPORT ImportDeclaration: public Statement
 {
 public:
     QQMLJS_DECLARE_AST_NODE(ImportDeclaration)
@@ -2755,7 +2755,7 @@ public:
 
     StatementList *buildStatementList(MemoryPool *pool) const;
 
-    void accept0(Visitor *visitor) override;
+    void accept0(Visitor *) override;
 
     SourceLocation firstSourceLocation() const override
     { return item->firstSourceLocation(); }
diff --git a/src/qml/parser/qqmljsastvisitor_p.h b/src/qml/parser/qqmljsastvisitor_p.h
index 3162bb1630..c925096de6 100644
--- a/src/qml/parser/qqmljsastvisitor_p.h
+++ b/src/qml/parser/qqmljsastvisitor_p.h
@@ -369,9 +369,6 @@ public:
     virtual bool visit(ModuleItem *) { return true; }
     virtual void endVisit(ModuleItem *) {}
 
-    virtual bool visit(ModuleItemList *) { return true; }
-    virtual void endVisit(ModuleItemList *) {}
-
     virtual bool visit(ESModule *) { return true; }
     virtual void endVisit(ESModule *) {}
 
diff --git a/tests/auto/qml/ecmascripttests/TestExpectations b/tests/auto/qml/ecmascripttests/TestExpectations
index a6c9b33360..fc9149b27e 100644
--- a/tests/auto/qml/ecmascripttests/TestExpectations
+++ b/tests/auto/qml/ecmascripttests/TestExpectations
@@ -2159,7 +2159,6 @@ language/literals/regexp/u-case-mapping.js fails
 language/literals/regexp/y-assertion-start.js fails
 language/module-code/eval-export-dflt-cls-anon.js strictFails
 language/module-code/eval-gtbndng-indirect-update-dflt.js strictFails
-language/module-code/eval-rqstd-order.js strictFails
 language/module-code/instn-iee-bndng-cls.js strictFails
 language/module-code/instn-iee-bndng-const.js strictFails
 language/module-code/instn-iee-bndng-let.js strictFails
@@ -2180,7 +2179,6 @@ language/module-code/instn-named-bndng-dflt-gen-named.js strictFails
 language/module-code/instn-named-bndng-dflt-named.js strictFails
 language/module-code/instn-named-bndng-dflt-star.js strictFails
 language/module-code/instn-named-bndng-let.js strictFails
-language/module-code/instn-same-global.js strictFails
 language/module-code/instn-star-equality.js strictFails
 language/module-code/namespace/internals/define-own-property.js strictFails
 language/module-code/namespace/internals/delete-exported-uninit.js strictFails