Fix using extraSearchPaths stack

We push productContext->searchPaths via SearchPathsManager in the
setupProductDependencies() and then add some paths via providers. This
results to the fact that SearchPathsManager pops providers path but not
the one added in setupProductDependencies().

Fix that by restoring the state completely where it was before
resolveDependencies() call. This is fine, since providers paths are also
added to productContext->searchPaths and second Dependency resolution
pass will use it.

This allows to get rid of the newlyAddedModuleProviderSearchPaths which
seems to be needed only to count elemets in the stack correctly.

This is implemented be remembering the extraSearchPathsStack.size() in
SearchPathsManager object.

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

4 files changed, 25 insertions, 16 deletions

diff --git a/src/lib/corelib/language/itemreader.cpp b/src/lib/corelib/language/itemreader.cpp
index bdc5570a0..727e10560 100644
--- a/src/lib/corelib/language/itemreader.cpp
+++ b/src/lib/corelib/language/itemreader.cpp
@@ -88,7 +88,7 @@ void ItemReader::popExtraSearchPaths()
     m_allSearchPaths.clear();
 }
 
-std::vector<QStringList> ItemReader::extraSearchPathsStack() const
+const std::vector<QStringList> &ItemReader::extraSearchPathsStack() const
 {
     return m_extraSearchPaths;
 }
diff --git a/src/lib/corelib/language/itemreader.h b/src/lib/corelib/language/itemreader.h
index 2a01d7640..b1ac6794b 100644
--- a/src/lib/corelib/language/itemreader.h
+++ b/src/lib/corelib/language/itemreader.h
@@ -74,7 +74,7 @@ public:
     void setSearchPaths(const QStringList &searchPaths);
     void pushExtraSearchPaths(const QStringList &extraSearchPaths);
     void popExtraSearchPaths();
-    std::vector<QStringList> extraSearchPathsStack() const;
+    const std::vector<QStringList> &extraSearchPathsStack() const;
     void setExtraSearchPathsStack(const std::vector<QStringList> &s);
     void clearExtraSearchPathsStack();
     const QStringList &allSearchPaths() const;
diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index c66c5cb90..5603c862b 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -227,15 +227,25 @@ private:
 
 class SearchPathsManager {
 public:
-    SearchPathsManager(ItemReader *itemReader, const QStringList &extraSearchPaths)
+    explicit SearchPathsManager(ItemReader *itemReader)
         : m_itemReader(itemReader)
+        , m_oldSize(itemReader->extraSearchPathsStack().size())
+    {
+    }
+    SearchPathsManager(ItemReader *itemReader, const QStringList &extraSearchPaths)
+        : SearchPathsManager(itemReader)
     {
         m_itemReader->pushExtraSearchPaths(extraSearchPaths);
     }
-    ~SearchPathsManager() { m_itemReader->popExtraSearchPaths(); }
+    ~SearchPathsManager()
+    {
+        while (m_itemReader->extraSearchPathsStack().size() > m_oldSize)
+            m_itemReader->popExtraSearchPaths();
+    }
 
 private:
     ItemReader * const m_itemReader;
+    size_t m_oldSize{0};
 };
 
 ModuleLoader::ModuleLoader(Evaluator *evaluator, Logger &logger)
@@ -590,9 +600,9 @@ void ModuleLoader::handleTopLevelProject(ModuleLoaderResult *loadResult, Item *p
                     throw err;
                 handleProductError(err, &productContext);
             }
-            for (std::size_t i = 0; i < productContext.newlyAddedModuleProviderSearchPaths.size(); ++i)
-                m_reader->popExtraSearchPaths();
-            productContext.newlyAddedModuleProviderSearchPaths.clear();
+            // extraSearchPathsStack is changed during dependency resolution, check
+            // that we've rolled back all the changes
+            QBS_CHECK(m_reader->extraSearchPathsStack() == projectContext->searchPathsStack);
         }
     }
     if (!m_productsWithDeferredDependsItems.empty() || !m_exportsWithDeferredDependsItems.empty()) {
@@ -673,8 +683,8 @@ void ModuleLoader::handleProject(ModuleLoaderResult *loadResult,
         return;
     }
     topLevelProjectContext->projects.push_back(p.release());
-    m_reader->pushExtraSearchPaths(readExtraSearchPaths(projectItem)
-                                   << projectItem->file()->dirPath());
+    SearchPathsManager searchPathsManager(m_reader.get(), readExtraSearchPaths(projectItem)
+                                          << projectItem->file()->dirPath());
     projectContext.searchPathsStack = m_reader->extraSearchPathsStack();
     projectContext.item = projectItem;
 
@@ -758,7 +768,6 @@ void ModuleLoader::handleProject(ModuleLoaderResult *loadResult,
             break;
         }
     }
-    m_reader->popExtraSearchPaths();
 }
 
 QString ModuleLoader::MultiplexInfo::toIdString(size_t row) const
@@ -1328,6 +1337,10 @@ void ModuleLoader::setupProductDependencies(ProductContext *productContext,
 
     if (m_dependencyResolvingPass == 1)
         setSearchPathsForProduct(productContext);
+
+    // Module providers may push some extra search paths which we will be cleared
+    // by this SearchPathsManager. However, they will be also added to productContext->searchPaths
+    // so second pass will take that into account
     SearchPathsManager searchPathsManager(m_reader.get(), productContext->searchPaths);
 
     DependsContext dependsContext;
@@ -3937,13 +3950,10 @@ ModuleLoader::ModuleProviderResult ModuleLoader::findModuleProvider(const Qualif
 
         // (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 the code that removes the product-specific search paths when
-        //     product handling is done.
-        // (4) is needed for possible re-use in subsequent products and builds.
+        // (3) is needed for possible re-use in subsequent products and builds.
         m_reader->pushExtraSearchPaths(searchPaths); // (1)
         product.searchPaths << searchPaths; // (2)
-        product.newlyAddedModuleProviderSearchPaths.push_back(searchPaths); // (3)
-        addToGlobalInfo(); // (4)
+        addToGlobalInfo(); // (3)
         return {true, true};
     }
     return {};
diff --git a/src/lib/corelib/language/moduleloader.h b/src/lib/corelib/language/moduleloader.h
index 3dc091003..a4cadd4fa 100644
--- a/src/lib/corelib/language/moduleloader.h
+++ b/src/lib/corelib/language/moduleloader.h
@@ -185,7 +185,6 @@ private:
         std::unordered_map<const Item *, std::vector<ErrorInfo>> unknownProfilePropertyErrors;
         QStringList searchPaths;
 
-        std::vector<QStringList> newlyAddedModuleProviderSearchPaths;
         Set<QualifiedId> knownModuleProviders;
         std::optional<QVariantMap> theModuleProviderConfig;