Fix potential false caching of module prototypes

Our code assumed that products with the same profile and the same
multiplex configuration would also evaluate a module's condition to the
same value, which is not true: Properties such as qbs.architecture,
which are commonly checked in module conditions, can be set in a product
item independently of multiplexing.
We now evaluate the module condition for every product. According to our
benchmarker, the slowdown is a very modest 2% and does not appear to
increase for larger projects.
This patch amends 08ce978733, which tried to address the same problem,
but succeeded only for a subset of the possible cases.

Change-Id: I992e0f0d5cf207949cf5d863f242b9476ecdfc05
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

9 files changed, 66 insertions, 25 deletions

diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 4b87809c4..79748d451 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -83,11 +83,6 @@ namespace Internal {
 
 static QString shadowProductPrefix() { return QStringLiteral("__shadow__"); }
 
-static QString multiplexConfigurationIdPropertyInternal()
-{
-    return QStringLiteral("__multiplexConfigIdForModulePrototypes");
-}
-
 static void handlePropertyError(const ErrorInfo &error, const SetupProjectParameters &params,
                                 Logger &logger)
 {
@@ -896,8 +891,6 @@ QList<Item *> ModuleLoader::multiplexProductItem(ProductContext *dummyContext, I
         const QString multiplexConfigurationId = multiplexInfo.toIdString(row);
         const VariantValuePtr multiplexConfigurationIdValue
             = VariantValue::create(multiplexConfigurationId);
-        item->setProperty(multiplexConfigurationIdPropertyInternal(),
-                          multiplexConfigurationIdValue);
         if (multiplexInfo.table.size() > 1 || aggregator) {
             multiplexConfigurationIdValues.push_back(multiplexConfigurationIdValue);
             item->setProperty(StringConstants::multiplexConfigurationIdProperty(),
@@ -1142,8 +1135,6 @@ void ModuleLoader::prepareProduct(ProjectContext *projectContext, Item *productI
     }
     productContext.multiplexConfigurationId = m_evaluator->stringValue(
                 productItem, StringConstants::multiplexConfigurationIdProperty());
-    productContext.multiplexConfigIdForModulePrototypes = m_evaluator->stringValue(
-                productItem, multiplexConfigurationIdPropertyInternal());
     QBS_CHECK(!productContext.profileName.isEmpty());
     const auto it = projectContext->result->profileConfigs.constFind(productContext.profileName);
     if (it == projectContext->result->profileConfigs.constEnd()) {
@@ -3107,16 +3098,11 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString
     if (!module)
         return nullptr;
 
-    auto &conditionInfoList = m_modulePrototypeEnabledInfo[module];
-
-    // TODO: This is not good enough. qbs properties can differ independent of multiplexing.
-    const QString uniqueConfigKey = productContext->multiplexConfigIdForModulePrototypes;
-
-    for (const auto &conditionInfo : conditionInfoList) {
-        if (conditionInfo.first == uniqueConfigKey) {
-            qCDebug(lcModuleLoader) << "prototype cache hit (level 2)";
-            return conditionInfo.second ? module : nullptr;
-        }
+    const auto key = std::make_pair(module, productContext);
+    const auto it = m_modulePrototypeEnabledInfo.find(key);
+    if (it != m_modulePrototypeEnabledInfo.end()) {
+        qCDebug(lcModuleLoader) << "prototype cache hit (level 2)";
+        return it.value() ? module : nullptr;
     }
 
     // Set the name before evaluating any properties. EvaluatorScriptClass reads the module name.
@@ -3129,7 +3115,7 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString
     deepestModuleInstance->setPrototype(origDeepestModuleInstancePrototype);
     if (!enabled) {
         qCDebug(lcModuleLoader) << "condition of module" << fullModuleName << "is false";
-        conditionInfoList.push_back(std::make_pair(uniqueConfigKey, false));
+        m_modulePrototypeEnabledInfo.insert(key, false);
         return nullptr;
     }
 
@@ -3138,7 +3124,7 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString
     else
         resolveParameterDeclarations(module);
 
-    conditionInfoList.push_back(std::make_pair(uniqueConfigKey, true));
+    m_modulePrototypeEnabledInfo.insert(key, true);
     return module;
 }
 
diff --git a/src/lib/corelib/language/moduleloader.h b/src/lib/corelib/language/moduleloader.h
index ce13364d7..7be9a0fcc 100644
--- a/src/lib/corelib/language/moduleloader.h
+++ b/src/lib/corelib/language/moduleloader.h
@@ -176,7 +176,6 @@ private:
         ModuleLoaderResult::ProductInfo info;
         QString profileName;
         QString multiplexConfigurationId;
-        QString multiplexConfigIdForModulePrototypes;
         QVariantMap moduleProperties;
         std::map<QString, ProductDependencies> productModuleDependencies;
         std::unordered_map<const Item *, std::vector<ErrorInfo>> unknownProfilePropertyErrors;
@@ -392,9 +391,9 @@ private:
     // The keys are file paths, the values are module prototype items accompanied by a profile.
     std::unordered_map<QString, std::vector<std::pair<Item *, QString>>> m_modulePrototypes;
 
-    // The keys are module prototypes, the values specify whether the module's condition
-    // is true for the respective configuration.
-    std::unordered_map<Item *, std::vector<std::pair<QString, bool>>> m_modulePrototypeEnabledInfo;
+    // The keys are module prototypes and products, the values specify whether the module's
+    // condition is true for that product.
+    QHash<std::pair<Item *, ProductContext *>, bool> m_modulePrototypeEnabledInfo;
 
     QHash<const Item *, Item::PropertyDeclarationMap> m_parameterDeclarations;
     Set<Item *> m_disabledItems;
diff --git a/tests/auto/blackbox/testdata/module-conditions/module-conditions.qbs b/tests/auto/blackbox/testdata/module-conditions/module-conditions.qbs
new file mode 100644
index 000000000..dc3768203
--- /dev/null
+++ b/tests/auto/blackbox/testdata/module-conditions/module-conditions.qbs
@@ -0,0 +1,21 @@
+import qbs
+
+Project {
+    Product {
+        name: "p1"
+        qbs.architecture: "a"
+        Depends { name: "m" }
+    }
+    Product {
+        name: "p2"
+        qbs.architecture: "b"
+        Depends { name: "m" }
+    }
+    Product {
+        name: "p3"
+        multiplexByQbsProperties: "architectures"
+        aggregate: false
+        qbs.architectures: ["b", "c", "d"]
+        Depends { name: "m" }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m1.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m1.qbs
new file mode 100644
index 000000000..884350c3f
--- /dev/null
+++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m1.qbs
@@ -0,0 +1,6 @@
+Module {
+    condition: qbs.architecture === "a"
+    validate: {
+        console.info("loaded m1");
+    }
+}
diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m2.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m2.qbs
new file mode 100644
index 000000000..bcec6f424
--- /dev/null
+++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m2.qbs
@@ -0,0 +1,6 @@
+Module {
+    condition: qbs.architecture === "b"
+    validate: {
+        console.info("loaded m2");
+    }
+}
diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m3.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m3.qbs
new file mode 100644
index 000000000..5453c617f
--- /dev/null
+++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m3.qbs
@@ -0,0 +1,6 @@
+Module {
+    condition: qbs.architecture === "c"
+    validate: {
+        console.info("loaded m3");
+    }
+}
diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m4.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m4.qbs
new file mode 100644
index 000000000..a4cb0350d
--- /dev/null
+++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m4.qbs
@@ -0,0 +1,6 @@
+Module {
+    condition: qbs.architecture === "d"
+    validate: {
+        console.info("loaded m4");
+    }
+}
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 06e135549..8068aee48 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -6004,6 +6004,16 @@ void TestBlackbox::missingOverridePrefix()
              m_qbsStderr.constData());
 }
 
+void TestBlackbox::moduleConditions()
+{
+    QDir::setCurrent(testDataDir + "/module-conditions");
+    QCOMPARE(runQbs(), 0);
+    QCOMPARE(m_qbsStdout.count("loaded m1"), 1);
+    QCOMPARE(m_qbsStdout.count("loaded m2"), 2);
+    QCOMPARE(m_qbsStdout.count("loaded m3"), 1);
+    QCOMPARE(m_qbsStdout.count("loaded m4"), 1);
+}
+
 void TestBlackbox::movedFileDependency()
 {
     QDir::setCurrent(testDataDir + "/moved-file-dependency");
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index 87a6b1006..0dcfc5b31 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -170,6 +170,7 @@ private slots:
     void missingDependency();
     void missingProjectFile();
     void missingOverridePrefix();
+    void moduleConditions();
     void movedFileDependency();
     void multipleChanges();
     void multipleConfigurations();