QWidget: cope with QObject::connect()'s incomplete SFINAE-friendliness

The plan for QObject::connect() (perfect) forwarders, such as
QWidget::addAction(), was to just use a variant of the Detection Idiom
to see whether QObject::connect() with the arguments as given would
compile and SFINAE out the forwarder otherwise.

It turns out that the "functor" overload of QObject::connect(), in
particular, is severly underconstrained and accepts e.g. QKeySequence
as a function object, only erroring out via a static_assert() in the
body of the function, and thus at instantiation time and not, as
needed, at overload resolution time.

At the same time, we don't really want QObject::connect() to SFINAE
out on argument mismatches between signal and slot, because the
resulting error messages would be ... unkind to users of the API. We
would like to keep the static_assert()s for easier error reporting.

Reconciling these two contradicting requirements has so far eluded
this author, so for now, to unblock progress, we explicitly black-
and, in one case, white-list possible arguments. Because QKeySequence,
in particular, is implicitly constructible from int(!), and therefore
any enum type(!), incl. Qt::ConnectionType, we need to do way too much
coding in the addAction() constraints. Hopefully, we'll be able to fix
the issue at the root cause, in QObject, before Qt 6.3 is out, but
until then, this is an ok-ish stop-gap measure.

Add thorough overload set checks (positive ones only, for now) to
tst_qwidget and tst_qmenu.

Change-Id: Ia05233df818bc82ecc924fc44c1b349af41cbbf1
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

1 files changed, 67 insertions, 0 deletions

diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
index 7ca08993ab..4fa568228a 100644
--- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
+++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
@@ -160,6 +160,7 @@ public slots:
     void initTestCase();
     void cleanup();
 private slots:
+    void addActionOverloads();
     void getSetCheck();
     void fontPropagation();
     void fontPropagation2();
@@ -644,6 +645,72 @@ void tst_QWidget::cleanup()
     QTRY_VERIFY(QApplication::topLevelWidgets().isEmpty());
 }
 
+template <typename T>
+struct ImplicitlyConvertibleTo {
+    T t;
+    operator const T() const { return t; }
+    operator T() { return t; }
+};
+
+void testFunction0() {}
+void testFunction1(bool) {}
+
+void tst_QWidget::addActionOverloads()
+{
+    // almost exhaustive check of addAction() overloads:
+    // (text), (icon, text), (icon, text, shortcut), (text, shortcut)
+    // each with a good sample of ways to QObject::connect() to
+    // QAction::triggered(bool)
+    QWidget w;
+
+    // don't just pass QString etc - that'd be too easy (think QStringBuilder)
+    ImplicitlyConvertibleTo<QString> text = {QStringLiteral("foo")};
+    ImplicitlyConvertibleTo<QIcon> icon;
+
+    const auto check = [&](auto &...args) { // don't need to perfectly-forward, only lvalues passed
+        w.addAction(args...);
+
+        w.addAction(args..., &w, SLOT(deleteLater()));
+        w.addAction(args..., &w, &QObject::deleteLater);
+        w.addAction(args..., testFunction0);
+        w.addAction(args..., &w, testFunction0);
+        w.addAction(args..., testFunction1);
+        w.addAction(args..., &w, testFunction1);
+        w.addAction(args..., [&](bool b) { w.setEnabled(b); });
+        w.addAction(args..., &w, [&](bool b) { w.setEnabled(b); });
+
+        w.addAction(args..., &w, SLOT(deleteLater()), Qt::QueuedConnection);
+        w.addAction(args..., &w, &QObject::deleteLater, Qt::QueuedConnection);
+        // doesn't exist: w.addAction(args..., testFunction0, Qt::QueuedConnection);
+        w.addAction(args..., &w, testFunction0, Qt::QueuedConnection);
+        // doesn't exist: w.addAction(args..., testFunction1, Qt::QueuedConnection);
+        w.addAction(args..., &w, testFunction1, Qt::QueuedConnection);
+        // doesn't exist: w.addAction(args..., [&](bool b) { w.setEnabled(b); }, Qt::QueuedConnection);
+        w.addAction(args..., &w, [&](bool b) { w.setEnabled(b); }, Qt::QueuedConnection);
+    };
+    const auto check1 = [&](auto &arg, auto &...args) {
+        check(arg, args...);
+        check(std::as_const(arg), args...);
+    };
+    const auto check2 = [&](auto &arg1, auto &arg2, auto &...args) {
+        check1(arg1, arg2, args...);
+        check1(arg1, std::as_const(arg2), args...);
+    };
+    [[maybe_unused]]
+    const auto check3 = [&](auto &arg1, auto &arg2, auto &arg3) {
+        check2(arg1, arg2, arg3);
+        check2(arg1, arg2, std::as_const(arg3));
+    };
+
+    check1(text);
+    check2(icon, text);
+#ifndef QT_NO_SHORTCUT
+    ImplicitlyConvertibleTo<QKeySequence> keySequence = {Qt::CTRL | Qt::Key_C};
+    check2(text, keySequence);
+    check3(icon, text, keySequence);
+#endif
+}
+
 void tst_QWidget::fontPropagation()
 {
     QScopedPointer<QWidget> testWidget(new QWidget);