Do not auto-convert values to property types

The JavaScript-style conversions are often unexpected and mask user
errors. They also make a mockery of our type system.
The exceptions are as follows:
    - Boolean properties are still auto-converted according to
      the JS rules, because otherwise user code would get tedious to
      write.
    - Lists of strings can still be assigned a single string, so
      the convenience pattern "files: 'main.cpp'" still works.
This patch also fixes some bugs in our own modules that were uncovered
by the change.

[ChangeLog] Added stricter type checking for properties.

Task-number: QBS-1013
Change-Id: I282c23eca3b3877c5551a49ef5c6dd8da91488fe
Reviewed-by: Jake Petroules <jake.petroules@qt.io>

5 files changed, 70 insertions, 35 deletions

diff --git a/src/lib/corelib/language/evaluator.cpp b/src/lib/corelib/language/evaluator.cpp
index 13f69a5f1..a801311bb 100644
--- a/src/lib/corelib/language/evaluator.cpp
+++ b/src/lib/corelib/language/evaluator.cpp
@@ -113,8 +113,7 @@ QString Evaluator::stringValue(const Item *item, const QString &name,
     return v.toString();
 }
 
-static QStringList toStringList(const QScriptValue &scriptValue,
-        const Item *item, const QString &propertyName)
+static QStringList toStringList(const QScriptValue &scriptValue)
 {
     if (scriptValue.isString()) {
         return QStringList(scriptValue.toString());
@@ -125,17 +124,6 @@ static QStringList toStringList(const QScriptValue &scriptValue,
             QScriptValue elem = scriptValue.property(i++);
             if (!elem.isValid())
                 break;
-            if (elem.isUndefined()) {
-                throw ErrorInfo(Tr::tr("Array element at index %1 is undefined. String expected.")
-                                .arg(i - 1),
-                                item->property(propertyName)->location());
-            }
-            if (elem.isArray() || elem.isObject()) {
-                // Let's assume all other JS types are convertible to string.
-                throw ErrorInfo(Tr::tr("Expected array element of type String at index %1.")
-                                .arg(i - 1),
-                                item->property(propertyName)->location());
-            }
             lst.append(elem.toString());
         }
         return lst;
@@ -149,7 +137,7 @@ QStringList Evaluator::stringListValue(const Item *item, const QString &name, bo
     if (propertyWasSet)
         *propertyWasSet = v.isValid() && !v.isUndefined();
     handleEvaluationError(item, name, v);
-    return toStringList(v, item, name);
+    return toStringList(v);
 }
 
 QScriptValue Evaluator::scriptValue(const Item *item)
diff --git a/src/lib/corelib/language/evaluatorscriptclass.cpp b/src/lib/corelib/language/evaluatorscriptclass.cpp
index bf14b2772..7ca472364 100644
--- a/src/lib/corelib/language/evaluatorscriptclass.cpp
+++ b/src/lib/corelib/language/evaluatorscriptclass.cpp
@@ -46,6 +46,7 @@
 #include "scriptengine.h"
 #include "propertydeclaration.h"
 #include "value.h"
+#include <logging/translator.h>
 #include <tools/architectures.h>
 #include <tools/fileinfo.h>
 #include <tools/hostosinfo.h>
@@ -406,12 +407,27 @@ static QString overriddenSourceDirectory(const Item *item)
     return v ? v->value().toString() : QString();
 }
 
-inline void convertToPropertyType(const Item *item, const PropertyDeclaration::Type t,
-        QScriptValue &v)
+static void makeTypeError(const ErrorInfo &error, QScriptValue &v)
+{
+    v = v.engine()->currentContext()->throwError(QScriptContext::TypeError,
+                                                 error.toString());
+}
+
+static void makeTypeError(const PropertyDeclaration &decl, const CodeLocation &location,
+                          QScriptValue &v)
+{
+    const ErrorInfo error(Tr::tr("Value assigned to property '%1' does not have type '%2'.")
+                          .arg(decl.name(), decl.typeString()), location);
+    makeTypeError(error, v);
+}
+
+static void convertToPropertyType(const Item *item, const PropertyDeclaration& decl,
+                                  const CodeLocation &location, QScriptValue &v)
 {
     if (v.isUndefined() || v.isError())
         return;
-    switch (t) {
+    QString srcDir;
+    switch (decl.type()) {
     case PropertyDeclaration::UnknownType:
     case PropertyDeclaration::Variant:
         break;
@@ -421,12 +437,14 @@ inline void convertToPropertyType(const Item *item, const PropertyDeclaration::T
         break;
     case PropertyDeclaration::Integer:
         if (!v.isNumber())
-            v = v.toNumber();
+            makeTypeError(decl, location, v);
         break;
     case PropertyDeclaration::Path:
     {
-        if (!v.isString())
-            v = v.toString();
+        if (!v.isString()) {
+            makeTypeError(decl, location, v);
+            break;
+        }
         const QString srcDir = overriddenSourceDirectory(item);
         if (!srcDir.isEmpty())
             v = v.engine()->toScriptValue(FileInfo::resolvePath(srcDir, v.toString()));
@@ -434,33 +452,46 @@ inline void convertToPropertyType(const Item *item, const PropertyDeclaration::T
     }
     case PropertyDeclaration::String:
         if (!v.isString())
-            v = v.toString();
+            makeTypeError(decl, location, v);
         break;
     case PropertyDeclaration::PathList:
+        srcDir = overriddenSourceDirectory(item);
+        // Fall-through.
+    case PropertyDeclaration::StringList:
     {
         if (!v.isArray()) {
             QScriptValue x = v.engine()->newArray(1);
-            x.setProperty(0, v.isString() ? v : v.toString());
+            x.setProperty(0, v);
             v = x;
         }
-        const QString srcDir = overriddenSourceDirectory(item);
-        if (srcDir.isEmpty())
-            break;
         const quint32 c = v.property(QLatin1String("length")).toUInt32();
         for (quint32 i = 0; i < c; ++i) {
             QScriptValue elem = v.property(i);
+            if (elem.isUndefined()) {
+                ErrorInfo error(Tr::tr("Element at index %1 of list property '%2' is undefined. "
+                                       "String expected.").arg(i).arg(decl.name()), location);
+                makeTypeError(error, v);
+                break;
+            }
+            if (elem.isNull()) {
+                ErrorInfo error(Tr::tr("Element at index %1 of list property '%2' is null. "
+                                       "String expected.").arg(i).arg(decl.name()), location);
+                makeTypeError(error, v);
+                break;
+            }
+            if (!elem.isString()) {
+                ErrorInfo error(Tr::tr("Element at index %1 of list property '%2' does not have "
+                                       "string type.").arg(i).arg(decl.name()), location);
+                makeTypeError(error, v);
+                break;
+            }
+            if (srcDir.isEmpty())
+                continue;
             elem = v.engine()->toScriptValue(FileInfo::resolvePath(srcDir, elem.toString()));
             v.setProperty(i, elem);
         }
         break;
     }
-    case PropertyDeclaration::StringList:
-        if (!v.isArray()) {
-            QScriptValue x = v.engine()->newArray(1);
-            x.setProperty(0, v.isString() ? v : v.toString());
-            v = x;
-        }
-        break;
     }
 }
 
@@ -506,7 +537,7 @@ QScriptValue EvaluatorScriptClass::property(const QScriptValue &object, const QS
         converter.start();
 
         const PropertyDeclaration decl = data->item->propertyDeclaration(name.toString());
-        convertToPropertyType(data->item, decl.type(), result);
+        convertToPropertyType(data->item, decl, value->location(), result);
     }
 
     if (debugProperties)
diff --git a/src/lib/corelib/language/propertydeclaration.cpp b/src/lib/corelib/language/propertydeclaration.cpp
index dd4920ea3..14b43e734 100644
--- a/src/lib/corelib/language/propertydeclaration.cpp
+++ b/src/lib/corelib/language/propertydeclaration.cpp
@@ -126,6 +126,21 @@ PropertyDeclaration::Type PropertyDeclaration::propertyTypeFromString(const QStr
     return PropertyDeclaration::UnknownType;
 }
 
+QString PropertyDeclaration::typeString() const
+{
+    switch (type()) {
+    case Boolean: return QLatin1String("bool");
+    case Integer: return QLatin1String("int");
+    case Path: return QLatin1String("path");
+    case PathList: return QLatin1String("pathList");
+    case String: return QLatin1String("string");
+    case StringList: return QLatin1String("stringList");
+    case Variant: return QLatin1String("variant");
+    case UnknownType: return QLatin1String("unknown");
+    }
+    Q_UNREACHABLE(); // For stupid compilers.
+}
+
 const QString &PropertyDeclaration::name() const
 {
     return d->name;
diff --git a/src/lib/corelib/language/propertydeclaration.h b/src/lib/corelib/language/propertydeclaration.h
index 711766b99..5c72dd0cb 100644
--- a/src/lib/corelib/language/propertydeclaration.h
+++ b/src/lib/corelib/language/propertydeclaration.h
@@ -87,6 +87,7 @@ public:
     bool isScalar() const;
 
     static Type propertyTypeFromString(const QString &typeName);
+    QString typeString() const;
 
     const QString &name() const;
     void setName(const QString &name);
diff --git a/src/lib/corelib/language/tst_language.cpp b/src/lib/corelib/language/tst_language.cpp
index a27d8c9d1..6647089ee 100644
--- a/src/lib/corelib/language/tst_language.cpp
+++ b/src/lib/corelib/language/tst_language.cpp
@@ -509,9 +509,9 @@ void TestLanguage::erroneousFiles_data()
     QTest::newRow("subproject_cycle")
             << "Cycle detected while loading subproject file 'subproject_cycle.qbs'.";
     QTest::newRow("invalid_stringlist_element")
-            << "Expected array element of type String at index 1.";
+            << "Element at index 1 of list property 'files' does not have string type.";
     QTest::newRow("undefined_stringlist_element")
-            << "Array element at index 1 is undefined. String expected.";
+            << "Element at index 1 of list property 'files' is undefined. String expected.";
     QTest::newRow("undeclared_item")
             << "Item 'cpp' is not declared.";
     QTest::newRow("undeclared_property_wrapper")