Fixed #9906 (False positive: constParameter (function pointer))

lib/checkother.cpp

@@ -1349,7 +1349,7 @@ void CheckOther::checkConstVariable()
         const Scope *scope = var->scope();
         if (!scope->function)
             continue;
-        Function *function = scope->function;
+        const Function *function = scope->function;
         if (var->isArgument()) {
             if (function->isImplicitlyVirtual() || function->templateDef)
                 continue;
@@ -1427,22 +1427,26 @@ void CheckOther::checkConstVariable()
                 continue;
         }
 
-        constVariableError(var);
+        constVariableError(var, function);
     }
 }
 
-void CheckOther::constVariableError(const Variable *var)
+void CheckOther::constVariableError(const Variable *var, const Function *function)
 {
-    const Token *tok = nullptr;
+    const std::string vartype((var && var->isArgument()) ? "Parameter" : "Variable");
-    std::string name = "x";
+    const std::string varname(var ? var->name() : std::string("x"));
-    std::string id = "Variable";
+
-    if (var) {
+    ErrorPath errorPath;
-        tok = var->nameToken();
+    std::string id = "const" + vartype;
-        name = var->name();
+    std::string message = "$symbol:" + varname + "\n" + vartype + " '$symbol' can be declared with const";
-        if (var->isArgument())
+    errorPath.push_back(ErrorPathItem(var ? var->nameToken() : nullptr, message));
-            id = "Parameter";
+    if (var && var->isArgument() && function && function->functionPointerUsage) {
+        errorPath.push_back(ErrorPathItem(function->functionPointerUsage, "You might need to cast the function pointer here"));
+        id += "Callback";
+        message += ". However it seems that '" + function->name() + "' is a callback function, if '$symbol' is declared with const you might also need to cast function pointer(s).";
     }
-    reportError(tok, Severity::style, "const" + id, id + " '" + name + "' can be declared with const", CWE398, false);
+
+    reportError(errorPath, Severity::style, id.c_str(), message, CWE398, false);
 }
 
 //---------------------------------------------------------------------------

lib/checkother.h

@@ -38,6 +38,7 @@ namespace ValueFlow {
 class Settings;
 class Token;
 class Tokenizer;
+class Function;
 class Variable;
 class ErrorLogger;
 
@@ -229,7 +230,7 @@ private:
     void cstyleCastError(const Token *tok);
     void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive, bool toIsInt);
     void passedByValueError(const Token *tok, const std::string &parname, bool inconclusive);
-    void constVariableError(const Variable *var);
+    void constVariableError(const Variable *var, const Function *function);
     void constStatementError(const Token *tok, const std::string &type, bool inconclusive);
     void signedCharArrayIndexError(const Token *tok);
     void unknownSignCharArrayIndexError(const Token *tok);
@@ -301,7 +302,7 @@ private:
         c.checkCastIntToCharAndBackError(nullptr, "func_name");
         c.cstyleCastError(nullptr);
         c.passedByValueError(nullptr, "parametername", false);
-        c.constVariableError(nullptr);
+        c.constVariableError(nullptr, nullptr);
         c.constStatementError(nullptr, "type", false);
         c.signedCharArrayIndexError(nullptr);
         c.unknownSignCharArrayIndexError(nullptr);

lib/symboldatabase.cpp

@@ -1053,10 +1053,26 @@ void SymbolDatabase::createSymbolDatabaseSetFunctionPointers(bool firstPass)
 
     // Set function call pointers
     for (const Token* tok = mTokenizer->list.front(); tok != mTokenizer->list.back(); tok = tok->next()) {
-        if (!tok->function() && tok->varId() == 0 && tok->linkAt(1) && Token::Match(tok, "%name% (") && !isReservedName(tok->str())) {
+        if (tok->isName() && !tok->function() && tok->varId() == 0 && Token::Match(tok, "%name% [(,)>]") && !isReservedName(tok->str())) {
+            if (tok->next()->str() == ">" && !tok->next()->link())
+                continue;
+
+            if (tok->next()->str() != "(") {
+                const Token *start = tok;
+                while (Token::Match(start->tokAt(-2), "%name% ::"))
+                    start = start->tokAt(-2);
+                if (!Token::Match(start->previous(), "[(,<]") && !Token::Match(start->tokAt(-2), "[(,<] &"))
+                    continue;
+            }
+
             const Function *function = findFunction(tok);
-            if (function)
+            if (!function)
+                continue;
+
             const_cast<Token *>(tok)->function(function);
+
+            if (tok->next()->str() != "(")
+                const_cast<Function *>(function)->functionPointerUsage = tok;
         }
     }
 
@@ -2063,6 +2079,7 @@ Function::Function(const Tokenizer *mTokenizer,
       noexceptArg(nullptr),
       throwArg(nullptr),
       templateDef(nullptr),
+      functionPointerUsage(nullptr),
       mFlags(0)
 {
     // operator function
@@ -2211,6 +2228,7 @@ Function::Function(const Token *tokenDef)
       noexceptArg(nullptr),
       throwArg(nullptr),
       templateDef(nullptr),
+      functionPointerUsage(nullptr),
       mFlags(0)
 {
 }
@@ -4602,10 +4620,7 @@ static std::string getTypeString(const Token *typeToken)
 
 const Function* Scope::findFunction(const Token *tok, bool requireConst) const
 {
-    // make sure this is a function call
+    const bool isCall = Token::Match(tok->next(), "(|{");
-    const Token *end = tok->linkAt(1);
-    if (!end)
-        return nullptr;
 
     const std::vector<const Token *> arguments = getArguments(tok);
 
@@ -4617,7 +4632,7 @@ const Function* Scope::findFunction(const Token *tok, bool requireConst) const
     auto addMatchingFunctions = [&](const Scope *scope) {
         for (std::multimap<std::string, const Function *>::const_iterator it = scope->functionMap.find(tok->str()); it != scope->functionMap.cend() && it->first == tok->str(); ++it) {
             const Function *func = it->second;
-            if (args == func->argCount() ||
+            if (!isCall || args == func->argCount() ||
                 (func->isVariadic() && args >= (func->argCount() - 1)) ||
                 (args < func->argCount() && args >= func->minArgCount())) {
                 matches.push_back(func);
@@ -4636,6 +4651,11 @@ const Function* Scope::findFunction(const Token *tok, bool requireConst) const
     // check in base classes
     findFunctionInBase(tok->str(), args, matches);
 
+    // Non-call => Do not match parameters
+    if (!isCall) {
+        return matches.empty() ? nullptr : matches[0];
+    }
+
     const Function* fallback1Func = nullptr;
     const Function* fallback2Func = nullptr;
 
@@ -4862,8 +4882,8 @@ const Function* SymbolDatabase::findFunction(const Token *tok) const
         }
 
         if (currScope) {
-            while (currScope && !(Token::Match(tok1, "%type% :: %any% (") ||
+            while (currScope && !(Token::Match(tok1, "%type% :: %name% [(),>]") ||
-                                  (Token::Match(tok1, "%type% <") && Token::Match(tok1->linkAt(1), "> :: %any% (")))) {
+                                  (Token::Match(tok1, "%type% <") && Token::Match(tok1->linkAt(1), "> :: %name% (")))) {
                 if (tok1->strAt(1) == "::")
                     tok1 = tok1->tokAt(2);
                 else

lib/symboldatabase.h

@@ -900,6 +900,7 @@ public:
     const Token *noexceptArg;         ///< noexcept token
     const Token *throwArg;            ///< throw token
     const Token *templateDef;         ///< points to 'template <' before function
+    const Token *functionPointerUsage; ///< function pointer usage
 
     static bool argsMatch(const Scope *scope, const Token *first, const Token *second, const std::string &path, nonneg int path_length);
 

test/testother.cpp

@@ -98,6 +98,7 @@ private:
         TEST_CASE(passedByValue_externC);
 
         TEST_CASE(constVariable);
+        TEST_CASE(constParameterCallback);
 
         TEST_CASE(switchRedundantAssignmentTest);
         TEST_CASE(switchRedundantOperationTest);
@@ -2522,6 +2523,30 @@ private:
         TODO_ASSERT_EQUALS("[test.cpp:16]: (style) Parameter 'i' can be declared with const\n", "", errout.str());
     }
 
+    void constParameterCallback() {
+        check("int callback(std::vector<int>& x) { return x[0]; }\n"
+              "void f() { dostuff(callback); }");
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Parameter 'x' can be declared with const. However it seems that 'callback' is a callback function, if 'x' is declared with const you might also need to cast function pointer(s).\n", errout.str());
+
+        // #9906
+        check("class EventEngine : public IEventEngine {\n"
+              "public:\n"
+              "    EventEngine();\n"
+              "\n"
+              "private:\n"
+              "    void signalEvent(ev::sig& signal, int revents);\n"
+              "};\n"
+              "\n"
+              "EventEngine::EventEngine() {\n"
+              "    mSigWatcher.set<EventEngine, &EventEngine::signalEvent>(this);\n"
+              "}\n"
+              "\n"
+              "void EventEngine::signalEvent(ev::sig& signal, int revents) {\n"
+              "    switch (signal.signum) {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:13] -> [test.cpp:10]: (style) Parameter 'signal' can be declared with const. However it seems that 'signalEvent' is a callback function, if 'signal' is declared with const you might also need to cast function pointer(s).\n", errout.str());
+    }
+
     void switchRedundantAssignmentTest() {
         check("void foo()\n"
               "{\n"