Make QQmlValueTypeFactory::valueType() and isValueType() consistent

If isValueType() returns true, we should really return a non-null value
from valueType(). Otherwise the assumption that
QQmlValueTypeWrapper::valueType is never null breaks. In particular, the
unknown type and various primitive types are _not_ value types. We
special case the, probably common, UnknownType and check the actual
return value of valueType() for anything else. In order to avoid looking
up the metaobject each time we request a type that is not a value type,
we keep an invalid value type as marker for "not checked yet" and
replace that with nullptr once we determine that the type in question is
indeed not a value type.

Fixes: QTBUG-76866
Change-Id: I797f4cdd4db48ffc1b8fa2d919afc8022f67fa94
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>
(cherry picked from commit f862b3edeb8a96a49a5d12620506d33d5a5aadca)
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

5 files changed, 91 insertions, 46 deletions

diff --git a/src/plugins/qmltooling/qmldbg_debugger/qqmlenginedebugservice.cpp b/src/plugins/qmltooling/qmldbg_debugger/qqmlenginedebugservice.cpp
index 4c104f01de..5f548a41c3 100644
--- a/src/plugins/qmltooling/qmldbg_debugger/qqmlenginedebugservice.cpp
+++ b/src/plugins/qmltooling/qmldbg_debugger/qqmlenginedebugservice.cpp
@@ -191,22 +191,18 @@ QQmlEngineDebugServiceImpl::propertyData(QObject *obj, int propIdx)
     if (binding)
         rv.binding = binding->expression();
 
-    if (QQmlValueTypeFactory::isValueType(prop.userType())) {
-        rv.type = QQmlObjectProperty::Basic;
-    } else if (QQmlMetaType::isQObject(prop.userType()))  {
+    rv.value = valueContents(prop.read(obj));
+
+    if (QQmlMetaType::isQObject(prop.userType()))  {
         rv.type = QQmlObjectProperty::Object;
     } else if (QQmlMetaType::isList(prop.userType())) {
         rv.type = QQmlObjectProperty::List;
     } else if (prop.userType() == QMetaType::QVariant) {
         rv.type = QQmlObjectProperty::Variant;
+    } else if (rv.value.isValid()) {
+        rv.type = QQmlObjectProperty::Basic;
     }
 
-    QVariant value;
-    if (rv.type != QQmlObjectProperty::Unknown && prop.userType() != 0) {
-        value = prop.read(obj);
-    }
-    rv.value = valueContents(value);
-
     return rv;
 }
 
@@ -271,10 +267,10 @@ QVariant QQmlEngineDebugServiceImpl::valueContents(QVariant value) const
                         return s;
                 }
             }
-
-            if (isSaveable(value))
-                return value;
         }
+
+        if (isSaveable(value))
+            return value;
     }
 
     if (QQmlMetaType::isQObject(userType)) {
diff --git a/src/plugins/qmltooling/qmldbg_debugger/qqmlwatcher.cpp b/src/plugins/qmltooling/qmldbg_debugger/qqmlwatcher.cpp
index 86571e6cbe..8caa5ac23e 100644
--- a/src/plugins/qmltooling/qmldbg_debugger/qqmlwatcher.cpp
+++ b/src/plugins/qmltooling/qmldbg_debugger/qqmlwatcher.cpp
@@ -109,12 +109,7 @@ QQmlWatchProxy::QQmlWatchProxy(int id,
 
 void QQmlWatchProxy::notifyValueChanged()
 {
-    QVariant v;
-    if (m_expr)
-        v = m_expr->evaluate();
-    else if (QQmlValueTypeFactory::isValueType(m_property.userType()))
-        v = m_property.read(m_object);
-
+    const QVariant v = m_expr ? m_expr->evaluate() : m_property.read(m_object);
     emit m_watch->propertyChanged(m_id, m_debugId, m_property, v);
 }
 
diff --git a/src/qml/compiler/qqmlpropertyvalidator.cpp b/src/qml/compiler/qqmlpropertyvalidator.cpp
index 5e821518a4..5ac6a41d16 100644
--- a/src/qml/compiler/qqmlpropertyvalidator.cpp
+++ b/src/qml/compiler/qqmlpropertyvalidator.cpp
@@ -45,6 +45,19 @@
 
 QT_BEGIN_NAMESPACE
 
+static bool isPrimitiveType(int typeId)
+{
+    switch (typeId) {
+#define HANDLE_PRIMITIVE(Type, id, T) \
+    case QMetaType::Type:
+QT_FOR_EACH_STATIC_PRIMITIVE_TYPE(HANDLE_PRIMITIVE);
+#undef HANDLE_PRIMITIVE
+        return true;
+    default:
+        return false;
+    }
+}
+
 QQmlPropertyValidator::QQmlPropertyValidator(QQmlEnginePrivate *enginePrivate, const QQmlImports &imports, const QQmlRefPointer<QV4::CompiledData::CompilationUnit> &compilationUnit)
     : enginePrivate(enginePrivate)
     , compilationUnit(compilationUnit)
@@ -276,11 +289,21 @@ QVector<QQmlCompileError> QQmlPropertyValidator::validateObject(int objectIndex,
                         return recordError(binding->location, tr("Invalid grouped property access"));
                     }
                 } else {
-                    if (!enginePrivate->propertyCacheForType(pd->propType())) {
+                    const int typeId = pd->propType();
+                    if (isPrimitiveType(typeId)) {
+                        return recordError(
+                                    binding->location,
+                                    tr("Invalid grouped property access: Property \"%1\" with primitive type \"%2\".")
+                                        .arg(name)
+                                        .arg(QString::fromLatin1(QMetaType::typeName(typeId)))
+                                    );
+                    }
+
+                    if (!enginePrivate->propertyCacheForType(typeId)) {
                         return recordError(binding->location,
                                            tr("Invalid grouped property access: Property \"%1\" with type \"%2\", which is not a value type")
                                            .arg(name)
-                                           .arg(QString::fromLatin1(QMetaType::typeName(pd->propType())))
+                                           .arg(QString::fromLatin1(QMetaType::typeName(typeId)))
                                           );
                     }
                 }
@@ -655,15 +678,21 @@ QQmlCompileError QQmlPropertyValidator::validateObjectBinding(QQmlPropertyData *
         return noError;
     }
 
-    if (QQmlMetaType::isInterface(property->propType())) {
+    const int propType = property->propType();
+    const auto rhsType = [&]() {
+        return stringAt(compilationUnit->objectAt(binding->value.objectIndex)
+                        ->inheritedTypeNameIndex);
+    };
+
+    if (QQmlMetaType::isInterface(propType)) {
         // Can only check at instantiation time if the created sub-object successfully casts to the
         // target interface.
         return noError;
-    } else if (property->propType() == QMetaType::QVariant || property->propType() == qMetaTypeId<QJSValue>()) {
+    } else if (propType == QMetaType::QVariant || propType == qMetaTypeId<QJSValue>()) {
         // We can convert everything to QVariant :)
         return noError;
     } else if (property->isQList()) {
-        const int listType = enginePrivate->listType(property->propType());
+        const int listType = enginePrivate->listType(propType);
         if (!QQmlMetaType::isInterface(listType)) {
             QQmlPropertyCache *source = propertyCaches.at(binding->value.objectIndex);
             if (!canCoerce(listType, source)) {
@@ -675,16 +704,23 @@ QQmlCompileError QQmlPropertyValidator::validateObjectBinding(QQmlPropertyData *
         return noError;
     } else if (binding->flags & QV4::CompiledData::Binding::IsSignalHandlerObject && property->isFunction()) {
         return noError;
-    } else if (QQmlValueTypeFactory::isValueType(property->propType())) {
-        return QQmlCompileError(binding->location, tr("Unexpected object assignment for property \"%1\"").arg(propertyName));
-    } else if (property->propType() == qMetaTypeId<QQmlScriptString>()) {
+    } else if (isPrimitiveType(propType)) {
+        auto typeName = QMetaType::typeName(propType);
+        return QQmlCompileError(binding->location, tr("Can not assign value of type \"%1\" to property \"%2\", expecting \"%3\"")
+                                                      .arg(rhsType())
+                                                      .arg(propertyName)
+                                                      .arg(QString::fromUtf8(typeName)));
+    } else if (QQmlValueTypeFactory::isValueType(propType)) {
+        return QQmlCompileError(binding->location, tr("Can not assign value of type \"%1\" to property \"%2\", expecting an object")
+                                                      .arg(rhsType()).arg(propertyName));
+    } else if (propType == qMetaTypeId<QQmlScriptString>()) {
         return QQmlCompileError(binding->valueLocation, tr("Invalid property assignment: script expected"));
     } else {
         // We want to use the raw metaObject here as the raw metaobject is the
         // actual property type before we applied any extensions that might
         // effect the properties on the type, but don't effect assignability
         // Using -1 for the minor version ensures that we get the raw metaObject.
-        QQmlPropertyCache *propertyMetaObject = enginePrivate->rawPropertyCacheForType(property->propType(), -1);
+        QQmlPropertyCache *propertyMetaObject = enginePrivate->rawPropertyCacheForType(propType, -1);
 
         // Will be true if the assigned type inherits propertyMetaObject
         bool isAssignable = false;
@@ -699,7 +735,7 @@ QQmlCompileError QQmlPropertyValidator::validateObjectBinding(QQmlPropertyData *
 
         if (!isAssignable) {
             return QQmlCompileError(binding->valueLocation, tr("Cannot assign object of type \"%1\" to property of type \"%2\" as the former is neither the same as the latter nor a sub-class of it.")
-                    .arg(stringAt(compilationUnit->objectAt(binding->value.objectIndex)->inheritedTypeNameIndex)).arg(QLatin1String(QMetaType::typeName(property->propType()))));
+                    .arg(rhsType()).arg(QLatin1String(QMetaType::typeName(propType))));
         }
     }
     return noError;
diff --git a/src/qml/qml/qqmlvaluetype.cpp b/src/qml/qml/qqmlvaluetype.cpp
index cb6a467c6c..96f692c504 100644
--- a/src/qml/qml/qqmlvaluetype.cpp
+++ b/src/qml/qml/qqmlvaluetype.cpp
@@ -63,11 +63,13 @@ struct QQmlValueTypeFactoryImpl
     QQmlValueType *valueTypes[QVariant::UserType];
     QHash<int, QQmlValueType *> userTypes;
     QMutex mutex;
+
+    QQmlValueType invalidValueType;
 };
 
 QQmlValueTypeFactoryImpl::QQmlValueTypeFactoryImpl()
 {
-    std::fill_n(valueTypes, int(QVariant::UserType), nullptr);
+    std::fill_n(valueTypes, int(QVariant::UserType), &invalidValueType);
 
     // See types wrapped in qqmlmodelindexvaluetype_p.h
     qRegisterMetaType<QItemSelectionRange>();
@@ -75,20 +77,18 @@ QQmlValueTypeFactoryImpl::QQmlValueTypeFactoryImpl()
 
 QQmlValueTypeFactoryImpl::~QQmlValueTypeFactoryImpl()
 {
-    qDeleteAll(valueTypes, valueTypes + QVariant::UserType);
+    for (QQmlValueType *type : valueTypes) {
+        if (type != &invalidValueType)
+            delete type;
+    }
     qDeleteAll(userTypes);
 }
 
-bool QQmlValueTypeFactoryImpl::isValueType(int idx)
+bool isInternalType(int idx)
 {
-    if (idx >= QMetaType::User)
-        return valueType(idx) != nullptr;
-
-    if (idx < 0)
-        return false;
-
     // Qt internal types
     switch (idx) {
+    case QMetaType::UnknownType:
     case QMetaType::QStringList:
     case QMetaType::QObjectStar:
     case QMetaType::VoidStar:
@@ -97,12 +97,20 @@ bool QQmlValueTypeFactoryImpl::isValueType(int idx)
     case QMetaType::QLocale:
     case QMetaType::QImage:  // scarce type, keep as QVariant
     case QMetaType::QPixmap: // scarce type, keep as QVariant
-        return false;
-    default:
         return true;
+    default:
+        return false;
     }
 }
 
+bool QQmlValueTypeFactoryImpl::isValueType(int idx)
+{
+    if (idx < 0 || isInternalType(idx))
+        return false;
+
+    return valueType(idx) != nullptr;
+}
+
 const QMetaObject *QQmlValueTypeFactoryImpl::metaObjectForMetaType(int t)
 {
     switch (t) {
@@ -158,15 +166,17 @@ QQmlValueType *QQmlValueTypeFactoryImpl::valueType(int idx)
     }
 
     QQmlValueType *rv = valueTypes[idx];
-    if (!rv) {
+    if (rv == &invalidValueType) {
         // No need for mutex protection - the most we can lose is a valueType instance
 
         // TODO: Investigate the performance/memory characteristics of
         // removing the preallocated array
-        if (const QMetaObject *mo = metaObjectForMetaType(idx)) {
-            rv = new QQmlValueType(idx, mo);
-            valueTypes[idx] = rv;
-        }
+        if (isInternalType(idx))
+            rv = valueTypes[idx] = nullptr;
+        else if (const QMetaObject *mo = metaObjectForMetaType(idx))
+            rv = valueTypes[idx] = new QQmlValueType(idx, mo);
+        else
+            rv = valueTypes[idx] = nullptr;
     }
 
     return rv;
@@ -196,6 +206,13 @@ void QQmlValueTypeFactory::registerValueTypes(const char *uri, int versionMajor,
     qmlRegisterValueTypeEnums<QQmlEasingValueType>(uri, versionMajor, versionMinor, "Easing");
 }
 
+QQmlValueType::QQmlValueType() :
+    _metaObject(nullptr),
+    gadgetPtr(nullptr),
+    metaType(QMetaType::UnknownType)
+{
+}
+
 QQmlValueType::QQmlValueType(int typeId, const QMetaObject *gadgetMetaObject)
     : gadgetPtr(QMetaType::create(typeId))
     , typeId(typeId)
@@ -214,7 +231,7 @@ QQmlValueType::QQmlValueType(int typeId, const QMetaObject *gadgetMetaObject)
 QQmlValueType::~QQmlValueType()
 {
     QObjectPrivate *op = QObjectPrivate::get(this);
-    Q_ASSERT(op->metaObject == this);
+    Q_ASSERT(op->metaObject == nullptr || op->metaObject == this);
     op->metaObject = nullptr;
     ::free(const_cast<QMetaObject *>(_metaObject));
     metaType.destroy(gadgetPtr);
diff --git a/src/qml/qml/qqmlvaluetype_p.h b/src/qml/qml/qqmlvaluetype_p.h
index 4ea71e8955..c6d36a3293 100644
--- a/src/qml/qml/qqmlvaluetype_p.h
+++ b/src/qml/qml/qqmlvaluetype_p.h
@@ -66,6 +66,7 @@ QT_BEGIN_NAMESPACE
 class Q_QML_PRIVATE_EXPORT QQmlValueType : public QObject, public QAbstractDynamicMetaObject
 {
 public:
+    QQmlValueType();
     QQmlValueType(int userType, const QMetaObject *metaObject);
     ~QQmlValueType() override;
     void read(QObject *, int);
@@ -91,7 +92,7 @@ public:
 class Q_QML_PRIVATE_EXPORT QQmlValueTypeFactory
 {
 public:
-    static bool isValueType(int);
+    static bool isValueType(int idx);
     static QQmlValueType *valueType(int idx);
     static const QMetaObject *metaObjectForMetaType(int type);