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>

6 files changed, 41 insertions, 19 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()) {