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

@@ -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")

lib/checkcondition.cpp

@@ -1507,13 +1507,14 @@ void CheckCondition::alwaysTrueFalse()
                 continue;
             if (Token::simpleMatch(tok, ":"))
                 continue;
-            if (tok->isComparisonOp() && isSameExpression(mTokenizer->isCPP(),
+            if (tok->isComparisonOp() && isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand1()) &&
-                                                          true,
+                isSameExpression(mTokenizer->isCPP(),
-                                                          tok->astOperand1(),
+                                 true,
-                                                          tok->astOperand2(),
+                                 tok->astOperand1(),
-                                                          mSettings->library,
+                                 tok->astOperand2(),
-                                                          true,
+                                 mSettings->library,
-                                                          true))
+                                 true,
+                                 true))
                 continue;
             if (isConstVarExpression(tok, [](const Token* tok) {
                 return Token::Match(tok, "[|(|&|+|-|*|/|%|^|>>|<<") && !Token::simpleMatch(tok, "( )");

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) {

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<y' is always false\n",
+                      errout.str());
 
         check("struct A {\n"
               "    void f();\n"
@@ -1408,7 +1457,7 @@ private:
               "  if (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<y' is always false\n",
+                      errout.str());
     }
 
     void incorrectLogicOperator13() {
@@ -2489,6 +2540,46 @@ private:
         ASSERT_EQUALS("[test.cpp:2] -> [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());