diff options
author | Ulf Hermann <ulf.hermann@qt.io> | 2023-08-28 11:18:04 +0200 |
---|---|---|
committer | Ulf Hermann <ulf.hermann@qt.io> | 2023-09-05 11:41:25 +0200 |
commit | 36478e252f0c4fff1553a611619aa32d1f79efb5 (patch) | |
tree | 9c1c36d481481a6bc9e5f203d4bf6dd5c42516fa /src/qml | |
parent | 8742807c2f1a3937b2d05aae8490a7a8e084e514 (diff) |
QtQml: Cache correct properties for signal handlers
This exposes some invalid QML code in our tests: We shouldn't override
signals in QML and we now notice. Since banning it outright would be a
pretty drastic change in behavior, we only print a warning for now.
Coverity-Id: 416620
Change-Id: I28c6b818c4abccb830b0e6998fe840bf229bf081
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Sami Shalayel <sami.shalayel@qt.io>
Diffstat (limited to 'src/qml')
-rw-r--r-- | src/qml/qml/qqmlpropertycache.cpp | 49 | ||||
-rw-r--r-- | src/qml/qml/qqmlpropertycachecreator_p.h | 66 | ||||
-rw-r--r-- | src/qml/qml/qqmlpropertydata_p.h | 21 |
3 files changed, 94 insertions, 42 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) |