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>

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);
             }
         }
     }