Introduce signal-with-return-value

It's a design smell to have signals returning values

12 files changed, 142 insertions, 0 deletions

diff --git a/Changelog b/Changelog
index 65f604ff..3d218076 100644
--- a/Changelog
+++ b/Changelog
@@ -109,6 +109,7 @@
 * v1.6 (, 2019)
   - New Checks:
     - heap-allocated-small-trivial-type
+    - signal-with-return-value
   - Moved all level3 checks to manual level. Doesn't make sense to enable all of them. Each one must be
   carefully considered.
   - Fixit infrastructure was overhauled
diff --git a/CheckSources.cmake b/CheckSources.cmake
index cb541688..34132e29 100644
--- a/CheckSources.cmake
+++ b/CheckSources.cmake
@@ -14,6 +14,7 @@ set(CLAZY_CHECKS_SRCS ${CLAZY_CHECKS_SRCS}
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/qvariant-template-instantiation.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/raw-environment-function.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/reserve-candidates.cpp
+  ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/signal-with-return-value.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/thread-with-slots.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/tr-non-literal.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/unneeded-cast.cpp
diff --git a/README.md b/README.md
index 538a6518..8809ca65 100644
--- a/README.md
+++ b/README.md
@@ -230,6 +230,7 @@ clazy runs all checks from level1 by default.
     - [qvariant-template-instantiation](docs/checks/README-qvariant-template-instantiation.md)
     - [raw-environment-function](docs/checks/README-raw-environment-function.md)
     - [reserve-candidates](docs/checks/README-reserve-candidates.md)
+    - [signal-with-return-value](docs/checks/README-signal-with-return-value.md)
     - [thread-with-slots](docs/checks/README-thread-with-slots.md)
     - [tr-non-literal](docs/checks/README-tr-non-literal.md)
     - [unneeded-cast](docs/checks/README-unneeded-cast.md)
diff --git a/checks.json b/checks.json
index 0b409176..58bfe403 100644
--- a/checks.json
+++ b/checks.json
@@ -11,6 +11,12 @@
             ]
         },
         {
+            "name"  : "signal-with-return-value",
+            "level" : -1,
+            "visits_decls" : true
+
+        },
+        {
             "name"  : "heap-allocated-small-trivial-type",
             "level" : -1,
             "categories" : ["performance"],
diff --git a/docs/checks/README-signal-with-return-value.md b/docs/checks/README-signal-with-return-value.md
new file mode 100644
index 00000000..144abcd3
--- /dev/null
+++ b/docs/checks/README-signal-with-return-value.md
@@ -0,0 +1,7 @@
+# signal-with-return-value
+
+Returning values from signals is considered bad design, as there might be more
+than 1 slot connected to it.  In general signals should just emit that some state
+has changed and know nothing about who's connected to it.
+
+Needing a return value is a design smell that you probably should be using a direct call instead.
diff --git a/readmes.cmake b/readmes.cmake
index 8d3953ac..f833076b 100644
--- a/readmes.cmake
+++ b/readmes.cmake
@@ -14,6 +14,7 @@ SET(README_manuallevel_FILES
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-qvariant-template-instantiation.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-raw-environment-function.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-reserve-candidates.md
+    ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-signal-with-return-value.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-thread-with-slots.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-tr-non-literal.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-unneeded-cast.md
diff --git a/src/Checks.h b/src/Checks.h
index 00cb6b9c..a2df0180 100644
--- a/src/Checks.h
+++ b/src/Checks.h
@@ -42,6 +42,7 @@
 #include "checks/manuallevel/qvariant-template-instantiation.h"
 #include "checks/manuallevel/raw-environment-function.h"
 #include "checks/manuallevel/reserve-candidates.h"
+#include "checks/manuallevel/signal-with-return-value.h"
 #include "checks/manuallevel/thread-with-slots.h"
 #include "checks/manuallevel/tr-non-literal.h"
 #include "checks/manuallevel/unneeded-cast.h"
@@ -135,6 +136,7 @@ void CheckManager::registerChecks()
     registerCheck(check<QVariantTemplateInstantiation>("qvariant-template-instantiation", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
     registerCheck(check<RawEnvironmentFunction>("raw-environment-function", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
     registerCheck(check<ReserveCandidates>("reserve-candidates", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
+    registerCheck(check<SignalWithReturnValue>("signal-with-return-value", ManualCheckLevel,  RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<ThreadWithSlots>("thread-with-slots", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts | RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<TrNonLiteral>("tr-non-literal", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
     registerCheck(check<UnneededCast>("unneeded-cast", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
diff --git a/src/checks/manuallevel/signal-with-return-value.cpp b/src/checks/manuallevel/signal-with-return-value.cpp
new file mode 100644
index 00000000..25884374
--- /dev/null
+++ b/src/checks/manuallevel/signal-with-return-value.cpp
@@ -0,0 +1,61 @@
+/*
+  This file is part of the clazy static checker.
+
+  Copyright (C) 2019 Sergio Martins <smartins@kde.org>
+
+  This library is free software; you can redistribute it and/or
+  modify it under the terms of the GNU Library General Public
+  License as published by the Free Software Foundation; either
+  version 2 of the License, or (at your option) any later version.
+
+  This library is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  Library General Public License for more details.
+
+  You should have received a copy of the GNU Library General Public License
+  along with this library; see the file COPYING.LIB.  If not, write to
+  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+  Boston, MA 02110-1301, USA.
+*/
+
+#include "signal-with-return-value.h"
+#include "Utils.h"
+#include "HierarchyUtils.h"
+#include "QtUtils.h"
+#include "TypeUtils.h"
+#include "AccessSpecifierManager.h"
+#include "ClazyContext.h"
+
+#include <clang/AST/AST.h>
+
+using namespace clang;
+using namespace std;
+
+
+SignalWithReturnValue::SignalWithReturnValue(const std::string &name, ClazyContext *context)
+    : CheckBase(name, context)
+{
+    context->enableAccessSpecifierManager();
+}
+
+void SignalWithReturnValue::VisitDecl(clang::Decl *decl)
+{
+    AccessSpecifierManager *accessSpecifierManager = m_context->accessSpecifierManager;
+    auto method = dyn_cast<CXXMethodDecl>(decl);
+    if (!accessSpecifierManager || !method)
+        return;
+
+    if (method->isThisDeclarationADefinition() && !method->hasInlineBody())
+        return;
+
+    const bool methodIsSignal = accessSpecifierManager->qtAccessSpecifierType(method) == QtAccessSpecifier_Signal;
+    if (!methodIsSignal)
+        return;
+
+    if (!method->getReturnType()->isVoidType()) {
+        if (!accessSpecifierManager->isScriptable(method)) {
+            emitWarning(decl, std::string(clazy::name(method)) + "() should return void. For a clean design signals shouldn't assume a single slot are connected to them.");
+        }
+    }
+}
diff --git a/src/checks/manuallevel/signal-with-return-value.h b/src/checks/manuallevel/signal-with-return-value.h
new file mode 100644
index 00000000..b94685e1
--- /dev/null
+++ b/src/checks/manuallevel/signal-with-return-value.h
@@ -0,0 +1,40 @@
+/*
+  This file is part of the clazy static checker.
+
+  Copyright (C) 2019 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com
+  Author: Sergio Martins <sergio.martins@kdab.com>
+
+  This library is free software; you can redistribute it and/or
+  modify it under the terms of the GNU Library General Public
+  License as published by the Free Software Foundation; either
+  version 2 of the License, or (at your option) any later version.
+
+  This library is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  Library General Public License for more details.
+
+  You should have received a copy of the GNU Library General Public License
+  along with this library; see the file COPYING.LIB.  If not, write to
+  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+  Boston, MA 02110-1301, USA.
+*/
+
+#ifndef CLAZY_SIGNAL_WITH_RETURN_VALUE_H
+#define CLAZY_SIGNAL_WITH_RETURN_VALUE_H
+
+#include "checkbase.h"
+
+
+/**
+ * See README-signal-with-return-value.md for more info.
+ */
+class SignalWithReturnValue : public CheckBase
+{
+public:
+    explicit SignalWithReturnValue(const std::string &name, ClazyContext *context);
+    void VisitDecl(clang::Decl *) override;
+private:
+};
+
+#endif
diff --git a/tests/signal-with-return-value/config.json b/tests/signal-with-return-value/config.json
new file mode 100644
index 00000000..e7e6e0cb
--- /dev/null
+++ b/tests/signal-with-return-value/config.json
@@ -0,0 +1,7 @@
+{
+    "tests" : [
+        {
+            "filename" : "main.cpp"
+        }
+    ]
+}
diff --git a/tests/signal-with-return-value/main.cpp b/tests/signal-with-return-value/main.cpp
new file mode 100644
index 00000000..19a7c113
--- /dev/null
+++ b/tests/signal-with-return-value/main.cpp
@@ -0,0 +1,13 @@
+#include <QtCore/QObject>
+#include <QtCore/QString>
+
+class Foo : public QObject
+{
+Q_OBJECT
+signals:
+    void voidSig(); // ok
+    int intSig(); // Warn
+    void* voidStarSig(); // warn
+    Q_SCRIPTABLE int scriptableIntSig(); // ok
+};
+
diff --git a/tests/signal-with-return-value/main.cpp.expected b/tests/signal-with-return-value/main.cpp.expected
new file mode 100644
index 00000000..8bb055ec
--- /dev/null
+++ b/tests/signal-with-return-value/main.cpp.expected
@@ -0,0 +1,2 @@
+signal-with-return-value/main.cpp:9:5: warning: intSig() should return void. For a clean design signals shouldn't assume a single slot are connected to them. [-Wclazy-signal-with-return-value]
+signal-with-return-value/main.cpp:10:5: warning: voidStarSig() should return void. For a clean design signals shouldn't assume a single slot are connected to them. [-Wclazy-signal-with-return-value]