QV4Engine: Unify fromValue and metaTypeToJS

Somehow, we ended up with two codepaths doing essentially the same
thing: constructing a JS value from a QVariant. metaTypeToJS is invoked
from QJSEngine::toScriptValue, whereas fromVariant() is used in various
places internally.

metaTypeToJS lacks proper handling for a number of cases, such as
builtin types like QPointF, which lead to toScriptValue(QPointF)
(incorrectly, and uselessly) constructing a VariantObject which couldn't
then do anything useful.

[ChangeLog][QtQml] QJSEngine::toScriptValue will now return correct
JavaScript objects in more cases, for example, for gadget types like
QPointF.

[ChangeLog][QtQml] QJSEngine::toScriptValue now uses the same
behavior as the rest of the engine when building JavaScript values, which
will cause the types of some returned JavaScript objects to change. For
instance, string lists are now returned as sequence objects, not array
objects, and QChar now constructs a JavaScript string.

Change-Id: I0290eb7c9c46e7b508d497cc33cd61d9378f3872
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>

4 files changed, 106 insertions, 133 deletions

diff --git a/src/qml/jsruntime/qv4engine.cpp b/src/qml/jsruntime/qv4engine.cpp
index 02cb9cae62..c04617dac0 100644
--- a/src/qml/jsruntime/qv4engine.cpp
+++ b/src/qml/jsruntime/qv4engine.cpp
@@ -1265,6 +1265,7 @@ static bool convertToNativeQObject(QV4::ExecutionEngine *e, const QV4::Value &va
                             const QByteArray &targetType,
                             void **result);
 static QV4::ReturnedValue variantListToJS(QV4::ExecutionEngine *v4, const QVariantList &lst);
+static QV4::ReturnedValue sequentialIterableToJS(QV4::ExecutionEngine *v4, const QSequentialIterable &lst);
 static QV4::ReturnedValue variantMapToJS(QV4::ExecutionEngine *v4, const QVariantMap &vmap);
 static QV4::ReturnedValue variantToJS(QV4::ExecutionEngine *v4, const QVariant &value)
 {
@@ -1443,33 +1444,6 @@ static QVariant objectToVariant(QV4::ExecutionEngine *e, const QV4::Object *o, V
     return result;
 }
 
-static QV4::ReturnedValue arrayFromVariantList(QV4::ExecutionEngine *e, const QVariantList &list)
-{
-    QV4::Scope scope(e);
-    QV4::ScopedArrayObject a(scope, e->newArrayObject());
-    int len = list.count();
-    a->arrayReserve(len);
-    QV4::ScopedValue v(scope);
-    for (int ii = 0; ii < len; ++ii)
-        a->arrayPut(ii, (v = scope.engine->fromVariant(list.at(ii))));
-
-    a->setArrayLengthUnchecked(len);
-    return a.asReturnedValue();
-}
-
-static QV4::ReturnedValue objectFromVariantMap(QV4::ExecutionEngine *e, const QVariantMap &map)
-{
-    QV4::Scope scope(e);
-    QV4::ScopedObject o(scope, e->newObject());
-    QV4::ScopedString s(scope);
-    QV4::ScopedValue v(scope);
-    for (QVariantMap::const_iterator iter = map.begin(), cend = map.end(); iter != cend; ++iter) {
-        s = e->newString(iter.key());
-        o->put(s, (v = e->fromVariant(iter.value())));
-    }
-    return o.asReturnedValue();
-}
-
 QV4::ReturnedValue QV4::ExecutionEngine::fromVariant(const QVariant &variant)
 {
     int type = variant.userType();
@@ -1537,9 +1511,9 @@ QV4::ReturnedValue QV4::ExecutionEngine::fromVariant(const QVariant &variant)
                 }
 #endif
             case QMetaType::QVariantList:
-                return arrayFromVariantList(this, *reinterpret_cast<const QVariantList *>(ptr));
+                return variantListToJS(this, *reinterpret_cast<const QVariantList *>(ptr));
             case QMetaType::QVariantMap:
-                return objectFromVariantMap(this, *reinterpret_cast<const QVariantMap *>(ptr));
+                return variantMapToJS(this, *reinterpret_cast<const QVariantMap *>(ptr));
             case QMetaType::QJsonValue:
                 return QV4::JsonObject::fromJsonValue(this, *reinterpret_cast<const QJsonValue *>(ptr));
             case QMetaType::QJsonObject:
@@ -1596,6 +1570,11 @@ QV4::ReturnedValue QV4::ExecutionEngine::fromVariant(const QVariant &variant)
             return retn->asReturnedValue();
 #endif
 
+        if (QMetaType::hasRegisteredConverterFunction(type, qMetaTypeId<QtMetaTypePrivate::QSequentialIterableImpl>())) {
+            QSequentialIterable lst = variant.value<QSequentialIterable>();
+            return sequentialIterableToJS(this, lst);
+        }
+
         if (const QMetaObject *vtmo = QQmlValueTypeFactory::metaObjectForMetaType(type))
             return QV4::QQmlValueTypeWrapper::create(this, variant, vtmo, type);
     }
@@ -1675,103 +1654,13 @@ QV4::ReturnedValue ExecutionEngine::metaTypeToJS(int type, const void *data)
 {
     Q_ASSERT(data != nullptr);
 
-    // check if it's one of the types we know
-    switch (QMetaType::Type(type)) {
-    case QMetaType::UnknownType:
-    case QMetaType::Void:
-        return QV4::Encode::undefined();
-    case QMetaType::Nullptr:
-    case QMetaType::VoidStar:
-        return QV4::Encode::null();
-    case QMetaType::Bool:
-        return QV4::Encode(*reinterpret_cast<const bool*>(data));
-    case QMetaType::Int:
-        return QV4::Encode(*reinterpret_cast<const int*>(data));
-    case QMetaType::UInt:
-        return QV4::Encode(*reinterpret_cast<const uint*>(data));
-    case QMetaType::LongLong:
-        return QV4::Encode(double(*reinterpret_cast<const qlonglong*>(data)));
-    case QMetaType::ULongLong:
-#if defined(Q_OS_WIN) && defined(_MSC_FULL_VER) && _MSC_FULL_VER <= 12008804
-#pragma message("** NOTE: You need the Visual Studio Processor Pack to compile support for 64bit unsigned integers.")
-        return QV4::Encode(double((qlonglong)*reinterpret_cast<const qulonglong*>(data)));
-#elif defined(Q_CC_MSVC) && !defined(Q_CC_MSVC_NET)
-        return QV4::Encode(double((qlonglong)*reinterpret_cast<const qulonglong*>(data)));
-#else
-        return QV4::Encode(double(*reinterpret_cast<const qulonglong*>(data)));
-#endif
-    case QMetaType::Double:
-        return QV4::Encode(*reinterpret_cast<const double*>(data));
-    case QMetaType::QString:
-        return newString(*reinterpret_cast<const QString*>(data))->asReturnedValue();
-    case QMetaType::QByteArray:
-        return newArrayBuffer(*reinterpret_cast<const QByteArray*>(data))->asReturnedValue();
-    case QMetaType::Float:
-        return QV4::Encode(*reinterpret_cast<const float*>(data));
-    case QMetaType::Short:
-        return QV4::Encode((int)*reinterpret_cast<const short*>(data));
-    case QMetaType::UShort:
-        return QV4::Encode((int)*reinterpret_cast<const unsigned short*>(data));
-    case QMetaType::Char:
-        return QV4::Encode((int)*reinterpret_cast<const char*>(data));
-    case QMetaType::UChar:
-        return QV4::Encode((int)*reinterpret_cast<const unsigned char*>(data));
-    case QMetaType::QChar:
-        return QV4::Encode((int)(*reinterpret_cast<const QChar*>(data)).unicode());
-    case QMetaType::QStringList:
-        return QV4::Encode(newArrayObject(*reinterpret_cast<const QStringList *>(data)));
-    case QMetaType::QVariantList:
-        return variantListToJS(this, *reinterpret_cast<const QVariantList *>(data));
-    case QMetaType::QVariantMap:
-        return variantMapToJS(this, *reinterpret_cast<const QVariantMap *>(data));
-    case QMetaType::QDateTime:
-        return QV4::Encode(newDateObject(*reinterpret_cast<const QDateTime *>(data)));
-    case QMetaType::QDate:
-        return QV4::Encode(newDateObject(QDateTime(*reinterpret_cast<const QDate *>(data))));
-    case QMetaType::QRegExp:
-        return QV4::Encode(newRegExpObject(*reinterpret_cast<const QRegExp *>(data)));
-#if QT_CONFIG(regularexpression)
-    case QMetaType::QRegularExpression:
-        return QV4::Encode(newRegExpObject(*reinterpret_cast<const QRegularExpression *>(data)));
-#endif
-    case QMetaType::QObjectStar:
-        return QV4::QObjectWrapper::wrap(this, *reinterpret_cast<QObject* const *>(data));
-    case QMetaType::QVariant:
+    QVariant variant(type, data);
+    if (QMetaType::Type(variant.type()) == QMetaType::QVariant) {
+        // unwrap it: this is tested in QJSEngine, and makes the most sense for
+        // end-user code too.
         return variantToJS(this, *reinterpret_cast<const QVariant*>(data));
-    case QMetaType::QJsonValue:
-        return QV4::JsonObject::fromJsonValue(this, *reinterpret_cast<const QJsonValue *>(data));
-    case QMetaType::QJsonObject:
-        return QV4::JsonObject::fromJsonObject(this, *reinterpret_cast<const QJsonObject *>(data));
-    case QMetaType::QJsonArray:
-        return QV4::JsonObject::fromJsonArray(this, *reinterpret_cast<const QJsonArray *>(data));
-    default:
-        if (type == qMetaTypeId<QJSValue>()) {
-            return QJSValuePrivate::convertedToValue(this, *reinterpret_cast<const QJSValue*>(data));
-        } else {
-            QByteArray typeName = QMetaType::typeName(type);
-            if (typeName.endsWith('*') && !*reinterpret_cast<void* const *>(data)) {
-                return QV4::Encode::null();
-            }
-            QMetaType mt(type);
-            if (auto metaObject = mt.metaObject()) {
-                auto flags = mt.flags();
-                if (flags & QMetaType::IsGadget) {
-                    return QV4::QQmlValueTypeWrapper::create(this, QVariant(type, data), metaObject, type);
-                } else if (flags & QMetaType::PointerToQObject) {
-                    return QV4::QObjectWrapper::wrap(this, *reinterpret_cast<QObject* const *>(data));
-                }
-            }
-            if (QMetaType::hasRegisteredConverterFunction(type, qMetaTypeId<QtMetaTypePrivate::QSequentialIterableImpl>())) {
-                auto v = QVariant(type, data);
-                QSequentialIterable lst = v.value<QSequentialIterable>();
-                return sequentialIterableToJS(this, lst);
-            }
-            // Fall back to wrapping in a QVariant.
-            return QV4::Encode(newVariantObject(QVariant(type, data)));
-        }
     }
-    Q_UNREACHABLE();
-    return 0;
+    return fromVariant(variant);
 }
 
 ReturnedValue ExecutionEngine::global()
diff --git a/tests/auto/qml/qjsengine/tst_qjsengine.cpp b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
index d2e0035b57..01b9465f58 100644
--- a/tests/auto/qml/qjsengine/tst_qjsengine.cpp
+++ b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
@@ -66,6 +66,10 @@ private slots:
     void newArray();
     void newArray_HooliganTask218092();
     void newArray_HooliganTask233836();
+    void toScriptValue_data();
+    void toScriptValue();
+    void toScriptValuenotroundtripped_data();
+    void toScriptValuenotroundtripped();
     void newVariant();
     void newVariant_valueOfToString();
     void newVariant_valueOfEnum();
@@ -481,17 +485,97 @@ void tst_QJSEngine::newArray_HooliganTask233836()
     }
 }
 
+void tst_QJSEngine::toScriptValue_data()
+{
+    QTest::addColumn<QVariant>("input");
+
+    QTest::newRow("UnknownType") << QVariant(int(QMetaType::UnknownType), nullptr);
+    QTest::newRow("Nullptr") << QVariant(int(QMetaType::Nullptr), nullptr);
+    QTest::newRow("true") << QVariant(true);
+    QTest::newRow("false") << QVariant(false);
+    QTest::newRow("int") << QVariant(int(42));
+    QTest::newRow("uint") << QVariant(uint(42));
+    QTest::newRow("longlong") << QVariant(qlonglong(4242));
+    QTest::newRow("ulonglong") << QVariant(qulonglong(4242));
+    QTest::newRow("double") << QVariant(double(42.42));
+    QTest::newRow("float") << QVariant(float(42.42));
+    QTest::newRow("qstring") << QVariant(QString::fromLatin1("hello"));
+    QTest::newRow("qbytearray") << QVariant(QByteArray("hello"));
+    QTest::newRow("short") << QVariant(short('r'));
+    QTest::newRow("ushort") << QVariant(short('b'));
+    QTest::newRow("char") << QVariant(char('r'));
+    QTest::newRow("uchar") << QVariant(uchar('b'));
+    QTest::newRow("qchar") << QVariant(QString::fromUtf8("å").at(0));
+    QTest::newRow("qdate") << QVariant(QDate(1925, 5, 8));
+    QTest::newRow("qtime") << QVariant(QTime(4, 5, 6));
+    QTest::newRow("qregularexpression") << QVariant(QRegularExpression(".*"));
+    QTest::newRow("qpointf") << QVariant(QPointF(42, 24));
+    QTest::newRow("qvariantlist") << QVariant(QVariantList() << 42.24 << 5 << "hello");
+    QTest::newRow("qvariantlist_point") << QVariant(QVariantList() << 42.24 << QPointF(42.24, 24.42) << QPointF(24.42, 42.24));
+    QVariantMap vm; vm.insert("test", 55); vm.insert("abc", 42.42);;
+    QTest::newRow("qvariantmap") << QVariant(vm);
+    vm.clear(); vm.insert("point1", QPointF(42.24, 24.42)); vm.insert("point2", QPointF(42.24, 24.42));
+    QTest::newRow("qvariantmap_point") << QVariant(vm);
+    QTest::newRow("qvariant") << QVariant(QVariant(42));
+    QTest::newRow("QList<int>") << QVariant::fromValue(QList<int>() << 1 << 2 << 3 << 4);
+    QTest::newRow("QVector<int>") << QVariant::fromValue(QVector<int>() << 1 << 2 << 3 << 4);
+    QTest::newRow("QList<QString>") << QVariant::fromValue(QVector<QString>() << "1" << "2" << "3" << "4");
+    QTest::newRow("QStringList") << QVariant::fromValue(QStringList() << "1" << "2" << "3" << "4");
+    QTest::newRow("QMap<QString, QString>") << QVariant::fromValue(QMap<QString, QString>{{ "1", "2" }, { "3", "4" }});
+    QTest::newRow("QHash<QString, QString>") << QVariant::fromValue(QHash<QString, QString>{{ "1", "2" }, { "3", "4" }});
+    QTest::newRow("QMap<QString, QPointF>") << QVariant::fromValue(QMap<QString, QPointF>{{ "1", { 42.24, 24.42 } }, { "3", { 24.42, 42.24 } }});
+    QTest::newRow("QHash<QString, QPointF>") << QVariant::fromValue(QHash<QString, QPointF>{{ "1", { 42.24, 24.42 } }, { "3", { 24.42, 42.24 } }});
+}
+
+void tst_QJSEngine::toScriptValue()
+{
+    QFETCH(QVariant, input);
+
+    QJSEngine engine;
+    QJSValue outputJS = engine.toScriptValue(input);
+    QVariant output = engine.fromScriptValue<QVariant>(outputJS);
+
+    QCOMPARE(input, output);
+}
+
+void tst_QJSEngine::toScriptValuenotroundtripped_data()
+{
+    QTest::addColumn<QVariant>("input");
+    QTest::addColumn<QVariant>("output");
+
+    QTest::newRow("QList<QObject*>") << QVariant::fromValue(QList<QObject*>() << this) << QVariant(QVariantList() << QVariant::fromValue(this));
+    QTest::newRow("QObjectList") << QVariant::fromValue(QObjectList() << this) << QVariant(QVariantList() << QVariant::fromValue(this));
+    QTest::newRow("QList<QPoint>") << QVariant::fromValue(QList<QPointF>() << QPointF(42.24, 24.42) << QPointF(42.24, 24.42)) << QVariant(QVariantList() << QPointF(42.24, 24.42) << QPointF(42.24, 24.42));
+    QTest::newRow("QVector<QPoint>") << QVariant::fromValue(QVector<QPointF>() << QPointF(42.24, 24.42) << QPointF(42.24, 24.42)) << QVariant(QVariantList() << QPointF(42.24, 24.42) << QPointF(42.24, 24.42));
+    QTest::newRow("VoidStar") << QVariant(int(QMetaType::VoidStar), nullptr) << QVariant(int(QMetaType::Nullptr), nullptr);
+    QTest::newRow("qregex") << QVariant(QRegExp(".*", Qt::CaseSensitive, QRegExp::RegExp2)) << QVariant(QRegularExpression(".*"));
+}
+
+// This is almost the same as toScriptValue, but the inputs don't roundtrip to
+// exactly the same value.
+void tst_QJSEngine::toScriptValuenotroundtripped()
+{
+    QFETCH(QVariant, input);
+    QFETCH(QVariant, output);
+
+    QJSEngine engine;
+    QJSValue outputJS = engine.toScriptValue(input);
+    QVariant actualOutput = engine.fromScriptValue<QVariant>(outputJS);
+
+    QCOMPARE(actualOutput, output);
+}
+
 void tst_QJSEngine::newVariant()
 {
     QJSEngine eng;
     {
         QJSValue opaque = eng.toScriptValue(QVariant(QPoint(1, 2)));
         QVERIFY(!opaque.isUndefined());
-        QCOMPARE(opaque.isVariant(), true);
+        QCOMPARE(opaque.isVariant(), false);
         QVERIFY(!opaque.isCallable());
         QCOMPARE(opaque.isObject(), true);
         QVERIFY(!opaque.prototype().isUndefined());
-        QCOMPARE(opaque.prototype().isVariant(), true);
+        QCOMPARE(opaque.prototype().isVariant(), false);
         QVERIFY(opaque.property("valueOf").callWithInstance(opaque).equals(opaque));
     }
 }
@@ -505,7 +589,7 @@ void tst_QJSEngine::newVariant_valueOfToString()
         QJSValue value = object.property("valueOf").callWithInstance(object);
         QVERIFY(value.isObject());
         QVERIFY(value.strictlyEquals(object));
-        QCOMPARE(object.toString(), QString::fromLatin1("QVariant(QPoint, QPoint(10,20))"));
+        QCOMPARE(object.toString(), QString::fromLatin1("QPoint(10, 20)"));
     }
 }
 
@@ -1503,7 +1587,7 @@ void tst_QJSEngine::valueConversion_QVariant()
     {
         QVariant var = qVariantFromValue(QPoint(123, 456));
         QJSValue val = eng.toScriptValue(var);
-        QVERIFY(val.isVariant());
+        QVERIFY(!val.isVariant());
         QCOMPARE(val.toVariant(), var);
     }
 
diff --git a/tests/auto/qml/qjsvalue/tst_qjsvalue.cpp b/tests/auto/qml/qjsvalue/tst_qjsvalue.cpp
index 21ebf6b10b..2b80970559 100644
--- a/tests/auto/qml/qjsvalue/tst_qjsvalue.cpp
+++ b/tests/auto/qml/qjsvalue/tst_qjsvalue.cpp
@@ -404,8 +404,8 @@ void tst_QJSValue::toString()
     // variant should use internal valueOf(), then fall back to QVariant::toString(),
     // then fall back to "QVariant(typename)"
     QJSValue variant = eng.toScriptValue(QPoint(10, 20));
-    QVERIFY(variant.isVariant());
-    QCOMPARE(variant.toString(), QString::fromLatin1("QVariant(QPoint, QPoint(10,20))"));
+    QVERIFY(!variant.isVariant());
+    QCOMPARE(variant.toString(), QString::fromLatin1("QPoint(10, 20)"));
     variant = eng.toScriptValue(QUrl());
     QVERIFY(variant.isVariant());
     QVERIFY(variant.toString().isEmpty());
@@ -2288,8 +2288,8 @@ void tst_QJSValue::strictlyEquals()
     {
         QJSValue var1 = eng.toScriptValue(QVariant(QStringList() << "a"));
         QJSValue var2 = eng.toScriptValue(QVariant(QStringList() << "a"));
-        QVERIFY(var1.isArray());
-        QVERIFY(var2.isArray());
+        QVERIFY(!var1.isArray());
+        QVERIFY(!var2.isArray());
         QVERIFY(!var1.strictlyEquals(var2));
     }
     {
diff --git a/tests/auto/qml/qqmlvaluetypes/tst_qqmlvaluetypes.cpp b/tests/auto/qml/qqmlvaluetypes/tst_qqmlvaluetypes.cpp
index 6ccfc77c25..8a01524b5b 100644
--- a/tests/auto/qml/qqmlvaluetypes/tst_qqmlvaluetypes.cpp
+++ b/tests/auto/qml/qqmlvaluetypes/tst_qqmlvaluetypes.cpp
@@ -1710,7 +1710,7 @@ void tst_qqmlvaluetypes::sequences()
         QJSValue value = engine.toScriptValue(qcharVector);
         QCOMPARE(value.property("length").toInt(), qcharVector.length());
         for (int i = 0; i < qcharVector.length(); ++i)
-            QCOMPARE(value.property(i).toInt(), qcharVector.at(i));
+            QCOMPARE(value.property(i).toString(), qcharVector.at(i));
     }
     {
         MyTypeObject a, b, c;