From b830f462e6d18f62406ac11afbc7a39247d57122 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Fri, 27 Apr 2018 11:12:09 +0200 Subject: [PATCH] Added missingOverride checker; Function 'f' overrides function in base class but does not have the 'override' keyword. --- lib/checkclass.cpp | 36 ++++++++++++++++ lib/checkclass.h | 10 ++++- lib/symboldatabase.cpp | 97 +++++++++++++++++++++--------------------- lib/symboldatabase.h | 49 +++++++++++---------- test/testclass.cpp | 24 +++++++++++ 5 files changed, 144 insertions(+), 72 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 464594ca7..9fcb0edf0 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2484,3 +2484,39 @@ void CheckClass::unsafeClassDivZeroError(const Token *tok, const std::string &cl const std::string s = className + "::" + methodName + "()"; reportError(tok, Severity::style, "unsafeClassDivZero", symbols + "Public interface of " + className + " is not safe. When calling " + s + ", if parameter " + varName + " is 0 that leads to division by zero."); } + +void CheckClass::checkOverride() +{ + if (!_settings->isEnabled(Settings::STYLE)) + return; + if (_settings->standards.cpp < Standards::CPP11) + return; + for (const Scope * classScope : symbolDatabase->classAndStructScopes) { + if (!classScope->definedType || classScope->definedType->derivedFrom.empty()) + continue; + for (const Function &func : classScope->functionList) { + if (func.hasOverrideKeyword()) + continue; + const Function *baseFunc = func.getOverridenFunction(); + if (baseFunc) + overrideError(baseFunc, &func); + } + } +} + +void CheckClass::overrideError(const Function *funcInBase, const Function *funcInDerived) +{ + const std::string functionName = funcInDerived ? funcInDerived->name() : ""; + + ErrorPath errorPath; + if (funcInBase && funcInDerived) { + errorPath.push_back(ErrorPathItem(funcInBase->tokenDef, "Virtual function in base class")); + errorPath.push_back(ErrorPathItem(funcInDerived->tokenDef, "Function in derived class")); + } + + reportError(errorPath, Severity::style, "missingOverride", + "$symbol:" + functionName + "\n" + "Function '$symbol' overrides function in base class but does not have the 'override' keyword.", + CWE(0U) /* Unknown CWE! */, + false); +} diff --git a/lib/checkclass.h b/lib/checkclass.h index af85f7a3f..c2b21bd05 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -90,6 +90,8 @@ public: checkClass.checkDuplInheritedMembers(); checkClass.checkExplicitConstructors(); checkClass.checkCopyCtorAndEqOperator(); + + checkClass.checkOverride(); } @@ -155,6 +157,9 @@ public: /** @brief Check that arbitrary usage of the public interface does not result in division by zero */ void checkUnsafeClassDivZero(bool test=false); + /** @brief Check that the override keyword is used when overriding virtual methods */ + void checkOverride(); + private: const SymbolDatabase *symbolDatabase; @@ -189,6 +194,7 @@ private: void duplInheritedMembersError(const Token* tok1, const Token* tok2, const std::string &derivedname, const std::string &basename, const std::string &variablename, bool derivedIsStruct, bool baseIsStruct); void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor); void unsafeClassDivZeroError(const Token *tok, const std::string &className, const std::string &methodName, const std::string &varName); + void overrideError(const Function *funcInBase, const Function *funcInDerived); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckClass c(nullptr, settings, errorLogger); @@ -222,6 +228,7 @@ private: c.unsafeClassDivZeroError(nullptr, "Class", "dostuff", "x"); c.pureVirtualFunctionCallInConstructorError(nullptr, std::list(), "f"); c.virtualFunctionCallInConstructorError(nullptr, std::list(), "f"); + c.overrideError(nullptr, nullptr); } static std::string myName() { @@ -249,7 +256,8 @@ private: "- Call of pure virtual function in constructor/destructor\n" "- Duplicated inherited data members\n" "- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n" - "- Check that arbitrary usage of public interface does not result in division by zero\n"; + "- Check that arbitrary usage of public interface does not result in division by zero\n" + "- Check that the 'override' keyword is used when overriding virtual methods\n"; } // operatorEqRetRefThis helper functions diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index f2db4b5b3..f258afa1b 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -3061,74 +3061,75 @@ bool Function::isImplicitlyVirtual(bool defaultVal) const { if (isVirtual()) return true; - return isOverride(defaultVal); -} - -bool Function::isOverride(bool defaultVal) const -{ - if (!nestedIn->isClassOrStruct()) - return false; - bool safe = true; - bool hasVirt = isOverrideRecursive(nestedIn->definedType, safe); - if (hasVirt) + bool foundAllBaseClasses = true; + if (getOverridenFunction(&foundAllBaseClasses)) return true; - else if (safe) + if (foundAllBaseClasses) return false; - else - return defaultVal; + return defaultVal; } -bool Function::isOverrideRecursive(const ::Type* baseType, bool& safe) const +const Function *Function::getOverridenFunction(bool *foundAllBaseClasses) const +{ + if (foundAllBaseClasses) + *foundAllBaseClasses = true; + if (!nestedIn->isClassOrStruct()) + return nullptr; + return getOverridenFunctionRecursive(nestedIn->definedType, foundAllBaseClasses); +} + +const Function * Function::getOverridenFunctionRecursive(const ::Type* baseType, bool *foundAllBaseClasses) const { // check each base class for (std::size_t i = 0; i < baseType->derivedFrom.size(); ++i) { const ::Type* derivedFromType = baseType->derivedFrom[i].type; // check if base class exists in database - if (derivedFromType && derivedFromType->classScope) { - const Scope *parent = derivedFromType->classScope; + if (!derivedFromType || !derivedFromType->classScope) { + if (foundAllBaseClasses) + *foundAllBaseClasses = false; + continue; + } - // check if function defined in base class - for (std::multimap::const_iterator it = parent->functionMap.find(tokenDef->str()); it != parent->functionMap.end() && it->first == tokenDef->str(); ++it) { - const Function * func = it->second; - if (func->isVirtual()) { // Base is virtual and of same name - const Token *temp1 = func->tokenDef->previous(); - const Token *temp2 = tokenDef->previous(); - bool returnMatch = true; + const Scope *parent = derivedFromType->classScope; - // check for matching return parameters - while (temp1->str() != "virtual") { - if (temp1->str() != temp2->str() && - !(temp1->str() == derivedFromType->name() && - temp2->str() == baseType->name())) { - returnMatch = false; - break; - } + // check if function defined in base class + for (std::multimap::const_iterator it = parent->functionMap.find(tokenDef->str()); it != parent->functionMap.end() && it->first == tokenDef->str(); ++it) { + const Function * func = it->second; + if (func->isVirtual()) { // Base is virtual and of same name + const Token *temp1 = func->tokenDef->previous(); + const Token *temp2 = tokenDef->previous(); + bool match = true; - temp1 = temp1->previous(); - temp2 = temp2->previous(); + // check for matching return parameters + while (temp1->str() != "virtual") { + if (temp1->str() != temp2->str() && + !(temp1->str() == derivedFromType->name() && + temp2->str() == baseType->name())) { + match = false; + break; } - // check for matching function parameters - if (returnMatch && argsMatch(baseType->classScope, func->argDef, argDef, emptyString, 0)) { - return true; - } + temp1 = temp1->previous(); + temp2 = temp2->previous(); } - } - if (!derivedFromType->derivedFrom.empty() && !derivedFromType->hasCircularDependencies()) { - // avoid endless recursion, see #5289 Crash: Stack overflow in isImplicitlyVirtual_rec when checking SVN and - // #5590 with a loop within the class hierarchy. - if (isOverrideRecursive(derivedFromType, safe)) { - return true; + // check for matching function parameters + if (match && argsMatch(baseType->classScope, func->argDef, argDef, emptyString, 0)) { + return func; } } - } else { - // unable to find base class so assume it has no virtual function - safe = false; - return false; + } + + if (!derivedFromType->derivedFrom.empty() && !derivedFromType->hasCircularDependencies()) { + // avoid endless recursion, see #5289 Crash: Stack overflow in isImplicitlyVirtual_rec when checking SVN and + // #5590 with a loop within the class hierarchy. + const Function *func = getOverridenFunctionRecursive(derivedFromType, foundAllBaseClasses); + if (func) { + return func; + } } } - return false; + return nullptr; } const Variable* Function::getArgumentVar(std::size_t num) const diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 2dfe98a00..5f529a6da 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -653,25 +653,25 @@ private: class CPPCHECKLIB Function { /** @brief flags mask used to access specific bit. */ enum { - fHasBody = (1 << 0), ///< @brief has implementation - fIsInline = (1 << 1), ///< @brief implementation in class definition - fIsConst = (1 << 2), ///< @brief is const - fIsVirtual = (1 << 3), ///< @brief is virtual - fIsPure = (1 << 4), ///< @brief is pure virtual - fIsStatic = (1 << 5), ///< @brief is static - fIsStaticLocal = (1 << 6), ///< @brief is static local - fIsExtern = (1 << 7), ///< @brief is extern - fIsFriend = (1 << 8), ///< @brief is friend - fIsExplicit = (1 << 9), ///< @brief is explicit - fIsDefault = (1 << 10), ///< @brief is default - fIsDelete = (1 << 11), ///< @brief is delete - fIsKwOverride = (1 << 12), ///< @brief does declaration contain 'override' keyword? - fIsNoExcept = (1 << 13), ///< @brief is noexcept - fIsThrow = (1 << 14), ///< @brief is throw - fIsOperator = (1 << 15), ///< @brief is operator - fHasLvalRefQual = (1 << 16), ///< @brief has & lvalue ref-qualifier - fHasRvalRefQual = (1 << 17), ///< @brief has && rvalue ref-qualifier - fIsVariadic = (1 << 18) ///< @brief is variadic + fHasBody = (1 << 0), ///< @brief has implementation + fIsInline = (1 << 1), ///< @brief implementation in class definition + fIsConst = (1 << 2), ///< @brief is const + fIsVirtual = (1 << 3), ///< @brief is virtual + fIsPure = (1 << 4), ///< @brief is pure virtual + fIsStatic = (1 << 5), ///< @brief is static + fIsStaticLocal = (1 << 6), ///< @brief is static local + fIsExtern = (1 << 7), ///< @brief is extern + fIsFriend = (1 << 8), ///< @brief is friend + fIsExplicit = (1 << 9), ///< @brief is explicit + fIsDefault = (1 << 10), ///< @brief is default + fIsDelete = (1 << 11), ///< @brief is delete + fHasOverrideKeyword = (1 << 12), ///< @brief does declaration contain 'override' keyword? + fIsNoExcept = (1 << 13), ///< @brief is noexcept + fIsThrow = (1 << 14), ///< @brief is throw + fIsOperator = (1 << 15), ///< @brief is operator + fHasLvalRefQual = (1 << 16), ///< @brief has & lvalue ref-qualifier + fHasRvalRefQual = (1 << 17), ///< @brief has && rvalue ref-qualifier + fIsVariadic = (1 << 18) ///< @brief is variadic }; /** @@ -729,11 +729,12 @@ public: return initArgCount; } void addArguments(const SymbolDatabase *symbolDatabase, const Scope *scope); + /** @brief check if this function is virtual in the base classes */ bool isImplicitlyVirtual(bool defaultVal = false) const; - /** @brief check if this function overrides a base class virtual function */ - bool isOverride(bool defaultVal = false) const; + /** @brief get function in base class that is overridden */ + const Function *getOverridenFunction(bool *foundAllBaseClasses = nullptr) const; bool isConstructor() const { return type==eConstructor || @@ -805,6 +806,9 @@ public: bool isThrow() const { return getFlag(fIsThrow); } + bool hasOverrideKeyword() const { + return getFlag(fHasOverrideKeyword); + } bool isOperator() const { return getFlag(fIsOperator); } @@ -847,7 +851,7 @@ public: private: /** Recursively determine if this function overrides a virtual method in a base class */ - bool isOverrideRecursive(const ::Type* baseType, bool& safe) const; + const Function * getOverridenFunctionRecursive(const ::Type* baseType, bool *foundAllBaseClasses) const; unsigned int flags; @@ -902,7 +906,6 @@ private: void isVariadic(bool state) { setFlag(fIsVariadic, state); } - }; class CPPCHECKLIB Scope { diff --git a/test/testclass.cpp b/test/testclass.cpp index 21225030e..06a0f0dd9 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -191,6 +191,8 @@ private: TEST_CASE(copyCtorAndEqOperator); TEST_CASE(unsafeClassDivZero); + + TEST_CASE(override1); } void checkCopyCtorAndEqOperator(const char code[]) { @@ -6597,6 +6599,28 @@ private: "void A::operator/(int x) { int a = 1000 / x; }"); ASSERT_EQUALS("", errout.str()); } + + void checkOverride(const char code[]) { + // Clear the error log + errout.str(""); + Settings settings; + settings.addEnabled("style"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + // Check.. + CheckClass checkClass(&tokenizer, &settings, this); + checkClass.checkOverride(); + } + + void override1() { + checkOverride("class Base { virtual void f(); };\n" + "class Derived : Base { virtual void f(); };"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Function 'f' overrides function in base class but does not have the 'override' keyword.\n", errout.str()); + } }; REGISTER_TEST(TestClass)