diff --git a/CheckSources.cmake b/CheckSources.cmake --- a/CheckSources.cmake +++ b/CheckSources.cmake @@ -60,6 +60,7 @@ ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/qhash-namespace.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/qlatin1string-non-ascii.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/qproperty-without-notify.cpp + ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/qproperty-type-mismatch.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/qstring-left.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/range-loop.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level1/returning-data-from-temporary.cpp diff --git a/docs/checks/README-qproperty-type-mismatch.md b/docs/checks/README-qproperty-type-mismatch.md new file mode 100644 --- /dev/null +++ b/docs/checks/README-qproperty-type-mismatch.md @@ -0,0 +1,18 @@ +# qproperty-type-mismatch + +Warns when any of the functions or variables declared in a Q_PROPERTY have types, arguments or return types differing with the Q_PROPERTY. + +That is, this will warn on every member function here : + +``` +class Widget : public QWidget { +Q_OBJECT +Q_PROPERTY(bool foo READ foo WRITE setFoo NOTIFY fooChanged) + +public: + int foo() const; + void setFoo(float); +signals: + void fooChanged(); +} +``` diff --git a/src/Checks.h b/src/Checks.h --- a/src/Checks.h +++ b/src/Checks.h @@ -88,6 +88,7 @@ #include "checks/level1/qhash-namespace.h" #include "checks/level1/qlatin1string-non-ascii.h" #include "checks/level1/qproperty-without-notify.h" +#include "checks/level1/qproperty-type-mismatch.h" #include "checks/level1/qstring-left.h" #include "checks/level1/range-loop.h" #include "checks/level1/returning-data-from-temporary.h" @@ -188,6 +189,7 @@ registerCheck(check("qhash-namespace", CheckLevel1, RegisteredCheck::Option_VisitsDecls)); registerCheck(check("qlatin1string-non-ascii", CheckLevel1, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("qproperty-without-notify", CheckLevel1, RegisteredCheck::Option_VisitsStmts)); + registerCheck(check("qproperty-type-mismatch", CheckLevel1, RegisteredCheck::Option_VisitsDecls)); registerCheck(check("qstring-left", CheckLevel1, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("range-loop", CheckLevel1, RegisteredCheck::Option_VisitsStmts)); registerFixIt(1, "fix-range-loop-add-ref", "range-loop"); diff --git a/src/checks/level1/qproperty-type-mismatch.h b/src/checks/level1/qproperty-type-mismatch.h new file mode 100644 --- /dev/null +++ b/src/checks/level1/qproperty-type-mismatch.h @@ -0,0 +1,73 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2019 Jean-Michaël Celerier + + 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_QPROPERTY_TYPE_MISMATCH_H +#define CLAZY_QPROPERTY_TYPE_MISMATCH_H + +#include "checkbase.h" + +#include + +#include +#include + +class ClazyContext; +namespace clang { +class CXXMethodDecl; +class FieldDecl; +class Decl; +class MacroInfo; +class Token; +} // namespace clang + +/** + * See README-qproperty-type-mismatch.md for more info. + */ +class QPropertyTypeMismatch + : public CheckBase +{ +public: + explicit QPropertyTypeMismatch(const std::string &name, ClazyContext *context); + void VisitDecl(clang::Decl *) override; +private: + void VisitMethod(const clang::CXXMethodDecl &); + void VisitField(const clang::FieldDecl &); + void VisitMacroExpands(const clang::Token &MacroNameTok, + const clang::SourceRange &range, const clang::MacroInfo *minfo = nullptr) override; + + struct Property + { + clang::SourceLocation loc; + bool member{}; + std::string name; + std::string type; + std::string read; + std::string write; + std::string notify; + }; + + std::vector m_qproperties; + std::string cleanupType(clang::QualType type); + void checkMethodAgainstProperty(const Property &prop, const clang::CXXMethodDecl &method, const std::string &methodName); + void checkFieldAgainstProperty(const Property &prop, const clang::FieldDecl &method, const std::string &methodName); +}; + +#endif diff --git a/src/checks/level1/qproperty-type-mismatch.cpp b/src/checks/level1/qproperty-type-mismatch.cpp new file mode 100644 --- /dev/null +++ b/src/checks/level1/qproperty-type-mismatch.cpp @@ -0,0 +1,275 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2019 Jean-Michaël Celerier + + 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 "qproperty-type-mismatch.h" +#include "HierarchyUtils.h" +#include "TypeUtils.h" +#include "ClazyContext.h" +#include "AccessSpecifierManager.h" +#include "SourceCompatibilityHelpers.h" +#include "StringUtils.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace clang { +class Decl; +class MacroInfo; +} // namespace clang + +using namespace clang; +using namespace std; + + +QPropertyTypeMismatch::QPropertyTypeMismatch(const std::string &name, ClazyContext *context) + : CheckBase(name, context) +{ + enablePreProcessorCallbacks(); +} + +void QPropertyTypeMismatch::VisitDecl(clang::Decl *decl) +{ + if (auto method = dyn_cast(decl)) + VisitMethod(*method); + else if (auto field = dyn_cast(decl)) + VisitField(*field); +} + +void QPropertyTypeMismatch::VisitMethod(const clang::CXXMethodDecl & method) +{ + if (method.isThisDeclarationADefinition() && !method.hasInlineBody()) + return; + + const auto& theClass = method.getParent(); + const auto& classRange = theClass->getSourceRange(); + const auto& methodName = method.getNameInfo().getName().getAsString(); + + for(const auto& prop : m_qproperties) + { + if(classRange.getBegin() < prop.loc && prop.loc < classRange.getEnd()) + { + checkMethodAgainstProperty(prop, method, methodName); + } + } +} + +void QPropertyTypeMismatch::VisitField(const FieldDecl & field) +{ + const auto& theClass = field.getParent(); + const auto& classRange = theClass->getSourceRange(); + const auto& methodName = field.getName().str(); + + for(const auto& prop : m_qproperties) + { + if(classRange.getBegin() < prop.loc && prop.loc < classRange.getEnd()) + { + checkFieldAgainstProperty(prop, field, methodName); + } + } +} + +std::string QPropertyTypeMismatch::cleanupType(QualType type) { + type = type.getNonReferenceType().getCanonicalType().getUnqualifiedType(); + //type.removeLocalCVRQualifiers(Qualifiers::CVRMask); + + std::string str = type.getAsString(); + if(str.compare(0, 6, "class ") == 0) + str = str.substr(6); + else if(str.compare(0, 7, "struct ") == 0) + str = str.substr(7); + + str.erase(std::remove_if(str.begin(), str.end(), [] (char c) { return std::isspace(c); }), str.end()); + + return str; +} + +void QPropertyTypeMismatch::checkMethodAgainstProperty (const Property& prop, const CXXMethodDecl& method, const std::string& methodName){ + + auto error_begin = [&] { return "Q_PROPERTY '" + prop.name + "' of type '" + prop.type + "' is mismatched with "; }; + + if(prop.read == methodName) + { + auto retTypeStr = cleanupType(method.getReturnType()); + if(prop.type != retTypeStr) + { + emitWarning(&method, error_begin() + "method '" + methodName + "' of return type '"+ retTypeStr +"'"); + } + } + else if(prop.write == methodName) + { + switch(method.getNumParams()) + { + case 0: + emitWarning(&method, error_begin() + "method '" + methodName + "' with no parameters"); + break; + case 1: + { + auto parmTypeStr = cleanupType(method.getParamDecl(0)->getType()); + if(prop.type != parmTypeStr) + { + emitWarning(&method, error_begin() + "method '" + methodName + "' with parameter of type '"+ parmTypeStr +"'"); + } + break; + } + default: + emitWarning(&method, error_begin() + "method '" + methodName + "' with too many parameters"); + break; + } + } + else if(prop.notify == methodName) + { + switch(method.getNumParams()) + { + case 0: + // Should this case be ok ? + // I don't think it is good practice to have signals of a property without + // the property value in parameter, but afaik it's valid in Qt. + emitWarning(&method, "Q_PROPERTY '" + prop.name + "' of type '" + prop.type + "' is mismatched with signal '" + methodName + "' with no parameters"); + break; + case 2: + { + auto param1TypeStr = cleanupType(method.getParamDecl(1)->getType()); + if(param1TypeStr.find("QPrivateSignal") == std::string::npos) + { + emitWarning(&method, error_begin() + "signal '" + methodName + "' with too many parameters" + param1TypeStr); + break; + } + + // We want to check the first parameter too : + [[fallthrough]]; + } + case 1: + { + auto param0TypeStr = cleanupType(method.getParamDecl(0)->getType()); + if(prop.type != param0TypeStr) + { + emitWarning(&method, error_begin() + "signal '" + methodName + "' with parameter of type '"+ param0TypeStr +"'"); + } + break; + } + default: + { + emitWarning(&method, error_begin() + "signal '" + methodName + "' with too many parameters"); + break; + } + } + } +} + +void QPropertyTypeMismatch::checkFieldAgainstProperty (const Property& prop, const FieldDecl& field, const std::string& fieldName) +{ + if(prop.member && prop.name == fieldName) + { + auto typeStr = cleanupType(field.getType()); + if(prop.type != typeStr) + { + emitWarning(&field, "Q_PROPERTY '" + prop.name + "' of type '" + prop.type + "' is mismatched with member '" + fieldName + "' of type '"+ typeStr +"'"); + } + } +} + +void QPropertyTypeMismatch::VisitMacroExpands(const clang::Token &MacroNameTok, const clang::SourceRange &range, const MacroInfo *) +{ + IdentifierInfo *ii = MacroNameTok.getIdentifierInfo(); + if(!ii) + return; + if(ii->getName() != "Q_PROPERTY") + return; + + CharSourceRange crange = Lexer::getAsCharRange(range, sm(), lo()); + + string text = Lexer::getSourceText(crange, sm(), lo()); + std::vector split = clazy::splitString(text, ' '); + if(split.size() < 2) + return; + + Property p; + p.loc = range.getBegin(); + + // Handle type + clazy::rtrim(split[0]); + p.type = split[0]; + if(p.type.find("Q_PROPERTY(") == 0) + p.type = p.type.substr(11); + + // Handle name + clazy::rtrim(split[1]); + p.name = split[1]; + + // Handle Q_PROPERTY functions + enum { + None, Read, Write, Notify + } next = None; + + for (std::string &token : split) { + clazy::rtrim(/*by-ref*/token); + switch(next) + { + case None: + { + if (token == "READ") { + next = Read; + continue; + } + else if (token == "WRITE") { + next = Write; + continue; + } + else if (token == "NOTIFY") { + next = Notify; + continue; + } + else if (token == "MEMBER") { + p.member = true; + break; + } + break; + } + case Read: + p.read = token; + break; + case Write: + p.write = token; + break; + case Notify: + p.notify = token; + break; + } + + next = None; + } + + m_qproperties.push_back(std::move(p)); +} + diff --git a/tests/qproperty-type-mismatch/config.json b/tests/qproperty-type-mismatch/config.json new file mode 100644 --- /dev/null +++ b/tests/qproperty-type-mismatch/config.json @@ -0,0 +1,7 @@ +{ + "tests" : [ + { + "filename" : "main.cpp" + } + ] +} diff --git a/tests/qproperty-type-mismatch/main.cpp b/tests/qproperty-type-mismatch/main.cpp new file mode 100644 --- /dev/null +++ b/tests/qproperty-type-mismatch/main.cpp @@ -0,0 +1,61 @@ +#include + +class MyObj : public QObject +{ + Q_OBJECT + Q_PROPERTY(int r_good READ r_good CONSTANT) + Q_PROPERTY(int r_bad READ r_bad CONSTANT) + + Q_PROPERTY(int m_good MEMBER) + Q_PROPERTY(int m_bad MEMBER) + + Q_PROPERTY(int rw_good READ rw_good WRITE set_rw_good NOTIFY rw_good_changed) + Q_PROPERTY(int rw_bad READ rw_bad WRITE set_rw_bad NOTIFY rw_bad_changed) + + Q_PROPERTY(int rw_good_cref READ rw_good_cref WRITE set_rw_good_cref NOTIFY rw_good_cref_changed) + Q_PROPERTY(int rw_bad_cref READ rw_bad_cref WRITE set_rw_bad_cref NOTIFY rw_bad_cref_changed) + + Q_PROPERTY(int* rw_good_ptr READ rw_good_ptr WRITE set_rw_good_ptr NOTIFY rw_good_ptr_changed) + Q_PROPERTY(int* rw_bad_ptr READ rw_bad_ptr WRITE set_rw_bad_ptr NOTIFY rw_bad_ptr_changed) + + int r_good(); // OK + float r_bad(); // Warn + + int m_good; // OK + float m_bad; // Warn + + int rw_good(); // OK + void set_rw_good(int); // OK + + float rw_bad(); // Warn + void set_rw_bad(float); // Warn + + const int& rw_good_cref(); // OK + void set_rw_good_cref(const int&); // OK + + const float& rw_bad_cref(); // Warn + void set_rw_bad_cref(const float&); // Warn + + int* rw_good_ptr(); // OK + void set_rw_good_ptr(int*); // OK + + float* rw_bad_ptr(); // Warn + void set_rw_bad_ptr(float*); // Warn +signals: + void rw_good_changed(int); // OK + void rw_bad_changed(float); // Warn + void rw_good_cref_changed(const int&); // OK + void rw_bad_cref_changed(const float&); // Warn + void rw_good_ptr_changed(int*); // OK + void rw_bad_ptr_changed(float*); // Warn +}; + +class MyGadget +{ + Q_GADGET + Q_PROPERTY(int good MEMBER) + Q_PROPERTY(int bad MEMBER) + + int good; // Ok + float bad; // Warn +}; diff --git a/tests/qproperty-type-mismatch/main.cpp.expected b/tests/qproperty-type-mismatch/main.cpp.expected new file mode 100644 --- /dev/null +++ b/tests/qproperty-type-mismatch/main.cpp.expected @@ -0,0 +1,12 @@ +qproperty-type-mismatch/main.cpp:22:5: warning: Q_PROPERTY 'r_bad' of type 'int' is mismatched with method 'r_bad' of return type 'float' [-Wclazy-qproperty-type-mismatch]c +qproperty-type-mismatch/main.cpp:25:5: warning: Q_PROPERTY 'm_bad' of type 'int' is mismatched with member 'm_bad' of type 'float' [-Wclazy-qproperty-type-mismatch]c +qproperty-type-mismatch/main.cpp:30:5: warning: Q_PROPERTY 'rw_bad' of type 'int' is mismatched with method 'rw_bad' of return type 'float' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:31:5: warning: Q_PROPERTY 'rw_bad' of type 'int' is mismatched with method 'set_rw_bad' with parameter of type 'float' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:36:5: warning: Q_PROPERTY 'rw_bad_cref' of type 'int' is mismatched with method 'rw_bad_cref' of return type 'float' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:37:5: warning: Q_PROPERTY 'rw_bad_cref' of type 'int' is mismatched with method 'set_rw_bad_cref' with parameter of type 'float' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:42:5: warning: Q_PROPERTY 'rw_bad_ptr' of type 'int*' is mismatched with method 'rw_bad_ptr' of return type 'float*' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:43:5: warning: Q_PROPERTY 'rw_bad_ptr' of type 'int*' is mismatched with method 'set_rw_bad_ptr' with parameter of type 'float*' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:46:5: warning: Q_PROPERTY 'rw_bad' of type 'int' is mismatched with signal 'rw_bad_changed' with parameter of type 'float' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:48:5: warning: Q_PROPERTY 'rw_bad_cref' of type 'int' is mismatched with signal 'rw_bad_cref_changed' with parameter of type 'float' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:50:5: warning: Q_PROPERTY 'rw_bad_ptr' of type 'int*' is mismatched with signal 'rw_bad_ptr_changed' with parameter of type 'float*' [-Wclazy-qproperty-type-mismatch] +qproperty-type-mismatch/main.cpp:60:5: warning: Q_PROPERTY 'bad' of type 'int' is mismatched with member 'bad' of type 'float' [-Wclazy-qproperty-type-mismatch]