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>

4 files changed, 16 insertions, 7 deletions

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: