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>

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)