From afbf7b699061a38b27c91d3c95890dcc1f92ebe9 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Thu, 25 Jan 2024 11:18:17 +0100 Subject: [PATCH] QmlCompiler: Handle non-resettable undefined assignment We need to generate an exception if undefined is assigned to a property that can't be reset. We don't want to reject everything that can potentially be undefined. Therefore, we use the QVariant fallback and examine the value for undefined at run time. Pick-to: 6.7 6.6 6.5 6.2 Change-Id: I0a034032f4522f017b452690d93319eb4bfedb1c Reviewed-by: Qt CI Bot Reviewed-by: Fabian Kosmale --- src/qml/qml/qqml.cpp | 41 +++++++++++------ src/qmlcompiler/qqmljsshadowcheck.cpp | 4 +- src/qmlcompiler/qqmljstypepropagator.cpp | 22 ++++----- .../qml/qmlcppcodegen/data/CMakeLists.txt | 1 + .../auto/qml/qmlcppcodegen/data/dynamicmeta.h | 39 +++++++++++++--- .../auto/qml/qmlcppcodegen/data/failures.qml | 6 --- .../qmlcppcodegen/data/fallbackresettable.qml | 23 ++++++++++ .../qml/qmlcppcodegen/data/resettable.qml | 8 ++++ .../qml/qmlcppcodegen/tst_qmlcppcodegen.cpp | 46 +++++++++++++++---- 9 files changed, 140 insertions(+), 50 deletions(-) create mode 100644 tests/auto/qml/qmlcppcodegen/data/fallbackresettable.qml diff --git a/src/qml/qml/qqml.cpp b/src/qml/qml/qqml.cpp index 46e83d532e..23cea45f9d 100644 --- a/src/qml/qml/qqml.cpp +++ b/src/qml/qml/qqml.cpp @@ -1195,10 +1195,17 @@ static ObjectPropertyResult changeObjectProperty(QV4::Lookup *l, QObject *object } template -static ObjectPropertyResult resetObjectProperty(QV4::Lookup *l, QObject *object) +static ObjectPropertyResult resetObjectProperty( + QV4::Lookup *l, QObject *object, QV4::ExecutionEngine *v4) { return changeObjectProperty(l, object, [&](const QQmlPropertyData *property) { - property->resetProperty(object, {}); + if (property->isResettable()) { + property->resetProperty(object, {}); + } else { + v4->throwError( + QLatin1String("Cannot assign [undefined] to ") + + QLatin1String(property->propType().name())); + } }); } @@ -1232,11 +1239,18 @@ static ObjectPropertyResult storeFallbackProperty(QV4::Lookup *l, QObject *objec }); } -static ObjectPropertyResult resetFallbackProperty(QV4::Lookup *l, QObject *object) +static ObjectPropertyResult resetFallbackProperty( + QV4::Lookup *l, QObject *object, const QMetaProperty *property, QV4::ExecutionEngine *v4) { return changeFallbackProperty(l, object, [&](const QMetaObject *metaObject, int coreIndex) { - void *args[] = { nullptr }; - metaObject->metacall(object, QMetaObject::ResetProperty, coreIndex, args); + if (property->isResettable()) { + void *args[] = { nullptr }; + metaObject->metacall(object, QMetaObject::ResetProperty, coreIndex, args); + } else { + v4->throwError( + QLatin1String("Cannot assign [undefined] to ") + + QLatin1String(property->typeName())); + } }); } @@ -1313,7 +1327,7 @@ static ObjectPropertyResult storeObjectAsVariant( return storeObjectProperty(l, object, variant); if (!variant->isValid()) - return resetObjectProperty(l, object); + return resetObjectProperty(l, object, v4); if (isTypeCompatible(variant->metaType(), propType)) return storeObjectProperty(l, object, variant->data()); @@ -1332,12 +1346,13 @@ static ObjectPropertyResult storeFallbackAsVariant( = reinterpret_cast(l->qobjectFallbackLookup.metaObject - 1); Q_ASSERT(metaObject); - const QMetaType propType = metaObject->property(l->qobjectFallbackLookup.coreIndex).metaType(); + const QMetaProperty property = metaObject->property(l->qobjectFallbackLookup.coreIndex); + const QMetaType propType = property.metaType(); if (propType == QMetaType::fromType()) return storeFallbackProperty(l, object, variant); - if (!propType.isValid()) - return resetFallbackProperty(l, object); + if (!variant->isValid()) + return resetFallbackProperty(l, object, &property, v4); if (isTypeCompatible(variant->metaType(), propType)) return storeFallbackProperty(l, object, variant->data()); @@ -1590,7 +1605,7 @@ void AOTCompiledContext::storeNameSloppy(uint nameIndex, void *value, QMetaType if (isTypeCompatible(type, propType)) { storeResult = storeObjectProperty(&l, qmlScopeObject, value); } else if (isUndefined(value, type)) { - storeResult = resetObjectProperty(&l, qmlScopeObject); + storeResult = resetObjectProperty(&l, qmlScopeObject, engine->handle()); } else { QVariant var(propType); QV4::ExecutionEngine *v4 = engine->handle(); @@ -1605,12 +1620,12 @@ void AOTCompiledContext::storeNameSloppy(uint nameIndex, void *value, QMetaType case ObjectLookupResult::Fallback: { const QMetaObject *metaObject = reinterpret_cast(l.qobjectFallbackLookup.metaObject - 1); - const QMetaType propType - = metaObject->property(l.qobjectFallbackLookup.coreIndex).metaType(); + const QMetaProperty property = metaObject->property(l.qobjectFallbackLookup.coreIndex); + const QMetaType propType = property.metaType(); if (isTypeCompatible(type, propType)) { storeResult = storeFallbackProperty(&l, qmlScopeObject, value); } else if (isUndefined(value, type)) { - storeResult = resetFallbackProperty(&l, qmlScopeObject); + storeResult = resetFallbackProperty(&l, qmlScopeObject, &property, engine->handle()); } else { QVariant var(propType); QV4::ExecutionEngine *v4 = engine->handle(); diff --git a/src/qmlcompiler/qqmljsshadowcheck.cpp b/src/qmlcompiler/qqmljsshadowcheck.cpp index 3b4bda7adc..94b908effb 100644 --- a/src/qmlcompiler/qqmljsshadowcheck.cpp +++ b/src/qmlcompiler/qqmljsshadowcheck.cpp @@ -87,11 +87,11 @@ void QQmlJSShadowCheck::handleStore(int base, const QString &memberName) if (checkShadowing(baseType, memberName, base) == Shadowable) return; - // If the property isn't shadowable but resettable, we have to turn the read register into + // If the property isn't shadowable, we have to turn the read register into // var if the accumulator can hold undefined. This has to be done in a second pass // because the accumulator may still turn into var due to its own shadowing. const QQmlJSRegisterContent member = m_typeResolver->memberType(baseType, memberName); - if (member.isProperty() && !member.property().reset().isEmpty()) + if (member.isProperty()) m_resettableStores.append({m_state.accumulatorIn(), instructionOffset}); } diff --git a/src/qmlcompiler/qqmljstypepropagator.cpp b/src/qmlcompiler/qqmljstypepropagator.cpp index 7df0d3a498..beb846bf81 100644 --- a/src/qmlcompiler/qqmljstypepropagator.cpp +++ b/src/qmlcompiler/qqmljstypepropagator.cpp @@ -653,9 +653,7 @@ void QQmlJSTypePropagator::generate_StoreNameCommon(int nameIndex) if (m_typeResolver->canHoldUndefined(in) && !m_typeResolver->canHoldUndefined(type)) { - if (type.property().reset().isEmpty()) - setError(u"Cannot assign potential undefined to %1"_s.arg(type.descriptiveName())); - else if (m_typeResolver->registerIsStoredIn(in, m_typeResolver->voidType())) + if (m_typeResolver->registerIsStoredIn(in, m_typeResolver->voidType())) addReadAccumulator(m_typeResolver->globalType(m_typeResolver->varType())); else addReadAccumulator(in); @@ -990,19 +988,19 @@ void QQmlJSTypePropagator::generate_StoreProperty(int nameIndex, int base) getCurrentBindingSourceLocation())); } - // If the property is resettable we must not coerce the input to the property type + // If the input can hold undefined we must not coerce it to the property type // as that might eliminate an undefined value. For example, undefined -> string // becomes "undefined". - // Therefore we explicitly require the value to be given as QVariant. This triggers - // the QVariant fallback path that also used for shadowable properties. QVariant can - // hold undefined and the lookup functions will handle that appropriately. + // We need the undefined value for either resetting the property if that is supported + // or generating an exception otherwise. Therefore we explicitly require the value to + // be given as QVariant. This triggers the QVariant fallback path that's also used for + // shadowable properties. QVariant can hold undefined and the lookup functions will + // handle that appropriately. const QQmlJSScope::ConstPtr varType = m_typeResolver->varType(); - const QQmlJSRegisterContent readType - = (property.property().reset().isEmpty() - || !m_typeResolver->canHoldUndefined(m_state.accumulatorIn())) - ? property - : property.storedIn(varType).castTo(varType); + const QQmlJSRegisterContent readType = m_typeResolver->canHoldUndefined(m_state.accumulatorIn()) + ? property.storedIn(varType).castTo(varType) + : property; addReadAccumulator(readType); addReadRegister(base, callBase); m_state.setHasSideEffects(true); diff --git a/tests/auto/qml/qmlcppcodegen/data/CMakeLists.txt b/tests/auto/qml/qmlcppcodegen/data/CMakeLists.txt index 67d8962983..8b56bc77ad 100644 --- a/tests/auto/qml/qmlcppcodegen/data/CMakeLists.txt +++ b/tests/auto/qml/qmlcppcodegen/data/CMakeLists.txt @@ -135,6 +135,7 @@ set(qml_files extendedTypes.qml failures.qml fallbacklookups.qml + fallbackresettable.qml fileDialog.qml flagEnum.qml fromBoolValue.qml diff --git a/tests/auto/qml/qmlcppcodegen/data/dynamicmeta.h b/tests/auto/qml/qmlcppcodegen/data/dynamicmeta.h index 64c2850bda..100e9d825c 100644 --- a/tests/auto/qml/qmlcppcodegen/data/dynamicmeta.h +++ b/tests/auto/qml/qmlcppcodegen/data/dynamicmeta.h @@ -5,34 +5,44 @@ #define DYNAMICMETA_H #include +#include #include -struct FreeDeleter { - void operator()(QMetaObject *meta) { free(meta); } -}; - template class MetaObjectData : public QDynamicMetaObjectData { Q_DISABLE_COPY_MOVE(MetaObjectData) public: - MetaObjectData() = default; - ~MetaObjectData() = default; + MetaObjectData() + { + QMetaObjectBuilder builder; + builder.setSuperClass(&T::staticMetaObject); + builder.setFlags(builder.flags() | DynamicMetaObject); + metaObject = builder.toMetaObject(); + }; + + ~MetaObjectData() { + free(metaObject); + }; QMetaObject *toDynamicMetaObject(QObject *) override { - return const_cast(&T::staticMetaObject); + return metaObject; } int metaCall(QObject *o, QMetaObject::Call call, int idx, void **argv) override { return o->qt_metacall(call, idx, argv); } + + QMetaObject *metaObject = nullptr; }; class DynamicMeta : public QObject { Q_OBJECT Q_PROPERTY(int foo READ foo WRITE setFoo NOTIFY fooChanged FINAL) + Q_PROPERTY(qreal value READ value WRITE setValue RESET resetValue NOTIFY valueChanged FINAL) + Q_PROPERTY(qreal shadowable READ shadowable CONSTANT) QML_ELEMENT public: @@ -54,11 +64,26 @@ public: Q_INVOKABLE int bar(int baz) { return baz + 12; } + qreal value() const { return m_value; } + qreal shadowable() const { return 25; } + +public slots: + void resetValue() { setValue(0); } + void setValue(qreal value) + { + if (m_value == value) + return; + m_value = value; + emit valueChanged(); + } + Q_SIGNALS: void fooChanged(); + void valueChanged(); private: int m_foo = 0; + qreal m_value = 0; }; class DynamicMetaSingleton : public DynamicMeta diff --git a/tests/auto/qml/qmlcppcodegen/data/failures.qml b/tests/auto/qml/qmlcppcodegen/data/failures.qml index ca46c806b3..3b0e4908ab 100644 --- a/tests/auto/qml/qmlcppcodegen/data/failures.qml +++ b/tests/auto/qml/qmlcppcodegen/data/failures.qml @@ -35,12 +35,6 @@ QtObject { onPartyStarted: (foozle) => { objectName = foozle } } - signal foo() - signal bar() - - // Cannot assign potential undefined - onFoo: objectName = self.bar() - property int enumFromGadget1: GadgetWithEnum.CONNECTED + 1 property int enumFromGadget2: TT2.GadgetWithEnum.CONNECTED + 1 diff --git a/tests/auto/qml/qmlcppcodegen/data/fallbackresettable.qml b/tests/auto/qml/qmlcppcodegen/data/fallbackresettable.qml new file mode 100644 index 0000000000..44b55e245a --- /dev/null +++ b/tests/auto/qml/qmlcppcodegen/data/fallbackresettable.qml @@ -0,0 +1,23 @@ +pragma Strict +import QtQml +import TestTypes + +DynamicMeta { + id: self + value: 999 + + property double notResettable: 10 + property double notResettable2: { return undefined } + + property DynamicMeta shadowing: DynamicMeta { + property var shadowable: undefined + } + + function doReset() { self.value = undefined } + function doReset2() { self.value = shadowing.shadowable } + function doNotReset() { self.notResettable = undefined } + + signal aaa() + signal bbb() + onAaa: objectName = self.bbb() +} diff --git a/tests/auto/qml/qmlcppcodegen/data/resettable.qml b/tests/auto/qml/qmlcppcodegen/data/resettable.qml index 59fa31645e..561655032d 100644 --- a/tests/auto/qml/qmlcppcodegen/data/resettable.qml +++ b/tests/auto/qml/qmlcppcodegen/data/resettable.qml @@ -6,10 +6,18 @@ Resettable { id: self value: 999 + property double notResettable: 10 + property double notResettable2: { return undefined } + property Resettable shadowing: Resettable { property var shadowable: undefined } function doReset() { self.value = undefined } function doReset2() { self.value = shadowing.shadowable } + function doNotReset() { self.notResettable = undefined } + + signal aaa() + signal bbb() + onAaa: objectName = self.bbb() } diff --git a/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp b/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp index 548ad20c2b..2fc5129230 100644 --- a/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp +++ b/tests/auto/qml/qmlcppcodegen/tst_qmlcppcodegen.cpp @@ -187,7 +187,10 @@ private slots: void registerElimination(); void registerPropagation(); void renameAdjust(); + void resettableProperty(); + void resettableProperty_data(); + void returnAfterReject(); void revisions(); void scopeIdLookup(); @@ -3841,23 +3844,46 @@ void tst_QmlCppCodegen::renameAdjust() void tst_QmlCppCodegen::resettableProperty() { + QFETCH(QString, url); + QQmlEngine engine; - QQmlComponent c(&engine, QUrl(u"qrc:/qt/qml/TestTypes/resettable.qml"_s)); + QQmlComponent c(&engine, QUrl(url)); QVERIFY2(c.isReady(), qPrintable(c.errorString())); + + QTest::ignoreMessage( + QtWarningMsg, qPrintable(url + u":10:5: Unable to assign [undefined] to double"_s)); + QScopedPointer o(c.create()); QVERIFY(o); - ResettableProperty *resettable = qobject_cast(o.data()); - QVERIFY(resettable); + QCOMPARE(o->property("value").toDouble(), 999); + QMetaObject::invokeMethod(o.data(), "doReset"); + QCOMPARE(o->property("value").toDouble(), 0); - QCOMPARE(resettable->value(), 999); - QMetaObject::invokeMethod(resettable, "doReset"); - QCOMPARE(resettable->value(), 0); + o->setProperty("value", double(82)); + QCOMPARE(o->property("value").toDouble(), 82); + QMetaObject::invokeMethod(o.data(), "doReset2"); + QCOMPARE(o->property("value").toDouble(), 0); - resettable->setValue(82); - QCOMPARE(resettable->value(), 82); - QMetaObject::invokeMethod(resettable, "doReset2"); - QCOMPARE(resettable->value(), 0); + QTest::ignoreMessage( + QtWarningMsg, qPrintable(url + u":18: Error: Cannot assign [undefined] to double"_s)); + QCOMPARE(o->property("notResettable").toDouble(), 10); + QMetaObject::invokeMethod(o.data(), "doNotReset"); + QCOMPARE(o->property("notResettable").toDouble(), 10); + QCOMPARE(o->property("notResettable2").toDouble(), 0); // not NaN + + o->setObjectName(u"namename"_s); + QTest::ignoreMessage( + QtWarningMsg, qPrintable(url + u":22: Error: Cannot assign [undefined] to QString"_s)); + QMetaObject::invokeMethod(o.data(), "aaa"); + QCOMPARE(o->objectName(), u"namename"_s); +} + +void tst_QmlCppCodegen::resettableProperty_data() +{ + QTest::addColumn("url"); + QTest::addRow("object lookups") << u"qrc:/qt/qml/TestTypes/resettable.qml"_s; + QTest::addRow("fallback lookups") << u"qrc:/qt/qml/TestTypes/fallbackresettable.qml"_s; } void tst_QmlCppCodegen::returnAfterReject()