fix calculation of added/removed artifacts

We kept lists of added and removed artifacts in ProductBuildData. It was
never quite clear when to invalidate those lists, which led to QBS-635.
Instead we let the RuleNode decide which artifacts it considers as
"added or removed for this rule".

Task-number: QBS-635
Change-Id: I390e0ab775c695045c6e91ade3ac7326692cb314
Reviewed-by: Christian Kandeler <christian.kandeler@digia.com>

9 files changed, 99 insertions, 125 deletions

diff --git a/src/lib/corelib/buildgraph/buildgraphloader.cpp b/src/lib/corelib/buildgraph/buildgraphloader.cpp
index f2f4c3c54..1b7fb1056 100644
--- a/src/lib/corelib/buildgraph/buildgraphloader.cpp
+++ b/src/lib/corelib/buildgraph/buildgraphloader.cpp
@@ -577,7 +577,6 @@ void BuildGraphLoader::onProductFileListChanged(const ResolvedProductPtr &restor
 
     QBS_CHECK(newlyResolvedProduct->enabled);
 
-    QList<Artifact *> addedArtifacts;
     ArtifactSet artifactsToRemove;
     QHash<QString, SourceArtifactConstPtr> oldArtifacts, newArtifacts;
 
@@ -616,7 +615,6 @@ void BuildGraphLoader::onProductFileListChanged(const ResolvedProductPtr &restor
                                                       newArtifact);
                 }
             }
-            addedArtifacts += newArtifact;
         }
     }
 
@@ -648,13 +646,14 @@ void BuildGraphLoader::onProductFileListChanged(const ResolvedProductPtr &restor
             // handle added filetags
             foreach (const FileTag &addedFileTag, changedArtifact->fileTags - a->fileTags) {
                 artifact->fileTags += addedFileTag;
-                newlyResolvedProduct->registerAddedFileTag(addedFileTag, artifact);
+                artifact->product->buildData->artifactsByFileTag[addedFileTag].insert(artifact);
             }
 
             // handle removed filetags
             foreach (const FileTag &removedFileTag, a->fileTags - changedArtifact->fileTags) {
                 artifact->fileTags -= removedFileTag;
-                newlyResolvedProduct->registerRemovedFileTag(removedFileTag, artifact);
+                removeArtifactFromSetByFileTag(artifact, removedFileTag,
+                                               artifact->product->buildData->artifactsByFileTag);
             }
         }
 
@@ -667,7 +666,7 @@ void BuildGraphLoader::onProductFileListChanged(const ResolvedProductPtr &restor
             newlyResolvedProduct->topLevelProject()->buildData
                 ->removeArtifactAndExclusiveDependents(oldArtifact, m_logger, true,
                                                        &artifactsToRemove);
-            addedArtifacts += createArtifact(newlyResolvedProduct, changedArtifact, m_logger);
+            createArtifact(newlyResolvedProduct, changedArtifact, m_logger);
         }
     }
 
@@ -677,8 +676,6 @@ void BuildGraphLoader::onProductFileListChanged(const ResolvedProductPtr &restor
             m_artifactsRemovedFromDisk << artifact->filePath();
         m_objectsToDelete << artifact;
     }
-    foreach (Artifact * const artifact, addedArtifacts)
-        newlyResolvedProduct->registerAddedArtifact(artifact);
 }
 
 static SourceArtifactConstPtr findSourceArtifact(const ResolvedProductConstPtr &product,
diff --git a/src/lib/corelib/buildgraph/executor.cpp b/src/lib/corelib/buildgraph/executor.cpp
index a8072c97c..92b664007 100644
--- a/src/lib/corelib/buildgraph/executor.cpp
+++ b/src/lib/corelib/buildgraph/executor.cpp
@@ -861,10 +861,6 @@ void Executor::finish()
                 m_artifactsRemovedFromDisk << filePath;
             }
             product->buildData->rescuableArtifactData.clear();
-
-            // Similar logic applies for the artifacts scheduled for potential rule application.
-            product->buildData->addedArtifactsByFileTag.clear();
-            product->buildData->removedArtifactsByFileTag.clear();
         }
     }
 
diff --git a/src/lib/corelib/buildgraph/productbuilddata.cpp b/src/lib/corelib/buildgraph/productbuilddata.cpp
index 5cdf69923..abea2c861 100644
--- a/src/lib/corelib/buildgraph/productbuilddata.cpp
+++ b/src/lib/corelib/buildgraph/productbuilddata.cpp
@@ -78,8 +78,6 @@ void ProductBuildData::load(PersistentPool &pool)
         rescuableArtifactData.insert(filePath, elem);
     }
     loadArtifactSetByFileTag(pool, artifactsByFileTag);
-    loadArtifactSetByFileTag(pool, addedArtifactsByFileTag);
-    loadArtifactSetByFileTag(pool, removedArtifactsByFileTag);
 
     pool.stream() >> count;
     for (int i = 0; i < count; ++i) {
@@ -112,8 +110,6 @@ void ProductBuildData::store(PersistentPool &pool) const
         it.value().store(pool);
     }
     storeArtifactSetByFileTag(pool, artifactsByFileTag);
-    storeArtifactSetByFileTag(pool, addedArtifactsByFileTag);
-    storeArtifactSetByFileTag(pool, removedArtifactsByFileTag);
 
     pool.stream() << artifactsWithChangedInputsPerRule.count();
     for (ArtifactSetByRule::ConstIterator it = artifactsWithChangedInputsPerRule.constBegin();
@@ -129,14 +125,21 @@ void addArtifactToSet(Artifact *artifact, ProductBuildData::ArtifactSetByFileTag
         container[tag] += artifact;
 }
 
+void removeArtifactFromSetByFileTag(Artifact *artifact, const FileTag &fileTag,
+        ProductBuildData::ArtifactSetByFileTag &container)
+{
+    ProductBuildData::ArtifactSetByFileTag::iterator it = container.find(fileTag);
+    if (it == container.end())
+        return;
+    it->remove(artifact);
+    if (it->isEmpty())
+        container.erase(it);
+}
+
 void removeArtifactFromSet(Artifact *artifact, ProductBuildData::ArtifactSetByFileTag &container)
 {
-    foreach (const FileTag &t, artifact->fileTags) {
-        ArtifactSet &s = container[t];
-        s.remove(artifact);
-        if (s.isEmpty())
-            container.remove(t);
-    }
+    foreach (const FileTag &t, artifact->fileTags)
+        removeArtifactFromSetByFileTag(artifact, t, container);
 }
 
 } // namespace Internal
diff --git a/src/lib/corelib/buildgraph/productbuilddata.h b/src/lib/corelib/buildgraph/productbuilddata.h
index 07193ea9a..89f71d4ad 100644
--- a/src/lib/corelib/buildgraph/productbuilddata.h
+++ b/src/lib/corelib/buildgraph/productbuilddata.h
@@ -64,8 +64,6 @@ public:
 
     typedef QHash<FileTag, ArtifactSet> ArtifactSetByFileTag;
     ArtifactSetByFileTag artifactsByFileTag;
-    ArtifactSetByFileTag addedArtifactsByFileTag;
-    ArtifactSetByFileTag removedArtifactsByFileTag;
 
     typedef QHash<RuleConstPtr, ArtifactSet> ArtifactSetByRule;
     ArtifactSetByRule artifactsWithChangedInputsPerRule;
@@ -76,6 +74,8 @@ private:
 };
 
 void addArtifactToSet(Artifact *artifact, ProductBuildData::ArtifactSetByFileTag &container);
+void removeArtifactFromSetByFileTag(Artifact *artifact, const FileTag &fileTag,
+        ProductBuildData::ArtifactSetByFileTag &container);
 void removeArtifactFromSet(Artifact *artifact, ProductBuildData::ArtifactSetByFileTag &container);
 
 } // namespace Internal
diff --git a/src/lib/corelib/buildgraph/projectbuilddata.cpp b/src/lib/corelib/buildgraph/projectbuilddata.cpp
index 978acecad..4f62bae3e 100644
--- a/src/lib/corelib/buildgraph/projectbuilddata.cpp
+++ b/src/lib/corelib/buildgraph/projectbuilddata.cpp
@@ -35,7 +35,6 @@
 #include "command.h"
 #include "rulegraph.h"
 #include "rulenode.h"
-#include "rulesapplicator.h"
 #include "rulesevaluationcontext.h"
 #include "transformer.h"
 #include <language/language.h>
@@ -276,10 +275,6 @@ void ProjectBuildData::removeArtifact(Artifact *artifact,
         removeArtifactFromSet(artifact, artifact->product->buildData->artifactsByFileTag);
     }
 
-    // If removal is requested and the executor has not run since the time the artifact was last
-    // added, we must undo the "register" operation.
-    artifact->product->unregisterAddedArtifact(artifact);
-
     disconnectArtifact(artifact, logger);
     if (artifact->transformer) {
         artifact->product->unregisterArtifactWithChangedInputs(artifact);
@@ -428,7 +423,7 @@ void BuildDataResolver::resolveProductBuildData(const ResolvedProductPtr &produc
     evalContext()->checkForCancelation();
 
     product->buildData.reset(new ProductBuildData);
-    ArtifactsPerFileTagMap artifactsPerFileTag;
+    ProductBuildData::ArtifactSetByFileTag artifactsPerFileTag;
 
     foreach (ResolvedProductPtr dependency, product->dependencies) {
         if (Q_UNLIKELY(!dependency->enabled)) {
@@ -449,7 +444,6 @@ void BuildDataResolver::resolveProductBuildData(const ResolvedProductPtr &produc
     }
     qbsFileArtifact->fileTags.insert("qbs");
     artifactsPerFileTag["qbs"].insert(qbsFileArtifact);
-    product->registerAddedArtifact(qbsFileArtifact);
 
     // read sources
     foreach (const SourceArtifactConstPtr &sourceArtifact, product->allEnabledFiles()) {
@@ -458,7 +452,6 @@ void BuildDataResolver::resolveProductBuildData(const ResolvedProductPtr &produc
             continue; // ignore duplicate artifacts
 
         Artifact *artifact = createArtifact(product, sourceArtifact, m_logger);
-        product->registerAddedArtifact(artifact);
         foreach (const FileTag &fileTag, artifact->fileTags)
             artifactsPerFileTag[fileTag].insert(artifact);
     }
@@ -493,7 +486,6 @@ void BuildDataResolver::resolveProductBuildData(const ResolvedProductPtr &produc
                 safeConnect(outputArtifact, inputArtifact, m_logger);
             foreach (const FileTag &fileTag, outputArtifact->fileTags)
                 artifactsPerFileTag[fileTag].insert(outputArtifact);
-            product->registerAddedArtifact(outputArtifact);
 
             RuleArtifactPtr ruleArtifact = RuleArtifact::create();
             ruleArtifact->filePath = outputArtifact->filePath();
diff --git a/src/lib/corelib/buildgraph/rulenode.cpp b/src/lib/corelib/buildgraph/rulenode.cpp
index efba23e68..3e01d7050 100644
--- a/src/lib/corelib/buildgraph/rulenode.cpp
+++ b/src/lib/corelib/buildgraph/rulenode.cpp
@@ -65,67 +65,49 @@ QString RuleNode::toString() const
 void RuleNode::apply(const Logger &logger, const ArtifactSet &changedInputs,
         ApplicationResult *result)
 {
-    bool hasAddedTags = false;
-    bool hasRemovedTags = false;
-    result->upToDate = changedInputs.isEmpty() && !usedDependenciesAdded();
+    const ArtifactSet oldInputs = oldInputArtifacts();
+    ArtifactSet allCompatibleInputs = currentInputArtifacts();
+    const ArtifactSet addedInputs = allCompatibleInputs - oldInputs;
+    const ArtifactSet removedInputs = oldInputs - allCompatibleInputs;
+    result->upToDate = changedInputs.isEmpty() && addedInputs.isEmpty() && removedInputs.isEmpty();
 
-    ProductBuildData::ArtifactSetByFileTag relevantArtifacts;
+    ArtifactSet inputs = changedInputs;
     if (product->isMarkedForReapplication(m_rule)) {
         QBS_CHECK(m_rule->multiplex);
         result->upToDate = false;
         product->unmarkForReapplication(m_rule);
         if (logger.traceEnabled())
             logger.qbsTrace() << "[BG] rule is marked for reapplication " << m_rule->toString();
-
-        foreach (Artifact *artifact, ArtifactSet::fromNodeSet(product->buildData->nodes)) {
-            if (m_rule->acceptsAsInput(artifact))
-                addArtifactToSet(artifact, relevantArtifacts);
-        }
+        inputs += allCompatibleInputs;
     } else {
-        foreach (const FileTag &tag, m_rule->inputs) {
-            if (product->addedArtifactsByFileTag(tag).count()) {
-                hasAddedTags = true;
-                result->upToDate = false;
-            }
-            if (product->removedArtifactsByFileTag(tag).count()) {
-                hasRemovedTags = true;
-                result->upToDate = false;
-            }
-            if (hasAddedTags && hasRemovedTags)
-                break;
-        }
-
-        relevantArtifacts = product->buildData->addedArtifactsByFileTag;
-        if (!changedInputs.isEmpty()) {
-            foreach (Artifact *artifact, changedInputs)
-                addArtifactToSet(artifact, relevantArtifacts);
-        }
+        inputs += addedInputs;
     }
     if (result->upToDate)
         return;
-    if (hasRemovedTags) {
+    if (!removedInputs.isEmpty()) {
         ArtifactSet outputArtifactsToRemove;
-        foreach (const FileTag &tag, m_rule->inputs) {
-            foreach (Artifact *artifact, product->removedArtifactsByFileTag(tag)) {
-                foreach (Artifact *parent, ArtifactSet::fromNodeSet(artifact->parents)) {
-                    if (!parent->transformer || parent->transformer->rule != m_rule
-                            || !parent->transformer->inputs.contains(artifact)) {
-                        // parent was not created by our rule.
-                        continue;
-                    }
-                    outputArtifactsToRemove += parent;
+        foreach (Artifact *artifact, removedInputs) {
+            foreach (Artifact *parent, ArtifactSet::fromNodeSet(artifact->parents)) {
+                if (!parent->transformer || parent->transformer->rule != m_rule
+                        || !parent->transformer->inputs.contains(artifact)) {
+                    // TODO: turn two of the three conditions above into QBS_CHECKs
+                    // parent must always have a transformer, because it's generated.
+                    //     QBS_CHECK(parent->transformer)
+                    // artifact is a former input of m_rule and parent was created by m_rule
+                    // the inputs of the transformer must contain artifact
+                    //     QBS_CHECK(parent->transformer->inputs.contains(artifact))
+
+                    // parent was not created by our rule.
+                    continue;
                 }
+                outputArtifactsToRemove += parent;
             }
         }
         RulesApplicator::handleRemovedRuleOutputs(outputArtifactsToRemove, logger);
     }
-    if (!relevantArtifacts.isEmpty()) {
-        RulesApplicator applicator(product, relevantArtifacts, logger);
-        result->createdNodes = applicator.applyRuleInEvaluationContext(m_rule);
-        foreach (BuildGraphNode *node, result->createdNodes) {
-            if (Artifact *artifact = dynamic_cast<Artifact *>(node))
-                product->registerAddedArtifact(artifact);
-        }
+    if (!inputs.isEmpty()) {
+        RulesApplicator applicator(product, logger);
+        result->createdNodes = applicator.applyRuleInEvaluationContext(m_rule, inputs);
     }
 }
 
@@ -141,17 +123,47 @@ void RuleNode::store(PersistentPool &pool) const
     pool.store(m_rule);
 }
 
-bool RuleNode::usedDependenciesAdded() const
+QList<TransformerPtr> RuleNode::createdTransformers() const
+{
+    QList<TransformerPtr> lst;
+    foreach (BuildGraphNode *parent, parents) {
+        if (parent->type() != ArtifactNodeType)
+            continue;
+        Artifact *artifact = static_cast<Artifact *>(parent);
+        if (!artifact->transformer || artifact->transformer->rule != m_rule)
+            continue;
+        lst.append(artifact->transformer);
+    }
+    return lst;
+}
+
+ArtifactSet RuleNode::oldInputArtifacts() const
 {
+    ArtifactSet s;
+    foreach (const TransformerPtr &t, createdTransformers())
+        s += t->inputs;
+    return s;
+}
+
+ArtifactSet RuleNode::currentInputArtifacts() const
+{
+    ArtifactSet s;
+    foreach (const FileTag &t, m_rule->inputs)
+        s += product->lookupArtifactsByFileTag(t);
+
     foreach (const ResolvedProductConstPtr &dep, product->dependencies) {
-        if (!dep->buildData || !dep->fileTags.matches(rule()->usings))
+        if (!dep->buildData)
             continue;
-        foreach (Artifact *a, dep->targetArtifacts()) {
-            if (a->fileTags.matches(rule()->usings) && a->product->isAdded(a))
-                return true;
+        ArtifactSet artifactsToCheck;
+        foreach (Artifact *targetArtifact, dep->targetArtifacts())
+            artifactsToCheck += targetArtifact->transformer->outputs;
+        foreach (Artifact *artifact, artifactsToCheck) {
+            if (artifact->fileTags.matches(m_rule->usings))
+                s += artifact;
         }
     }
-    return false;
+
+    return s;
 }
 
 } // namespace Internal
diff --git a/src/lib/corelib/buildgraph/rulenode.h b/src/lib/corelib/buildgraph/rulenode.h
index dce9a6381..027094211 100644
--- a/src/lib/corelib/buildgraph/rulenode.h
+++ b/src/lib/corelib/buildgraph/rulenode.h
@@ -32,6 +32,7 @@
 
 #include "artifactset.h"
 #include "buildgraphnode.h"
+#include "forward_decls.h"
 #include <language/forward_decls.h>
 
 namespace qbs {
@@ -65,7 +66,9 @@ protected:
     void store(PersistentPool &pool) const;
 
 private:
-    bool usedDependenciesAdded() const;
+    QList<TransformerPtr> createdTransformers() const;
+    ArtifactSet oldInputArtifacts() const;
+    ArtifactSet currentInputArtifacts() const;
 
     RuleConstPtr m_rule;
 };
diff --git a/src/lib/corelib/buildgraph/rulesapplicator.cpp b/src/lib/corelib/buildgraph/rulesapplicator.cpp
index aad148ab3..f5d8f9da9 100644
--- a/src/lib/corelib/buildgraph/rulesapplicator.cpp
+++ b/src/lib/corelib/buildgraph/rulesapplicator.cpp
@@ -51,10 +51,8 @@
 namespace qbs {
 namespace Internal {
 
-RulesApplicator::RulesApplicator(const ResolvedProductPtr &product,
-                                 ArtifactsPerFileTagMap &artifactsPerFileTag, const Logger &logger)
+RulesApplicator::RulesApplicator(const ResolvedProductPtr &product, const Logger &logger)
     : m_product(product)
-    , m_artifactsPerFileTag(artifactsPerFileTag)
     , m_mocScanner(0)
     , m_logger(logger)
 {
@@ -65,39 +63,21 @@ RulesApplicator::~RulesApplicator()
     delete m_mocScanner;
 }
 
-NodeSet RulesApplicator::applyRuleInEvaluationContext(const RuleConstPtr &rule)
+NodeSet RulesApplicator::applyRuleInEvaluationContext(const RuleConstPtr &rule,
+        const ArtifactSet &inputArtifacts)
 {
     m_createdArtifacts.clear();
     RulesEvaluationContext::Scope s(m_product->topLevelProject()->buildData->evaluationContext.data());
-    applyRule(rule);
+    applyRule(rule, inputArtifacts);
     return m_createdArtifacts;
 }
 
-void RulesApplicator::applyRule(const RuleConstPtr &rule)
+void RulesApplicator::applyRule(const RuleConstPtr &rule, const ArtifactSet &inputArtifacts)
 {
-    m_rule = rule;
-
-    ArtifactSet inputArtifacts;
-    foreach (const FileTag &fileTag, m_rule->inputs)
-        inputArtifacts.unite(m_artifactsPerFileTag.value(fileTag));
-
-    ArtifactSet usingsArtifacts;
-    if (!m_rule->usings.isEmpty()) {
-        foreach (const ResolvedProductPtr &dep, m_product->dependencies) {
-            QBS_CHECK(dep->buildData);
-            ArtifactSet artifactsToCheck;
-            foreach (Artifact *targetArtifact, dep->targetArtifacts())
-                artifactsToCheck.unite(targetArtifact->transformer->outputs);
-            foreach (Artifact *artifact, artifactsToCheck) {
-                if (artifact->fileTags.matches(m_rule->usings))
-                    usingsArtifacts.insert(artifact);
-            }
-        }
-    }
-
-    if (inputArtifacts.isEmpty() && usingsArtifacts.isEmpty())
+    if (inputArtifacts.isEmpty())
         return;
 
+    m_rule = rule;
     if (rule->name == QLatin1String("QtCoreMocRule")) {
         delete m_mocScanner;
         m_mocScanner = new QtMocScanner(m_product, scope(), m_logger);
@@ -108,10 +88,9 @@ void RulesApplicator::applyRule(const RuleConstPtr &rule)
     setupScriptEngineForProduct(engine(), m_product, m_rule->module, prepareScriptContext, &observer);
 
     if (m_rule->multiplex) { // apply the rule once for a set of inputs
-        inputArtifacts.unite(usingsArtifacts);
         doApply(inputArtifacts, prepareScriptContext);
     } else { // apply the rule once for each input
-        foreach (Artifact * const inputArtifact, inputArtifacts + usingsArtifacts) {
+        foreach (Artifact * const inputArtifact, inputArtifacts) {
             ArtifactSet lst;
             lst += inputArtifact;
             doApply(lst, prepareScriptContext);
@@ -199,13 +178,9 @@ void RulesApplicator::doApply(const ArtifactSet &inputArtifacts, QScriptValue &p
         return;
 
     foreach (Artifact *outputArtifact, outputArtifacts) {
-        // insert the output artifacts into the pool of artifacts
-        foreach (const FileTag &fileTag, outputArtifact->fileTags)
-            m_artifactsPerFileTag[fileTag].insert(outputArtifact);
-
         // connect artifacts that match the file tags in explicitlyDependsOn
         foreach (const FileTag &fileTag, m_rule->explicitlyDependsOn)
-            foreach (Artifact *dependency, m_artifactsPerFileTag.value(fileTag))
+            foreach (Artifact *dependency, m_product->lookupArtifactsByFileTag(fileTag))
                 loggedConnect(outputArtifact, dependency, m_logger);
 
         m_transformer->outputs.insert(outputArtifact);
diff --git a/src/lib/corelib/buildgraph/rulesapplicator.h b/src/lib/corelib/buildgraph/rulesapplicator.h
index 334dfa084..f3223410c 100644
--- a/src/lib/corelib/buildgraph/rulesapplicator.h
+++ b/src/lib/corelib/buildgraph/rulesapplicator.h
@@ -46,16 +46,14 @@ class BuildGraphNode;
 class QtMocScanner;
 class ScriptEngine;
 
-typedef QHash<FileTag, ArtifactSet> ArtifactsPerFileTagMap;
-
 class RulesApplicator
 {
 public:
-    RulesApplicator(const ResolvedProductPtr &product, ArtifactsPerFileTagMap &artifactsPerFileTag,
-                    const Logger &logger);
+    RulesApplicator(const ResolvedProductPtr &product, const Logger &logger);
     ~RulesApplicator();
-    NodeSet applyRuleInEvaluationContext(const RuleConstPtr &rule);
-    void applyRule(const RuleConstPtr &rule);
+    NodeSet applyRuleInEvaluationContext(const RuleConstPtr &rule,
+            const ArtifactSet &inputArtifacts);
+    void applyRule(const RuleConstPtr &rule, const ArtifactSet &inputArtifacts);
     static void handleRemovedRuleOutputs(ArtifactSet artifactsToRemove, const Logger &logger);
 
 private:
@@ -75,9 +73,7 @@ private:
     QScriptValue scope() const;
 
     const ResolvedProductPtr m_product;
-    ArtifactsPerFileTagMap &m_artifactsPerFileTag;
     NodeSet m_createdArtifacts;
-
     RuleConstPtr m_rule;
     TransformerPtr m_transformer;
     QtMocScanner *m_mocScanner;