Fix spurious "unknown property" error on loading module candidates

When loading a module file, we apply property overrides from profiles
and throw an error if ther profile sets a non-existing property on the
module.
This should only be done if that module file actually gets chosen to
represent the module. In the past, it was enough to check the item
condition, but this is no longer enough, because since commit 885bd2ec92
even module files with a matching condition might not get chosen because
another one has a higher priority.
This patch fixes the problem by delaying the check until we know the
chosen candidate and applying it only on that one.
We also improve the error message: There was no context given, so users
had no clue where the problem came from.

Change-Id: I9cb1b8ab118072e6be612c466552d9dd97796e8a
Reviewed-by: Jake Petroules <jake.petroules@qt.io>

9 files changed, 93 insertions, 21 deletions

diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 4e26362e5..be3343e3e 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -2480,25 +2480,38 @@ Item *ModuleLoader::searchAndLoadModuleFile(ProductContext *productContext,
         }
     }
 
-    if (candidates.size() > 1) {
+    if (candidates.empty()) {
+        if (!isRequired)
+            return createNonPresentModule(fullName, QLatin1String("not found"), nullptr);
+        if (Q_UNLIKELY(triedToLoadModule))
+            throw ErrorInfo(Tr::tr("Module %1 could not be loaded.").arg(fullName),
+                        dependsItemLocation);
+        return nullptr;
+    }
+
+    Item *moduleItem;
+    if (candidates.size() == 1) {
+        moduleItem = candidates.at(0).item;
+    } else {
         for (auto &candidate : candidates) {
             candidate.priority = m_evaluator->intValue(candidate.item, QStringLiteral("priority"),
                                                        candidate.priority);
         }
-        return chooseModuleCandidate(candidates, fullName);
+        moduleItem = chooseModuleCandidate(candidates, fullName);
     }
 
-    if (candidates.size() == 1)
-        return candidates.at(0).item;
-
-    if (!isRequired)
-        return createNonPresentModule(fullName, QLatin1String("not found"), nullptr);
-
-    if (Q_UNLIKELY(triedToLoadModule))
-        throw ErrorInfo(Tr::tr("Module %1 could not be loaded.").arg(fullName),
-                    dependsItemLocation);
-
-    return 0;
+    const auto it = productContext->unknownProfilePropertyErrors.find(moduleItem);
+    if (it != productContext->unknownProfilePropertyErrors.cend()) {
+        const QString fullProductName = ResolvedProduct::fullDisplayName
+                (productContext->name, productContext->multiplexConfigurationId);
+        ErrorInfo error(Tr::tr("Loading module '%1' for product '%2' failed due to invalid values "
+                               "in profile '%3':").arg(fullName, fullProductName,
+                                                       productContext->profileName));
+        for (const ErrorInfo &e : it->second)
+            error.append(e.toString());
+        handlePropertyError(error, m_parameters, m_logger);
+    }
+    return moduleItem;
 }
 
 // returns QVariant::Invalid for types that do not need conversion
@@ -2588,9 +2601,8 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString
             vmit != profileModuleProperties.end(); ++vmit)
     {
         if (Q_UNLIKELY(!module->hasProperty(vmit.key()))) {
-            const ErrorInfo error(Tr::tr("Unknown property: %1.%2").arg(fullModuleName,
-                                                                        vmit.key()));
-            unknownProfilePropertyErrors.append(error);
+            productContext->unknownProfilePropertyErrors[module].emplace_back
+                    (Tr::tr("Unknown property: %1.%2").arg(fullModuleName, vmit.key()));
             continue;
         }
         const PropertyDeclaration decl = module->propertyDeclaration(vmit.key());
@@ -2612,9 +2624,6 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString
     else
         resolveParameterDeclarations(module);
 
-    for (const ErrorInfo &error : qAsConst(unknownProfilePropertyErrors))
-        handlePropertyError(error, m_parameters, m_logger);
-
     m_modulePrototypeItemCache.insert(cacheKey, ItemCacheValue(module, true));
     return module;
 }
diff --git a/src/lib/corelib/language/moduleloader.h b/src/lib/corelib/language/moduleloader.h
index 5b672e49c..aa4fe917f 100644
--- a/src/lib/corelib/language/moduleloader.h
+++ b/src/lib/corelib/language/moduleloader.h
@@ -167,6 +167,7 @@ private:
         QString multiplexConfigIdForModulePrototypes;
         QVariantMap moduleProperties;
         std::map<QString, ProductDependencies> productModuleDependencies;
+        std::unordered_map<const Item *, std::vector<ErrorInfo>> unknownProfilePropertyErrors;
 
         QString uniqueName() const;
     };
diff --git a/tests/auto/language/testdata/modules/multiple-backends/backend1.qbs b/tests/auto/language/testdata/modules/multiple_backends/backend1.qbs
index 011f96af8..011f96af8 100644
--- a/tests/auto/language/testdata/modules/multiple-backends/backend1.qbs
+++ b/tests/auto/language/testdata/modules/multiple_backends/backend1.qbs
diff --git a/tests/auto/language/testdata/modules/multiple-backends/backend2.qbs b/tests/auto/language/testdata/modules/multiple_backends/backend2.qbs
index 1d2d817f7..93532005e 100644
--- a/tests/auto/language/testdata/modules/multiple-backends/backend2.qbs
+++ b/tests/auto/language/testdata/modules/multiple_backends/backend2.qbs
@@ -3,4 +3,5 @@ import qbs
 Module {
     condition: qbs.targetOS.contains("os2")
     property string prop: "backend 2"
+    property string backend2Prop
 }
diff --git a/tests/auto/language/testdata/modules/multiple_backends/backend3.qbs b/tests/auto/language/testdata/modules/multiple_backends/backend3.qbs
new file mode 100644
index 000000000..fda80b4ad
--- /dev/null
+++ b/tests/auto/language/testdata/modules/multiple_backends/backend3.qbs
@@ -0,0 +1,7 @@
+import qbs
+
+Module {
+    condition: qbs.targetOS.contains("os2") && qbs.toolchain.contains("tc")
+    priority: 1
+    property string backend3Prop
+}
diff --git a/tests/auto/language/testdata/non-applicable-module-property-in-profile.qbs b/tests/auto/language/testdata/non-applicable-module-property-in-profile.qbs
new file mode 100644
index 000000000..ebcefbb56
--- /dev/null
+++ b/tests/auto/language/testdata/non-applicable-module-property-in-profile.qbs
@@ -0,0 +1,16 @@
+Project {
+    property string targetOS
+    property string toolchain
+    Product {
+        name: "p"
+        multiplexByQbsProperties: ["profiles"]
+        qbs.profiles: ["theProfile"]
+        Depends { name: "multiple_backends" }
+        Profile {
+            name: "theProfile"
+            qbs.targetOS: [project.targetOS]
+            qbs.toolchain: [project.toolchain]
+            multiple_backends.backend3Prop: "value"
+        }
+    }
+}
diff --git a/tests/auto/language/testdata/overridden-properties-and-prototypes.qbs b/tests/auto/language/testdata/overridden-properties-and-prototypes.qbs
index d87611d7c..b0b37d804 100644
--- a/tests/auto/language/testdata/overridden-properties-and-prototypes.qbs
+++ b/tests/auto/language/testdata/overridden-properties-and-prototypes.qbs
@@ -2,5 +2,5 @@ import qbs
 
 Product {
     name: "p"
-    Depends { name: "multiple-backends" }
+    Depends { name: "multiple_backends" }
 }
diff --git a/tests/auto/language/tst_language.cpp b/tests/auto/language/tst_language.cpp
index 472ae6508..ce89c9f52 100644
--- a/tests/auto/language/tst_language.cpp
+++ b/tests/auto/language/tst_language.cpp
@@ -1799,6 +1799,42 @@ void TestLanguage::multiplexingByProfile_data()
     QTest::newRow("dependency by non-multiplexed with Depends.profile") << "p4.qbs" << true;
 }
 
+void TestLanguage::nonApplicableModulePropertyInProfile()
+{
+    QFETCH(QString, targetOS);
+    QFETCH(QString, toolchain);
+    QFETCH(bool, successExpected);
+    try {
+        SetupProjectParameters params = defaultParameters;
+        params.setProjectFilePath(testProject("non-applicable-module-property-in-profile.qbs"));
+        params.setOverriddenValues(QVariantMap{std::make_pair("project.targetOS", targetOS),
+                                               std::make_pair("project.toolchain", toolchain)});
+        const TopLevelProjectPtr project = loader->loadProject(params);
+        QVERIFY(!!project);
+        QVERIFY(successExpected);
+    } catch (const ErrorInfo &e) {
+        QVERIFY2(!successExpected, qPrintable(e.toString()));
+        QVERIFY2(e.toString().contains("Loading module 'multiple_backends' for product 'p' failed "
+                                       "due to invalid values in profile 'theProfile'"),
+                 qPrintable(e.toString()));
+        QVERIFY2(e.toString().contains("backend3Prop"), qPrintable(e.toString()));
+    }
+}
+
+void TestLanguage::nonApplicableModulePropertyInProfile_data()
+{
+    QTest::addColumn<QString>("targetOS");
+    QTest::addColumn<QString>("toolchain");
+    QTest::addColumn<bool>("successExpected");
+
+    QTest::newRow("no matching property (1)") << "os1" << QString() << false;
+    QTest::newRow("no matching property (2)") << "os2" << QString() << false;
+
+    // The point here is that there's a second, lower-prioritized candidate with a matching
+    // condition that doesn't have the property. This candidate must not throw an error.
+    QTest::newRow("matching property") << "os2" << "tc" << true;
+}
+
 void TestLanguage::nonRequiredProducts()
 {
     bool exceptionCaught = false;
@@ -1896,7 +1932,7 @@ void TestLanguage::overriddenPropertiesAndPrototypes()
         QVERIFY(!!project);
         QCOMPARE(project->products.count(), 1);
         QCOMPARE(project->products.first()->moduleProperties->moduleProperty(
-                     "multiple-backends", "prop").toString(), backendName);
+                     "multiple_backends", "prop").toString(), backendName);
     }
     catch (const ErrorInfo &e) {
         exceptionCaught = true;
diff --git a/tests/auto/language/tst_language.h b/tests/auto/language/tst_language.h
index 46ae657b3..db7ff31f4 100644
--- a/tests/auto/language/tst_language.h
+++ b/tests/auto/language/tst_language.h
@@ -124,6 +124,8 @@ private slots:
     void modules();
     void multiplexingByProfile();
     void multiplexingByProfile_data();
+    void nonApplicableModulePropertyInProfile();
+    void nonApplicableModulePropertyInProfile_data();
     void nonRequiredProducts();
     void nonRequiredProducts_data();
     void outerInGroup();