Warn if signals and slots are out of order

The last improvement to signals and slots registration has removed
the problem that mixin classes were not handled the same as other classes.
The key idea was to use the MRO to inspect all involved classes
at type parsing time.

The signals and slots were then sorted in ‘updateMetaObject’.

The current patch enhances this solution in the following way:

In ‘parsePythonType’ we re-ordered the introspection loop in a way
that now all signals are extracted first. This way, they are ordered before
slots automatically, and a later sorting is not necessary. Furthermore,
instead of using qStableSort in ‘updateMetaObject’, we now check the
sorting only with ‘is_sorted’. If the sort order becomes invalid, it will issue a warning.

The latest change removes quadratic time behavior from the warning output.

Important notes and implications:
It is no longer relevant if slots are decorated with @QtCore.Slot().
Signals will work in normally created classes of all shapes.
If classes are modified later, they may grow an incompatibility
when signals appear after slots. Then you get a warning.

Adendum: It makes sense to use slots whenever possible. This way,
constructions as in static_metaobject_test.py work.

Now handling the warning correctly when it is turned into an error.
Correction: We leave the warning as-is and ignore the error!

Question: The static_metaobject_test.py passes its tests suddenly, when you use @Slot() !
Why? Should we open an extra issue for that?

Task-number: PYSIDE-315
Change-Id: I75c9c88787cd93251b1439db0088cd66fc0c3c97
Reviewed-by: Alexandru Croitor <alexandru.croitor@qt.io>

4 files changed, 156 insertions, 21 deletions

diff --git a/libpyside/dynamicqmetaobject.cpp b/libpyside/dynamicqmetaobject.cpp
index 11085f1c..651ed70c 100644
--- a/libpyside/dynamicqmetaobject.cpp
+++ b/libpyside/dynamicqmetaobject.cpp
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2017 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of PySide2.
@@ -131,9 +131,9 @@ public:
 
 bool sortMethodSignalSlot(const MethodData &m1, const MethodData &m2)
 {
-   if (m1.methodType() == QMetaMethod::Signal)
-      return m2.methodType() == QMetaMethod::Slot;
-   return false;
+    if (m1.methodType() == QMetaMethod::Signal)
+        return m2.methodType() == QMetaMethod::Slot;
+    return false;
 }
 
 static int registerString(const QByteArray& s, QLinkedList<QByteArray>& strings)
@@ -510,7 +510,8 @@ int DynamicQMetaObject::addProperty(const char* propertyName, PyObject* data)
     return  m_d->m_propertyOffset + index;
 }
 
-int DynamicQMetaObject::DynamicQMetaObjectPrivate::getPropertyNotifyId(PySideProperty *property) const {
+int DynamicQMetaObject::DynamicQMetaObjectPrivate::getPropertyNotifyId(PySideProperty *property) const
+{
     int notifyId = -1;
     if (property->d->notify) {
         const char *signalNotify = PySide::Property::getNotifyName(property);
@@ -607,8 +608,7 @@ void DynamicQMetaObject::parsePythonType(PyTypeObject *type)
 
     // Prepend the actual type that we are parsing.
     basesToCheck.prepend(type);
-
-    Shiboken::AutoDecRef slotAttrName(Shiboken::String::fromCString(PYSIDE_SLOT_LIST_ATTR));
+    // PYSIDE-315: Handle all signals first, in all involved types.
     for (int baseIndex = 0, baseEnd = basesToCheck.size(); baseIndex < baseEnd; ++baseIndex) {
         PyTypeObject *baseType = basesToCheck[baseIndex];
         PyObject *attrs = baseType->tp_dict;
@@ -616,17 +616,8 @@ void DynamicQMetaObject::parsePythonType(PyTypeObject *type)
         PyObject *value = 0;
         Py_ssize_t pos = 0;
 
-        typedef std::pair<const char *, PyObject *> PropPair;
-        QVector<PropPair> properties;
-
         while (PyDict_Next(attrs, &pos, &key, &value)) {
-            if (Property::checkType(value)) {
-                // Leave the properties to be registered after signals because they may depend on
-                // notify signals.
-                int index = d.superdata->indexOfProperty(Shiboken::String::toCString(key));
-                if (index == -1)
-                    properties << PropPair(Shiboken::String::toCString(key), value);
-            } else if (Signal::checkType(value)) {
+            if (Signal::checkType(value)) {
                 // Register signals.
                 PySideSignal *data = reinterpret_cast<PySideSignal *>(value);
                 const char *signalName = Shiboken::String::toCString(key);
@@ -642,6 +633,31 @@ void DynamicQMetaObject::parsePythonType(PyTypeObject *type)
                     if (d.superdata->indexOfSignal(sig) == -1)
                         addSignal(sig, "void");
                 }
+            }
+        }
+    }
+
+    Shiboken::AutoDecRef slotAttrName(Shiboken::String::fromCString(PYSIDE_SLOT_LIST_ATTR));
+    // PYSIDE-315: Now take care of the rest.
+    // Signals and slots should be separated, unless the types are modified, later.
+    // We check for this using "is_sorted()". Sorting no longer happens at all.
+    for (int baseIndex = 0, baseEnd = basesToCheck.size(); baseIndex < baseEnd; ++baseIndex) {
+        PyTypeObject *baseType = basesToCheck[baseIndex];
+        PyObject *attrs = baseType->tp_dict;
+        PyObject *key = 0;
+        PyObject *value = 0;
+        Py_ssize_t pos = 0;
+
+        typedef std::pair<const char *, PyObject *> PropPair;
+        QVector<PropPair> properties;
+
+        while (PyDict_Next(attrs, &pos, &key, &value)) {
+            if (Property::checkType(value)) {
+                // Leave the properties to be registered after signals because they may depend on
+                // notify signals.
+                int index = d.superdata->indexOfProperty(Shiboken::String::toCString(key));
+                if (index == -1)
+                    properties << PropPair(Shiboken::String::toCString(key), value);
             } else if (PyFunction_Check(value)) {
                 // Register slots.
                 if (PyObject_HasAttr(value, slotAttrName)) {
@@ -722,6 +738,26 @@ void DynamicQMetaObject::DynamicQMetaObjectPrivate::writeStringData(char *out,
    }
 }
 
+QList<MethodData>::iterator is_sorted_until(QList<MethodData>::iterator first,
+                                            QList<MethodData>::iterator last,
+                                            bool comp(const MethodData &m1, const MethodData &m2))
+{
+    if (first != last) {
+        QList<MethodData>::iterator next = first;
+        while (++next != last) {
+            if (comp(*next, *first))
+                return next;
+            ++first;
+        }
+    }
+    return last;
+}
+
+bool is_sorted(QList<MethodData>::iterator first, QList<MethodData>::iterator last,
+               bool comp(const MethodData &m1, const MethodData &m2))
+{
+    return is_sorted_until(first, last, comp) == last;
+}
 
 void DynamicQMetaObject::DynamicQMetaObjectPrivate::updateMetaObject(QMetaObject* metaObj)
 {
@@ -757,7 +793,33 @@ void DynamicQMetaObject::DynamicQMetaObjectPrivate::updateMetaObject(QMetaObject
     // Write methods first, then properties, to be consistent with moc.
     // Write signals/slots (signals must be written first, see indexOfMethodRelative in
     // qmetaobject.cpp).
-    qStableSort(m_methods.begin(), m_methods.end(), sortMethodSignalSlot);
+
+    QList<MethodData>::iterator it;
+    // PYSIDE-315: Instead of sorting the items and maybe breaking indices,
+    // we ensure that the signals and slots are sorted by the improved parsePythonType().
+    // The order can only become distorted if the class is modified after creation.
+    // In that case, we give a warning.
+    if (!is_sorted(m_methods.begin(), m_methods.end(), sortMethodSignalSlot)) {
+        const char *metaObjectName = this->m_className.data();
+        PyObject *txt = PyBytes_FromFormat("\n\n*** Sort Warning ***\n"
+                            "Signals and slots in QMetaObject '%s' are not ordered correctly, "
+                            "this may lead to issues.\n", metaObjectName);
+        it = m_methods.begin();
+        QList<MethodData>::iterator end = m_methods.end();
+        QList<MethodData>::iterator until = is_sorted_until(m_methods.begin(), m_methods.end(),
+                                                            sortMethodSignalSlot);
+        for (; it != end; ++it) {
+            PyObject *atxt = PyBytes_FromFormat("%d%s %s %s\n", it - m_methods.begin() + 1,
+                                 until >= it + 1 ? " " : "!",
+                                 it->methodType() == QMetaMethod::Signal ? "Signal" : "Slot  ",
+                                 it->signature().data() );
+            PyBytes_ConcatAndDel(&txt, atxt);
+        }
+        PyErr_WarnEx(PyExc_RuntimeWarning, PyBytes_AsString(txt), 0);
+        Py_DECREF(txt);
+        // Prevent a warning from being turned into an error. We cannot easily unwind.
+        PyErr_Clear();
+    }
 
     if (m_methods.size()) {
         if (data[5] == 0)
@@ -768,8 +830,7 @@ void DynamicQMetaObject::DynamicQMetaObjectPrivate::updateMetaObject(QMetaObject
 
     // Write signal/slots parameters.
     if (m_methods.size()) {
-       QList<MethodData>::iterator it = m_methods.begin();
-       for (; it != m_methods.end(); ++it) {
+       for (it = m_methods.begin(); it != m_methods.end(); ++it) {
           QList<QByteArray> paramTypeNames = it->parameterTypes();
           int paramCount = paramTypeNames.size();
           for (int i = -1; i < paramCount; ++i) {
diff --git a/tests/pysidetest/CMakeLists.txt b/tests/pysidetest/CMakeLists.txt
index 576996e8..09534904 100644
--- a/tests/pysidetest/CMakeLists.txt
+++ b/tests/pysidetest/CMakeLists.txt
@@ -113,3 +113,4 @@ PYSIDE_TEST(typedef_signal_test.py)
 PYSIDE_TEST(bug_1016.py)
 PYSIDE_TEST(utils_test.py)
 PYSIDE_TEST(mixin_signal_slots_test.py)
+PYSIDE_TEST(signal_slot_warning.py)
diff --git a/tests/pysidetest/signal_slot_warning.py b/tests/pysidetest/signal_slot_warning.py
new file mode 100644
index 00000000..fd3d41b5
--- /dev/null
+++ b/tests/pysidetest/signal_slot_warning.py
@@ -0,0 +1,70 @@
+#!/usr/bin/python
+
+#############################################################################
+##
+## Copyright (C) 2017 The Qt Company Ltd.
+## Contact: https://www.qt.io/licensing/
+##
+## This file is part of the test suite of PySide2.
+##
+## $QT_BEGIN_LICENSE:GPL-EXCEPT$
+## Commercial License Usage
+## Licensees holding valid commercial Qt licenses may use this file in
+## accordance with the commercial license agreement provided with the
+## Software or, alternatively, in accordance with the terms contained in
+## a written agreement between you and The Qt Company. For licensing terms
+## and conditions see https://www.qt.io/terms-conditions. For further
+## information use the contact form at https://www.qt.io/contact-us.
+##
+## GNU General Public License Usage
+## Alternatively, this file may be used under the terms of the GNU
+## General Public License version 3 as published by the Free Software
+## Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+## included in the packaging of this file. Please review the following
+## information to ensure the GNU General Public License requirements will
+## be met: https://www.gnu.org/licenses/gpl-3.0.html.
+##
+## $QT_END_LICENSE$
+##
+#############################################################################
+
+''' PYSIDE-315: https://bugreports.qt.io/browse/PYSIDE-315
+    Test that creating a signal in the wrong order triggers a warning. '''
+
+from __future__ import print_function
+
+import unittest
+import PySide2.QtCore as QtCore
+import sys
+import warnings
+
+
+class Whatever(QtCore.QObject):
+    echoSignal = QtCore.Signal(str)
+
+    def __init__(self):
+        super(Whatever, self).__init__()
+        self.echoSignal.connect(self.mySlot)
+
+    def mySlot(self, v):
+        pass
+
+class WarningTest(unittest.TestCase):
+    def testSignalSlotWarning(self):
+        # we create an object. This gives no warning.
+        obj = Whatever()
+        # then we insert a signal after slots have been created.
+        setattr(Whatever, "foo", QtCore.Signal())
+        with warnings.catch_warnings(record=True) as w:
+            # Cause all warnings to always be triggered.
+            warnings.simplefilter("always")
+            # Trigger a warning.
+            obj.foo.connect(obj.mySlot)
+            # Verify some things
+            assert issubclass(w[-1].category, RuntimeWarning)
+            assert "*** Sort Warning ***" in str(w[-1].message)
+            # note that this warning cannot be turned into an error (too hard)
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/tests/signals/static_metaobject_test.py b/tests/signals/static_metaobject_test.py
index ae16ce3b..e0575bc3 100644
--- a/tests/signals/static_metaobject_test.py
+++ b/tests/signals/static_metaobject_test.py
@@ -32,7 +32,7 @@
 
 import unittest
 
-from PySide2.QtCore import QObject, SIGNAL
+from PySide2.QtCore import QObject, SIGNAL, Slot
 from helper import UsesQCoreApplication
 
 class MyObject(QObject):
@@ -40,6 +40,9 @@ class MyObject(QObject):
         QObject.__init__(self, parent)
         self._slotCalledCount = 0
 
+    # this '@Slot()' is needed to get the right sort order in testSharedSignalEmission.
+    # For some reason, it also makes the tests actually work!
+    @Slot()
     def mySlot(self):
         self._slotCalledCount = self._slotCalledCount + 1