From 117a753b1006a1116829f053a1c8e3912e22bde1 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 8 Sep 2022 15:08:38 -0500 Subject: [PATCH] Partial fix 11154: FN: knownConditionTrueFalse (#4453) * Partial fix 11154: FN: knownConditionTrueFalse * Formay * Add more tests * FOrmat * Fix FP * Add test * Check for side effects * Format * Update tests * Format --- lib/astutils.cpp | 18 ++++++++ lib/checkcondition.cpp | 15 ++++--- lib/valueflow.cpp | 20 +++++++-- test/testcondition.cpp | 99 ++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 138 insertions(+), 14 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 4fdf93a57..bb376684e 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1616,6 +1616,9 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token if (!cond1 || !cond2) return false; + if (isSameExpression(cpp, true, cond1, cond2, library, pure, followVar, errors)) + return false; + if (!isNot && cond1->str() == "&&" && cond2->str() == "&&") { for (const Token* tok1: { cond1->astOperand1(), cond1->astOperand2() @@ -1631,6 +1634,21 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token } } + if (cond1->str() != cond2->str() && (cond1->str() == "||" || cond2->str() == "||")) { + const Token* orCond = nullptr; + const Token* otherCond = nullptr; + if (cond1->str() == "||") { + orCond = cond1; + otherCond = cond2; + } + if (cond2->str() == "||") { + orCond = cond2; + otherCond = cond1; + } + return isOppositeCond(isNot, cpp, orCond->astOperand1(), otherCond, library, pure, followVar, errors) && + isOppositeCond(isNot, cpp, orCond->astOperand2(), otherCond, library, pure, followVar, errors); + } + if (cond1->str() == "!") { if (cond2->str() == "!=") { if (cond2->astOperand1() && cond2->astOperand1()->str() == "0") diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index a9e629197..f9da0661d 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1507,13 +1507,14 @@ void CheckCondition::alwaysTrueFalse() continue; if (Token::simpleMatch(tok, ":")) continue; - if (tok->isComparisonOp() && isSameExpression(mTokenizer->isCPP(), - true, - tok->astOperand1(), - tok->astOperand2(), - mSettings->library, - true, - true)) + if (tok->isComparisonOp() && isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand1()) && + isSameExpression(mTokenizer->isCPP(), + true, + tok->astOperand1(), + tok->astOperand2(), + mSettings->library, + true, + true)) continue; if (isConstVarExpression(tok, [](const Token* tok) { return Token::Match(tok, "[|(|&|+|-|*|/|%|^|>>|<<") && !Token::simpleMatch(tok, "( )"); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 6f8712e88..e09580a00 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -3058,6 +3058,20 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer { } }; +struct SameExpressionAnalyzer : ExpressionAnalyzer { + + SameExpressionAnalyzer() : ExpressionAnalyzer() {} + + SameExpressionAnalyzer(const Token* e, const ValueFlow::Value& val, const TokenList* t) + : ExpressionAnalyzer(e, val, t) + {} + + bool match(const Token* tok) const override + { + return isSameExpression(isCPP(), true, expr, tok, getSettings()->library, true, true); + } +}; + struct OppositeExpressionAnalyzer : ExpressionAnalyzer { bool isNot; @@ -4929,7 +4943,7 @@ static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* { for (const Token* condTok2 : getConditions(condTok, "&&")) { if (is1) { - ExpressionAnalyzer a1(condTok2, makeConditionValue(1, condTok2, true), tokenlist); + SameExpressionAnalyzer a1(condTok2, makeConditionValue(1, condTok2, true), tokenlist); valueFlowGenericForward(startTok, startTok->link(), a1, settings); } @@ -4944,7 +4958,7 @@ static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* if (Token::simpleMatch(startTok->link(), "} else {")) { startTok = startTok->link()->tokAt(2); for (const Token* condTok2:conds) { - ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist); + SameExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist); valueFlowGenericForward(startTok, startTok->link(), a1, settings); if (is1) { @@ -4964,7 +4978,7 @@ static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* continue; } for (const Token* condTok2:conds) { - ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist); + SameExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist); valueFlowGenericForward(startTok->link()->next(), scope2->bodyEnd, a1, settings); if (is1) { diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 4602a89a3..8adfbf054 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -105,6 +105,7 @@ private: TEST_CASE(oppositeInnerCondition2); TEST_CASE(oppositeInnerCondition3); TEST_CASE(oppositeInnerConditionAnd); + TEST_CASE(oppositeInnerConditionOr); TEST_CASE(oppositeInnerConditionEmpty); TEST_CASE(oppositeInnerConditionFollowVar); @@ -1386,6 +1387,52 @@ private: } void incorrectLogicOperator12() { // #8696 + check("struct A {\n" + " void f() const;\n" + "};\n" + "void foo(A a, A b) {\n" + " A x = b;\n" + " A y = b;\n" + " y.f();\n" + " if (a > x && a < y)\n" + " return;\n" + "}"); + ASSERT_EQUALS( + "[test.cpp:5] -> [test.cpp:6] -> [test.cpp:8]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", + errout.str()); + + check("struct A {\n" + " void f();\n" + "};\n" + "void foo(A a, A b) {\n" + " A x = b;\n" + " A y = b;\n" + " y.f();\n" + " if (a > x && a < y)\n" + " return;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(A a, A b) {\n" + " A x = b;\n" + " A y = b;\n" + " y.f();\n" + " if (a > x && a < y)\n" + " return;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(A a, A b) {\n" + " const A x = b;\n" + " const A y = b;\n" + " y.f();\n" + " if (a > x && a < y)\n" + " return;\n" + "}"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:3] -> [test.cpp:5]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", + errout.str()); + check("struct A {\n" " void f() const;\n" "};\n" @@ -1396,7 +1443,9 @@ private: " if (a > x && a < y)\n" " return;\n" "}"); - ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:6] -> [test.cpp:8]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8]: (style) Condition 'a>x' is always false\n" + "[test.cpp:8]: (style) Condition 'a x && a < y)\n" " return;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:8]: (style) Condition 'a>x' is always false\n", errout.str()); check("void foo(A a) {\n" " A x = a;\n" @@ -1417,7 +1466,7 @@ private: " if (a > x && a < y)\n" " return;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (style) Condition 'a>x' is always false\n", errout.str()); check("void foo(A a) {\n" " const A x = a;\n" @@ -1426,7 +1475,9 @@ private: " if (a > x && a < y)\n" " return;\n" "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:5]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (style) Condition 'a>x' is always false\n" + "[test.cpp:5]: (style) Condition 'a [test.cpp:3]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str()); } + void oppositeInnerConditionOr() + { + check("void f(int x) {\n" + " if (x == 1 || x == 2) {\n" + " if (x == 3) {}\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); + + check("void f(int x) {\n" + " if (x == 1 || x == 2) {\n" + " if (x == 1) {}\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x == 1 || x == 2) {\n" + " if (x == 2) {}\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(std::string x) {\n" + " if (x == \"1\" || x == \"2\") {\n" + " if (x == \"1\") {}\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x < 1 || x > 3) {\n" + " if (x == 3) {}\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); + } + void oppositeInnerConditionEmpty() { check("void f1(const std::string &s) { if(s.size() > 42) if(s.empty()) {}}"); ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());