Fix required properties detection in QQmlObjectCreator

If the type comes from QML (either because it's invalid - defined in
another document/compilation unit OR because it's an inline component),
it means that our instance has a QML-originated base class. That class
might have bindings on required properties (making them initialized).

If those properties come from some more-distant C++ base type, they'll
be in the propertyCache of currently processed type (in one of the
parent caches), but we must not include them into the
sharedState->requiredProperties as we lack the bindings from the
QML-originated base class (so we may accidentally see uninitialized
required properties that were actually initialized, just in a parent
QML document). So: only consider own properties in such case, leaving
the handling of parent-and-own properties to the first QML-defined type
that has an immediate C++ base class - since we use sharedState to
collect required properties, this'll work fine across different
(sub)creators

The only problematic case is when attached and group properties are
involved. We need to decide what to do with them (maybe just leave the old
logic for simplicity)

While fixing the behavior, also update places that used propertyCount()
implementation without using the function itself

Fixes: QTBUG-96200
Task-number: QTBUG-96544
Change-Id: I28f45037790faddaa1b2f07e78544839f60563c7
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit 6f36faf96b488330f2388bdc8728d843f14c38f9)
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

4 files changed, 88 insertions, 9 deletions

diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index 0e8a8aca42..449552c9c3 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -796,7 +796,8 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *bindingProper
         }
         QObject *qmlObject = qmlAttachedPropertiesObject(
                 _qobject, attachedType.attachedPropertiesFunction(QQmlEnginePrivate::get(engine)));
-        if (!populateInstance(binding->value.objectIndex, qmlObject, qmlObject, /*value type property*/nullptr))
+        if (!populateInstance(binding->value.objectIndex, qmlObject, qmlObject,
+                              /*value type property*/ nullptr, binding))
             return false;
         return true;
     }
@@ -861,7 +862,8 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *bindingProper
                 bindingTarget = groupObject;
             }
 
-            if (!populateInstance(binding->value.objectIndex, groupObject, bindingTarget, valueTypeProperty))
+            if (!populateInstance(binding->value.objectIndex, groupObject, bindingTarget,
+                                  valueTypeProperty, binding))
                 return false;
 
             if (valueType)
@@ -1522,7 +1524,9 @@ void QQmlObjectCreator::clear()
     phase = Done;
 }
 
-bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QObject *bindingTarget, const QQmlPropertyData *valueTypeProperty)
+bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QObject *bindingTarget,
+                                         const QQmlPropertyData *valueTypeProperty,
+                                         const QV4::CompiledData::Binding *binding)
 {
     QQmlData *declarativeData = QQmlData::get(instance, /*create*/true);
 
@@ -1561,6 +1565,7 @@ bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QObject *
     if (_compiledObject->flags & QV4::CompiledData::Object::HasDeferredBindings)
         _ddata->deferData(_compiledObjectIndex, compilationUnit, context);
 
+    const qsizetype oldRequiredPropertiesCount = sharedState->requiredProperties.size();
     QSet<QString> postHocRequired;
     for (auto it = _compiledObject->requiredPropertyExtraDataBegin(); it != _compiledObject->requiredPropertyExtraDataEnd(); ++it)
         postHocRequired.insert(stringAt(it->nameIndex));
@@ -1581,10 +1586,48 @@ bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QObject *
 
     }
 
-    for (int i = 0; i <= _propertyCache->propertyOffset(); ++i) {
+    const auto getPropertyCacheRange = [&]() -> std::pair<int, int> {
+        // the logic in a nutshell: we work with QML instances here. every
+        // instance has a QQmlType:
+        // * if QQmlType is valid && not an inline component, it's a C++ type
+        // * otherwise, it's a QML-defined type (a.k.a. Composite type), where
+        //   invalid type == "comes from another QML document"
+        //
+        // 1. if the type we inherit from comes from C++, we must check *all*
+        //    properties in the property cache so far - since we can have
+        //    required properties defined in C++
+        // 2. otherwise - the type comes from QML, it's enough to check just
+        //    *own* properties in the property cache, because there's a previous
+        //    type in the hierarchy that has checked the C++ properties (via 1.)
+        // 3. required attached properties are explicitly not supported. to
+        //    achieve that, go through all its properties
+        // 4. required group properties: the group itself is covered by 1.
+        //    required sub-properties are not properly handled (QTBUG-96544), so
+        //    just return the old range here for consistency
+        QV4::ResolvedTypeReference *typeRef = resolvedType(_compiledObject->inheritedTypeNameIndex);
+        if (!typeRef) { // inside a binding on attached/group property
+            Q_ASSERT(binding);
+            if (binding->isAttachedProperty())
+                return { 0, _propertyCache->propertyCount() }; // 3.
+            Q_ASSERT(binding->isGroupProperty());
+            return { 0, _propertyCache->propertyOffset() + 1 }; // 4.
+        }
+        Q_ASSERT(!(_compiledObject->flags & QV4::CompiledData::Object::IsComponent));
+        QQmlType type = typeRef->type();
+        if (type.isValid() && !type.isInlineComponentType()) {
+            return { 0, _propertyCache->propertyCount() }; // 1.
+        }
+        // Q_ASSERT(type.isComposite());
+        return { _propertyCache->propertyOffset(), _propertyCache->propertyCount() }; // 2.
+    };
+    const auto [offset, count] = getPropertyCacheRange();
+    for (int i = offset; i < count; ++i) {
         QQmlPropertyData *propertyData = _propertyCache->maybeUnresolvedProperty(i);
         if (!propertyData)
             continue;
+        // TODO: the property might be a group property (in which case we need
+        // to dive into its sub-properties and check whether there are any
+        // required elements there) - QTBUG-96544
         if (!propertyData->isRequired() && postHocRequired.isEmpty())
             continue;
         QString name = propertyData->name(_qobject);
@@ -1596,8 +1639,43 @@ bool QQmlObjectCreator::populateInstance(int index, QObject *instance, QObject *
             postHocRequired.erase(postHocIt);
 
         sharedState->hadRequiredProperties = true;
-        sharedState->requiredProperties.insert(propertyData, RequiredPropertyInfo {name,  compilationUnit->finalUrl(), _compiledObject->location, {}});
+        sharedState->requiredProperties.insert(
+                propertyData,
+                RequiredPropertyInfo {
+                        name, compilationUnit->finalUrl(), _compiledObject->location, {} });
+    }
+
+    if (binding && binding->isAttachedProperty()
+        && sharedState->requiredProperties.size() != oldRequiredPropertiesCount) {
+        recordError(
+                binding->location,
+                QLatin1String("Attached property has required properties. This is not supported"));
+    }
+
+    // Note: there's a subtle case with the above logic: if we process a random
+    // QML-defined leaf type, it could have a required attribute overwrite on an
+    // *existing* property: `import QtQuick; Text { required text }`. in this
+    // case, we must add the property to a required list
+    if (!postHocRequired.isEmpty()) {
+        // NB: go through [0, offset) range as [offset, count) is already done
+        for (int i = 0; i < offset; ++i) {
+            QQmlPropertyData *propertyData = _propertyCache->maybeUnresolvedProperty(i);
+            if (!propertyData)
+                continue;
+            QString name = propertyData->name(_qobject);
+            auto postHocIt = postHocRequired.find(name);
+            if (postHocRequired.end() == postHocIt)
+                continue;
+            postHocRequired.erase(postHocIt);
+
+            sharedState->hadRequiredProperties = true;
+            sharedState->requiredProperties.insert(
+                    propertyData,
+                    RequiredPropertyInfo {
+                            name, compilationUnit->finalUrl(), _compiledObject->location, {} });
+        }
     }
+
     if (!postHocRequired.isEmpty() && hadInheritedRequiredProperties)
         recordError({}, QLatin1String("Property %1 was marked as required but does not exist").arg(*postHocRequired.begin()));
 
diff --git a/src/qml/qml/qqmlobjectcreator_p.h b/src/qml/qml/qqmlobjectcreator_p.h
index 1881b3eb23..50489fd55b 100644
--- a/src/qml/qml/qqmlobjectcreator_p.h
+++ b/src/qml/qml/qqmlobjectcreator_p.h
@@ -155,8 +155,9 @@ private:
 
     QObject *createInstance(int index, QObject *parent = nullptr, bool isContextObject = false);
 
-    bool populateInstance(int index, QObject *instance,
-                          QObject *bindingTarget, const QQmlPropertyData *valueTypeProperty);
+    bool populateInstance(int index, QObject *instance, QObject *bindingTarget,
+                          const QQmlPropertyData *valueTypeProperty,
+                          const QV4::CompiledData::Binding *binding = nullptr);
 
     // If qmlProperty and binding are null, populate all properties, otherwise only the given one.
     void populateDeferred(QObject *instance, int deferredIndex,
diff --git a/src/qml/qml/qqmlpropertycache.cpp b/src/qml/qml/qqmlpropertycache.cpp
index 269f02afbd..315f9f07fa 100644
--- a/src/qml/qml/qqmlpropertycache.cpp
+++ b/src/qml/qml/qqmlpropertycache.cpp
@@ -350,7 +350,7 @@ const QMetaObject *QQmlPropertyCache::createMetaObject()
 
 QQmlPropertyData *QQmlPropertyCache::maybeUnresolvedProperty(int index) const
 {
-    if (index < 0 || index >= (propertyIndexCacheStart + propertyIndexCache.count()))
+    if (index < 0 || index >= propertyCount())
         return nullptr;
 
     QQmlPropertyData *rv = nullptr;
diff --git a/src/qml/qml/qqmlpropertycache_p.h b/src/qml/qml/qqmlpropertycache_p.h
index f3f81fbc78..93e776527e 100644
--- a/src/qml/qml/qqmlpropertycache_p.h
+++ b/src/qml/qml/qqmlpropertycache_p.h
@@ -326,7 +326,7 @@ inline const QMetaObject *QQmlPropertyCache::firstCppMetaObject() const
 
 inline QQmlPropertyData *QQmlPropertyCache::property(int index) const
 {
-    if (index < 0 || index >= (propertyIndexCacheStart + propertyIndexCache.count()))
+    if (index < 0 || index >= propertyCount())
         return nullptr;
 
     if (index < propertyIndexCacheStart)