From 95fc84a26b11d50f02cbe7f4bf262c12f7f8c87c Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 8 Apr 2018 07:43:19 -0500 Subject: [PATCH] Find duplicate expressions assigned to the same variable (#1129) * Check for duplicate assignments * Improve checking of expression * Add more tests * Use simple match * Improve robustness of check * check for null * Reduce side effects by checking for side effects * Improve verbose message * Reword the error message --- lib/astutils.cpp | 2 +- lib/checkother.cpp | 40 +++++++++++++++ lib/checkother.h | 1 + test/testother.cpp | 122 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 164 insertions(+), 1 deletion(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 3b480ee1a..234cbe76a 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -165,7 +165,7 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2 if (tok1->isName() && tok1->next()->str() == "(" && tok1->str() != "sizeof") { if (!tok1->function() && !Token::Match(tok1->previous(), ".|::") && pure && !library.isFunctionConst(tok1->str(), true) && !tok1->isAttributeConst() && !tok1->isAttributePure()) return false; - else if (tok1->function() && !tok1->function()->isConst() && !tok1->function()->isAttributeConst() && !tok1->function()->isAttributePure()) + else if (tok1->function() && !tok1->function()->isConst() && !tok1->function()->isAttributeConst() && !tok1->function()->isAttributePure() && pure) return false; } // templates/casts diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 38b196e8b..2952c54cf 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1928,6 +1928,35 @@ void CheckOther::checkDuplicateExpression() continue; for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) { + if(tok->str() == "=" && Token::Match(tok->astOperand1(), "%var%")) { + const Token * endStatement = Token::findsimplematch(tok, ";"); + if(Token::Match(endStatement, "; %type% %var% ;")) { + endStatement = endStatement->tokAt(4); + } + if(Token::Match(endStatement, "%var% %assign%")) { + const Token * nextAssign = endStatement->tokAt(1); + const Token * var1 = tok->astOperand1(); + const Token * var2 = nextAssign->astOperand1(); + if(var1 && var2 && + Token::Match(var1->previous(), ";|{|} %var%") && + Token::Match(var2->previous(), ";|{|} %var%") && + var2->valueType() && var1->valueType() && + var2->valueType()->originalTypeName == var1->valueType()->originalTypeName && + var2->valueType()->pointer == var1->valueType()->pointer && + var2->valueType()->constness == var1->valueType()->constness && + var2->varId() != var1->varId() && ( + tok->astOperand2()->isArithmeticalOp() || + tok->astOperand2()->str() == "." || + Token::Match(tok->astOperand2()->previous(), "%name% (") + ) && + tok->next()->tokType() != Token::eType && + tok->next()->tokType() != Token::eName && + isSameExpression(_tokenizer->isCPP(), true, tok->next(), nextAssign->next(), _settings->library, true) && + isSameExpression(_tokenizer->isCPP(), true, tok->astOperand2(), nextAssign->astOperand2(), _settings->library, false)) { + duplicateAssignExpressionError(var1, var2); + } + } + } if (tok->isOp() && tok->astOperand1() && !Token::Match(tok, "+|*|<<|>>|+=|*=|<<=|>>=")) { if (Token::Match(tok, "==|!=|-") && astIsFloat(tok->astOperand1(), true)) continue; @@ -1987,6 +2016,17 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, "determine if it is correct.", CWE398, false); } +void CheckOther::duplicateAssignExpressionError(const Token *tok1, const Token *tok2) +{ + const std::list toks = make_container< std::list >() << tok2 << tok1; + + reportError(toks, Severity::style, "duplicateAssignExpression", + "Same expression used in consecutive assignments of '" + tok1->str() + "' and '" + tok2->str() + "'.\n" + "Finding variables '" + tok1->str() + "' and '" + tok2->str() + "' that are assigned the same expression " + "is suspicious and might indicate a cut and paste or logic error. Please examine this code carefully to " + "determine if it is correct.", CWE398, false); +} + void CheckOther::duplicateExpressionTernaryError(const Token *tok) { reportError(tok, Severity::style, "duplicateExpressionTernary", "Same expression in both branches of ternary operator.\n" diff --git a/lib/checkother.h b/lib/checkother.h index 6f11f4a7d..fec815eeb 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -238,6 +238,7 @@ private: void selfAssignmentError(const Token *tok, const std::string &varname); void misusedScopeObjectError(const Token *tok, const std::string &varname); void duplicateBranchError(const Token *tok1, const Token *tok2); + void duplicateAssignExpressionError(const Token *tok1, const Token *tok2); void duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op); void duplicateValueTernaryError(const Token *tok); void duplicateExpressionTernaryError(const Token *tok); diff --git a/test/testother.cpp b/test/testother.cpp index 96abc2ed0..c382642b1 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -131,6 +131,7 @@ private: TEST_CASE(duplicateValueTernary); TEST_CASE(duplicateExpressionTernary); // #6391 TEST_CASE(duplicateExpressionTemplate); // #6930 + TEST_CASE(duplicateVarExpression); TEST_CASE(checkSignOfUnsignedVariable); TEST_CASE(checkSignOfPointer); @@ -3918,6 +3919,127 @@ private: ASSERT_EQUALS("", errout.str()); } + void duplicateVarExpression() { + check("int f() __attribute__((pure));\n" + "void test() {\n" + " int i = f();\n" + " int j = f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + + check("struct Foo { int f() const; };\n" + "void test() {\n" + " Foo f = Foo{};\n" + " int i = f.f();\n" + " int j = f.f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + + check("int f() __attribute__((pure));\n" + "void test() {\n" + " int i = 1 + f();\n" + " int j = 1 + f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + + check("int f() __attribute__((pure));\n" + "void test() {\n" + " int i = f() + f();\n" + " int j = f() + f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + + check("int f(int) __attribute__((pure));\n" + "void test() {\n" + " int i = f(0);\n" + " int j = f(0);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + + check("void test(int * p) {\n" + " int i = *p;\n" + " int j = *p;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + + check("struct A { int x; };" + "void test(A a) {\n" + " int i = a.x;\n" + " int j = a.x;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + + check("void test() {\n" + " int i = 0;\n" + " int j = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void test() {\n" + " int i = -1;\n" + " int j = -1;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int f(int);\n" + "void test() {\n" + " int i = f(0);\n" + " int j = f(1);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int f();\n" + "void test() {\n" + " int i = f() || f();\n" + " int j = f() && f();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("struct Foo {};\n" + "void test() {\n" + " Foo i = Foo();\n" + " Foo j = Foo();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("struct Foo {};\n" + "void test() {\n" + " Foo i = Foo{};\n" + " Foo j = Foo{};\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void test() {\n" + " int i = f();\n" + " int j = f();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void test(int x) {\n" + " int i = ++x;\n" + " int j = ++x;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void test(int x) {\n" + " int i = x++;\n" + " int j = x++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void test(int x) {\n" + " int i = --x;\n" + " int j = --x;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void test(int x) {\n" + " int i = x--;\n" + " int j = x--;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void checkSignOfUnsignedVariable() { check( "void foo() {\n"