From a336048d14ccc184fc049003c215289524c929e5 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Sun, 2 Apr 2023 20:36:23 +0200 Subject: [PATCH] Fix #11599 false negative: constParameter (#4902) * Partial fix for #11599 false negative: constParameter * Adjust test * Update testother.cpp * Update testother.cpp * Fix #11599 false negative: constParameter * Fix new warnings * Format * Add difference_type * Remove isAliased() * Undo * Adjust test * Add test * Improve const check * Tweak constness, add tests * Add tests * Use new helper function, fixtest * Remove bailout, fix check for cast * Prevent FP * Fix constVariable check, add tests * Format * Format * Add test for #11632 --- lib/astutils.cpp | 2 +- lib/checkother.cpp | 51 +++++++++++++------ lib/symboldatabase.cpp | 7 +++ lib/symboldatabase.h | 1 + lib/tokenize.cpp | 2 - test/testastutils.cpp | 111 ++++++++++++++++++++++++++++++++++++++++- test/testother.cpp | 37 ++++++++++++-- test/testuninitvar.cpp | 6 +-- 8 files changed, 188 insertions(+), 29 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 88500158d..0b50bf399 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2407,7 +2407,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti continue; conclusive = true; if (indirect > 0) { - if (!arg->isConst() && arg->isPointer()) + if (arg->isPointer() && !(arg->valueType() && arg->valueType()->isConst(indirect))) return true; // If const is applied to the pointer, then the value can still be modified if (Token::simpleMatch(arg->typeEndToken(), "* const")) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 7193bc527..c7305e89e 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1391,6 +1391,14 @@ static bool isVariableMutableInInitializer(const Token* start, const Token * end return false; } +static const Token* isFuncArg(const Token* tok) { + while (Token::simpleMatch(tok, ",")) + tok = tok->astParent(); + if (Token::simpleMatch(tok, "(") && Token::Match(tok->astOperand1(), "%name% (")) + return tok->astOperand1(); + return nullptr; +} + void CheckOther::checkConstVariable() { if (!mSettings->severity.isEnabled(Severity::style) || mTokenizer->isC()) @@ -1433,8 +1441,6 @@ void CheckOther::checkConstVariable() continue; if (var->isVolatile()) continue; - if (isAliased(var)) - continue; if (isStructuredBindingVariable(var)) // TODO: check all bound variables continue; if (isVariableChanged(var, mSettings, mTokenizer->isCPP())) @@ -1446,7 +1452,7 @@ void CheckOther::checkConstVariable() functionScope = functionScope->nestedIn; } while (functionScope && !(function = functionScope->function)); } - if (function && Function::returnsReference(function) && !Function::returnsConst(function)) { + if (function && (Function::returnsReference(function) || Function::returnsPointer(function)) && !Function::returnsConst(function)) { std::vector returns = Function::findReturns(function); if (std::any_of(returns.cbegin(), returns.cend(), [&](const Token* retTok) { if (retTok->varId() == var->declarationId()) @@ -1455,13 +1461,12 @@ void CheckOther::checkConstVariable() retTok = retTok->astOperand2(); while (Token::simpleMatch(retTok, ".")) retTok = retTok->astOperand2(); + if (Token::simpleMatch(retTok, "&")) + retTok = retTok->astOperand1(); return ValueFlow::hasLifetimeToken(getParentLifetime(retTok), var->nameToken()); })) continue; } - // Skip if address is taken - if (Token::findmatch(var->nameToken(), "& %varid%", scope->bodyEnd, var->declarationId())) - continue; // Skip if another non-const variable is initialized with this variable { //Is it the right side of an initialization of a non-const reference @@ -1474,6 +1479,25 @@ void CheckOther::checkConstVariable() break; } } + if (Token::Match(tok, "& %varid%", var->declarationId())) { + const Token* opTok = tok->astParent(); + if (opTok->isComparisonOp() || opTok->isAssignmentOp() || opTok->isCalculation()) { + if (opTok->isComparisonOp() || opTok->isCalculation()) { + if (opTok->astOperand1() != tok) + opTok = opTok->astOperand1(); + else + opTok = opTok->astOperand2(); + } + if (opTok->valueType() && var->valueType() && opTok->valueType()->isConst(var->valueType()->pointer)) + continue; + } else if (const Token* ftok = isFuncArg(opTok)) { + bool inconclusive{}; + if (var->valueType() && !isVariableChangedByFunctionCall(ftok, var->valueType()->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive) + continue; + } + usedInAssignment = true; + break; + } if (astIsRangeBasedForDecl(tok) && Token::Match(tok->astParent()->astOperand2(), "%varid%", var->declarationId())) { const Variable* refvar = tok->astParent()->astOperand1()->variable(); if (refvar && refvar->isReference() && !refvar->isConst()) { @@ -1494,7 +1518,7 @@ void CheckOther::checkConstVariable() castToNonConst = true; // safe guess break; } - const bool isConst = 0 != (tok->valueType()->constness & (1 << tok->valueType()->pointer)); + const bool isConst = tok->valueType()->isConst(tok->valueType()->pointer); if (!isConst) { castToNonConst = true; break; @@ -1565,15 +1589,10 @@ void CheckOther::checkConstPointer() continue; } else if (Token::simpleMatch(gparent, "[") && gparent->astOperand2() == parent) continue; - else if (Token::Match(gparent, "(|,")) { - const Token* ftok = gparent; - while (Token::simpleMatch(ftok, ",")) - ftok = ftok->astParent(); - if (ftok && Token::Match(ftok->astOperand1(), "%name% (")) { - bool inconclusive{}; - if (!isVariableChangedByFunctionCall(ftok->astOperand1(), vt->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive) - continue; - } + else if (const Token* ftok = isFuncArg(gparent)) { + bool inconclusive{}; + if (!isVariableChangedByFunctionCall(ftok, vt->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive) + continue; } } else { if (Token::Match(parent, "%oror%|%comp%|&&|?|!|-")) diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index f52ca9b4f..b6e87291d 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -3009,6 +3009,13 @@ bool Function::returnsReference(const Function* function, bool unknown) }); } +bool Function::returnsPointer(const Function* function, bool unknown) +{ + return checkReturns(function, unknown, false, [](UNUSED const Token* defStart, const Token* defEnd) { + return Token::simpleMatch(defEnd->previous(), "*"); + }); +} + bool Function::returnsStandardType(const Function* function, bool unknown) { return checkReturns(function, unknown, true, [](UNUSED const Token* defStart, const Token* defEnd) { diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 3fd2bd448..2b0560583 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -934,6 +934,7 @@ public: static bool returnsConst(const Function* function, bool unknown = false); + static bool returnsPointer(const Function* function, bool unknown = false); static bool returnsReference(const Function* function, bool unknown = false); static bool returnsStandardType(const Function* function, bool unknown = false); diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 9da474b1c..c66def4f0 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -2182,8 +2182,6 @@ void Tokenizer::simplifyTypedefCpp() syntaxError(tok2); if (tok2->str() == "=") { - if (!tok2->next()) - syntaxError(tok2); if (tok2->next()->str() == "{") tok2 = tok2->next()->link()->next(); else if (tok2->next()->str().at(0) == '\"') diff --git a/test/testastutils.cpp b/test/testastutils.cpp index b3967b088..cc8975b26 100644 --- a/test/testastutils.cpp +++ b/test/testastutils.cpp @@ -244,7 +244,9 @@ private: std::istringstream istr(code); ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line); const Token * const argtok = Token::findmatch(tokenizer.tokens(), pattern); - return (isVariableChangedByFunctionCall)(argtok, 0, &settings, inconclusive); + ASSERT_LOC(argtok, file, line); + int indirect = (argtok->variable() && argtok->variable()->isArray()); + return (isVariableChangedByFunctionCall)(argtok, indirect, &settings, inconclusive); } void isVariableChangedByFunctionCallTest() { @@ -262,8 +264,113 @@ private: code = "int f(int x) {\n" "return int(x);\n" "}\n"; + inconclusive = false; ASSERT_EQUALS(false, isVariableChangedByFunctionCall(code, "x ) ;", &inconclusive)); - TODO_ASSERT_EQUALS(false, true, inconclusive); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(int* p);\n" + "void f(int x) {\n" + " return g(&x);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(true, isVariableChangedByFunctionCall(code, "x ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(const int* p);\n" + "void f(int x) {\n" + " return g(&x);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(false, isVariableChangedByFunctionCall(code, "x ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(int** pp);\n" + "void f(int* p) {\n" + " return g(&p);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(true, isVariableChangedByFunctionCall(code, "p ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(int* const* pp);\n" + "void f(int* p) {\n" + " return g(&p);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(false, isVariableChangedByFunctionCall(code, "p ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(int a[2]);\n" + "void f() {\n" + " int b[2] = {};\n" + " return g(b);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(true, isVariableChangedByFunctionCall(code, "b ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(const int a[2]);\n" + "void f() {\n" + " int b[2] = {};\n" + " return g(b);\n" + "}\n"; + inconclusive = false; + TODO_ASSERT_EQUALS(false, true, isVariableChangedByFunctionCall(code, "b ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(std::array a);\n" + "void f() {\n" + " std::array b = {};\n" + " return g(b);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(false, isVariableChangedByFunctionCall(code, "b ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(std::array& a);\n" + "void f() {\n" + " std::array b = {};\n" + " return g(b);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(true, isVariableChangedByFunctionCall(code, "b ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(const std::array& a);\n" + "void f() {\n" + " std::array b = {};\n" + " return g(b);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(false, isVariableChangedByFunctionCall(code, "b ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "void g(std::array* p);\n" + "void f() {\n" + " std::array b = {};\n" + " return g(&b);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(true, isVariableChangedByFunctionCall(code, "b ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "struct S {};\n" + "void g(S);\n" + "void f(S* s) {\n" + " g(*s);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(false, isVariableChangedByFunctionCall(code, "s ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); + + code = "struct S {};\n" + "void g(const S&);\n" + "void f(S* s) {\n" + " g(*s);\n" + "}\n"; + inconclusive = false; + ASSERT_EQUALS(false, isVariableChangedByFunctionCall(code, "s ) ;", &inconclusive)); + ASSERT_EQUALS(false, inconclusive); } #define isExpressionChanged(code, var, startPattern, endPattern) \ diff --git a/test/testother.cpp b/test/testother.cpp index f3679d080..ec94525e8 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1419,6 +1419,19 @@ private: " }\n" " return *iter;\n" "}"); + ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'vec' can be declared as reference to const\n", errout.str()); + + check("auto& foo(std::vector& vec, bool flag) {\n" + " std::vector dummy;\n" + " std::vector::iterator iter;\n" + " if (flag)\n" + " iter = vec.begin();\n" + " else {\n" + " dummy.push_back(42);\n" + " iter = dummy.begin();\n" + " }\n" + " return *iter;\n" + "}"); ASSERT_EQUALS("", errout.str()); } @@ -3073,6 +3086,14 @@ private: " return r;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("template void f(std::vector& d, const std::vector& s) {\n" // #11632 + " for (const auto& e : s) {\n" + " T* newE = new T(*e);\n" + " d.push_back(newE);\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void constParameterCallback() { @@ -3406,6 +3427,8 @@ private: "void h(const S&);\n" "void h(int, int, const S&);\n" "void i(S&);\n" + "void j(const S*);\n" + "void j(int, int, const S*);\n" "void f1(S* s) {\n" " g(*s);\n" "}\n" @@ -3417,10 +3440,18 @@ private: "}\n" "void f4(S* s) {\n" " i(*s);\n" + "}\n" + "void f5(S& s) {\n" + " j(&s);\n" + "}\n" + "void f6(S& s) {\n" + " j(1, 2, &s);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:6]: (style) Parameter 's' can be declared as pointer to const\n" - "[test.cpp:9]: (style) Parameter 's' can be declared as pointer to const\n" - "[test.cpp:12]: (style) Parameter 's' can be declared as pointer to const\n", + ASSERT_EQUALS("[test.cpp:20]: (style) Parameter 's' can be declared as reference to const\n" + "[test.cpp:23]: (style) Parameter 's' can be declared as reference to const\n" + "[test.cpp:8]: (style) Parameter 's' can be declared as pointer to const\n" + "[test.cpp:11]: (style) Parameter 's' can be declared as pointer to const\n" + "[test.cpp:14]: (style) Parameter 's' can be declared as pointer to const\n", errout.str()); } diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 39924ef8e..fbc2128af 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -6551,11 +6551,7 @@ private: " foo(123, &abc);\n" " return abc.b;\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: abc.a\n" - "[test.cpp:5]: (error) Uninitialized variable: abc.b\n" - "[test.cpp:5]: (error) Uninitialized variable: abc.c\n", - "", - errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: &abc\n", errout.str()); valueFlowUninit("struct ABC { int a; int b; int c; };\n" "void foo() {\n"