aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAndrei Golubev <andrei.golubev@qt.io>2021-09-15 15:21:10 +0200
committerAndrei Golubev <andrei.golubev@qt.io>2021-09-22 20:39:52 +0200
commit6f36faf96b488330f2388bdc8728d843f14c38f9 (patch)
tree80867e117fc47e405fa951aed35b9a4c6f26870b
parentcb79a6dafe4cf247e32053d1b28caa03eadcc331 (diff)
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 Pick-to: 6.2 Fixes: QTBUG-96200 Task-number: QTBUG-96544 Change-Id: I28f45037790faddaa1b2f07e78544839f60563c7 Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Diffstat
-rw-r--r--src/qml/qml/qqmlobjectcreator.cpp88
-rw-r--r--src/qml/qml/qqmlobjectcreator_p.h5
-rw-r--r--src/qml/qml/qqmlpropertycache.cpp2
-rw-r--r--src/qml/qml/qqmlpropertycache_p.h2
-rw-r--r--tests/auto/qml/qqmlcomponent/data/RerequiredNotSet.qml3
-rw-r--r--tests/auto/qml/qqmlcomponent/data/requiredGroup.bad.qml3
-rw-r--r--tests/auto/qml/qqmlcomponent/data/requiredGroup.good.qml4
-rw-r--r--tests/auto/qml/qqmlcomponent/data/requiredSetLater.rerequired.qml3
-rw-r--r--tests/auto/qml/qqmlcomponent/data/rerequiredSet.qml4
-rw-r--r--tests/auto/qml/qqmlcomponent/data/rerequiredSetLater.qml4
-rw-r--r--tests/auto/qml/qqmlcomponent/tst_qqmlcomponent.cpp51
11 files changed, 136 insertions, 33 deletions
diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index a683e78d33..aaa260ab92 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)
@@ -1508,7 +1510,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);
@@ -1547,6 +1551,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));
@@ -1567,10 +1572,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);
@@ -1582,8 +1625,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)
diff --git a/tests/auto/qml/qqmlcomponent/data/RerequiredNotSet.qml b/tests/auto/qml/qqmlcomponent/data/RerequiredNotSet.qml
new file mode 100644
index 0000000000..2dd33e8196
--- /dev/null
+++ b/tests/auto/qml/qqmlcomponent/data/RerequiredNotSet.qml
@@ -0,0 +1,3 @@
+RequiredSetInSameFile {
+ required i
+} // error: we explicitly want required here, but it's not set
diff --git a/tests/auto/qml/qqmlcomponent/data/requiredGroup.bad.qml b/tests/auto/qml/qqmlcomponent/data/requiredGroup.bad.qml
new file mode 100644
index 0000000000..744c9131dc
--- /dev/null
+++ b/tests/auto/qml/qqmlcomponent/data/requiredGroup.bad.qml
@@ -0,0 +1,3 @@
+import qt.test 1.0
+RequiredGroup {
+} // error: required group property is not set
diff --git a/tests/auto/qml/qqmlcomponent/data/requiredGroup.good.qml b/tests/auto/qml/qqmlcomponent/data/requiredGroup.good.qml
new file mode 100644
index 0000000000..836b4c6968
--- /dev/null
+++ b/tests/auto/qml/qqmlcomponent/data/requiredGroup.good.qml
@@ -0,0 +1,4 @@
+import qt.test 1.0
+RequiredGroup {
+ group.disabled.buttonText: "salmon"
+}
diff --git a/tests/auto/qml/qqmlcomponent/data/requiredSetLater.rerequired.qml b/tests/auto/qml/qqmlcomponent/data/requiredSetLater.rerequired.qml
new file mode 100644
index 0000000000..975f797a20
--- /dev/null
+++ b/tests/auto/qml/qqmlcomponent/data/requiredSetLater.rerequired.qml
@@ -0,0 +1,3 @@
+RerequiredNotSet {
+ i: 43
+}
diff --git a/tests/auto/qml/qqmlcomponent/data/rerequiredSet.qml b/tests/auto/qml/qqmlcomponent/data/rerequiredSet.qml
new file mode 100644
index 0000000000..316d5b3f4b
--- /dev/null
+++ b/tests/auto/qml/qqmlcomponent/data/rerequiredSet.qml
@@ -0,0 +1,4 @@
+RequiredSetInSameFile {
+ required i
+ i: 43
+}
diff --git a/tests/auto/qml/qqmlcomponent/data/rerequiredSetLater.qml b/tests/auto/qml/qqmlcomponent/data/rerequiredSetLater.qml
new file mode 100644
index 0000000000..4e31431095
--- /dev/null
+++ b/tests/auto/qml/qqmlcomponent/data/rerequiredSetLater.qml
@@ -0,0 +1,4 @@
+RerequiredNotSet {
+ required i
+ i: 43
+} // note: even though parent type is borked, this type is fine
diff --git a/tests/auto/qml/qqmlcomponent/tst_qqmlcomponent.cpp b/tests/auto/qml/qqmlcomponent/tst_qqmlcomponent.cpp
index 800587179c..a92da98d2b 100644
--- a/tests/auto/qml/qqmlcomponent/tst_qqmlcomponent.cpp
+++ b/tests/auto/qml/qqmlcomponent/tst_qqmlcomponent.cpp
@@ -35,6 +35,7 @@
#include <QtQuick>
#include <QtQuick/private/qquickrectangle_p.h>
#include <QtQuick/private/qquickmousearea_p.h>
+#include <QtQuick/private/qquickpalette_p.h>
#include <QtQuickTestUtils/private/qmlutils_p.h>
#include <QtQuickTestUtils/private/testhttpserver_p.h>
#include <private/qqmlguardedcontextdata_p.h>
@@ -752,6 +753,18 @@ public:
TwoRequiredProperties *group() { return &m_group; }
};
+class RequiredGroup : public QObject
+{
+ Q_OBJECT
+ // use QQuickPalette not to create an extra test-only
+ Q_PROPERTY(QQuickPalette *group READ group REQUIRED)
+ QQuickPalette m_group;
+
+public:
+ RequiredGroup(QObject *parent = nullptr) : QObject(parent) { }
+ QQuickPalette *group() { return &m_group; }
+};
+
void tst_qqmlcomponent::testRequiredProperties_data()
{
qmlRegisterType<RequiredDefaultCpp>("qt.test", 1, 0, "RequiredDefaultCpp");
@@ -761,6 +774,7 @@ void tst_qqmlcomponent::testRequiredProperties_data()
"qt.test", 1, 0, "AttachedRequiredProperty",
tr("AttachedRequiredProperties is an attached property"));
qmlRegisterType<GroupedRequiredProperties>("qt.test", 1, 0, "GroupedRequiredProperty");
+ qmlRegisterType<RequiredGroup>("qt.test", 1, 0, "RequiredGroup");
QTest::addColumn<QUrl>("testFile");
QTest::addColumn<bool>("shouldSucceed");
@@ -774,6 +788,12 @@ void tst_qqmlcomponent::testRequiredProperties_data()
QTest::addRow("requiredSetViaAlias3") << testFileUrl("requiredSetViaAliasParentFile.qml") << true << "";
QTest::addRow("requiredSetInBase") << testFileUrl("requiredChildOfGoodBase.qml") << true << "";
QTest::addRow("requiredNotSetInBase") << testFileUrl("requiredChildOfBadBase.qml") << false << "Required property i was not initialized";
+ QTest::addRow("rerequiredSet") << testFileUrl("rerequiredSet.qml") << true << "";
+ QTest::addRow("rerequiredNotSet") << testFileUrl("RerequiredNotSet.qml") << false
+ << "Required property i was not initialized";
+ QTest::addRow("requiredSetLater(rerequired)")
+ << testFileUrl("requiredSetLater.rerequired.qml") << true << "";
+ QTest::addRow("rerequiredSetLater") << testFileUrl("rerequiredSetLater.qml") << true << "";
QTest::addRow("shadowing") << testFileUrl("shadowing.qml") << false << "Required property i was not initialized";
QTest::addRow("shadowing (C++)") << testFileUrl("shadowingFromCpp.qml") << false
<< "Required property name was not initialized";
@@ -818,6 +838,11 @@ void tst_qqmlcomponent::testRequiredProperties_data()
QTest::addRow("required two set two (attached indirect)")
<< testFileUrl("requiredPropertiesInAttachedIndirect.qml") << false
<< "Attached property has required properties. This is not supported";
+ QTest::addRow("required itself not set (group)")
+ << testFileUrl("requiredGroup.bad.qml") << false
+ << "Required property group was not initialized";
+ QTest::addRow("required itself set (group)")
+ << testFileUrl("requiredGroup.good.qml") << true << "";
QTest::addRow("required not set (group)")
<< testFileUrl("requiredPropertiesInGroup.bad.qml") << false
<< "Required property index was not initialized";
@@ -839,32 +864,13 @@ void tst_qqmlcomponent::testRequiredProperties()
QQmlComponent comp(&eng);
comp.loadUrl(testFile);
QScopedObjPointer obj {comp.create()};
- QEXPECT_FAIL("required two set one (C++)", "QTBUG-96200", Abort);
- QEXPECT_FAIL("required two set two (C++ indirect)",
- "Properties are set in the QML base class, but this is not recognized", Abort);
- QEXPECT_FAIL("required set (inline component, C++)",
- "Properties are set in the QML inline component, but this is not recognized",
- Abort);
- QEXPECT_FAIL("required set (inline component, C++ indirect)",
- "Properties are set in the QML base class of QML inline component, but this is "
- "not recognized",
- Abort);
- QEXPECT_FAIL("required two set one (attached)",
- "Required attached properties are not supported",
- Abort);
- QEXPECT_FAIL("required two set two (attached)",
- "Required attached properties are not supported",
- Abort);
- QEXPECT_FAIL("required two set two (attached indirect)",
- "Required attached properties are not supported",
- Abort);
QEXPECT_FAIL("required not set (group)",
"We fail to recognize required sub-properties inside a group property when that "
- "group property is unused",
+ "group property is unused (QTBUG-96544)",
Abort);
QEXPECT_FAIL("required two set one (group)",
"We fail to recognized required sub-properties inside a group property, even when "
- "that group property is used",
+ "that group property is used (QTBUG-96544)",
Abort);
if (shouldSucceed) {
QVERIFY2(comp.isReady(), qPrintable(comp.errorString()));
@@ -872,9 +878,6 @@ void tst_qqmlcomponent::testRequiredProperties()
} else {
QVERIFY2(!obj, "The object is valid when it shouldn't be");
QFETCH(QString, errorMsg);
- QEXPECT_FAIL("required not set (attached)",
- "Required attached properties are not supported",
- Abort);
QVERIFY2(comp.errorString().contains(errorMsg), qPrintable(comp.errorString()));
}
}
generated by cgit v1.2.3 (git 2.25.1) at 2025-12-24 06:56:38 +0000