Fix false positives in artifacts map change tracking

We used to invalidate a script if it accessed the artifacts map and that
map's keys are now different from what they were. However, that logic
was too coarse: All rules that run after the command will add new
artifacts, so such changes will be the norm, at least when the project
is built for the first time. As a result, some artifacts might get
rebuilt unnecessarily on the next qbs invocation.
The reason why we did it that way was that we needed to consider the
case where a script iterated over the elements of an artifacts map, e.g.
using a for..in loop. In such a case, we cannot know which part of the
map the code is interested in and the script needs to be re-run if the
keys change. This is now solved with the help of a QScriptClass, which
informs us if a script enumerates the keys. In all other cases, the
script only accesses well-known tags, so we only have to compare the
respective artifact lists and do not care about other keys getting added
or removed.

Change-Id: I182a50c34ece28f7ff3f7eba7a8f246e9b79b30e
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

7 files changed, 161 insertions, 24 deletions

diff --git a/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp b/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp
index 0fcede324..c32e95b2a 100644
--- a/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp
+++ b/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp
@@ -45,6 +45,7 @@
 #include <language/language.h>
 #include <language/scriptengine.h>
 
+#include <QtScript/qscriptclass.h>
 #include <QtScript/qscriptcontext.h>
 
 namespace qbs {
@@ -52,7 +53,44 @@ namespace Internal {
 
 enum BuildGraphScriptValueCommonPropertyKeys : quint32 {
     CachedValueKey,
-    FileTagKey
+    FileTagKey,
+    ProductPtrKey,
+};
+
+class ArtifactsScriptClass : public QScriptClass
+{
+public:
+    ArtifactsScriptClass(QScriptEngine *engine) : QScriptClass(engine) { }
+
+private:
+    QueryFlags queryProperty(const QScriptValue &object, const QScriptString &name,
+                             QueryFlags flags, uint *id) override
+    {
+        getProduct(object);
+        qbsEngine()->setNonExistingArtifactSetRequested(m_product, name.toString());
+        return QScriptClass::queryProperty(object, name, flags, id);
+    }
+
+    QScriptClassPropertyIterator *newIterator(const QScriptValue &object) override
+    {
+        getProduct(object);
+        qbsEngine()->setArtifactsEnumerated(m_product);
+        return QScriptClass::newIterator(object);
+    }
+
+    void getProduct(const QScriptValue &object)
+    {
+        if (m_lastObjectId != object.objectId()) {
+            m_lastObjectId = object.objectId();
+            m_product = reinterpret_cast<const ResolvedProduct *>(
+                        object.data().property(ProductPtrKey).toVariant().value<quintptr>());
+        }
+    }
+
+    ScriptEngine *qbsEngine() const { return static_cast<ScriptEngine *>(engine()); }
+
+    qint64 m_lastObjectId = 0;
+    const ResolvedProduct *m_product = nullptr;
 };
 
 static bool isRelevantArtifact(const ResolvedProduct *, const Artifact *artifact)
@@ -74,6 +112,27 @@ static ArtifactSetByFileTag artifactsMap(const ResolvedModule *module)
     return artifactsMap(module->product);
 }
 
+static QScriptValue createArtifactsObject(const ResolvedProduct *product, ScriptEngine *engine)
+{
+    QScriptClass *scriptClass = engine->artifactsScriptClass();
+    if (!scriptClass) {
+        scriptClass = new ArtifactsScriptClass(engine);
+        engine->setArtifactsScriptClass(scriptClass);
+    }
+    QScriptValue artifactsObj = engine->newObject(scriptClass);
+    QScriptValue data = engine->newObject();
+    QVariant v;
+    v.setValue<quintptr>(reinterpret_cast<quintptr>(product));
+    data.setProperty(ProductPtrKey, engine->newVariant(v));
+    artifactsObj.setData(data);
+    return artifactsObj;
+}
+
+static QScriptValue createArtifactsObject(const ResolvedModule *, ScriptEngine *engine)
+{
+    return engine->newObject();
+}
+
 static bool checkAndSetArtifactsMapUpToDateFlag(const ResolvedProduct *p)
 {
     return p->buildData->checkAndSetJsArtifactsMapUpToDateFlag();
@@ -119,7 +178,7 @@ template<class ProductOrModule> static QScriptValue js_artifacts(
     QScriptValue artifactsObj = ctx->callee().property(CachedValueKey);
     if (artifactsObj.isObject() && checkAndSetArtifactsMapUpToDateFlag(productOrModule))
         return artifactsObj;
-    artifactsObj = engine->newObject();
+    artifactsObj = createArtifactsObject(productOrModule, engine);
     ctx->callee().setProperty(CachedValueKey, artifactsObj);
     const auto &map = artifactsMap(productOrModule);
     for (auto it = map.cbegin(); it != map.cend(); ++it) {
diff --git a/src/lib/corelib/buildgraph/requestedartifacts.cpp b/src/lib/corelib/buildgraph/requestedartifacts.cpp
index 0028c21bb..200f226ce 100644
--- a/src/lib/corelib/buildgraph/requestedartifacts.cpp
+++ b/src/lib/corelib/buildgraph/requestedartifacts.cpp
@@ -95,6 +95,20 @@ void RequestedArtifacts::setArtifactsForTag(const ResolvedProduct *product,
         filePaths.insert(a->filePath());
 }
 
+void RequestedArtifacts::setNonExistingTagRequested(const ResolvedProduct *product,
+                                                    const QString &tag)
+{
+    RequestedArtifactsPerProduct &ra = m_requestedArtifactsPerProduct[product->uniqueName()];
+    QBS_ASSERT(!ra.allTags.empty(), ;);
+    Set<QString> &filePaths = ra.requestedTags[tag];
+    QBS_CHECK(filePaths.empty());
+}
+
+void RequestedArtifacts::setArtifactsEnumerated(const ResolvedProduct *product)
+{
+    m_requestedArtifactsPerProduct[product->uniqueName()].artifactsEnumerated = true;
+}
+
 void RequestedArtifacts::unite(const RequestedArtifacts &other)
 {
     for (auto it = other.m_requestedArtifactsPerProduct.begin();
@@ -129,24 +143,33 @@ bool RequestedArtifacts::RequestedArtifactsPerProduct::isUpToDate(
                               << "is now disabled";
         return false;
     }
+
+    if (!artifactsEnumerated && requestedTags.empty())
+        return true;
+
     const ArtifactSetByFileTag currentArtifacts = product->buildData->artifactsByFileTag();
-    Set<QString> currentTags;
-    for (auto it = currentArtifacts.begin(); it != currentArtifacts.end(); ++it) {
-        const QString tagString = it.key().toString();
-        currentTags.insert(tagString);
-        const auto reqTagsIt = requestedTags.find(tagString);
-        if (reqTagsIt == requestedTags.end())
-            continue;
+    for (auto reqIt = requestedTags.cbegin(); reqIt != requestedTags.cend(); ++reqIt) {
+        const FileTag tag = FileTag(reqIt->first.toUtf8());
+        const auto currentIt = currentArtifacts.constFind(tag);
         Set<QString> currentFilePathsForTag;
-        for (const Artifact * const a : it.value())
-            currentFilePathsForTag.insert(a->filePath());
-        if (currentFilePathsForTag != reqTagsIt->second) {
+        if (currentIt != currentArtifacts.constEnd()) {
+            for (const Artifact * const a : currentIt.value())
+                currentFilePathsForTag.insert(a->filePath());
+        }
+        if (currentFilePathsForTag != reqIt->second) {
             qCDebug(lcBuildGraph) << "artifacts map not up to date: requested artifact set for "
-                                     "file tag" << tagString << "in product"
+                                     "file tag" << reqIt->first << "in product"
                                   << product->uniqueName() << "differs from the current one";
             return false;
         }
     }
+
+    if (!artifactsEnumerated)
+        return true;
+
+    Set<QString> currentTags;
+    for (auto it = currentArtifacts.begin(); it != currentArtifacts.end(); ++it)
+        currentTags.insert(it.key().toString());
     if (currentTags != allTags) {
         qCDebug(lcBuildGraph) << "artifacts map not up to date: overall file tags differ for "
                               << "product" << product->uniqueName();
@@ -169,10 +192,8 @@ void RequestedArtifacts::RequestedArtifactsPerProduct::unite(
 
 void RequestedArtifacts::RequestedArtifactsPerProduct::doSanityChecks() const
 {
-    Set<QString> requestedTagsSet;
     for (auto it = requestedTags.begin(); it != requestedTags.end(); ++it)
-        requestedTagsSet.insert(it->first);
-    QBS_CHECK(allTags.contains(requestedTagsSet));
+        QBS_CHECK(allTags.contains(it->first) || it->second.empty());
 }
 
 void RequestedArtifacts::RequestedArtifactsPerProduct::load(PersistentPool &pool)
diff --git a/src/lib/corelib/buildgraph/requestedartifacts.h b/src/lib/corelib/buildgraph/requestedartifacts.h
index 4d4a01297..82c393803 100644
--- a/src/lib/corelib/buildgraph/requestedartifacts.h
+++ b/src/lib/corelib/buildgraph/requestedartifacts.h
@@ -61,6 +61,8 @@ public:
     void clear() { m_requestedArtifactsPerProduct.clear(); }
     void setAllArtifactTags(const ResolvedProduct *product);
     void setArtifactsForTag(const ResolvedProduct *product, const FileTag &tag);
+    void setNonExistingTagRequested(const ResolvedProduct *product, const QString &tag);
+    void setArtifactsEnumerated(const ResolvedProduct *product);
     void unite(const RequestedArtifacts &other);
 
     void doSanityChecks() const;
@@ -78,6 +80,7 @@ private:
     {
         Set<QString> allTags;
         std::unordered_map<QString, Set<QString>> requestedTags;
+        bool artifactsEnumerated = false;
 
         bool isUpToDate(const ResolvedProduct *product) const;
 
@@ -87,7 +90,7 @@ private:
 
         template<PersistentPool::OpType opType> void serializationOp(PersistentPool &pool)
         {
-            pool.serializationOp<opType>(allTags, requestedTags);
+            pool.serializationOp<opType>(allTags, requestedTags, artifactsEnumerated);
         }
 
         void load(PersistentPool &pool);
diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h
index 11ed51a49..bfe238b09 100644
--- a/src/lib/corelib/language/scriptengine.h
+++ b/src/lib/corelib/language/scriptengine.h
@@ -128,6 +128,14 @@ public:
     {
         m_requestedArtifacts.setArtifactsForTag(product, tag);
     }
+    void setNonExistingArtifactSetRequested(const ResolvedProduct *product, const QString &tag)
+    {
+        m_requestedArtifacts.setNonExistingTagRequested(product, tag);
+    }
+    void setArtifactsEnumerated(const ResolvedProduct *product)
+    {
+        m_requestedArtifacts.setArtifactsEnumerated(product);
+    }
     void addPropertyRequestedFromArtifact(const Artifact *artifact, const Property &property);
     void addRequestedExport(const ResolvedProduct *product) { m_requestedExports.insert(product); }
     void clearRequestedProperties() {
@@ -238,6 +246,12 @@ public:
         m_productPropertyScriptClass = productPropertyScriptClass;
     }
 
+    QScriptClass *artifactsScriptClass() const { return m_artifactsScriptClass; }
+    void setArtifactsScriptClass(QScriptClass *artifactsScriptClass)
+    {
+        m_artifactsScriptClass = artifactsScriptClass;
+    }
+
     void addResourceAcquiringScriptObject(ResourceAcquiringScriptObject *obj);
     void releaseResourcesOfScriptObjects();
 
@@ -298,6 +312,7 @@ private:
     ScriptImporter *m_scriptImporter;
     QScriptClass *m_modulePropertyScriptClass;
     QScriptClass *m_productPropertyScriptClass = nullptr;
+    QScriptClass *m_artifactsScriptClass = nullptr;
     QHash<JsImport, QScriptValue> m_jsImportCache;
     bool m_propertyCacheEnabled;
     bool m_active;
diff --git a/src/lib/corelib/tools/persistence.cpp b/src/lib/corelib/tools/persistence.cpp
index 4ad024c8d..e705cb2da 100644
--- a/src/lib/corelib/tools/persistence.cpp
+++ b/src/lib/corelib/tools/persistence.cpp
@@ -49,7 +49,7 @@
 namespace qbs {
 namespace Internal {
 
-static const char QBS_PERSISTENCE_MAGIC[] = "QBSPERSISTENCE-118";
+static const char QBS_PERSISTENCE_MAGIC[] = "QBSPERSISTENCE-119";
 
 NoBuildGraphError::NoBuildGraphError(const QString &filePath)
     : ErrorInfo(Tr::tr("Build graph not found for configuration '%1'. Expected location was '%2'.")
diff --git a/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs b/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs
index 92579ad7c..0d3c361f6 100644
--- a/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs
+++ b/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs
@@ -56,4 +56,31 @@ Project {
             }
         }
     }
+
+    Product {
+        name: "p"
+        type: "p_type"
+        Rule {
+            multiplex: true
+            Artifact { filePath: "dummy1"; fileTags: "d_type" }
+            prepare: {
+                var cmd = new JavaScriptCommand();
+                cmd.description = "generating " + output.fileName;
+                cmd.sourceCode = function() {
+                    var blubb = product.artifacts.qbs;
+                };
+                return cmd;
+            }
+        }
+        Rule {
+            inputs: "d_type"
+            Artifact { filePath: "dummy2"; fileTags: "p_type" }
+            prepare: {
+                var cmd = new JavaScriptCommand();
+                cmd.description = "generating " + output.fileName;
+                cmd.sourceCode = function() { };
+                return cmd;
+            }
+        }
+    }
 }
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 3aac681eb..2a229954f 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -376,12 +376,14 @@ void TestBlackbox::artifactsMapChangeTracking()
     QVERIFY2(m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("linking"), m_qbsStdout.constData());
-    QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0);
+    QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0);
     QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("test.txt"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("TheBinary"), m_qbsStdout.constData());
+    QVERIFY2(m_qbsStdout.contains("dummy1"), m_qbsStdout.constData());
+    QVERIFY2(m_qbsStdout.contains("dummy2"), m_qbsStdout.constData());
 
     // Change name of target binary. Command must be re-run, because the file name of an
     // artifact changed.
@@ -393,13 +395,15 @@ void TestBlackbox::artifactsMapChangeTracking()
     QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("linking"), m_qbsStdout.constData());
-    QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0);
+    QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0);
     QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("test.txt"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("TheNewBinary"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("TheBinary"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData());
 
     // Add file tag to generated artifact. Command must be re-run, because it enumerates the keys
     // of the artifacts map.
@@ -409,12 +413,14 @@ void TestBlackbox::artifactsMapChangeTracking()
     QVERIFY2(m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("linking"), m_qbsStdout.constData());
-    QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0);
+    QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0);
     QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("test.txt"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("TheNewBinary"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData());
 
     // Add redundant file tag to generated artifact. Command must not be re-run, because
     // the artifacts map has not changed.
@@ -426,8 +432,10 @@ void TestBlackbox::artifactsMapChangeTracking()
     QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("linking"), m_qbsStdout.constData());
-    QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0);
+    QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0);
     QVERIFY2(!m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData());
 
     // Rebuild the app. Command must not be re-run, because the artifacts map has not changed.
     WAIT_FOR_NEW_TIMESTAMP();
@@ -436,8 +444,10 @@ void TestBlackbox::artifactsMapChangeTracking()
     QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("linking"), m_qbsStdout.constData());
-    QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0);
+    QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0);
     QVERIFY2(!m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData());
 
     // Add source file to app. Command must be re-run, because the artifacts map has changed.
     WAIT_FOR_NEW_TIMESTAMP();
@@ -447,12 +457,14 @@ void TestBlackbox::artifactsMapChangeTracking()
     QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData());
     QVERIFY2(!m_qbsStdout.contains("linking"), m_qbsStdout.constData());
-    QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0);
+    QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0);
     QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("test.txt"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData());
     QVERIFY2(m_qbsStdout.contains("TheNewBinary"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData());
 }
 
 void TestBlackbox::artifactsMapInvalidation()