Check that virtual function non-narrow access modifier in derived class (#2229)

* Check that virtual function has not narrowed access in derived class

* motivation info added

* error reporting moved to func

* added suppression for CI

.travis_suppressions

@@ -7,6 +7,7 @@ shadowFunction
 functionConst
 functionStatic
 bitwiseOnBoolean
+hidingInheritedPublic
 
 # temporary suppressions - fix the warnings!
 duplicateBranch:lib/checkunusedvar.cpp

lib/checkclass.cpp

@@ -2625,6 +2625,36 @@ void CheckClass::overrideError(const Function *funcInBase, const Function *funcI
                 false);
 }
 
+void CheckClass::checkAccessModifierVirtualFunctions()
+{
+    // Motivation:
+    // isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public
+    // stackoverflow.com/questions/484592/overriding-public-virtual-functions-with-private-functions-in-c
+    if (!mSettings->isEnabled(Settings::STYLE))
+        return;
+    for (const Scope * classScope : mSymbolDatabase->classAndStructScopes) {
+        if (!classScope->definedType || classScope->definedType->derivedFrom.empty())
+            continue;
+        for (const Function &func : classScope->functionList) {
+            const Function *baseFunc = func.getOverriddenFunction();
+            if(baseFunc) {
+                if(AccessControl::Public == baseFunc->access && AccessControl::Public != func.access) {
+                    checkAccessModifierVirtualFunctionsError(func.tokenDef, func.name());
+                }
+            }
+        }
+    }
+}
+
+void CheckClass::checkAccessModifierVirtualFunctionsError(const Token *tok, const std::string& func)
+{
+    reportError(tok, Severity::style, "hidingInheritedPublic",
+        "$symbol:" + func + "\n"
+        "The function '$symbol' has more narrow access modifier in a derived class. It could violate a LSP principle.",
+        CWE(0U) /* Unknown CWE! */,
+        false);
+}
+
 void CheckClass::checkUnsafeClassRefMember()
 {
     if (!mSettings->safeChecks.classes || !mSettings->isEnabled(Settings::WARNING))

lib/checkclass.h

@@ -79,6 +79,7 @@ public:
         checkClass.checkCopyCtorAndEqOperator();
         checkClass.checkOverride();
         checkClass.checkUnsafeClassRefMember();
+        checkClass.checkAccessModifierVirtualFunctions();
     }
 
     /** @brief %Check that all class constructors are ok */
@@ -146,6 +147,8 @@ public:
     /** @brief Unsafe class check - const reference member */
     void checkUnsafeClassRefMember();
 
+    /** @brief Check that virtuial function has not least access in derived class */
+    void checkAccessModifierVirtualFunctions();
 private:
     const SymbolDatabase *mSymbolDatabase;
 
@@ -183,6 +186,7 @@ private:
     void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor);
     void overrideError(const Function *funcInBase, const Function *funcInDerived);
     void unsafeClassRefMemberError(const Token *tok, const std::string &varname);
+    void checkAccessModifierVirtualFunctionsError(const Token *tok, const std::string& func);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
         CheckClass c(nullptr, settings, errorLogger);
@@ -220,6 +224,7 @@ private:
         c.virtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
         c.overrideError(nullptr, nullptr);
         c.unsafeClassRefMemberError(nullptr, "UnsafeClass::var");
+        c.checkAccessModifierVirtualFunctionsError(nullptr, "f");
     }
 
     static std::string myName() {

test/testclass.cpp

@@ -221,6 +221,8 @@ private:
         TEST_CASE(overrideCVRefQualifiers);
 
         TEST_CASE(unsafeClassRefMember);
+
+        TEST_CASE(accessModifierVirtualFunctions);
     }
 
     void checkCopyCtorAndEqOperator(const char code[]) {
@@ -7146,6 +7148,32 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void checkAccessModifierVirtualFunctions(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.checkAccessModifierVirtualFunctions();
+    }
+
+    void accessModifierVirtualFunctions() {
+        checkAccessModifierVirtualFunctions("struct Base { virtual void f(); };\n"
+                                            "struct Derived : Base { private: virtual void f(); };");
+        ASSERT_EQUALS("[test.cpp:2]: (style) The function 'f' has more narrow access modifier in a derived class. It could violate a LSP principle.\n", errout.str());
+
+        checkAccessModifierVirtualFunctions("struct Base { virtual void f(); };\n"
+                                            "struct Derived : Base { virtual void f(); };");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void checkUnsafeClassRefMember(const char code[]) {
         // Clear the error log
         errout.str("");