Do not modify the global state after running provider

Instead, cache the result of the provider in a QHash using the provider/
module name and config as the key. This allows to have a clean state for
each Depends item, so the order in which they appear does not influence
which modules are generated. This is required for the later patch that
implements "named" providers.

Change-Id: Ia395cc94430763ed33d7ff5f2ee39e36d64f195e
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

5 files changed, 36 insertions, 51 deletions

diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 84482d5ff..6e1d74a03 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -238,6 +238,10 @@ public:
     }
     ~SearchPathsManager()
     {
+        reset();
+    }
+    void reset()
+    {
         while (m_itemReader->extraSearchPathsStack().size() > m_oldSize)
             m_itemReader->popExtraSearchPaths();
     }
@@ -2238,8 +2242,6 @@ void ModuleLoader::setSearchPathsForProduct(ModuleLoader::ProductContext *produc
         if (!currentSearchPaths.contains(p) && FileInfo(p).exists())
             product->searchPaths << p;
     }
-
-    m_moduleProviderLoader->setupKnownModuleProviders(*product);
 }
 
 ModuleLoader::ShadowProductInfo ModuleLoader::getShadowProductInfo(
@@ -3034,6 +3036,7 @@ Item *ModuleLoader::loadModule(ProductContext *productContext, Item *exportingPr
         }
     }
 
+    SearchPathsManager searchPathsManager(m_reader.get()); // paths can be added by providers
     Item *modulePrototype = nullptr;
     ProductModuleInfo * const pmi = productModule(productContext, fullName,
                                                   multiplexId, *isProductDependency);
@@ -3050,6 +3053,8 @@ Item *ModuleLoader::loadModule(ProductContext *productContext, Item *exportingPr
     if (!modulePrototype)
         return nullptr;
 
+    searchPathsManager.reset(); // deps must be processed in a clean state
+
     instantiateModule(productContext, exportingProductItem, item, moduleInstance, modulePrototype,
                       moduleName, pmi);
     return moduleInstance;
diff --git a/src/lib/corelib/language/moduleloader.h b/src/lib/corelib/language/moduleloader.h
index b619686c0..cea3be518 100644
--- a/src/lib/corelib/language/moduleloader.h
+++ b/src/lib/corelib/language/moduleloader.h
@@ -76,6 +76,7 @@ class ItemReader;
 class ModuleProviderLoader;
 class ProgressObserver;
 class QualifiedId;
+class SearchPathsManager;
 
 using ModulePropertiesPerGroup = std::unordered_map<const Item *, QualifiedIdSet>;
 
@@ -187,7 +188,6 @@ private:
         std::unordered_map<const Item *, std::vector<ErrorInfo>> unknownProfilePropertyErrors;
         QStringList searchPaths;
 
-        Set<QualifiedId> knownModuleProviders;
         std::optional<QVariantMap> theModuleProviderConfig;
 
         // The key corresponds to DeferredDependsContext.exportingProductItem, which is the
diff --git a/src/lib/corelib/language/moduleproviderinfo.h b/src/lib/corelib/language/moduleproviderinfo.h
index d55459221..8ed6f008d 100644
--- a/src/lib/corelib/language/moduleproviderinfo.h
+++ b/src/lib/corelib/language/moduleproviderinfo.h
@@ -76,11 +76,13 @@ public:
 
     template<PersistentPool::OpType opType> void completeSerializationOp(PersistentPool &pool)
     {
-        pool.serializationOp<opType>(reinterpret_cast<QStringList &>(name), config, searchPaths);
+        pool.serializationOp<opType>(
+                reinterpret_cast<QStringList &>(name), config, providerFile, searchPaths);
     }
 
     QualifiedId name;
     QVariantMap config;
+    QString providerFile;
     QStringList searchPaths;
     bool transientOutput = false; // Not to be serialized.
 };
@@ -90,7 +92,10 @@ using ModuleProviderInfoList = std::vector<ModuleProviderInfo>;
 // Persistent info stored between sessions
 struct StoredModuleProviderInfo
 {
-    ModuleProviderInfoList providers;
+    using CacheKey = std::tuple<QString /*name*/, QVariantMap /*config*/, int /*lookup*/>;
+    using ModuleProvidersCache = QHash<CacheKey, ModuleProviderInfo>;
+
+    ModuleProvidersCache providers;
 
     template<PersistentPool::OpType opType> void completeSerializationOp(PersistentPool &pool)
     {
diff --git a/src/lib/corelib/language/moduleproviderloader.cpp b/src/lib/corelib/language/moduleproviderloader.cpp
index c1da85490..48eaba0c9 100644
--- a/src/lib/corelib/language/moduleproviderloader.cpp
+++ b/src/lib/corelib/language/moduleproviderloader.cpp
@@ -66,38 +66,15 @@ ModuleProviderLoader::ModuleProviderLoader(ItemReader *reader, Evaluator *evalua
 {
 }
 
-void ModuleProviderLoader::setupKnownModuleProviders(ProductContext &product)
-{
-    // Existing module provider search paths are re-used if and only if the provider configuration
-    // at setup time was the same as the current one for the respective module provider.
-    if (!m_storedModuleProviderInfo.providers.empty()) {
-        const QVariantMap configForProduct = moduleProviderConfig(product);
-        for (const ModuleProviderInfo &c : m_storedModuleProviderInfo.providers) {
-            if (configForProduct.value(c.name.toString()).toMap() == c.config) {
-                qCDebug(lcModuleLoader) << "re-using search paths" << c.searchPaths
-                                        << "from module provider" << c.name
-                                        << "for product" << product.name;
-                product.knownModuleProviders.insert(c.name);
-                product.searchPaths << c.searchPaths;
-            }
-        }
-    }
-}
-
 ModuleProviderLoader::ModuleProviderResult ModuleProviderLoader::executeModuleProvider(
         ProductContext &productContext,
         const CodeLocation &dependsItemLocation,
         const QualifiedId &moduleName,
         FallbackMode fallbackMode)
 {
-    bool moduleAlreadyKnown = false;
     ModuleProviderResult result;
     for (QualifiedId providerName = moduleName; !providerName.empty();
          providerName.pop_back()) {
-        if (!productContext.knownModuleProviders.insert(providerName).second) {
-            moduleAlreadyKnown = true;
-            break;
-        }
         qCDebug(lcModuleLoader) << "Module" << moduleName.toString()
                                 << "not found, checking for module providers";
         result = findModuleProvider(providerName, productContext,
@@ -105,8 +82,7 @@ ModuleProviderLoader::ModuleProviderResult ModuleProviderLoader::executeModulePr
         if (result.providerFound)
             break;
     }
-    if (fallbackMode == FallbackMode::Enabled && !result.providerFound
-            && !moduleAlreadyKnown) {
+    if (fallbackMode == FallbackMode::Enabled && !result.providerFound) {
         qCDebug(lcModuleLoader) << "Specific module provider not found for"
                                 << moduleName.toString()  << ", setting up fallback.";
         result = findModuleProvider(moduleName, productContext,
@@ -121,32 +97,32 @@ ModuleProviderLoader::ModuleProviderResult ModuleProviderLoader::findModuleProvi
         ModuleProviderLookup lookupType,
         const CodeLocation &dependsItemLocation)
 {
-    const QString providerFile = findModuleProviderFile(name, lookupType);
-    if (providerFile.isEmpty())
-        return {};
-
     const QVariantMap config = moduleProviderConfig(product).value(name.toString()).toMap();
-    const QStringList searchPaths
-            = getProviderSearchPaths(name, providerFile, product, config, dependsItemLocation);
-    const auto addToGlobalInfo = [=] {
-        m_storedModuleProviderInfo.providers.emplace_back(
-                ModuleProviderInfo(name, config, searchPaths, m_parameters.dryRun()));
-    };
-    if (searchPaths.empty()) {
+    ModuleProviderInfo &info =
+            m_storedModuleProviderInfo.providers[{name.toString(), config, int(lookupType)}];
+    if (info.name.isEmpty()) { // not found in cache
+        info.name = name;
+        info.config = config;
+        info.providerFile = findModuleProviderFile(name, lookupType);
+        if (info.providerFile.isEmpty())
+            return {};
+        info.searchPaths = getProviderSearchPaths(
+                name, info.providerFile, product, config, dependsItemLocation);
+        info.transientOutput = m_parameters.dryRun();
+    } else {
+        if (info.providerFile.isEmpty())
+            return {};
+        qCDebug(lcModuleLoader) << "Re-using provider" << name << "from cache";
+    }
+    if (info.searchPaths.empty()) {
         qCDebug(lcModuleLoader) << "Module provider did run, but did not set up "
-                                       "any modules.";
-        addToGlobalInfo();
+                                    "any modules.";
         return {true, false};
     }
-    qCDebug(lcModuleLoader) << "Module provider added" << searchPaths.size()
+    qCDebug(lcModuleLoader) << "Module provider added" << info.searchPaths.size()
                             << "new search path(s)";
 
-    // (1) is needed so the immediate new look-up works.
-    // (2) is needed so the next use of SearchPathManager considers the new paths.
-    // (3) is needed for possible re-use in subsequent products and builds.
-    m_reader->pushExtraSearchPaths(searchPaths); // (1)
-    product.searchPaths << searchPaths; // (2)
-    addToGlobalInfo(); // (3)
+    m_reader->pushExtraSearchPaths(info.searchPaths);
     return {true, true};
 }
 
diff --git a/src/lib/corelib/language/moduleproviderloader.h b/src/lib/corelib/language/moduleproviderloader.h
index 694cf4b5c..c720d4202 100644
--- a/src/lib/corelib/language/moduleproviderloader.h
+++ b/src/lib/corelib/language/moduleproviderloader.h
@@ -82,7 +82,6 @@ public:
         m_parameters = std::move(parameters);
     }
 
-    void setupKnownModuleProviders(ProductContext &product);
     ModuleProviderResult executeModuleProvider(
             ProductContext &productContext,
             const CodeLocation &dependsItemLocation,