Fix group-level module property evaluation for nested groups

The original fix for module properties in groups was done in the 1.6
branch, which does not have nested groups.

Task-number: QBS-1005
Change-Id: Ib173dc4d28458ffc0db3fc1e068be86b41096357
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

3 files changed, 181 insertions, 21 deletions

diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index f6257408d..bcdad9cdf 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -1206,45 +1206,68 @@ void ModuleLoader::adjustDefiningItemsInGroupModuleInstances(const Item::Module
 {
     // There are three cases:
     //     a) The defining item is the "main" module instance, i.e. the one instantiated in the
-    //        product directly.
-    //     b) The defining item refers to the module prototype.
+    //        product directly (or a parent group).
+    //     b) The defining item refers to the module prototype (or the replacement of it
+    //        created in the module merger [for products] or in this function [for parent groups]).
     //     c) The defining item is a different instance of the module, i.e. it was instantiated
     //        in some other module.
-    // TODO: Needs adjustments for nested groups.
 
     QHash<Item *, Item *> definingItemReplacements;
-    const Item::PropertyMap &protoProps = module.item->prototype()->properties();
-    for (auto propIt = protoProps.begin(); propIt != protoProps.end(); ++propIt) {
-        const QString &propName = propIt.key();
+
+    Item *modulePrototype = module.item->prototype();
+    while (modulePrototype->prototype())
+        modulePrototype = modulePrototype->prototype();
+
+    // TODO: Why are there module instances whose top-level prototype is of type ModuleInstance?
+    // QBS_CHECK(modulePrototype->type() == ItemType::Module);
+
+    const Item::PropertyDeclarationMap &propDecls = modulePrototype->propertyDeclarations();
+    for (const auto &decl : propDecls) {
+        const QString &propName = decl.name();
 
         // Module properties assigned in the group are not relevant here, as nothing
         // gets inherited in that case. In particular, setting a list property
-        // overwrites the value from the product's (merged) instance completely,
+        // overwrites the value from the product's (or parent group's) instance completely,
         // rather than appending to it (concatenation happens via outer.concat()).
-        if (module.item->properties().contains(propIt.key()))
+        ValueConstPtr propValue = module.item->properties().value(propName);
+        if (propValue)
             continue;
 
-        if (propIt.value()->type() != Value::JSSourceValueType)
+        // Find the nearest prototype instance that has the value assigned.
+        // The result is either an instance of a parent group (or the parent group's
+        // parent group and so on) or the instance of the product or the module prototype.
+        // In the latter case, we don't have to do anything.
+        const Item *instanceWithProperty = module.item;
+        int prototypeChainLen = 0;
+        do {
+            instanceWithProperty = instanceWithProperty->prototype();
+            ++prototypeChainLen;
+            propValue = instanceWithProperty->properties().value(propName);
+        } while (!propValue);
+        QBS_CHECK(propValue);
+
+        if (propValue->type() != Value::JSSourceValueType)
             continue;
 
-        const PropertyDeclaration &propDecl = module.item->propertyDeclaration(propName);
         bool hasDefiningItem = false;
-        for (ValuePtr v = propIt.value(); v && !hasDefiningItem; v = v->next())
+        for (ValueConstPtr v = propValue; v && !hasDefiningItem; v = v->next())
             hasDefiningItem = v->definingItem();
         if (!hasDefiningItem) {
-            QBS_CHECK(propDecl.isScalar());
+            QBS_CHECK(decl.isScalar() || instanceWithProperty == modulePrototype);
             continue;
         }
 
-        const ValuePtr clonedValue = propIt.value()->clone();
+        const ValuePtr clonedValue = propValue->clone();
         for (ValuePtr v = clonedValue; v; v = v->next()) {
             QBS_CHECK(v->definingItem());
 
             Item *& replacement = definingItemReplacements[v->definingItem()];
-            if (v->definingItem() == module.item->prototype()) {
+            static const QString caseA = QLatin1String("__group_case_a");
+            if (v->definingItem() == instanceWithProperty
+                    || v->definingItem()->variantProperty(caseA)) {
                 // Case a)
-                // For values whose defining item is the product's instance, we take its scope
-                // and replace references to module instances with those from the
+                // For values whose defining item is the product's (or parent group's) instance,
+                // we take its scope and replace references to module instances with those from the
                 // group's instance. This handles cases like the following:
                 // Product {
                 //    name: "theProduct"
@@ -1272,14 +1295,15 @@ void ModuleLoader::adjustDefiningItemsInGroupModuleInstances(const Item::Module
                             scopeScope->setProperty(propIt.key(), propIt.value());
                     }
                 }
-                replacement->setPropertyDeclaration(propName, propDecl);
+                replacement->setPropertyDeclaration(propName, decl);
                 replacement->setProperty(propName, v);
+                replacement->setProperty(caseA, VariantValue::create(QVariant()));
             }  else if (v->definingItem()->type() == ItemType::Module) {
                 // Case b)
                 // For values whose defining item is the module prototype, we change the scope to
                 // the group's instance, analogous to what we do in
                 // ModuleMerger::appendPrototypeValueToNextChain().
-                QBS_CHECK(!propDecl.isScalar());
+                QBS_CHECK(!decl.isScalar());
                 QBS_CHECK(!v->next());
                 Item *& replacement = definingItemReplacements[v->definingItem()];
                 if (!replacement) {
@@ -1287,6 +1311,7 @@ void ModuleLoader::adjustDefiningItemsInGroupModuleInstances(const Item::Module
                                                         ItemType::Module);
                     replacement->setScope(module.item);
                 }
+                QBS_CHECK(!replacement->hasOwnProperty(caseA));
                 if (m_logger.traceEnabled()) {
                     m_logger.qbsTrace() << "[LDR] replacing defining item for prototype; module is "
                             << module.name.toString() << module.item
@@ -1298,7 +1323,7 @@ void ModuleLoader::adjustDefiningItemsInGroupModuleInstances(const Item::Module
                             << ", value source code is "
                             << v.staticCast<JSSourceValue>()->sourceCode().toString();
                 }
-                replacement->setPropertyDeclaration(propName, propDecl);
+                replacement->setPropertyDeclaration(propName, decl);
                 replacement->setProperty(propName, v);
             } else {
                 // Look for instance scopes of other module instances in defining items and
@@ -1311,7 +1336,10 @@ void ModuleLoader::adjustDefiningItemsInGroupModuleInstances(const Item::Module
                 for (auto depIt = dependentModules.cbegin(); depIt != dependentModules.cend();
                      ++depIt) {
                     const Item::Module &depMod = *depIt;
-                    if (v->definingItem()->scope()->scope() != depMod.item->prototype())
+                    const Item *depModPrototype = depMod.item->prototype();
+                    for (int i = 1; i < prototypeChainLen; ++i)
+                        depModPrototype = depModPrototype->prototype();
+                    if (v->definingItem()->scope()->scope() != depModPrototype)
                         continue;
 
                     found = true;
@@ -1325,6 +1353,7 @@ void ModuleLoader::adjustDefiningItemsInGroupModuleInstances(const Item::Module
                         replacement->setScope(Item::create(v->definingItem()->pool()));
                         replacement->scope()->setScope(depMod.item);
                     }
+                    QBS_CHECK(!replacement->hasOwnProperty(caseA));
                     if (m_logger.traceEnabled()) {
                         m_logger.qbsTrace() << "[LDR] reset instance scope of module "
                                 << depMod.name.toString() << " in property "
diff --git a/src/lib/corelib/language/testdata/modulepropertiesingroups.qbs b/src/lib/corelib/language/testdata/modulepropertiesingroups.qbs
index 6b1976e10..7a0a99b3e 100644
--- a/src/lib/corelib/language/testdata/modulepropertiesingroups.qbs
+++ b/src/lib/corelib/language/testdata/modulepropertiesingroups.qbs
@@ -12,7 +12,6 @@ Project {
         gmod.gmod1.gmod1_list3: ["product"]
         gmod.gmod1.p1: 1
 
-        // TODO: Also test nested groups in >= 1.7
         Group  {
             name: "g1"
             files: ["Banana"]
@@ -24,6 +23,28 @@ Project {
             gmod2.commonName: "g1"
             gmod3.gmod3_string: "g1_gmod3"
             gmod4.gmod4_string: "g1_gmod4"
+
+            Group {
+                name: "g1.1"
+
+                gmod.gmod1.gmod1_string: name
+                gmod.gmod1.gmod1_list2: outer.concat([name])
+                gmod.gmod1.p2: 4
+                gmod2.prop: 2
+                gmod2.commonName: name
+                gmod3.gmod3_string: "g1.1_gmod3"
+                gmod4.gmod4_string: "g1.1_gmod4"
+            }
+
+            Group {
+                name: "g1.2"
+
+                gmod.gmod1.gmod1_string: name
+                gmod.gmod1.gmod1_list2: outer.concat([name])
+                gmod.gmod1.p2: 8
+                gmod2.commonName: name
+                gmod3.gmod3_string: "g1.2_gmod3"
+            }
         }
 
         Group  {
@@ -36,6 +57,17 @@ Project {
             gmod2.prop: 2
             gmod3.gmod3_string: name + "_gmod3"
             gmod4.gmod4_string: name + "_gmod4"
+
+            Group {
+                name: "g2.1"
+
+                Group {
+                    name: "g2.1.1"
+
+                    gmod.gmod1.gmod1_list2: [name]
+                    gmod.gmod1.p2: 15
+                }
+            }
         }
     }
 }
diff --git a/src/lib/corelib/language/tst_language.cpp b/src/lib/corelib/language/tst_language.cpp
index 03ca80047..3af8e51d8 100644
--- a/src/lib/corelib/language/tst_language.cpp
+++ b/src/lib/corelib/language/tst_language.cpp
@@ -1172,15 +1172,31 @@ void TestLanguage::modulePropertiesInGroups()
         const ResolvedProductPtr product = products.value("grouptest");
         QVERIFY(product);
         GroupConstPtr g1;
+        GroupConstPtr g11;
+        GroupConstPtr g12;
         GroupConstPtr g2;
+        GroupConstPtr g21;
+        GroupConstPtr g211;
         foreach (const GroupConstPtr &g, product->groups) {
             if (g->name == "g1")
                 g1= g;
             else if (g->name == "g2")
                 g2 = g;
+            else if (g->name == "g1.1")
+                g11 = g;
+            else if (g->name == "g1.2")
+                g12 = g;
+            else if (g->name == "g2.1")
+                g21 = g;
+            else if (g->name == "g2.1.1")
+                g211 = g;
         }
         QVERIFY(g1);
         QVERIFY(g2);
+        QVERIFY(g11);
+        QVERIFY(g12);
+        QVERIFY(g21);
+        QVERIFY(g211);
 
         const QVariantMap productProps = product->moduleProperties->value();
         PropertyFinder pf;
@@ -1230,6 +1246,50 @@ void TestLanguage::modulePropertiesInGroups()
         QCOMPARE(g1Gmod2List, QStringList() << "g1" << "commonName_in_gmod1" << "g1_gmod4_g1_gmod3"
                  << "g1_gmod3" << "gmod2_list_proto");
 
+        const QVariantMap g11Props = g11->properties->value();
+        const auto &g11Gmod1List1 = pf.propertyValue(g11Props, "gmod.gmod1", "gmod1_list1")
+                .toStringList();
+        QCOMPARE(g11Gmod1List1, QStringList() << "gmod1_list1_proto" << "g1.1");
+        const auto &g11Gmod1List2 = pf.propertyValue(g11Props, "gmod.gmod1", "gmod1_list2")
+                .toStringList();
+        QCOMPARE(g11Gmod1List2, QStringList() << "grouptest" << "gmod1_string_proto"
+                 << "gmod1_list2_proto" << "g1" << "g1.1");
+        const auto &g11Gmod1List3 = pf.propertyValue(g11Props, "gmod.gmod1", "gmod1_list3")
+                .toStringList();
+        QCOMPARE(g11Gmod1List3, QStringList() << "product" << "g1.1");
+        const int g11P0 = pf.propertyValue(g11Props, "gmod.gmod1", "p0").toInt();
+        QCOMPARE(g11P0, 5);
+        const int g11DepProp = pf.propertyValue(g11Props, "gmod.gmod1", "depProp").toInt();
+        QCOMPARE(g11DepProp, 2);
+        const auto &g11Gmod2String = pf.propertyValue(g11Props, "gmod2", "gmod2_string").toString();
+        QCOMPARE(g11Gmod2String, QString("g1.1"));
+        const auto &g11Gmod2List = pf.propertyValue(g11Props, "gmod2", "gmod2_list")
+                .toStringList();
+        QCOMPARE(g11Gmod2List, QStringList() << "g1.1" << "commonName_in_gmod1"
+                 << "g1.1_gmod4_g1.1_gmod3" << "g1.1_gmod3" << "gmod2_list_proto");
+
+        const QVariantMap g12Props = g12->properties->value();
+        const auto &g12Gmod1List1 = pf.propertyValue(g12Props, "gmod.gmod1", "gmod1_list1")
+                .toStringList();
+        QCOMPARE(g12Gmod1List1, QStringList() << "gmod1_list1_proto" << "g1.2");
+        const auto &g12Gmod1List2 = pf.propertyValue(g12Props, "gmod.gmod1", "gmod1_list2")
+                .toStringList();
+        QCOMPARE(g12Gmod1List2, QStringList() << "grouptest" << "gmod1_string_proto"
+                 << "gmod1_list2_proto" << "g1" << "g1.2");
+        const auto &g12Gmod1List3 = pf.propertyValue(g12Props, "gmod.gmod1", "gmod1_list3")
+                .toStringList();
+        QCOMPARE(g12Gmod1List3, QStringList() << "product" << "g1.2");
+        const int g12P0 = pf.propertyValue(g12Props, "gmod.gmod1", "p0").toInt();
+        QCOMPARE(g12P0, 9);
+        const int g12DepProp = pf.propertyValue(g12Props, "gmod.gmod1", "depProp").toInt();
+        QCOMPARE(g12DepProp, 1);
+        const auto &g12Gmod2String = pf.propertyValue(g12Props, "gmod2", "gmod2_string").toString();
+        QCOMPARE(g12Gmod2String, QString("g1.2"));
+        const auto &g12Gmod2List = pf.propertyValue(g12Props, "gmod2", "gmod2_list")
+                .toStringList();
+        QCOMPARE(g12Gmod2List, QStringList() << "g1.2" << "commonName_in_gmod1"
+                 << "g1_gmod4_g1.2_gmod3" << "g1.2_gmod3" << "gmod2_list_proto");
+
         const QVariantMap g2Props = g2->properties->value();
         const auto &g2Gmod1List1 = pf.propertyValue(g2Props, "gmod.gmod1", "gmod1_list1")
                 .toStringList();
@@ -1247,6 +1307,45 @@ void TestLanguage::modulePropertiesInGroups()
                 .toStringList();
         QCOMPARE(g2Gmod2List, QStringList() << "g2" << "commonName_in_gmod1" << "g2_gmod4_g2_gmod3"
                  << "g2_gmod3" << "gmod2_list_proto");
+
+        const QVariantMap g21Props = g21->properties->value();
+        const auto &g21Gmod1List1 = pf.propertyValue(g21Props, "gmod.gmod1", "gmod1_list1")
+                .toStringList();
+        QCOMPARE(g21Gmod1List1, QStringList() << "gmod1_list1_proto" << "g2");
+        const auto &g21Gmod1List2 = pf.propertyValue(g21Props, "gmod.gmod1", "gmod1_list2")
+                .toStringList();
+        QEXPECT_FAIL(0, "no re-eval when no module props set", Continue);
+        QCOMPARE(g21Gmod1List2, QStringList() << "grouptest" << "g2.1" << "gmod1_list2_proto");
+        const int g21P0 = pf.propertyValue(g21Props, "gmod.gmod1", "p0").toInt();
+        QCOMPARE(g21P0, 6);
+        const int g21DepProp = pf.propertyValue(g21Props, "gmod.gmod1", "depProp").toInt();
+        QCOMPARE(g21DepProp, 2);
+        const auto &g21Gmod2String = pf.propertyValue(g21Props, "gmod2", "gmod2_string").toString();
+        QCOMPARE(g21Gmod2String, QString("g2"));
+        const auto &g21Gmod2List = pf.propertyValue(g21Props, "gmod2", "gmod2_list")
+                .toStringList();
+        QEXPECT_FAIL(0, "no re-eval when no module props set", Continue);
+        QCOMPARE(g21Gmod2List, QStringList() << "g2" << "commonName_in_gmod1"
+                 << "g2.1_gmod4_g2.1_gmod3" << "g2.1_gmod3" << "gmod2_list_proto");
+
+        const QVariantMap g211Props = g211->properties->value();
+        const auto &g211Gmod1List1 = pf.propertyValue(g211Props, "gmod.gmod1", "gmod1_list1")
+                .toStringList();
+        QCOMPARE(g211Gmod1List1, QStringList() << "gmod1_list1_proto" << "g2");
+        const auto &g211Gmod1List2 = pf.propertyValue(g211Props, "gmod.gmod1", "gmod1_list2")
+                .toStringList();
+        QCOMPARE(g211Gmod1List2, QStringList() << "g2.1.1");
+        const int g211P0 = pf.propertyValue(g211Props, "gmod.gmod1", "p0").toInt();
+        QCOMPARE(g211P0, 17);
+        const int g211DepProp = pf.propertyValue(g211Props, "gmod.gmod1", "depProp").toInt();
+        QCOMPARE(g211DepProp, 2);
+        const auto &g211Gmod2String
+                = pf.propertyValue(g211Props, "gmod2", "gmod2_string").toString();
+        QCOMPARE(g211Gmod2String, QString("g2.1.1"));
+        const auto &g211Gmod2List = pf.propertyValue(g211Props, "gmod2", "gmod2_list")
+                .toStringList();
+        QCOMPARE(g211Gmod2List, QStringList() << "g2.1.1" << "commonName_in_gmod1"
+                 << "g2.1.1_gmod4_g2.1.1_gmod3" << "g2.1.1_gmod3" << "gmod2_list_proto");
     } catch (const ErrorInfo &e) {
         exceptionCaught = true;
         qDebug() << e.toString();