diff options
9 files changed, 215 insertions, 46 deletions
diff --git a/src/qml/qml/qqmlpropertycache.cpp b/src/qml/qml/qqmlpropertycache.cpp index d368433949..e99c26cec8 100644 --- a/src/qml/qml/qqmlpropertycache.cpp +++ b/src/qml/qml/qqmlpropertycache.cpp @@ -352,6 +352,18 @@ QQmlPropertyCache::copyAndAppend(const QMetaObject *metaObject, return rv; } +static QHashedString signalNameToHandlerName(const QHashedString &methodName) +{ + return QQmlSignalNames::signalNameToHandlerName(methodName); +} + +static QHashedString signalNameToHandlerName(const QHashedCStringRef &methodName) +{ + return QQmlSignalNames::signalNameToHandlerName( + QLatin1StringView{ methodName.constData(), methodName.length() }); +} + + void QQmlPropertyCache::append(const QMetaObject *metaObject, QTypeRevision typeVersion, QQmlPropertyData::Flags propertyFlags, @@ -440,40 +452,29 @@ void QQmlPropertyCache::append(const QMetaObject *metaObject, QQmlPropertyData *old = nullptr; - if (utf8) { - QHashedString methodName(QString::fromUtf8(rawName, cptr - rawName)); + const auto doSetNamedProperty = [&](const auto &methodName) { if (StringCache::mapped_type *it = stringCache.value(methodName)) { if (handleOverride(methodName, data, (old = it->second)) == InvalidOverride) - continue; + return; } - setNamedProperty(methodName, ii, data); - if (data->isSignal()) { - QHashedString on(QQmlSignalNames::signalNameToHandlerName(methodName)); - setNamedProperty(on, ii, sigdata); - ++signalHandlerIndex; - } - } else { - QHashedCStringRef methodName(rawName, cptr - rawName); - if (StringCache::mapped_type *it = stringCache.value(methodName)) { - if (handleOverride(methodName, data, (old = it->second)) == InvalidOverride) - continue; - } setNamedProperty(methodName, ii, data); if (data->isSignal()) { - QHashedString on(QQmlSignalNames::signalNameToHandlerName( - QLatin1StringView{ methodName.constData(), methodName.length() })); - setNamedProperty(on, ii, data); + + // TODO: Remove this once we can. Signals should not be overridable. + if (!utf8) + data->m_flags.setIsOverridableSignal(true); + + setNamedProperty(signalNameToHandlerName(methodName), ii, sigdata); ++signalHandlerIndex; } - } + }; - if (old) { - // We only overload methods in the same class, exactly like C++ - if (old->isFunction() && old->coreIndex() >= methodOffset) - data->m_flags.setIsOverload(true); - } + if (utf8) + doSetNamedProperty(QHashedString(QString::fromUtf8(rawName, cptr - rawName))); + else + doSetNamedProperty(QHashedCStringRef(rawName, cptr - rawName)); } int propCount = metaObject->propertyCount(); diff --git a/src/qml/qml/qqmlpropertycachecreator_p.h b/src/qml/qml/qqmlpropertycachecreator_p.h index fc0353b15d..ae3c669939 100644 --- a/src/qml/qml/qqmlpropertycachecreator_p.h +++ b/src/qml/qml/qqmlpropertycachecreator_p.h @@ -461,8 +461,13 @@ inline QQmlError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject( // For property change signal override detection. // We prepopulate a set of signal names which already exist in the object, // and throw an error if there is a signal/method defined as an override. - QSet<QString> seenSignals; - seenSignals << QStringLiteral("destroyed") << QStringLiteral("parentChanged") << QStringLiteral("objectNameChanged"); + // TODO: Remove AllowOverride once we can. No override should be allowed. + enum class AllowOverride { No, Yes }; + QHash<QString, AllowOverride> seenSignals { + { QStringLiteral("destroyed"), AllowOverride::No }, + { QStringLiteral("parentChanged"), AllowOverride::No }, + { QStringLiteral("objectNameChanged"), AllowOverride::No } + }; const QQmlPropertyCache *parentCache = cache.data(); while ((parentCache = parentCache->parent().data())) { if (int pSigCount = parentCache->signalCount()) { @@ -473,7 +478,15 @@ inline QQmlError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject( for (QQmlPropertyCache::StringCache::ConstIterator iter = parentCache->stringCache.begin(); iter != parentCache->stringCache.end(); ++iter) { if (currPSig == (*iter).second) { - seenSignals.insert(iter.key()); + if (currPSig->isOverridableSignal()) { + const qsizetype oldSize = seenSignals.size(); + AllowOverride &entry = seenSignals[iter.key()]; + if (seenSignals.size() != oldSize) + entry = AllowOverride::Yes; + } else { + seenSignals[iter.key()] = AllowOverride::No; + } + break; } } @@ -489,7 +502,7 @@ inline QQmlError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject( const QString changedSigName = QQmlSignalNames::propertyNameToChangedSignalName(stringAt(p->nameIndex)); - seenSignals.insert(changedSigName); + seenSignals[changedSigName] = AllowOverride::No; cache->appendSignal(changedSigName, flags, effectiveMethodIndex++); } @@ -501,7 +514,7 @@ inline QQmlError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject( const QString changedSigName = QQmlSignalNames::propertyNameToChangedSignalName(stringAt(a->nameIndex())); - seenSignals.insert(changedSigName); + seenSignals[changedSigName] = AllowOverride::No; cache->appendSignal(changedSigName, flags, effectiveMethodIndex++); } @@ -553,10 +566,26 @@ inline QQmlError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject( flags.setHasArguments(true); QString signalName = stringAt(s->nameIndex); - if (seenSignals.contains(signalName)) - return qQmlCompileError(s->location, QQmlPropertyCacheCreatorBase::tr("Duplicate signal name: invalid override of property change signal or superclass signal")); - seenSignals.insert(signalName); - + const auto it = seenSignals.find(signalName); + if (it == seenSignals.end()) { + seenSignals[signalName] = AllowOverride::No; + } else { + // TODO: Remove the AllowOverride::Yes branch once we can. + QQmlError message = qQmlCompileError( + s->location, + QQmlPropertyCacheCreatorBase::tr( + "Duplicate signal name: " + "invalid override of property change signal or superclass signal")); + switch (*it) { + case AllowOverride::No: + return message; + case AllowOverride::Yes: + message.setUrl(objectContainer->url()); + enginePrivate->warning(message); + *it = AllowOverride::No; // No further overriding allowed. + break; + } + } cache->appendSignal(signalName, flags, effectiveMethodIndex++, paramCount?paramTypes.constData():nullptr, names); } @@ -569,8 +598,23 @@ inline QQmlError QQmlPropertyCacheCreator<ObjectContainer>::createMetaObject( auto flags = QQmlPropertyData::defaultSlotFlags(); const QString slotName = stringAt(function->nameIndex); - if (seenSignals.contains(slotName)) - return qQmlCompileError(function->location, QQmlPropertyCacheCreatorBase::tr("Duplicate method name: invalid override of property change signal or superclass signal")); + const auto it = seenSignals.constFind(slotName); + if (it != seenSignals.constEnd()) { + // TODO: Remove the AllowOverride::Yes branch once we can. + QQmlError message = qQmlCompileError( + function->location, + QQmlPropertyCacheCreatorBase::tr( + "Duplicate method name: " + "invalid override of property change signal or superclass signal")); + switch (*it) { + case AllowOverride::No: + return message; + case AllowOverride::Yes: + message.setUrl(objectContainer->url()); + enginePrivate->warning(message); + break; + } + } // Note: we don't append slotName to the seenSignals list, since we don't // protect against overriding change signals or methods with properties. diff --git a/src/qml/qml/qqmlpropertydata_p.h b/src/qml/qml/qqmlpropertydata_p.h index 6653d99c55..8ca2a19d6f 100644 --- a/src/qml/qml/qqmlpropertydata_p.h +++ b/src/qml/qml/qqmlpropertydata_p.h @@ -63,7 +63,7 @@ public: // b when type equals FunctionType. For that reason, the semantic meaning of the bit is // overloaded, and the accessor functions are used to get the correct value // - // Moreover, isSignalHandler, isOverload and isCloned make only sense + // Moreover, isSignalHandler, isOverridableSignal and isCloned make only sense // for functions, too (and could at a later point be reused for flags that only make sense // for non-functions) // @@ -76,7 +76,10 @@ public: unsigned isAliasORisVMESignal : 1; // Is a QML alias to another property OR Signal was added by QML unsigned isFinalORisV4Function : 1; // Has FINAL flag OR Function takes QQmlV4Function* args unsigned isSignalHandler : 1; // Function is a signal handler - unsigned isOverload : 1; // Function is an overload of another function + + // TODO: Remove this once we can. Signals should not be overridable. + unsigned isOverridableSignal : 1; // Function is an overridable signal + unsigned isRequiredORisCloned : 1; // Has REQUIRED flag OR The function was marked as cloned unsigned isConstructorORisBindable : 1; // The function was marked is a constructor OR property is backed by QProperty<T> unsigned isOverridden : 1; // Is overridden by a extension property @@ -157,9 +160,11 @@ public: isSignalHandler = b; } - void setIsOverload(bool b) { + // TODO: Remove this once we can. Signals should not be overridable. + void setIsOverridableSignal(bool b) { Q_ASSERT(type == FunctionType); - isOverload = b; + Q_ASSERT(isResettableORisSignal); + isOverridableSignal = b; } void setIsCloned(bool b) { @@ -209,8 +214,10 @@ public: bool isVMESignal() const { return isFunction() && m_flags.isAliasORisVMESignal; } bool isV4Function() const { return isFunction() && m_flags.isFinalORisV4Function; } bool isSignalHandler() const { return m_flags.isSignalHandler; } - bool isOverload() const { return m_flags.isOverload; } - void setOverload(bool onoff) { m_flags.isOverload = onoff; } + + // TODO: Remove this once we can. Signals should not be overridable. + bool isOverridableSignal() const { return m_flags.isOverridableSignal; } + bool isCloned() const { return isFunction() && m_flags.isRequiredORisCloned; } bool isConstructor() const { return isFunction() && m_flags.isConstructorORisBindable; } bool isBindable() const { return !isFunction() && m_flags.isConstructorORisBindable; } @@ -417,7 +424,7 @@ QQmlPropertyData::Flags::Flags() , isAliasORisVMESignal(false) , isFinalORisV4Function(false) , isSignalHandler(false) - , isOverload(false) + , isOverridableSignal(false) , isRequiredORisCloned(false) , isConstructorORisBindable(false) , isOverridden(false) diff --git a/tests/auto/qml/qmlcppcodegen/data/ProgressBar/TimelineAnimation.qml b/tests/auto/qml/qmlcppcodegen/data/ProgressBar/TimelineAnimation.qml index b97e8956bf..75ad2245f2 100644 --- a/tests/auto/qml/qmlcppcodegen/data/ProgressBar/TimelineAnimation.qml +++ b/tests/auto/qml/qmlcppcodegen/data/ProgressBar/TimelineAnimation.qml @@ -2,5 +2,5 @@ import QtQuick NumberAnimation { property bool pingPong - signal finished() + signal finishedEvil() } diff --git a/tests/auto/qml/qqmlpropertycache/data/overriddenSignal.qml b/tests/auto/qml/qqmlpropertycache/data/overriddenSignal.qml new file mode 100644 index 0000000000..c948d47a1b --- /dev/null +++ b/tests/auto/qml/qqmlpropertycache/data/overriddenSignal.qml @@ -0,0 +1,36 @@ +import QtQml +import Test.PropertyCache + +QtObject { + id: root + + property BaseObject obj: null + property Connections connection: Connections { + target: obj + function onPropertyAChanged() { ++root.a } + } + onObjChanged: { + connection.target = obj // Make sure this takes effect before sending the signal + obj.propertyAChanged() + } + + property BaseObject obj2: null + property Connections connection2: Connections { + target: obj2 + function onSignalA() { ++root.b } + } + onObj2Changed: { + connection2.target = obj2 // Make sure this takes effect before sending the signal + obj2.signalA(); + } + + property BaseObject theObj: BaseObject {} + Component.onCompleted: { + // Make sure the change signals are triggered also initially + obj = theObj; + obj2 = theObj; + } + + property int a: 0 + property int b: 0 +} diff --git a/tests/auto/qml/qqmlpropertycache/data/qmlOverriddenSignal.qml b/tests/auto/qml/qqmlpropertycache/data/qmlOverriddenSignal.qml new file mode 100644 index 0000000000..b8fee08978 --- /dev/null +++ b/tests/auto/qml/qqmlpropertycache/data/qmlOverriddenSignal.qml @@ -0,0 +1,8 @@ +import QtQml +import Test.PropertyCache + +BaseObject { + property int callCount: 0 + function signalA() { ++callCount } + Component.onCompleted: signalA() +} diff --git a/tests/auto/qml/qqmlpropertycache/data/qmlOverriddenSignal2.qml b/tests/auto/qml/qqmlpropertycache/data/qmlOverriddenSignal2.qml new file mode 100644 index 0000000000..cbf1cfe037 --- /dev/null +++ b/tests/auto/qml/qqmlpropertycache/data/qmlOverriddenSignal2.qml @@ -0,0 +1,6 @@ +import QtQml +import Test.PropertyCache + +BaseObject { + signal propertyAChanged +} diff --git a/tests/auto/qml/qqmlpropertycache/tst_qqmlpropertycache.cpp b/tests/auto/qml/qqmlpropertycache/tst_qqmlpropertycache.cpp index cfea7c0da1..589f95a3de 100644 --- a/tests/auto/qml/qqmlpropertycache/tst_qqmlpropertycache.cpp +++ b/tests/auto/qml/qqmlpropertycache/tst_qqmlpropertycache.cpp @@ -8,6 +8,7 @@ #include <QtQml/qqmlcomponent.h> #include <private/qmetaobjectbuilder_p.h> #include <private/qqmlcontextdata_p.h> +#include <private/qqmlpropertycachecreator_p.h> #include <QCryptographicHash> #include <QtQuickTestUtils/private/qmlutils_p.h> @@ -34,6 +35,7 @@ private slots: void derivedGadgetMethod(); void restrictRegistrationVersion(); void rejectOverriddenFinal(); + void overriddenSignals(); private: QQmlEngine engine; @@ -164,6 +166,19 @@ Q_SIGNALS: void signalB(); }; +class OverriddenSignal : public BaseObject +{ + Q_OBJECT +public: + OverriddenSignal(QObject *parent = nullptr) : BaseObject(parent) {} + + Q_INVOKABLE void propertyAChanged() { ++propertyAChangedCalled; } + int propertyAChangedCalled = 0; + +Q_SIGNALS: + void signalA(); +}; + const QQmlPropertyData *cacheProperty(const QQmlPropertyCache::ConstPtr &cache, const char *name) { return cache->property(QLatin1String(name), nullptr, nullptr); @@ -707,4 +722,56 @@ void tst_qqmlpropertycache::rejectOverriddenFinal() QCOMPARE(o->property("c").toInt(), 0); } +void tst_qqmlpropertycache::overriddenSignals() +{ + qmlRegisterTypesAndRevisions<BaseObject>("Test.PropertyCache", 3); + QQmlEngine engine; + + QQmlComponent c1(&engine, testFileUrl("overriddenSignal.qml")); + QVERIFY2(!c1.isError(), qPrintable(c1.errorString())); + + QScopedPointer<QObject> o(c1.create()); + + // the propertyAChanged _signal_ is sent once (initially). + QCOMPARE(o->property("a").toInt(), 1); + + // signalA() is invoked once as signal, and the other time as method since both are C++. + QCOMPARE(o->property("b").toInt(), 1); + + OverriddenSignal *derived = new OverriddenSignal(o.data()); + + // Does call our overridden method, since that is defined in C++ + QCOMPARE(derived->propertyAChangedCalled, 0); + o->setProperty("obj", QVariant::fromValue(derived)); + QCOMPARE(derived->propertyAChangedCalled, 1); + + o->setProperty("obj2", QVariant::fromValue(derived)); + + // the propertyAChanged _signal_ is sent once (initially). + QCOMPARE(o->property("a").toInt(), 1); + + // We get to receive both signalA() signals since we only match by name. + QCOMPARE(o->property("b").toInt(), 2); + + // We shouldn't be allowed to define such things in QML, though. + + const QUrl c2Url = testFileUrl("qmlOverriddenSignal.qml"); + QTest::ignoreMessage( + QtWarningMsg, + qPrintable(c2Url.toString() + QLatin1String( + ":6:14: Duplicate method name: " + "invalid override of property change signal or superclass signal"))); + QQmlComponent c2(&engine, c2Url); + // Should be an error, but we can't enforce it yet. + + const QUrl c3Url = testFileUrl("qmlOverriddenSignal2.qml"); + QTest::ignoreMessage( + QtWarningMsg, + qPrintable(c3Url.toString() + QLatin1String( + ":5:12: Duplicate signal name: " + "invalid override of property change signal or superclass signal"))); + QQmlComponent c3(&engine, c3Url); + // Should be an error, but we can't enforce it yet. +} + QTEST_MAIN(tst_qqmlpropertycache) diff --git a/tests/auto/quick/qquickcanvasitem/data/tst_invalidContext.qml b/tests/auto/quick/qquickcanvasitem/data/tst_invalidContext.qml index 0df536cc02..690e00a822 100644 --- a/tests/auto/quick/qquickcanvasitem/data/tst_invalidContext.qml +++ b/tests/auto/quick/qquickcanvasitem/data/tst_invalidContext.qml @@ -28,16 +28,16 @@ Item { anchors.fill: parent property var paintContext: null - function paint() { + function doPaint() { paintContext.fillStyle = Qt.rgba(1, 0, 0, 1); paintContext.fillRect(0, 0, width, height); - requestAnimationFrame(paint); + requestAnimationFrame(doPaint); } onAvailableChanged: { if (available) { paintContext = getContext("2d") - requestAnimationFrame(paint); + requestAnimationFrame(doPaint); } } } |