Fix the outer value for Properties items

The outerItem we created contained just one property and was quite
fragile when the 'outer' source code contained references to other
properties, especially whenever item inheritance was involved.

We now directly obtain the value of outer as soon an alternative
requests it.

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

4 files changed, 74 insertions, 28 deletions

diff --git a/src/lib/corelib/language/evaluatorscriptclass.cpp b/src/lib/corelib/language/evaluatorscriptclass.cpp
index fbc7b0193..cb88788f6 100644
--- a/src/lib/corelib/language/evaluatorscriptclass.cpp
+++ b/src/lib/corelib/language/evaluatorscriptclass.cpp
@@ -140,7 +140,8 @@ private:
         extraScope->setProperty(conveniencePropertyName, valueToSet);
     }
 
-    std::pair<QScriptValue, bool> createExtraScope(const JSSourceValue *value, Item *outerItem)
+    std::pair<QScriptValue, bool> createExtraScope(const JSSourceValue *value, Item *outerItem,
+                                                   QScriptValue *outerScriptValue)
     {
         std::pair<QScriptValue, bool> result;
         auto &extraScope = result.first;
@@ -154,14 +155,20 @@ private:
             }
             setupConvenienceProperty(QLatin1String("base"), &extraScope, baseValue);
         }
-        if (value->sourceUsesOuter() && outerItem) {
-            const QScriptValue v = data->evaluator->property(outerItem, *propertyName);
-            if (engine->hasErrorOrException(v)) {
-                extraScope = engine->lastErrorValue(v);
-                result.second = false;
-                return result;
+        if (value->sourceUsesOuter()) {
+            QScriptValue v;
+            if (outerItem) {
+                v = data->evaluator->property(outerItem, *propertyName);
+                if (engine->hasErrorOrException(v)) {
+                    extraScope = engine->lastErrorValue(v);
+                    result.second = false;
+                    return result;
+                }
+            } else if (outerScriptValue) {
+                v = *outerScriptValue;
             }
-            setupConvenienceProperty(QLatin1String("outer"), &extraScope, v);
+            if (v.isValid())
+                setupConvenienceProperty(QLatin1String("outer"), &extraScope, v);
         }
         if (value->sourceUsesOriginal()) {
             QScriptValue originalValue;
@@ -205,53 +212,55 @@ private:
 
     void handle(JSSourceValue *value) override
     {
-        Item *outerItem = data->item->outerItem();
+        QScriptValue outerScriptValue;
         for (const JSSourceValue::Alternative &alternative : value->alternatives()) {
-            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 (alternative.value->sourceUsesOuter()
+                    && !data->item->outerItem()
+                    && !outerScriptValue.isValid()) {
+                JSSourceValueEvaluationResult sver = evaluateJSSourceValue(value, nullptr);
+                if (sver.hasError) {
+                    *result = sver.scriptValue;
+                    return;
+                }
+                outerScriptValue = sver.scriptValue;
             }
             JSSourceValueEvaluationResult sver = evaluateJSSourceValue(alternative.value.get(),
-                                                                       outerItem, &alternative,
-                                                                       value);
-            if (!sver.tryNextAlternative) {
+                                                                       data->item->outerItem(),
+                                                                       &alternative,
+                                                                       value, &outerScriptValue);
+            if (!sver.tryNextAlternative || sver.hasError) {
                 *result = sver.scriptValue;
                 return;
             }
         }
-        *result = evaluateJSSourceValue(value, outerItem).scriptValue;
+        *result = evaluateJSSourceValue(value, data->item->outerItem()).scriptValue;
     }
 
     struct JSSourceValueEvaluationResult
     {
         QScriptValue scriptValue;
         bool tryNextAlternative = true;
+        bool hasError = false;
     };
 
     JSSourceValueEvaluationResult evaluateJSSourceValue(const JSSourceValue *value, Item *outerItem,
             const JSSourceValue::Alternative *alternative = nullptr,
-            JSSourceValue *elseCaseValue = nullptr)
+            JSSourceValue *elseCaseValue = nullptr, QScriptValue *outerScriptValue = nullptr)
     {
         JSSourceValueEvaluationResult result;
         QBS_ASSERT(!alternative || value == alternative->value.get(), return result);
         AutoScopePopper autoScopePopper(this);
-        auto maybeExtraScope = createExtraScope(value, outerItem);
+        auto maybeExtraScope = createExtraScope(value, outerItem, outerScriptValue);
         if (!maybeExtraScope.second) {
             result.scriptValue = maybeExtraScope.first;
-            result.tryNextAlternative = false;
+            result.hasError = true;
             return result;
         }
         const Evaluator::FileContextScopes fileCtxScopes
                 = data->evaluator->fileContextScopes(value->file());
         if (fileCtxScopes.importScope.isError()) {
             result.scriptValue = fileCtxScopes.importScope;
-            result.tryNextAlternative = false;
+            result.hasError = true;
             return result;
         }
         pushScope(fileCtxScopes.fileScope);
@@ -268,7 +277,7 @@ private:
             QScriptValue sv = engine->evaluate(alternative->condition);
             if (engine->hasErrorOrException(sv)) {
                 result.scriptValue = sv;
-                result.tryNextAlternative = false;
+                result.hasError = true;
                 return result;
             }
             if (sv.toBool()) {
@@ -282,7 +291,7 @@ private:
             sv = engine->evaluate(alternative->overrideListProperties);
             if (engine->hasErrorOrException(sv)) {
                 result.scriptValue = sv;
-                result.tryNextAlternative = false;
+                result.hasError = true;
                 return result;
             }
             if (sv.toBool())
diff --git a/tests/auto/language/testdata/propertiesblocks.qbs b/tests/auto/language/testdata/propertiesblocks.qbs
index c67612b28..d6a8dee23 100644
--- a/tests/auto/language/testdata/propertiesblocks.qbs
+++ b/tests/auto/language/testdata/propertiesblocks.qbs
@@ -46,6 +46,29 @@ Project {
             dummy.defines: outer.concat(["TWO"])
         }
     }
+    ProductBase {
+        name: "property_append_to_indirect_derived_outer1"
+        Properties {
+            condition: true
+            dummy.cFlags: outer.concat("PROPS")
+        }
+    }
+    ProductBase {
+        name: "property_append_to_indirect_derived_outer2"
+        Properties {
+            condition: true
+            dummy.cFlags: outer.concat("PROPS")
+        }
+        dummy.cFlags: ["PRODUCT"]
+    }
+    ProductBase {
+        name: "property_append_to_indirect_derived_outer3"
+        Properties {
+            condition: true
+            dummy.cFlags: outer.concat("PROPS")
+        }
+        dummy.cFlags: base.concat("PRODUCT")
+    }
     Product {
         name: "property_append_to_indirect_merged_outer"
         Depends { name: "dummy" }
diff --git a/tests/auto/language/testdata/propertiesblocks_base.qbs b/tests/auto/language/testdata/propertiesblocks_base.qbs
index 71b09a3da..d68c5127f 100644
--- a/tests/auto/language/testdata/propertiesblocks_base.qbs
+++ b/tests/auto/language/testdata/propertiesblocks_base.qbs
@@ -8,4 +8,6 @@ Product {
         dummy.defines: ["BASE"]
     }
     dummy.defines: ["SOMETHING"]
+    property stringList myCFlags: ["BASE"]
+    dummy.cFlags: myCFlags
 }
diff --git a/tests/auto/language/tst_language.cpp b/tests/auto/language/tst_language.cpp
index 694c5d0c4..cb3c79c4e 100644
--- a/tests/auto/language/tst_language.cpp
+++ b/tests/auto/language/tst_language.cpp
@@ -2113,6 +2113,18 @@ void TestLanguage::propertiesBlocks_data()
             << QString("dummy.defines")
             << QVariant(QStringList() << QString("ONE") << QString("TWO"))
             << QString();
+    QTest::newRow("property_append_to_indirect_derived_outer1")
+            << QString("dummy.cFlags")
+            << QVariant(QStringList() << QString("BASE") << QString("PROPS"))
+            << QString();
+    QTest::newRow("property_append_to_indirect_derived_outer2")
+            << QString("dummy.cFlags")
+            << QVariant(QStringList() << QString("PRODUCT") << QString("PROPS"))
+            << QString();
+    QTest::newRow("property_append_to_indirect_derived_outer3")
+            << QString("dummy.cFlags")
+            << QVariant(QStringList() << QString("BASE") << QString("PRODUCT") << QString("PROPS"))
+            << QString();
     QTest::newRow("property_append_to_indirect_merged_outer")
             << QString("dummy.rpaths")
             << QVariant(QStringList() << QString("ONE") << QString("TWO") << QString("$ORIGIN"))