Fix setting stringlist properties in module providers

...when using foo,bar,baz syntax

Change-Id: I013a55f02c5d6d4bbbccf809b9524bed3c486df4
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

9 files changed, 92 insertions, 65 deletions

diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 72e248433..3ffd0736e 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -3194,55 +3194,6 @@ QStringList &ModuleLoader::getModuleFileNames(const QString &dirPath)
     return moduleFileNames;
 }
 
-// returns QMetaType::UnknownType for types that do not need conversion
-static QMetaType::Type variantType(PropertyDeclaration::Type t)
-{
-    switch (t) {
-    case PropertyDeclaration::UnknownType:
-        break;
-    case PropertyDeclaration::Boolean:
-        return QMetaType::Bool;
-    case PropertyDeclaration::Integer:
-        return QMetaType::Int;
-    case PropertyDeclaration::Path:
-        return QMetaType::QString;
-    case PropertyDeclaration::PathList:
-        return QMetaType::QStringList;
-    case PropertyDeclaration::String:
-        return QMetaType::QString;
-    case PropertyDeclaration::StringList:
-        return QMetaType::QStringList;
-    case PropertyDeclaration::VariantList:
-        return QMetaType::QVariantList;
-    case PropertyDeclaration::Variant:
-        break;
-    }
-    return QMetaType::UnknownType;
-}
-
-static QVariant convertToPropertyType(const QVariant &v, PropertyDeclaration::Type t,
-    const QStringList &namePrefix, const QString &key)
-{
-    if (v.isNull() || !v.isValid())
-        return v;
-    const auto vt = variantType(t);
-    if (vt == QMetaType::UnknownType)
-        return v;
-
-    // Handle the foo,bar,bla stringlist syntax.
-    if (t == PropertyDeclaration::StringList && v.userType() == QMetaType::QString)
-        return v.toString().split(QLatin1Char(','));
-
-    QVariant c = v;
-    if (!qVariantConvert(c, vt)) {
-        QStringList name = namePrefix;
-        name << key;
-        throw ErrorInfo(Tr::tr("Value '%1' of property '%2' has incompatible type.")
-                        .arg(v.toString(), name.join(QLatin1Char('.'))));
-    }
-    return c;
-}
-
 static Item *findDeepestModuleInstance(Item *instance)
 {
     while (instance->prototype() && instance->prototype()->type() == ItemType::ModuleInstance)
@@ -3324,8 +3275,9 @@ std::pair<Item *, bool> ModuleLoader::getModulePrototype(ProductContext *product
             continue;
         }
         const PropertyDeclaration decl = module->propertyDeclaration(it.key());
-        VariantValuePtr v = VariantValue::create(convertToPropertyType(it.value(), decl.type(),
-                QStringList(fullModuleName), it.key()));
+        VariantValuePtr v = VariantValue::create(
+                PropertyDeclaration::convertToPropertyType(it.value(), decl.type(),
+                        QStringList(fullModuleName), it.key()));
         module->setProperty(it.key(), v);
     }
 
@@ -3813,8 +3765,8 @@ void ModuleLoader::overrideItemProperties(Item *item, const QString &buildConfig
             continue;
         }
         item->setProperty(it.key(),
-                VariantValue::create(convertToPropertyType(it.value(), decl.type(),
-                        QStringList(buildConfigKey), it.key())));
+                VariantValue::create(PropertyDeclaration::convertToPropertyType(
+                        it.value(), decl.type(), QStringList(buildConfigKey), it.key())));
     }
 }
 
diff --git a/src/lib/corelib/language/moduleproviderloader.cpp b/src/lib/corelib/language/moduleproviderloader.cpp
index f036e36ab..d8acad651 100644
--- a/src/lib/corelib/language/moduleproviderloader.cpp
+++ b/src/lib/corelib/language/moduleproviderloader.cpp
@@ -302,13 +302,18 @@ QStringList ModuleProviderLoader::getProviderSearchPaths(
                  BuiltinDeclarations::instance().nameForType(ItemType::ModuleProvider)));
     }
     providerItem->setParent(product.item);
+
+    // TODO: this looks like ModuleLoader::overrideItemProperties(), merge them?
     for (auto it = moduleConfig.begin(); it != moduleConfig.end(); ++it) {
         const PropertyDeclaration decl = providerItem->propertyDeclaration(it.key());
         if (!decl.isValid()) {
             throw ErrorInfo(Tr::tr("No such property '%1' in module provider '%2'.")
                             .arg(it.key(), name.toString()));
         }
-        providerItem->setProperty(it.key(), VariantValue::create(it.value()));
+        VariantValuePtr v = VariantValue::create(
+                PropertyDeclaration::convertToPropertyType(it.value(), decl.type(),
+                        QStringList(name), it.key()));
+        providerItem->setProperty(it.key(), v);
     }
     EvalContextSwitcher contextSwitcher(m_evaluator->engine(), EvalContext::ModuleProvider);
     return m_evaluator->stringListValue(providerItem, QStringLiteral("searchPaths"));
diff --git a/src/lib/corelib/language/propertydeclaration.cpp b/src/lib/corelib/language/propertydeclaration.cpp
index 14ed52d1e..2dbe41afd 100644
--- a/src/lib/corelib/language/propertydeclaration.cpp
+++ b/src/lib/corelib/language/propertydeclaration.cpp
@@ -42,14 +42,46 @@
 #include "deprecationinfo.h"
 #include <api/languageinfo.h>
 
+#include <logging/translator.h>
+
+#include <tools/error.h>
+#include <tools/qttools.h>
 #include <tools/stringconstants.h>
 
+#include <QtCore/qmetatype.h>
 #include <QtCore/qshareddata.h>
 #include <QtCore/qstringlist.h>
+#include <QtCore/qvariant.h>
 
 namespace qbs {
 namespace Internal {
 
+// returns QMetaType::UnknownType for types that do not need conversion
+static QMetaType::Type variantType(PropertyDeclaration::Type t)
+{
+    switch (t) {
+    case PropertyDeclaration::UnknownType:
+        break;
+    case PropertyDeclaration::Boolean:
+        return QMetaType::Bool;
+    case PropertyDeclaration::Integer:
+        return QMetaType::Int;
+    case PropertyDeclaration::Path:
+        return QMetaType::QString;
+    case PropertyDeclaration::PathList:
+        return QMetaType::QStringList;
+    case PropertyDeclaration::String:
+        return QMetaType::QString;
+    case PropertyDeclaration::StringList:
+        return QMetaType::QStringList;
+    case PropertyDeclaration::VariantList:
+        return QMetaType::QVariantList;
+    case PropertyDeclaration::Variant:
+        break;
+    }
+    return QMetaType::UnknownType;
+}
+
 class PropertyDeclarationData : public QSharedData
 {
 public:
@@ -243,5 +275,29 @@ void PropertyDeclaration::setDeprecationInfo(const DeprecationInfo &deprecationI
     d->deprecationInfo = deprecationInfo;
 }
 
+// see also: EvaluatorScriptClass::convertToPropertyType()
+QVariant PropertyDeclaration::convertToPropertyType(const QVariant &v, Type t,
+    const QStringList &namePrefix, const QString &key)
+{
+    if (v.isNull() || !v.isValid())
+        return v;
+    const auto vt = variantType(t);
+    if (vt == QMetaType::UnknownType)
+        return v;
+
+    // Handle the foo,bar,bla stringlist syntax.
+    if (t == PropertyDeclaration::StringList && v.userType() == QMetaType::QString)
+        return v.toString().split(QLatin1Char(','));
+
+    QVariant c = v;
+    if (!qVariantConvert(c, vt)) {
+        QStringList name = namePrefix;
+        name << key;
+        throw ErrorInfo(Tr::tr("Value '%1' of property '%2' has incompatible type.")
+                        .arg(v.toString(), name.join(QLatin1Char('.'))));
+    }
+    return c;
+}
+
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/corelib/language/propertydeclaration.h b/src/lib/corelib/language/propertydeclaration.h
index 77b6837f5..137315d14 100644
--- a/src/lib/corelib/language/propertydeclaration.h
+++ b/src/lib/corelib/language/propertydeclaration.h
@@ -43,6 +43,10 @@
 #include <QtCore/qshareddata.h>
 #include <QtCore/qstring.h>
 
+QT_BEGIN_NAMESPACE
+class QVariant;
+QT_END_NAMESPACE
+
 namespace qbs {
 namespace Internal {
 class DeprecationInfo;
@@ -113,6 +117,9 @@ public:
     const DeprecationInfo &deprecationInfo() const;
     void setDeprecationInfo(const DeprecationInfo &deprecationInfo);
 
+    static QVariant convertToPropertyType(
+            const QVariant &v, Type t, const QStringList &namePrefix, const QString &key);
+
 private:
     QSharedDataPointer<PropertyDeclarationData> d;
 };
diff --git a/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_a.qbs b/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_a.qbs
index 7985199c1..ab9d475d8 100644
--- a/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_a.qbs
+++ b/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_a.qbs
@@ -1,9 +1,9 @@
 import "../../qbs-module-providers-helpers.js" as Helpers
 
 ModuleProvider {
-    property string someProp: "provider_a"
+    property stringList someProp: "provider_a"
     relativeSearchPaths: {
-        Helpers.writeModule(outputBaseDir, "qbsmetatestmodule", someProp);
+        Helpers.writeModule(outputBaseDir, "qbsmetatestmodule", undefined, someProp);
         return "";
     }
 }
diff --git a/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_b.qbs b/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_b.qbs
index 58042d8d3..1b2a79979 100644
--- a/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_b.qbs
+++ b/tests/auto/blackbox/testdata/providers-properties/module-providers/provider_b.qbs
@@ -1,9 +1,9 @@
 import "../../qbs-module-providers-helpers.js" as Helpers
 
 ModuleProvider {
-    property string someProp: "provider_b"
+    property stringList someProp: "provider_b"
     relativeSearchPaths: {
-        Helpers.writeModule(outputBaseDir, "qbsothermodule", someProp);
+        Helpers.writeModule(outputBaseDir, "qbsothermodule", undefined, someProp);
         return "";
     }
 }
diff --git a/tests/auto/blackbox/testdata/providers-properties/providers-properties.qbs b/tests/auto/blackbox/testdata/providers-properties/providers-properties.qbs
index e29536c17..258a973fa 100644
--- a/tests/auto/blackbox/testdata/providers-properties/providers-properties.qbs
+++ b/tests/auto/blackbox/testdata/providers-properties/providers-properties.qbs
@@ -5,7 +5,8 @@ Product {
     Depends { name: "qbsothermodule" }
     moduleProviders.provider_a.someProp: "someValue"
     property bool dummy: {
-        console.info("p.qbsmetatestmodule.prop: " + qbsmetatestmodule.prop);
-        console.info("p.qbsothermodule.prop: " + qbsothermodule.prop);
+        console.info("p.qbsmetatestmodule.listProp: "
+                + JSON.stringify(qbsmetatestmodule.listProp));
+        console.info("p.qbsothermodule.listProp: " + JSON.stringify(qbsothermodule.listProp));
     }
 }
diff --git a/tests/auto/blackbox/testdata/qbs-module-providers-helpers.js b/tests/auto/blackbox/testdata/qbs-module-providers-helpers.js
index 2a2acfdcb..b33b87329 100644
--- a/tests/auto/blackbox/testdata/qbs-module-providers-helpers.js
+++ b/tests/auto/blackbox/testdata/qbs-module-providers-helpers.js
@@ -1,14 +1,19 @@
 var File = require("qbs.File");
 var FileInfo = require("qbs.FileInfo");
 var TextFile = require("qbs.TextFile");
+var ModUtils = require("qbs.ModUtils");
 
-function writeModule(outputBaseDir, name, prop) {
+function writeModule(outputBaseDir, name, prop, listProp) {
     console.info("Running setup script for " + name);
     var moduleDir = FileInfo.joinPaths(outputBaseDir, "modules", name);
     File.makePath(moduleDir);
     var module = new TextFile(FileInfo.joinPaths(moduleDir, "module.qbs"), TextFile.WriteOnly);
     module.writeLine("Module {");
-    module.writeLine("    property string prop: '" + prop + "'");
+    module.writeLine("    property string prop: " + ModUtils.toJSLiteral(prop));
+    if (listProp) {
+        module.writeLine("    property stringList listProp: "
+            + ModUtils.toJSLiteral(listProp));
+    }
     module.writeLine("}");
     module.close();
 }
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 75dec843b..3cacd67e9 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -5852,10 +5852,11 @@ void TestBlackbox::providersProperties()
     QDir::setCurrent(testDataDir + "/providers-properties");
 
     QbsRunParameters params("build");
-    params.arguments = QStringList("moduleProviders.provider_b.someProp: \"otherValue\"");
+    params.arguments = QStringList("moduleProviders.provider_b.someProp: \"first,second\"");
     QCOMPARE(runQbs(params), 0);
-    QVERIFY2(m_qbsStdout.contains("p.qbsmetatestmodule.prop: someValue"), m_qbsStdout);
-    QVERIFY2(m_qbsStdout.contains("p.qbsothermodule.prop: otherValue"), m_qbsStdout);
+    QVERIFY2(m_qbsStdout.contains("p.qbsmetatestmodule.listProp: [\"someValue\"]"), m_qbsStdout);
+    QVERIFY2(m_qbsStdout.contains(
+            "p.qbsothermodule.listProp: [\"first\",\"second\"]"), m_qbsStdout);
 }
 
 void TestBlackbox::pseudoMultiplexing()