Index: ClazySources.cmake =================================================================== --- ClazySources.cmake +++ ClazySources.cmake @@ -36,6 +36,7 @@ ${CMAKE_CURRENT_LIST_DIR}/src/checks/level0/unused-non-trivial-variable.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level0/writingtotemporary.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level0/wrong-qglobalstatic.cpp + ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/autoconnect-slot.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/autounexpectedqstringbuilder.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/child-event-qobject-cast.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/connect-3arg-lambda.cpp Index: checks.json =================================================================== --- checks.json +++ checks.json @@ -152,6 +152,11 @@ }, { + "name" : "autoconnect-slot", + "level" : 1, + "categories" : ["bug", "readability"] + }, + { "name" : "auto-unexpected-qstringbuilder", "level" : 1, "categories" : ["bug", "qstring"], Index: src/checks/level1/README-autoconnect-slot.md =================================================================== --- /dev/null +++ src/checks/level1/README-autoconnect-slot.md @@ -0,0 +1,10 @@ +# autoconnect-slot + +Warns when use of the autoconnected slot behavior from uic generated setupUi() is detected. +E.g. slots following the naming convention "on_btnRefresh_clicked()". + +The connections are very fragile, since any changes to either the widget's name or type +will cause the connection to silently fail without any compile time or run time warnings. + +Instead, explicit connections should be created for these slots to ensure compilation +failures if changes to the widget would break the slot connections. Index: src/checks/level1/autoconnect-slot.h =================================================================== --- /dev/null +++ src/checks/level1/autoconnect-slot.h @@ -0,0 +1,50 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2017 Sergio Martins + + 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_AUTOCONNECT_SLOT_H +#define CLAZY_AUTOCONNECT_SLOT_H + +#include "checkbase.h" +#include + +class CXXRecordDecl; +class CXXMethodDecl; +class FixItHint; +struct StmtBodyRange; + +/** + * See README-autoconnect-slot.md for more info. + */ +class AutoConnectSlot : public CheckBase +{ +public: + explicit AutoConnectSlot(const std::string &name, ClazyContext *context); + void VisitDecl(clang::Decl *decl) override; +private: + + clang::FieldDecl *getClassMember(clang::CXXRecordDecl *record, const std::string &memberName); + clang::CallExpr *findSetupUi(const StmtBodyRange &bodyRange); + void fixIts(const std::string &objectName, const std::string &signalName, + const std::string &objectTypeName, + clang::CXXRecordDecl *record, clang::CXXMethodDecl *method, std::vector& fixits); +}; + +#endif Index: src/checks/level1/autoconnect-slot.cpp =================================================================== --- /dev/null +++ src/checks/level1/autoconnect-slot.cpp @@ -0,0 +1,165 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2017 Sergio Martins + + 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 "autoconnect-slot.h" +#include "QtUtils.h" +#include "checkmanager.h" +#include "ClazyContext.h" +#include "StmtBodyRange.h" +#include "AccessSpecifierManager.h" +#include "FixItUtils.h" +#include "StringUtils.h" +#include + +enum Fixit +{ + FixitNone = 0, + FixItConnects = 1 +}; + +AutoConnectSlot::AutoConnectSlot(const std::string &name, ClazyContext *context) + : CheckBase(name, context) +{ + context->enableAccessSpecifierManager(); +} + +void AutoConnectSlot::VisitDecl(clang::Decl *decl) +{ + AccessSpecifierManager *a = m_context->accessSpecifierManager; + if (!a) + return; + + auto method = clang::dyn_cast(decl); + if (!method) + return; + + if (!isFixitEnabled(FixItConnects) && method->isThisDeclarationADefinition() && !method->hasInlineBody()) // Don't warn twice + return; + + QtAccessSpecifierType specifierType = a->qtAccessSpecifierType(method); + + if (specifierType != QtAccessSpecifier_Slot) + return; + + std::string name = method->getNameAsString(); + + static std::regex rx(R"(on_(.*)_(.*))"); + std::smatch match; + if (!regex_match(name, match, rx)) + return; + + std::string objectName = match[1].str(); + std::string signalName = match[2].str(); + + clang::CXXRecordDecl *record = method->getParent(); + if (clang::FieldDecl *field = getClassMember(record, objectName)) + { + clang::QualType type = field->getType(); + if (QtUtils::isQObject(type)) + { + std::string objectTypeName = StringUtils::simpleTypeName(TypeUtils::pointeeQualType(type), lo()); + + std::vector fixits; + if (isFixitEnabled(FixItConnects)) + { + fixIts(objectName, signalName, objectTypeName, record, method, fixits); + } + emitWarning(decl->getLocStart(), "Use of autoconnected UI slot: " + name, fixits); + } + } +} + +void AutoConnectSlot::fixIts(const std::string &objectName, const std::string &signalName, + const std::string &objectTypeName, + clang::CXXRecordDecl *record, clang::CXXMethodDecl *method, std::vector &fixits) +{ + std::string newName = objectName + "_" + signalName; + + clang::FixItHint fixit = FixItUtils::createReplacement(method->getNameInfo().getLoc(), newName); + fixits.push_back(fixit); + + // don't add duplicate connects + if (!(method->isThisDeclarationADefinition() && !method->hasInlineBody())) + { + std::string newConnect = "connect( " + objectName + ", &" + objectTypeName + "::" + signalName + ", this, &" + record->getNameAsString() + "::" + newName + " );"; + for (clang::CXXMethodDecl *baseMethod : record->methods()) + { + StmtBodyRange bodyRange(baseMethod->getBody()); + if (clang::CallExpr *setupCall = findSetupUi(bodyRange)) + { + // hack - use getLocWithOffset to skip ");" + clang::FixItHint connectHint = FixItUtils::createInsertion(setupCall->getLocEnd().getLocWithOffset(2), "\n" + newConnect); + fixits.push_back(connectHint); + } + } + } +} + +clang::CallExpr *AutoConnectSlot::findSetupUi(const StmtBodyRange &bodyRange) +{ + if (!bodyRange.isValid()) + return nullptr; + + clang::Stmt *body = bodyRange.body; + std::vector callExprs; + HierarchyUtils::getChilds(body, callExprs); + for (clang::CallExpr *callexpr : callExprs) + { + if (bodyRange.isOutsideRange(callexpr)) + continue; + + clang::FunctionDecl *fDecl = callexpr->getDirectCallee(); + if (!fDecl) + continue; + + if (fDecl->getNameAsString() == "setupUi") + return callexpr; + } + return nullptr; +} + +clang::FieldDecl *AutoConnectSlot::getClassMember(clang::CXXRecordDecl *record, const std::string &memberName) +{ + if (!record) + return nullptr; + + for (auto field : record->fields()) + { + if (field->getNameAsString() == memberName) + return field; + } + + // Also include the base classes + for (const auto &base : record->bases()) + { + clang::CXXRecordDecl *baseRecord = TypeUtils::recordFromBaseSpecifier(base); + if (clang::FieldDecl *field = getClassMember(baseRecord, memberName)) + { + return field; + } + } + + return nullptr; +} + +const char *const s_checkName = "autoconnect-slot"; +REGISTER_CHECK(s_checkName, AutoConnectSlot, CheckLevel1) +REGISTER_FIXIT(FixItConnects, "fix-autoconnect-slot", s_checkName) Index: tests/autoconnect-slot/config.json =================================================================== --- /dev/null +++ tests/autoconnect-slot/config.json @@ -0,0 +1,11 @@ +{ + "tests" : [ + { + "filename" : "main.cpp" + }, + { + "filename" : "main.cpp_fixed.cpp", + "isFixedFile" : true + } + ] +} Index: tests/autoconnect-slot/main.cpp =================================================================== --- /dev/null +++ tests/autoconnect-slot/main.cpp @@ -0,0 +1,42 @@ +#include +#include "ui_widget.h" + +class MyObject : public QWidget, private Ui::Widget +{ + Q_OBJECT +public: + MyObject() + { + setupUi( this ); + } + bool on_mButton_something(); // No warn + +public Q_SLOTS: + void slot1() const {} // No warn + void slot2_something() {} // No warn + void slot3_something_(); // No warn + void on_mButton_clicked(); // Warn + void on_button_clicked(); // Warn + void on_mButton_pressed(); // Warn + void on_mButton_released() {} // Warn + void on_mButton(); // No warn + void on_notButton_clicked(); // No warn + void on_mDouble_clicked(); // No warn - not a widget + void on_mButton_updated(); // No warn - not a signal + + private: + + double mDouble; + QPushButton* button; + +}; + +void MyObject::on_mButton_pressed() // OK, already warned +{ + +} + +void test() +{ + MyObject o; +} Index: tests/autoconnect-slot/main.cpp.expected =================================================================== --- /dev/null +++ tests/autoconnect-slot/main.cpp.expected @@ -0,0 +1,5 @@ +autoconnect-slot/main.cpp:18:5: warning: Use of autoconnected UI slot: on_mButton_clicked [-Wclazy-autoconnect-slot] +autoconnect-slot/main.cpp:19:5: warning: Use of autoconnected UI slot: on_button_clicked [-Wclazy-autoconnect-slot] +autoconnect-slot/main.cpp:20:5: warning: Use of autoconnected UI slot: on_mButton_pressed [-Wclazy-autoconnect-slot] +autoconnect-slot/main.cpp:21:5: warning: Use of autoconnected UI slot: on_mButton_released [-Wclazy-autoconnect-slot] +autoconnect-slot/main.cpp:33:1: warning: Use of autoconnected UI slot: on_mButton_pressed [-Wclazy-autoconnect-slot] Index: tests/autoconnect-slot/main.cpp_fixed.cpp.expected =================================================================== --- /dev/null +++ tests/autoconnect-slot/main.cpp_fixed.cpp.expected @@ -0,0 +1,45 @@ +#include +#include "ui_widget.h" + +class MyObject : public QWidget, private Ui::Widget +{ + Q_OBJECT +public: + MyObject() + { + setupUi( this ); +connect( mButton, &QPushButton::clicked, this, &MyObject::mButton_clicked ); +connect( button, &QPushButton::clicked, this, &MyObject::button_clicked ); +connect( mButton, &QPushButton::pressed, this, &MyObject::mButton_pressed ); +connect( mButton, &QPushButton::released, this, &MyObject::mButton_released ); + } + bool on_mButton_something(); // No warn + +public Q_SLOTS: + void slot1() const {} // No warn + void slot2_something() {} // No warn + void slot3_something_(); // No warn + void mButton_clicked(); // Warn + void button_clicked(); // Warn + void mButton_pressed(); // Warn + void mButton_released() {} // Warn + void on_mButton(); // No warn + void on_notButton_clicked(); // No warn + void on_mDouble_clicked(); // No warn - not a widget + + private: + + double mDouble; + QPushButton* button; + +}; + +void MyObject::mButton_pressed() // OK, already warned +{ + +} + +void test() +{ + MyObject o; +} Index: tests/autoconnect-slot/ui_widget.h =================================================================== --- /dev/null +++ tests/autoconnect-slot/ui_widget.h @@ -0,0 +1,43 @@ +/******************************************************************************** +** Form generated from reading UI file 'widget.ui' +** +** Created by: Qt User Interface Compiler version 5.7.1 +** +** WARNING! All changes made in this file will be lost when recompiling UI file! +********************************************************************************/ + +#ifndef UI_WIDGET_H +#define UI_WIDGET_H + +#include +#include + +QT_BEGIN_NAMESPACE + +class Ui_Widget +{ +public: + + QPushButton *mButton; + + void setupUi(QWidget *Widget) + { + if (Widget->objectName().isEmpty()) + Widget->setObjectName(QStringLiteral("Widget")); + + mButton = new QPushButton(); + mButton->setObjectName(QStringLiteral("mButton")); + + QMetaObject::connectSlotsByName(Widget); + } // setupUi + +}; + +namespace Ui { + class Widget: public Ui_Widget {}; +} // namespace Ui + +QT_END_NAMESPACE + +#endif // UI_WIDGET_H +