Fix #11757 Detect useless overriding functions (#5155)

Author: chrchr-github

lib/checkclass.cpp

@@ -3036,6 +3036,70 @@ void CheckClass::overrideError(const Function *funcInBase, const Function *funcI
                 Certainty::normal);
 }
 
+void CheckClass::uselessOverrideError(const Function *funcInBase, const Function *funcInDerived)
+{
+    const std::string functionName = funcInDerived ? ((funcInDerived->isDestructor() ? "~" : "") + funcInDerived->name()) : "";
+    const std::string funcType = (funcInDerived && funcInDerived->isDestructor()) ? "destructor" : "function";
+
+    ErrorPath errorPath;
+    if (funcInBase && funcInDerived) {
+        errorPath.emplace_back(funcInBase->tokenDef, "Virtual " + funcType + " in base class");
+        errorPath.emplace_back(funcInDerived->tokenDef, char(std::toupper(funcType[0])) + funcType.substr(1) + " in derived class");
+    }
+
+    reportError(errorPath, Severity::style, "uselessOverride",
+                "$symbol:" + functionName + "\n"
+                "The " + funcType + " '$symbol' overrides a " + funcType + " in a base class but just delegates back to the base class.",
+                CWE(0U) /* Unknown CWE! */,
+                Certainty::normal);
+}
+
+static const Token* getSingleFunctionCall(const Scope* scope) {
+    const Token* const start = scope->bodyStart->next();
+    const Token* const end = Token::findsimplematch(start, ";", 1, scope->bodyEnd);
+    if (!end || end->next() != scope->bodyEnd)
+        return nullptr;
+    const Token* ftok = start;
+    if (ftok->str() == "return")
+        ftok = ftok->astOperand1();
+    else {
+        while (Token::Match(ftok, "%name%|::"))
+            ftok = ftok->next();
+    }
+    if (Token::simpleMatch(ftok, "(") && ftok->previous()->function())
+        return ftok->previous();
+    return nullptr;
+}
+
+void CheckClass::checkUselessOverride()
+{
+    if (!mSettings->severity.isEnabled(Severity::style))
+        return;
+
+    for (const Scope* classScope : mSymbolDatabase->classAndStructScopes) {
+        if (!classScope->definedType || classScope->definedType->derivedFrom.size() != 1)
+            continue;
+        for (const Function& func : classScope->functionList) {
+            if (!func.functionScope)
+                continue;
+            const Function* baseFunc = func.getOverriddenFunction();
+            if (!baseFunc || baseFunc->isPure())
+                continue;
+            if (const Token* const call = getSingleFunctionCall(func.functionScope)) {
+                if (call->function() != baseFunc)
+                    continue;
+                std::vector<const Token*> funcArgs = getArguments(func.tokenDef);
+                std::vector<const Token*> callArgs = getArguments(call);
+                if (!std::equal(funcArgs.begin(), funcArgs.end(), callArgs.begin(), [](const Token* t1, const Token* t2) {
+                    return t1->str() == t2->str();
+                }))
+                    continue;
+                uselessOverrideError(baseFunc, &func);
+            }
+        }
+    }
+}
+
 void CheckClass::checkThisUseAfterFree()
 {
     if (!mSettings->severity.isEnabled(Severity::warning))

lib/checkclass.h

@@ -83,6 +83,7 @@ class CPPCHECKLIB CheckClass : public Check {
         checkClass.checkExplicitConstructors();
         checkClass.checkCopyCtorAndEqOperator();
         checkClass.checkOverride();
+        checkClass.checkUselessOverride();
         checkClass.checkThisUseAfterFree();
         checkClass.checkUnsafeClassRefMember();
     }
@@ -146,6 +147,9 @@ class CPPCHECKLIB CheckClass : public Check {
     /** @brief Check that the override keyword is used when overriding virtual functions */
     void checkOverride();
 
+    /** @brief Check that the overriden function is not identical to the base function */
+    void checkUselessOverride();
+
     /** @brief When "self pointer" is destroyed, 'this' might become invalid. */
     void checkThisUseAfterFree();
 
@@ -227,6 +231,7 @@ class CPPCHECKLIB CheckClass : public Check {
     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 overrideError(const Function *funcInBase, const Function *funcInDerived);
+    void uselessOverrideError(const Function *funcInBase, const Function *funcInDerived);
     void thisUseAfterFree(const Token *self, const Token *free, const Token *use);
     void unsafeClassRefMemberError(const Token *tok, const std::string &varname);
     void checkDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase);

test/testclass.cpp

@@ -230,6 +230,8 @@ class TestClass : public TestFixture {
         TEST_CASE(override1);
         TEST_CASE(overrideCVRefQualifiers);
 
+        TEST_CASE(uselessOverride);
+
         TEST_CASE(thisUseAfterFree);
 
         TEST_CASE(unsafeClassRefMember);
@@ -8338,6 +8340,70 @@ class TestClass : public TestFixture {
         ASSERT_EQUALS("", errout.str());
     }
 
+    #define checkUselessOverride(code) checkUselessOverride_(code, __FILE__, __LINE__)
+    void checkUselessOverride_(const char code[], const char* file, int line) {
+        // Clear the error log
+        errout.str("");
+
+        const Settings settings = settingsBuilder().severity(Severity::style).build();
+
+        Preprocessor preprocessor(settings);
+
+        // Tokenize..
+        Tokenizer tokenizer(&settings, this, &preprocessor);
+        std::istringstream istr(code);
+        ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line);
+
+        // Check..
+        CheckClass checkClass(&tokenizer, &settings, this);
+        (checkClass.checkUselessOverride)();
+    }
+
+    void uselessOverride() {
+        checkUselessOverride("struct B { virtual int f() { return 5; } };\n" // #11757
+                             "struct D : B {\n"
+                             "    int f() override { return B::f(); }\n"
+                             "};");
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str());
+
+        checkUselessOverride("struct B { virtual void f(); };\n"
+                             "struct D : B {\n"
+                             "    void f() override { B::f(); }\n"
+                             "};");
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str());
+
+        checkUselessOverride("struct B { virtual int f() = 0; };\n"
+                             "int B::f() { return 5; }\n"
+                             "struct D : B {\n"
+                             "    int f() override { return B::f(); }\n"
+                             "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkUselessOverride("struct B { virtual int f(int i); };\n"
+                             "struct D : B {\n"
+                             "    int f(int i) override { return B::f(i); }\n"
+                             "};");
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str());
+
+        checkUselessOverride("struct B { virtual int f(int i); };\n"
+                             "struct D : B {\n"
+                             "    int f(int i) override { return B::f(i + 1); }\n"
+                             "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkUselessOverride("struct B { virtual int f(int i, int j); };\n"
+                             "struct D : B {\n"
+                             "    int f(int i, int j) override { return B::f(j, i); }\n"
+                             "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkUselessOverride("struct B { virtual int f(); };\n"
+                             "struct I { virtual int f() = 0; };\n"
+                             "struct D : B, I {\n"
+                             "    int f() override { return B::f(); }\n"
+                             "};");
+        ASSERT_EQUALS("", errout.str());
+    }
 
 #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__)
     void checkUnsafeClassRefMember_(const char code[], const char* file, int line) {