DependenciesResolver: Don't attach properties on non-present modules

We used to do that anyway and then suppress the resulting exception, but
that can lead to an unexpected state later. The sequence that triggered
the linked bug was as follows:
  - load module via non-required dependency in other module
  - exchange placeholder item in the product item and try to attach
    non-existing property on the module instance
  - suppress the resulting exception (because the dependency
    was not required) and continue
  - load module again (required this time) on product level
  - exchange placeholder item again -> assertion, because we have a
    module instance instead of the expected placeholder
Another way to fix this would have been to revert exchanging the
placeholder item after encountering an error, but that would be more
complex, and there is no reason to attach properties to non-existing
modules anyway.

Fixes: QBS-1776
Change-Id: I581a076d1d872616b186e4015873baeea211b647
Reviewed-by: Ivan Komissarov <ABBAPOH@gmail.com>

13 files changed, 74 insertions, 19 deletions

diff --git a/share/qbs/module-providers/__fallback/fallback.qbs b/share/qbs/module-providers/__fallback/fallback.qbs
index 52a3f080a..349af3a52 100644
--- a/share/qbs/module-providers/__fallback/fallback.qbs
+++ b/share/qbs/module-providers/__fallback/fallback.qbs
@@ -46,6 +46,8 @@ Module {
 
     property string theName: FileInfo.completeBaseName(filePath)
 
+    readonly property bool __fallback: true // Hack, please look away
+
     Probes.PkgConfigProbe {
         id: pkgConfigProbe
         condition: pkgconfig.present
diff --git a/src/lib/corelib/language/item.cpp b/src/lib/corelib/language/item.cpp
index d7ad9f5f5..ae48d1930 100644
--- a/src/lib/corelib/language/item.cpp
+++ b/src/lib/corelib/language/item.cpp
@@ -283,6 +283,11 @@ bool Item::isPresentModule() const
     return v && v->type() == Value::JSSourceValueType;
 }
 
+bool Item::isFallbackModule() const
+{
+    return hasProperty(QLatin1String("__fallback"));
+}
+
 void Item::setupForBuiltinType(DeprecationWarningMode deprecationMode, Logger &logger)
 {
     assertModuleLocked();
diff --git a/src/lib/corelib/language/item.h b/src/lib/corelib/language/item.h
index aadf2516b..69e4a5fec 100644
--- a/src/lib/corelib/language/item.h
+++ b/src/lib/corelib/language/item.h
@@ -165,6 +165,7 @@ public:
     static void removeChild(Item *parent, Item *child);
     void dump() const;
     bool isPresentModule() const;
+    bool isFallbackModule() const;
     void setupForBuiltinType(DeprecationWarningMode deprecationMode, Logger &logger);
     void copyProperty(const QString &propertyName, Item *target) const;
     void overrideProperties(
diff --git a/src/lib/corelib/loader/dependenciesresolver.cpp b/src/lib/corelib/loader/dependenciesresolver.cpp
index 431abc9fe..b46ed44a0 100644
--- a/src/lib/corelib/loader/dependenciesresolver.cpp
+++ b/src/lib/corelib/loader/dependenciesresolver.cpp
@@ -360,13 +360,6 @@ HandleDependency DependenciesResolver::handleResolvedDependencies()
             if (dependency.name.toString() == StringConstants::qbsModule())
                 throw e;
 
-            // This can happen when a property is set unconditionally on a non-required,
-            // non-present dependency. We allow this for user convenience.
-            if (!dependency.requiredLocally) {
-                state.pendingResolvedDependencies.pop();
-                continue;
-            }
-
             // See QBS-1338 for why we do not abort handling the product.
             state.pendingResolvedDependencies.pop();
             Item::Modules &modules = m_product.item->modules();
diff --git a/src/lib/corelib/loader/moduleinstantiator.cpp b/src/lib/corelib/loader/moduleinstantiator.cpp
index ab67bc270..1c0359217 100644
--- a/src/lib/corelib/loader/moduleinstantiator.cpp
+++ b/src/lib/corelib/loader/moduleinstantiator.cpp
@@ -135,6 +135,16 @@ void ModuleInstantiator::exchangePlaceholderItem(Item *loadingItem, Item *module
         return;
     }
 
+    if (!moduleItemForItemValues->isPresentModule())
+        return;
+
+    // This will yield false negatives for the case where there is an invalid property attached
+    // for a module that is actually found by pkg-config via the fallback provider.
+    // However, this is extremely rare compared to the case where the presence of the fallback
+    // module simply indicates "not present".
+    if (moduleItemForItemValues->isFallbackModule())
+        return;
+
     // If the old and the new items are the same, it means the existing item value already
     // pointed to a module instance (rather than a placeholder).
     // This can happen in two cases:
diff --git a/tests/auto/blackbox/testdata/property-assignment-on-non-present-module/property-assignment-on-non-present-module.qbs b/tests/auto/blackbox/testdata/property-assignment-on-non-present-module/property-assignment-on-non-present-module.qbs
deleted file mode 100644
index a01d6c561..000000000
--- a/tests/auto/blackbox/testdata/property-assignment-on-non-present-module/property-assignment-on-non-present-module.qbs
+++ /dev/null
@@ -1,4 +0,0 @@
-Product {
-    Depends { name: "nein"; required: false }
-    nein.doch: "ohhh!"
-}
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 4e97ba92c..7f6d1b8cf 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -3480,13 +3480,6 @@ void TestBlackbox::productProperties()
     QVERIFY(regularFileExists(relativeExecutableFilePath("blubb_user")));
 }
 
-void TestBlackbox::propertyAssignmentOnNonPresentModule()
-{
-    QDir::setCurrent(testDataDir + "/property-assignment-on-non-present-module");
-    QCOMPARE(runQbs(), 0);
-    QVERIFY2(m_qbsStderr.isEmpty(), m_qbsStderr.constData());
-}
-
 void TestBlackbox::propertyAssignmentInFailedModule()
 {
     QDir::setCurrent(testDataDir + "/property-assignment-in-failed-module");
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index 5d7d33526..8d37ad03d 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -248,7 +248,6 @@ private slots:
     void productDependenciesByType();
     void productInExportedModule();
     void productProperties();
-    void propertyAssignmentOnNonPresentModule();
     void propertyAssignmentInFailedModule();
     void propertyChanges();
     void propertyEvaluationContext();
diff --git a/tests/auto/language/testdata/invalid-prop-on-non-required-module/invalid-prop-on-non-required-module.qbs b/tests/auto/language/testdata/invalid-prop-on-non-required-module/invalid-prop-on-non-required-module.qbs
new file mode 100644
index 000000000..80ae6ad93
--- /dev/null
+++ b/tests/auto/language/testdata/invalid-prop-on-non-required-module/invalid-prop-on-non-required-module.qbs
@@ -0,0 +1,19 @@
+Project {
+    property bool useExistingModule
+
+    Product {
+        name: "a"
+        condition: project.useExistingModule
+        Depends { name: "deploader" }
+        Depends { name: "dep" }
+        dep.nosuchprop: true
+    }
+
+    Product {
+        name: "b"
+        condition: !project.useExistingModule
+        Depends { name: "deploader" }
+        Depends { name: "random"; required: false }
+        random.nosuchprop: true
+    }
+}
diff --git a/tests/auto/language/testdata/invalid-prop-on-non-required-module/modules/dep/dep.qbs b/tests/auto/language/testdata/invalid-prop-on-non-required-module/modules/dep/dep.qbs
new file mode 100644
index 000000000..5e45122de
--- /dev/null
+++ b/tests/auto/language/testdata/invalid-prop-on-non-required-module/modules/dep/dep.qbs
@@ -0,0 +1 @@
+Module {}
diff --git a/tests/auto/language/testdata/invalid-prop-on-non-required-module/modules/deploader/deploader.qbs b/tests/auto/language/testdata/invalid-prop-on-non-required-module/modules/deploader/deploader.qbs
new file mode 100644
index 000000000..15a1b5309
--- /dev/null
+++ b/tests/auto/language/testdata/invalid-prop-on-non-required-module/modules/deploader/deploader.qbs
@@ -0,0 +1,7 @@
+Module {
+    // This indirection exists to properly model QBS-1776.
+    // "deploader" corresponds to "bundle", and "dep" corresponds to "codesign"
+    Depends { condition: project.useExistingModule; name: "dep"; required: false }
+
+    Depends { condition: !project.useExistingModule; name: "random"; required: false }
+}
diff --git a/tests/auto/language/tst_language.cpp b/tests/auto/language/tst_language.cpp
index 6a29876dc..2fbffdc03 100644
--- a/tests/auto/language/tst_language.cpp
+++ b/tests/auto/language/tst_language.cpp
@@ -1614,6 +1614,33 @@ void TestLanguage::invalidOverrides_data()
             << "products.MyOtherProduct.cpp.useRPaths" << QString();
 }
 
+void TestLanguage::invalidPropOnNonRequiredModule_data()
+{
+    QTest::addColumn<bool>("useExistingModule");
+    QTest::addColumn<bool>("errorExpected");
+
+    QTest::newRow("existing module") << true << true;
+    QTest::newRow("non-existing module") << false << false;
+}
+
+void TestLanguage::invalidPropOnNonRequiredModule()
+{
+    QFETCH(bool, useExistingModule);
+    QFETCH(bool, errorExpected);
+
+    try {
+        defaultParameters.setOverriddenValues(
+                    {std::make_pair("project.useExistingModule", useExistingModule)});
+        resolveProject("invalid-prop-on-non-required-module");
+        QVERIFY(!errorExpected);
+    } catch (const ErrorInfo &e) {
+        const QString errorString = e.toString();
+        QVERIFY2(errorExpected, qPrintable(errorString));
+        QVERIFY2(errorString.contains("Property 'nosuchprop' is not declared"),
+                 qPrintable(errorString));
+    }
+}
+
 void TestLanguage::itemPrototype()
 {
     FileContextPtr fileContext = FileContext::create();
diff --git a/tests/auto/language/tst_language.h b/tests/auto/language/tst_language.h
index 5effb05b7..a7c39cec2 100644
--- a/tests/auto/language/tst_language.h
+++ b/tests/auto/language/tst_language.h
@@ -110,6 +110,8 @@ private slots:
     void invalidBindingInDisabledItem();
     void invalidOverrides();
     void invalidOverrides_data();
+    void invalidPropOnNonRequiredModule_data();
+    void invalidPropOnNonRequiredModule();
     void itemPrototype();
     void itemScope();
     void jsExtensions();