Implement missing check for allowed values in PropertyOptions

========== Performance data for Resolving ==========
    Old instruction count: 10195378481
    New instruction count: 10238464294
    Relative change: 0 %

[ChangeLog] Qbs now checks string and stringList values according to the
allowedValues property in PropertyOptions

Change-Id: Ide88987c74b35f4172ffaf71aacd991536131ee5
Reviewed-by: Richard Weickelt <richard@weickelt.de>
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

12 files changed, 170 insertions, 13 deletions

diff --git a/share/qbs/modules/cpp/CppModule.qbs b/share/qbs/modules/cpp/CppModule.qbs
index b2897c30c..ce672b8e7 100644
--- a/share/qbs/modules/cpp/CppModule.qbs
+++ b/share/qbs/modules/cpp/CppModule.qbs
@@ -314,14 +314,12 @@ Module {
     property stringList cLanguageVersion
     PropertyOptions {
         name: "cLanguageVersion"
-        allowedValues: ["c89", "c99", "c11"]
         description: "The version of the C standard with which the code must comply."
     }
 
     property stringList cxxLanguageVersion
     PropertyOptions {
         name: "cxxLanguageVersion"
-        allowedValues: ["c++98", "c++11", "c++14", "c++17"]
         description: "The version of the C++ standard with which the code must comply."
     }
 
diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 4eefe9d47..5c4033c3f 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -90,14 +90,6 @@ namespace Internal {
 using MultiplexConfigurationByIdTable = QThreadStorage<QHash<QString, QVariantMap> >;
 Q_GLOBAL_STATIC(MultiplexConfigurationByIdTable, multiplexConfigurationsById);
 
-static void handlePropertyError(const ErrorInfo &error, const SetupProjectParameters &params,
-                                Logger &logger)
-{
-    if (params.propertyCheckingMode() == ErrorHandlingMode::Strict)
-        throw error;
-    logger.printWarning(error);
-}
-
 static bool multiplexConfigurationIntersects(const QVariantMap &lhs, const QVariantMap &rhs)
 {
     QBS_CHECK(!lhs.isEmpty() && !rhs.isEmpty());
@@ -1767,6 +1759,9 @@ void ModuleLoader::handlePropertyOptions(Item *optionsItem)
                 optionsItem, StringConstants::descriptionProperty());
     const auto removalVersion = Version::fromString(m_evaluator->stringValue(optionsItem,
             StringConstants::removalVersionProperty()));
+    const auto allowedValues = m_evaluator->stringListValue(
+                optionsItem, StringConstants::allowedValuesProperty());
+
     PropertyDeclaration decl = optionsItem->parent()->propertyDeclaration(name);
     if (!decl.isValid()) {
         decl.setName(name);
@@ -1777,6 +1772,7 @@ void ModuleLoader::handlePropertyOptions(Item *optionsItem)
         DeprecationInfo di(removalVersion, description);
         decl.setDeprecationInfo(di);
     }
+    decl.setAllowedValues(allowedValues);
     const ValuePtr property = optionsItem->parent()->property(name);
     if (!property && !decl.isExpired()) {
         throw ErrorInfo(Tr::tr("PropertyOptions item refers to non-existing property '%1'")
diff --git a/src/lib/corelib/language/projectresolver.cpp b/src/lib/corelib/language/projectresolver.cpp
index 4e8e94b4e..570b3513f 100644
--- a/src/lib/corelib/language/projectresolver.cpp
+++ b/src/lib/corelib/language/projectresolver.cpp
@@ -1778,6 +1778,7 @@ void ProjectResolver::evaluateProperty(const Item *item, const QString &propName
         } else if (pd.type() == PropertyDeclaration::VariantList) {
             v = v.toList();
         }
+        checkAllowedValues(v, propValue->location(), pd, propName);
         result[propName] = v;
         break;
     }
@@ -1788,15 +1789,52 @@ void ProjectResolver::evaluateProperty(const Item *item, const QString &propName
         VariantValuePtr vvp = std::static_pointer_cast<VariantValue>(propValue);
         QVariant v = vvp->value();
 
-        if (v.isNull() && !item->propertyDeclaration(propName).isScalar()) // QTBUG-51237
+        const PropertyDeclaration pd = item->propertyDeclaration(propName);
+        if (v.isNull() && !pd.isScalar()) // QTBUG-51237
             v = QStringList();
 
+        checkAllowedValues(v, propValue->location(), pd, propName);
         result[propName] = v;
         break;
     }
     }
 }
 
+void ProjectResolver::checkAllowedValues(
+        const QVariant &value, const CodeLocation &loc, const PropertyDeclaration &decl,
+        const QString &key) const
+{
+    const auto type = decl.type();
+    if (type != PropertyDeclaration::String && type != PropertyDeclaration::StringList)
+        return;
+
+    if (value.isNull())
+        return;
+
+    const auto &allowedValues = decl.allowedValues();
+    if (allowedValues.isEmpty())
+        return;
+
+    const auto checkValue = [this, &loc, &allowedValues, &key](const QString &value)
+    {
+        if (!allowedValues.contains(value)) {
+            const auto message = Tr::tr("Value '%1' is not allowed for property '%2'.")
+                    .arg(value, key);
+            ErrorInfo error(message, loc);
+            handlePropertyError(error, m_setupParams, m_logger);
+        }
+    };
+
+    if (type == PropertyDeclaration::StringList) {
+        const auto strings = value.toStringList();
+        for (const auto &string: strings) {
+            checkValue(string);
+        }
+    } else if (type == PropertyDeclaration::String) {
+        checkValue(value.toString());
+    }
+}
+
 void ProjectResolver::collectPropertiesForExportItem(const QualifiedId &moduleName,
          const ValuePtr &value, Item *moduleInstance, QVariantMap &moduleProps)
 {
diff --git a/src/lib/corelib/language/projectresolver.h b/src/lib/corelib/language/projectresolver.h
index a1e24a555..73f3442ac 100644
--- a/src/lib/corelib/language/projectresolver.h
+++ b/src/lib/corelib/language/projectresolver.h
@@ -132,6 +132,9 @@ private:
                                    bool checkErrors);
     void evaluateProperty(const Item *item, const QString &propName, const ValuePtr &propValue,
                           QVariantMap &result, bool checkErrors);
+    void checkAllowedValues(
+            const QVariant &v, const CodeLocation &loc, const PropertyDeclaration &decl,
+            const QString &key) const;
     void createProductConfig(ResolvedProduct *product);
     ProjectContext createProjectContext(ProjectContext *parentProjectContext) const;
     void adaptExportedPropertyValues(const Item *shadowProductItem);
diff --git a/src/lib/corelib/language/propertydeclaration.cpp b/src/lib/corelib/language/propertydeclaration.cpp
index abe6a1626..14ed52d1e 100644
--- a/src/lib/corelib/language/propertydeclaration.cpp
+++ b/src/lib/corelib/language/propertydeclaration.cpp
@@ -62,6 +62,7 @@ public:
     QString name;
     PropertyDeclaration::Type type;
     PropertyDeclaration::Flags flags;
+    QStringList allowedValues;
     QString description;
     QString initialValueSource;
     QStringList functionArgumentNames;
@@ -182,6 +183,16 @@ void PropertyDeclaration::setFlags(Flags f)
     d->flags = f;
 }
 
+const QStringList &PropertyDeclaration::allowedValues() const
+{
+    return d->allowedValues;
+}
+
+void PropertyDeclaration::setAllowedValues(const QStringList &v)
+{
+    d->allowedValues = v;
+}
+
 const QString &PropertyDeclaration::description() const
 {
     return d->description;
diff --git a/src/lib/corelib/language/propertydeclaration.h b/src/lib/corelib/language/propertydeclaration.h
index 874275bde..5ced7eccb 100644
--- a/src/lib/corelib/language/propertydeclaration.h
+++ b/src/lib/corelib/language/propertydeclaration.h
@@ -100,6 +100,9 @@ public:
     Flags flags() const;
     void setFlags(Flags f);
 
+    const QStringList &allowedValues() const;
+    void setAllowedValues(const QStringList &v);
+
     const QString &description() const;
     void setDescription(const QString &str);
 
diff --git a/src/lib/corelib/tools/error.cpp b/src/lib/corelib/tools/error.cpp
index ff7ce46cf..fe5027313 100644
--- a/src/lib/corelib/tools/error.cpp
+++ b/src/lib/corelib/tools/error.cpp
@@ -42,6 +42,8 @@
 #include "persistence.h"
 #include "qttools.h"
 #include "stringconstants.h"
+#include "setupprojectparameters.h"
+#include "logging/logger.h"
 
 #include <QtCore/qjsonarray.h>
 #include <QtCore/qjsonobject.h>
@@ -325,4 +327,12 @@ void appendError(ErrorInfo &dst, const ErrorInfo &src)
         dst.append(item);
 }
 
+void handlePropertyError(
+        const ErrorInfo &error, const SetupProjectParameters &params, Internal::Logger &logger)
+{
+    if (params.propertyCheckingMode() == ErrorHandlingMode::Strict)
+        throw error;
+    logger.printWarning(error);
+}
+
 } // namespace qbs
diff --git a/src/lib/corelib/tools/error.h b/src/lib/corelib/tools/error.h
index 36cf5e0ea..3ae9399f1 100644
--- a/src/lib/corelib/tools/error.h
+++ b/src/lib/corelib/tools/error.h
@@ -54,9 +54,14 @@ class QStringList;
 QT_END_NAMESPACE
 
 namespace qbs {
-namespace Internal { class PersistentPool; }
+namespace Internal {
+class PersistentPool;
+class Logger;
+} // namespace Internal
 class CodeLocation;
 
+class SetupProjectParameters;
+
 class QBS_EXPORT ErrorItem
 {
     friend class ErrorInfo;
@@ -120,6 +125,9 @@ private:
 };
 
 void appendError(ErrorInfo &dst, const ErrorInfo &src);
+void handlePropertyError(
+        const ErrorInfo &error, const SetupProjectParameters &params, Internal::Logger &logger);
+
 inline uint qHash(const ErrorInfo &e) { return qHash(e.toString()); }
 inline bool operator==(const ErrorInfo &e1, const ErrorInfo &e2) {
     return e1.toString() == e2.toString();
diff --git a/tests/auto/blackbox/testdata/allowed-values/allowed-values.qbs b/tests/auto/blackbox/testdata/allowed-values/allowed-values.qbs
new file mode 100644
index 000000000..699713770
--- /dev/null
+++ b/tests/auto/blackbox/testdata/allowed-values/allowed-values.qbs
@@ -0,0 +1,19 @@
+Product {
+    Depends { name: "a" }
+    // tests VariantValue
+    property string prop
+    PropertyOptions {
+        name: "prop"
+        description: "Some prop"
+        allowedValues: "foo"
+    }
+    // tests JSValue
+    property string prop2 // setter for otherProp
+    property string otherProp: prop2
+    PropertyOptions {
+        name: "otherProp"
+        description: "Some other prop"
+        allowedValues: "foo"
+    }
+    name: "p"
+}
diff --git a/tests/auto/blackbox/testdata/allowed-values/modules/a/a.qbs b/tests/auto/blackbox/testdata/allowed-values/modules/a/a.qbs
new file mode 100644
index 000000000..2bbcde525
--- /dev/null
+++ b/tests/auto/blackbox/testdata/allowed-values/modules/a/a.qbs
@@ -0,0 +1,18 @@
+Module {
+    // tests VariantValue
+    property stringList prop
+    PropertyOptions {
+        name: "prop"
+        description: "Some prop"
+        allowedValues: ["foo", "bar"]
+    }
+    // tests JSValue
+    property stringList prop2 // setter for otherProp
+    property stringList otherProp: prop2
+    PropertyOptions {
+        name: "otherProp"
+        description: "Some other prop"
+        allowedValues: ["foo", "bar"]
+    }
+}
+
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 721796b19..20116ff56 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -363,6 +363,57 @@ TestBlackbox::TestBlackbox() : TestBlackboxBase (SRCDIR "/testdata", "blackbox")
 {
 }
 
+void TestBlackbox::allowedValues()
+{
+    QFETCH(QString, property);
+    QFETCH(QString, value);
+    QFETCH(QString, invalidValue);
+
+    QDir::setCurrent(testDataDir + "/allowed-values");
+    rmDirR(relativeBuildDir());
+
+    QbsRunParameters params;
+    if (!property.isEmpty() && !value.isEmpty()) {
+        params.arguments << QStringLiteral("%1:%2").arg(property, value);
+    }
+
+    params.expectFailure = !invalidValue.isEmpty();
+    QCOMPARE(runQbs(params) == 0, !params.expectFailure);
+    if (params.expectFailure) {
+        const auto errorString =
+                QStringLiteral("Value '%1' is not allowed for property").arg(invalidValue);
+        QVERIFY2(m_qbsStderr.contains(errorString.toUtf8()), m_qbsStderr.constData());
+    }
+}
+
+void TestBlackbox::allowedValues_data()
+{
+    QTest::addColumn<QString>("property");
+    QTest::addColumn<QString>("value");
+    QTest::addColumn<QString>("invalidValue");
+
+    QTest::newRow("default") << QString() << QString() << QString();
+
+    QTest::newRow("allowed (product, CLI)") << "products.p.prop" << "foo" << QString();
+    QTest::newRow("not allowed (product, CLI)") << "products.p.prop" << "bar" << "bar";
+    QTest::newRow("allowed (product, JS)") << "products.p.prop2" << "foo" << QString();
+    QTest::newRow("not allowed (product, JS)") << "products.p.prop2" << "bar" << "bar";
+
+    QTest::newRow("allowed single (module, CLI)") << "modules.a.prop" << "foo" << QString();
+    QTest::newRow("not allowed single (module, CLI)") << "modules.a.prop" << "baz" << "baz";
+    QTest::newRow("allowed mult (module, CLI)") << "modules.a.prop" << "foo,bar" << QString();
+    QTest::newRow("not allowed mult (module, CLI)") << "modules.a.prop" << "foo,baz" << "baz";
+
+    QTest::newRow("allowed single (module, JS)") << "modules.a.prop2" << "foo" << QString();
+    QTest::newRow("not allowed single (module, JS)") << "modules.a.prop2" << "baz" << "baz";
+    QTest::newRow("allowed mult (module, JS)") << "modules.a.prop2" << "foo,bar" << QString();
+    QTest::newRow("not allowed mult (module, JS)") << "modules.a.prop2" << "foo,baz" << "baz";
+
+    // undefined should always be allowed
+    QTest::newRow("undefined (product)") << "products.p.prop" << "undefined" << QString();
+    QTest::newRow("undefined (module)") << "modules.a.prop" << "undefined" << QString();
+}
+
 void TestBlackbox::addFileTagToGeneratedArtifact()
 {
     QDir::setCurrent(testDataDir + "/add-filetag-to-generated-artifact");
@@ -3943,7 +3994,7 @@ void TestBlackbox::exportsQbs()
 
     // Trying to build with an unsupported build variant must fail.
     paramsExternalBuild.arguments = QStringList{"-f", "consumer.qbs",
-            "modules.qbs.buildVariant:unknown"};
+            "modules.qbs.buildVariant:profiling"};
     paramsExternalBuild.buildDirectory = QDir::currentPath() + "/external-consumer-profile";
     paramsExternalBuild.expectFailure = true;
     QVERIFY(runQbs(paramsExternalBuild) != 0);
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index 4b87e45a8..8cc21378a 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -40,6 +40,8 @@ public:
     TestBlackbox();
 
 private slots:
+    void allowedValues();
+    void allowedValues_data();
     void addFileTagToGeneratedArtifact();
     void alwaysRun();
     void alwaysRun_data();