From d5b6d49d967651ba3b195eb3b73c2ea05e9750ae Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 28 Jun 2020 15:28:08 -0500 Subject: [PATCH 1/3] Fix issue 9578: false negative: (style) Condition '...' is always false --- lib/astutils.cpp | 19 ++++++++++++++---- lib/astutils.h | 1 + lib/checkcondition.cpp | 5 ++++- lib/valueflow.cpp | 45 ++++++++++++++++++++---------------------- test/testcondition.cpp | 45 +++++++++++++++++++++++++++++------------- 5 files changed, 72 insertions(+), 43 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 712847e46..08e595348 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -20,6 +20,7 @@ //--------------------------------------------------------------------------- #include "astutils.h" +#include "config.h" #include "library.h" #include "mathlib.h" #include "settings.h" @@ -32,13 +33,13 @@ #include #include - -void visitAstNodes(const Token *ast, std::function visitor) +template)> +void visitAstNodesGeneric(T *ast, std::function visitor) { - std::stack tokens; + std::stack tokens; tokens.push(ast); while (!tokens.empty()) { - const Token *tok = tokens.top(); + T *tok = tokens.top(); tokens.pop(); if (!tok) continue; @@ -54,6 +55,16 @@ void visitAstNodes(const Token *ast, std::function visitor) +{ + visitAstNodesGeneric(ast, std::move(visitor)); +} + +void visitAstNodes(Token *ast, std::function visitor) +{ + visitAstNodesGeneric(ast, std::move(visitor)); +} + static void astFlattenRecursive(const Token *tok, std::vector *result, const char* op, nonneg int depth = 0) { ++depth; diff --git a/lib/astutils.h b/lib/astutils.h index 3cd883f4d..26e67f643 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -47,6 +47,7 @@ enum class ChildrenToVisit { * Visit AST nodes recursively. The order is not "well defined" */ void visitAstNodes(const Token *ast, std::function visitor); +void visitAstNodes(Token *ast, std::function visitor); std::vector astFlatten(const Token* tok, const char* op); diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index ec9b33c39..9635dbb01 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1401,6 +1401,8 @@ void CheckCondition::alwaysTrueFalse() condition = parent->astOperand1(); else if (Token::Match(parent->previous(), "if|while (")) condition = parent->astOperand2(); + else if (Token::simpleMatch(parent, "return")) + condition = parent->astOperand1(); else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() && Token::simpleMatch(parent->astParent()->astParent()->previous(), "for (")) condition = parent->astOperand1(); else @@ -1427,8 +1429,9 @@ void CheckCondition::alwaysTrueFalse() const bool constValExpr = tok->isNumber() && Token::Match(tok->astParent(),"%oror%|&&|?"); // just one number in boolean expression const bool compExpr = Token::Match(tok, "%comp%|!"); // a compare expression const bool ternaryExpression = Token::simpleMatch(tok->astParent(), "?"); + const bool returnExpression = Token::simpleMatch(tok->astTop(), "return") && (tok->isComparisonOp() || Token::Match(tok, "&&|%oror%")); - if (!(constIfWhileExpression || constValExpr || compExpr || ternaryExpression)) + if (!(constIfWhileExpression || constValExpr || compExpr || ternaryExpression || returnExpression)) continue; // Don't warn when there are expanded macros.. diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 8b10c6ba2..6904aabc1 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4071,39 +4071,36 @@ struct ValueFlowConditionHandler { if (Token::Match(tok->astParent(), "%oror%|&&")) { Token *parent = tok->astParent(); - const std::string &op(parent->str()); - - if (parent->astOperand1() == tok && ((op == "&&" && Token::Match(tok, "==|>=|<=|!")) || - (op == "||" && Token::Match(tok, "%name%|!=")))) { - for (; parent && parent->str() == op; parent = parent->astParent()) { - std::stack tokens; - tokens.push(parent->astOperand2()); - bool assign = false; - while (!tokens.empty()) { - Token *rhstok = tokens.top(); - tokens.pop(); - if (!rhstok) - continue; - tokens.push(rhstok->astOperand1()); - tokens.push(rhstok->astOperand2()); - if (isSameExpression( - tokenlist->isCPP(), false, cond.vartok, rhstok, settings->library, true, false)) - setTokenValue(rhstok, cond.true_values.front(), settings); - else if (Token::Match(rhstok, "++|--|=") && isSameExpression(tokenlist->isCPP(), + if (astIsRHS(tok) && parent->astParent() && parent->str() == parent->astParent()->str()) + parent = parent->astParent(); + else if (!astIsLHS(tok)) { + parent = nullptr; + } + if (parent) { + const std::string &op(parent->str()); + bool assign = false; + std::list values = cond.true_values; + if (Token::Match(tok, "==|!=")) + changePossibleToKnown(values); + if ((op == "&&" && Token::Match(tok, "==|>=|<=|!")) || + (op == "||" && Token::Match(tok, "%name%|!="))) { + visitAstNodes(parent->astOperand2(), [&](Token* tok2) { + if (isSameExpression(tokenlist->isCPP(), false, cond.vartok, tok2, settings->library, true, false)) + setTokenValue(tok2, values.front(), settings); + else if (Token::Match(tok2, "++|--|=") && isSameExpression(tokenlist->isCPP(), false, cond.vartok, - rhstok->astOperand1(), + tok2->astOperand1(), settings->library, true, false)) { assign = true; - break; + return ChildrenToVisit::done; } - } + return ChildrenToVisit::op1_and_op2; + }); if (assign) break; - while (parent->astParent() && parent == parent->astParent()->astOperand2()) - parent = parent->astParent(); } } } diff --git a/test/testcondition.cpp b/test/testcondition.cpp index d02abd8ad..3862f28bc 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -835,14 +835,16 @@ private: " a++;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always true\n", errout.str()); check("void f(int x) {\n" " if (1 != x || 3 != x)\n" " a++;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition '3!=x' is always true\n", errout.str()); check("void f(int x) {\n" " if (x<0 && !x) {}\n" @@ -852,12 +854,14 @@ private: check("void f(int x) {\n" " if (x==0 && x) {}\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x' is always false\n", errout.str()); check("void f(int x) {\n" // ast.. " if (y == 1 && x == 1 && x == 7) { }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==7' is always false\n", errout.str()); check("void f(int x, int y) {\n" " if (x != 1 || y != 1)\n" @@ -878,7 +882,7 @@ private: " a++;\n" "}\n" ); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always true\n", errout.str()); check("void f(int x) {\n" " if ((x != 1) && (x != 3))\n" @@ -921,7 +925,8 @@ private: " a++;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=6' is always true\n", errout.str()); check("void f(unsigned int a, unsigned int b, unsigned int c) {\n" " if((a != b) || (c != b) || (c != a))\n" @@ -945,13 +950,14 @@ private: " if ((x == 1) && (x == 0x00000001))\n" " a++;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==0x00000001' is always true\n", errout.str()); check("void f(int x) {\n" " if (x == 1 && x == 3)\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str()); check("void f(int x) {\n" " if (x == 1.0 && x == 3.0)\n" @@ -1074,7 +1080,8 @@ private: " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=4' is always true\n", errout.str()); check("void f(int x) {\n" " if ((x!=4) && (x==3))\n" @@ -1092,13 +1099,15 @@ private: " if ((x!=4) || (x==3))\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str()); check("void f(int x) {\n" " if ((x==3) && (x!=3))\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always false\n", errout.str()); check("void f(int x) {\n" " if ((x==6) || (x!=6))\n" @@ -1190,7 +1199,8 @@ private: check("void f(char x) {\n" " if (x == '1' && x == '2') {}\n" "}", "test.cpp", true); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n" + "[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x=='2'' is always false\n", errout.str()); check("int f(char c) {\n" " return (c >= 'a' && c <= 'z');\n" @@ -1264,7 +1274,8 @@ private: " if ((t == A) && (t == B))\n" " {}\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n" + "[test.cpp:3] -> [test.cpp:3]: (style) Condition 't==B' is always false\n", errout.str()); } void incorrectLogicOperator11() { @@ -2864,7 +2875,7 @@ private: " if(x == 0) { x++; return x == 0; } \n" " return false;\n" "}"); - TODO_ASSERT_EQUALS("return value is always true?", "", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==0' is always false\n", errout.str()); check("void f() {\n" // #6898 (Token::expressionString) " int x = 0;\n" @@ -3757,6 +3768,12 @@ private: check("bool f();\n" "void foo() { bool x = true; if(x&&f()) {}}\n"); ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str()); + + // #9578 + check("bool f(const std::string &s) {\n" + " return s.size()>2U && s[0]=='4' && s[0]=='2';\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str()); } void pointerAdditionResultNotNull() { From edcf668ae28f42b3c935a90ce79e204f5875a6fd Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 29 Jun 2020 10:15:36 -0500 Subject: [PATCH 2/3] Update test mesg --- test/testcondition.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 3862f28bc..2193a9880 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -3773,7 +3773,7 @@ private: check("bool f(const std::string &s) {\n" " return s.size()>2U && s[0]=='4' && s[0]=='2';\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 's[0]=='2'' is always false\n", errout.str()); } void pointerAdditionResultNotNull() { From 0def5d7a9aad9e2b098230c7671238c5f26648a5 Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 21 Jul 2020 19:09:58 -0500 Subject: [PATCH 3/3] Reduce variable scope --- lib/valueflow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index b224754c1..a74685c39 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4078,12 +4078,12 @@ struct ValueFlowConditionHandler { } if (parent) { const std::string &op(parent->str()); - bool assign = false; std::list values = cond.true_values; if (Token::Match(tok, "==|!=")) changePossibleToKnown(values); if ((op == "&&" && Token::Match(tok, "==|>=|<=|!")) || (op == "||" && Token::Match(tok, "%name%|!="))) { + bool assign = false; visitAstNodes(parent->astOperand2(), [&](Token* tok2) { if (isSameExpression(tokenlist->isCPP(), false, cond.vartok, tok2, settings->library, true, false)) setTokenValue(tok2, values.front(), settings);