Fix evaluation of Properties.condition

Use the same code path for the evaluation of Properties.condition and
the actual values.

Task-number: QBS-1240
Change-Id: I1cfb2e13349d73bf347bf90ee5aef4b0835630f6
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

4 files changed, 124 insertions, 111 deletions

diff --git a/src/lib/corelib/language/evaluatorscriptclass.cpp b/src/lib/corelib/language/evaluatorscriptclass.cpp
index d0c508037..fbc7b0193 100644
--- a/src/lib/corelib/language/evaluatorscriptclass.cpp
+++ b/src/lib/corelib/language/evaluatorscriptclass.cpp
@@ -101,6 +101,25 @@ public:
     }
 
 private:
+    friend class AutoScopePopper;
+
+    class AutoScopePopper
+    {
+    public:
+        AutoScopePopper(SVConverter *converter)
+            : m_converter(converter)
+        {
+        }
+
+        ~AutoScopePopper()
+        {
+            m_converter->popScopes();
+        }
+
+    private:
+        SVConverter *m_converter;
+    };
+
     void setupConvenienceProperty(const QString &conveniencePropertyName, QScriptValue *extraScope,
                                   const QScriptValue &scriptValue)
     {
@@ -186,100 +205,54 @@ private:
 
     void handle(JSSourceValue *value) override
     {
-        const Item *conditionScopeItem = 0;
-        QScriptValue conditionScope;
-        QScriptValue conditionFileScope;
         Item *outerItem = data->item->outerItem();
         for (const JSSourceValue::Alternative &alternative : value->alternatives()) {
-            const Evaluator::FileContextScopes fileCtxScopes
-                    = data->evaluator->fileContextScopes(value->file());
-            if (!conditionScopeItem) {
-                // We have to differentiate between module instances and normal items here.
-                //
-                // The module instance case:
-                // Product {
-                //     property bool something: true
-                //     Properties {
-                //         condition: something
-                //         cpp.defines: ["ABC"]
-                //     }
-                // }
-                //
-                // data->item points to cpp and the condition's scope chain must contain cpp's
-                // scope, which is the item where cpp is instantiated. The scope chain must not
-                // contain data->item itself.
-                //
-                // The normal item case:
-                // Product {
-                //     property bool something: true
-                //     property string value: "ABC"
-                //     Properties {
-                //         condition: something
-                //         value: "DEF"
-                //     }
-                // }
-                //
-                // data->item points to the product and the condition's scope chain must contain
-                // the product item.
-                conditionScopeItem = data->item->type() == ItemType::ModuleInstance
-                        ? data->item->scope() : data->item;
-                conditionScope = data->evaluator->scriptValue(conditionScopeItem);
-                QBS_ASSERT(conditionScope.isObject(), return);
-                conditionFileScope = fileCtxScopes.fileScope;
-            }
-            scriptContext->pushScope(conditionFileScope);
-            pushItemScopes(conditionScopeItem);
-            if (alternative.value->definingItem())
-                pushItemScopes(alternative.value->definingItem());
-            scriptContext->pushScope(conditionScope);
-            const QScriptValue &theImportScope = fileCtxScopes.importScope;
-            if (theImportScope.isError()) {
-                scriptContext->popScope();
-                scriptContext->popScope();
-                popScopes();
-                *result = theImportScope;
-                return;
+            if (alternative.value->sourceUsesOuter() && !outerItem) {
+                // Clone value but without alternatives.
+                JSSourceValuePtr outerValue =
+                        std::static_pointer_cast<JSSourceValue>(value->clone());
+                outerValue->setNext(ValuePtr());
+                outerValue->clearCreatedByPropertiesBlock();
+                outerValue->clearAlternatives();
+                outerItem = Item::create(data->item->pool(), ItemType::Outer);
+                outerItem->setProperty(propertyName->toString(), outerValue);
             }
-            scriptContext->pushScope(theImportScope);
-            const QScriptValue cr = engine->evaluate(alternative.condition);
-            const QScriptValue overrides = engine->evaluate(alternative.overrideListProperties);
-            scriptContext->popScope();
-            scriptContext->popScope();
-            scriptContext->popScope();
-            popScopes();
-            if (engine->hasErrorOrException(cr)) {
-                *result = engine->lastErrorValue(cr);
+            JSSourceValueEvaluationResult sver = evaluateJSSourceValue(alternative.value.get(),
+                                                                       outerItem, &alternative,
+                                                                       value);
+            if (!sver.tryNextAlternative) {
+                *result = sver.scriptValue;
                 return;
             }
-            if (cr.toBool()) {
-                // condition is true, let's use the value of this alternative
-                if (alternative.value->sourceUsesOuter() && !outerItem) {
-                    // Clone value but without alternatives.
-                    JSSourceValuePtr outerValue =
-                            std::static_pointer_cast<JSSourceValue>(value->clone());
-                    outerValue->setNext(ValuePtr());
-                    outerValue->clearCreatedByPropertiesBlock();
-                    outerValue->clearAlternatives();
-                    outerItem = Item::create(data->item->pool(), ItemType::Outer);
-                    outerItem->setProperty(propertyName->toString(), outerValue);
-                }
-                if (overrides.toBool())
-                    value->setIsExclusiveListValue();
-                value = alternative.value.get();
-                break;
-            }
         }
+        *result = evaluateJSSourceValue(value, outerItem).scriptValue;
+    }
 
+    struct JSSourceValueEvaluationResult
+    {
+        QScriptValue scriptValue;
+        bool tryNextAlternative = true;
+    };
+
+    JSSourceValueEvaluationResult evaluateJSSourceValue(const JSSourceValue *value, Item *outerItem,
+            const JSSourceValue::Alternative *alternative = nullptr,
+            JSSourceValue *elseCaseValue = nullptr)
+    {
+        JSSourceValueEvaluationResult result;
+        QBS_ASSERT(!alternative || value == alternative->value.get(), return result);
+        AutoScopePopper autoScopePopper(this);
         auto maybeExtraScope = createExtraScope(value, outerItem);
         if (!maybeExtraScope.second) {
-            *result = maybeExtraScope.first;
-            return;
+            result.scriptValue = maybeExtraScope.first;
+            result.tryNextAlternative = false;
+            return result;
         }
         const Evaluator::FileContextScopes fileCtxScopes
                 = data->evaluator->fileContextScopes(value->file());
         if (fileCtxScopes.importScope.isError()) {
-            *result = fileCtxScopes.importScope;
-            return;
+            result.scriptValue = fileCtxScopes.importScope;
+            result.tryNextAlternative = false;
+            return result;
         }
         pushScope(fileCtxScopes.fileScope);
         pushItemScopes(data->item);
@@ -291,9 +264,33 @@ private:
             pushItemScopes(value->definingItem());
         pushScope(maybeExtraScope.first);
         pushScope(fileCtxScopes.importScope);
-        *result = engine->evaluate(value->sourceCodeForEvaluation(), value->file()->filePath(),
-                                   value->line());
-        popScopes();
+        if (alternative) {
+            QScriptValue sv = engine->evaluate(alternative->condition);
+            if (engine->hasErrorOrException(sv)) {
+                result.scriptValue = sv;
+                result.tryNextAlternative = false;
+                return result;
+            }
+            if (sv.toBool()) {
+                // The condition is true. Continue evaluating the value.
+                result.tryNextAlternative = false;
+            } else {
+                // The condition is false. Try the next alternative or the else value.
+                result.tryNextAlternative = true;
+                return result;
+            }
+            sv = engine->evaluate(alternative->overrideListProperties);
+            if (engine->hasErrorOrException(sv)) {
+                result.scriptValue = sv;
+                result.tryNextAlternative = false;
+                return result;
+            }
+            if (sv.toBool())
+                elseCaseValue->setIsExclusiveListValue();
+        }
+        result.scriptValue = engine->evaluate(value->sourceCodeForEvaluation(),
+                                              value->file()->filePath(), value->line());
+        return result;
     }
 
     void handle(ItemValue *value) override
diff --git a/tests/auto/language/testdata/modules/module-with-properties-item/module-with-properties-item.qbs b/tests/auto/language/testdata/modules/module-with-properties-item/module-with-properties-item.qbs
new file mode 100644
index 000000000..3c2ab4ab3
--- /dev/null
+++ b/tests/auto/language/testdata/modules/module-with-properties-item/module-with-properties-item.qbs
@@ -0,0 +1,8 @@
+Module {
+    property bool boolProperty: true
+    property string stringProperty: "set in Module item"
+    Properties {
+        condition: boolProperty
+        stringProperty: "overridden in Properties item"
+    }
+}
diff --git a/tests/auto/language/testdata/propertiesblocks.qbs b/tests/auto/language/testdata/propertiesblocks.qbs
index 8566166d9..c67612b28 100644
--- a/tests/auto/language/testdata/propertiesblocks.qbs
+++ b/tests/auto/language/testdata/propertiesblocks.qbs
@@ -206,4 +206,8 @@ Project {
             dummy.defines: ["a string"]
         }
     }
+    Product {
+        name: "use-module-with-properties-item"
+        Depends { name: "module-with-properties-item" }
+    }
 }
diff --git a/tests/auto/language/tst_language.cpp b/tests/auto/language/tst_language.cpp
index 462a3e2af..694c5d0c4 100644
--- a/tests/auto/language/tst_language.cpp
+++ b/tests/auto/language/tst_language.cpp
@@ -2089,107 +2089,111 @@ void TestLanguage::productDirectories()
 void TestLanguage::propertiesBlocks_data()
 {
     QTest::addColumn<QString>("propertyName");
-    QTest::addColumn<QStringList>("expectedValues");
+    QTest::addColumn<QVariant>("expectedValue");
     QTest::addColumn<QString>("expectedStringValue");
 
-    QTest::newRow("init") << QString() << QStringList() << QString();
+    QTest::newRow("init") << QString() << QVariant() << QString();
     QTest::newRow("property_overwrite")
             << QString("dummy.defines")
-            << QStringList("OVERWRITTEN")
+            << QVariant(QStringList("OVERWRITTEN"))
             << QString();
     QTest::newRow("property_set_indirect")
             << QString("dummy.cFlags")
-            << QStringList("VAL")
+            << QVariant(QStringList("VAL"))
             << QString();
     QTest::newRow("property_overwrite_no_outer")
             << QString("dummy.defines")
-            << QStringList("OVERWRITTEN")
+            << QVariant(QStringList("OVERWRITTEN"))
             << QString();
     QTest::newRow("property_append_to_outer")
             << QString("dummy.defines")
-            << (QStringList() << QString("ONE") << QString("TWO"))
+            << QVariant(QStringList() << QString("ONE") << QString("TWO"))
             << QString();
     QTest::newRow("property_append_to_indirect_outer")
             << QString("dummy.defines")
-            << (QStringList() << QString("ONE") << QString("TWO"))
+            << QVariant(QStringList() << QString("ONE") << QString("TWO"))
             << QString();
     QTest::newRow("property_append_to_indirect_merged_outer")
             << QString("dummy.rpaths")
-            << (QStringList() << QString("ONE") << QString("TWO") << QString("$ORIGIN"))
+            << QVariant(QStringList() << QString("ONE") << QString("TWO") << QString("$ORIGIN"))
             << QString();
 
     QTest::newRow("multiple_exclusive_properties")
             << QString("dummy.defines")
-            << QStringList("OVERWRITTEN")
+            << QVariant(QStringList("OVERWRITTEN"))
             << QString();
     QTest::newRow("multiple_exclusive_properties_no_outer")
             << QString("dummy.defines")
-            << QStringList("OVERWRITTEN")
+            << QVariant(QStringList("OVERWRITTEN"))
             << QString();
     QTest::newRow("multiple_exclusive_properties_append_to_outer")
             << QString("dummy.defines")
-            << (QStringList() << QString("ONE") << QString("TWO"))
+            << QVariant(QStringList() << QString("ONE") << QString("TWO"))
             << QString();
 
     QTest::newRow("condition_refers_to_product_property")
             << QString("dummy.defines")
-            << QStringList("OVERWRITTEN")
+            << QVariant(QStringList("OVERWRITTEN"))
             << QString("OVERWRITTEN");
     QTest::newRow("condition_refers_to_project_property")
             << QString("dummy.defines")
-            << QStringList("OVERWRITTEN")
+            << QVariant(QStringList("OVERWRITTEN"))
             << QString("OVERWRITTEN");
 
     QTest::newRow("ambiguous_properties")
             << QString("dummy.defines")
-            << (QStringList() << QString("ONE") << QString("TWO"))
+            << QVariant(QStringList() << QString("ONE") << QString("TWO"))
             << QString();
     QTest::newRow("inheritance_overwrite_in_subitem")
             << QString("dummy.defines")
-            << (QStringList() << QString("OVERWRITTEN_IN_SUBITEM"))
+            << QVariant(QStringList() << QString("OVERWRITTEN_IN_SUBITEM"))
             << QString();
     QTest::newRow("inheritance_retain_base1")
             << QString("dummy.defines")
-            << (QStringList() << QString("BASE") << QString("SUB"))
+            << QVariant(QStringList() << QString("BASE") << QString("SUB"))
             << QString();
     QTest::newRow("inheritance_retain_base2")
             << QString("dummy.defines")
-            << (QStringList() << QString("BASE") << QString("SUB"))
+            << QVariant(QStringList() << QString("BASE") << QString("SUB"))
             << QString();
     QTest::newRow("inheritance_retain_base3")
             << QString("dummy.defines")
-            << (QStringList() << QString("BASE") << QString("SUB"))
+            << QVariant(QStringList() << QString("BASE") << QString("SUB"))
             << QString();
     QTest::newRow("inheritance_retain_base4")
             << QString("dummy.defines")
-            << (QStringList() << QString("BASE"))
+            << QVariant(QStringList() << QString("BASE"))
             << QString();
     QTest::newRow("inheritance_condition_in_subitem1")
             << QString("dummy.defines")
-            << (QStringList() << QString("SOMETHING") << QString("SUB"))
+            << QVariant(QStringList() << QString("SOMETHING") << QString("SUB"))
             << QString();
     QTest::newRow("inheritance_condition_in_subitem2")
             << QString("dummy.defines")
-            << (QStringList() << QString("SOMETHING"))
+            << QVariant(QStringList() << QString("SOMETHING"))
             << QString();
     QTest::newRow("condition_references_id")
             << QString("dummy.defines")
-            << (QStringList() << QString("OVERWRITTEN"))
+            << QVariant(QStringList() << QString("OVERWRITTEN"))
             << QString();
     QTest::newRow("using_derived_Properties_item") << "dummy.defines"
-            << (QStringList() << "string from MyProperties") << QString();
+            << QVariant(QStringList() << "string from MyProperties") << QString();
     QTest::newRow("conditional-depends")
             << QString("dummy.defines")
-            << QStringList()
+            << QVariant()
             << QString();
-    QTest::newRow("cleanup") << QString() << QStringList() << QString();
+    QTest::newRow("use-module-with-properties-item")
+            << QString("module-with-properties-item.stringProperty")
+            << QVariant(QString("overridden in Properties item"))
+            << QString();
+    QTest::newRow("cleanup") << QString() << QVariant() << QString();
 }
 
 void TestLanguage::propertiesBlocks()
 {
     HANDLE_INIT_CLEANUP_DATATAGS("propertiesblocks.qbs");
     QFETCH(QString, propertyName);
-    QFETCH(QStringList, expectedValues);
+    QFETCH(QVariant, expectedValue);
     QFETCH(QString, expectedStringValue);
     QVERIFY(!!project);
     QHash<QString, ResolvedProductPtr> products = productsFromProject(project);
@@ -2198,7 +2202,7 @@ void TestLanguage::propertiesBlocks()
     QVERIFY(!!product);
     QCOMPARE(product->name, productName);
     QVariant v = productPropertyValue(product, propertyName);
-    QCOMPARE(v.toStringList(), expectedValues);
+    QCOMPARE(v, expectedValue);
     if (!expectedStringValue.isEmpty()) {
         v = productPropertyValue(product, "someString");
         QCOMPARE(v.toString(), expectedStringValue);