From 6ce5c24f21f4e3074ce6ae9cecc85f445164a653 Mon Sep 17 00:00:00 2001
From: Paul Fultz II <pfultz2@yahoo.com>
Date: Sun, 4 Sep 2022 03:24:45 -0500
Subject: [PATCH] Refactor knownConditionTrueFalse check and isUsedAsBool
 function (#4432)

* Refactor knownConditionTrueFalse check and isUsedAsBool function

* Format1

* Format

* Skip assign
---
 lib/astutils.cpp       | 161 +++++++++++++++++++++++++++--------------
 lib/astutils.h         |   4 +
 lib/checkcondition.cpp |  61 +++++++---------
 lib/valueflow.cpp      | 109 ++++++----------------------
 test/testcondition.cpp |   5 +-
 5 files changed, 163 insertions(+), 177 deletions(-)

diff --git a/lib/astutils.cpp b/lib/astutils.cpp
index b1b3f3973..ea771509d 100644
--- a/lib/astutils.cpp
+++ b/lib/astutils.cpp
@@ -633,6 +633,89 @@ const Token* getParentLifetime(bool cpp, const Token* tok, const Library* librar
     return *it;
 }
 
+static bool isInConstructorList(const Token* tok)
+{
+    if (!tok)
+        return false;
+    if (!astIsRHS(tok))
+        return false;
+    const Token* parent = tok->astParent();
+    if (!Token::Match(parent, "{|("))
+        return false;
+    if (!Token::Match(parent->previous(), "%var% {|("))
+        return false;
+    if (!parent->astOperand1() || !parent->astOperand2())
+        return false;
+    do {
+        parent = parent->astParent();
+    } while (Token::simpleMatch(parent, ","));
+    return Token::simpleMatch(parent, ":") && !Token::simpleMatch(parent->astParent(), "?");
+}
+
+std::vector<ValueType> getParentValueTypes(const Token* tok, const Settings* settings, const Token** parent)
+{
+    if (!tok)
+        return {};
+    if (!tok->astParent())
+        return {};
+    if (isInConstructorList(tok)) {
+        if (parent)
+            *parent = tok->astParent()->astOperand1();
+        if (tok->astParent()->astOperand1()->valueType())
+            return {*tok->astParent()->astOperand1()->valueType()};
+        return {};
+    } else if (Token::Match(tok->astParent(), "(|{|,")) {
+        int argn = -1;
+        const Token* ftok = getTokenArgumentFunction(tok, argn);
+        const Token* typeTok = nullptr;
+        if (ftok && argn >= 0) {
+            if (ftok->function()) {
+                std::vector<ValueType> result;
+                const Token* nameTok = nullptr;
+                for (const Variable* var : getArgumentVars(ftok, argn)) {
+                    if (!var)
+                        continue;
+                    if (!var->valueType())
+                        continue;
+                    nameTok = var->nameToken();
+                    result.push_back(*var->valueType());
+                }
+                if (result.size() == 1 && nameTok && parent) {
+                    *parent = nameTok;
+                }
+                return result;
+            } else if (const Type* t = Token::typeOf(ftok, &typeTok)) {
+                if (astIsPointer(typeTok))
+                    return {*typeTok->valueType()};
+                const Scope* scope = t->classScope;
+                // Check for aggregate constructors
+                if (scope && scope->numConstructors == 0 && t->derivedFrom.empty() &&
+                    (t->isClassType() || t->isStructType()) && numberOfArguments(ftok) < scope->varlist.size()) {
+                    assert(argn < scope->varlist.size());
+                    auto it = std::next(scope->varlist.begin(), argn);
+                    if (it->valueType())
+                        return {*it->valueType()};
+                }
+            }
+        }
+    }
+    if (settings && Token::Match(tok->astParent()->tokAt(-2), ". push_back|push_front|insert|push (") &&
+        astIsContainer(tok->astParent()->tokAt(-2)->astOperand1())) {
+        const Token* contTok = tok->astParent()->tokAt(-2)->astOperand1();
+        const ValueType* vtCont = contTok->valueType();
+        if (!vtCont->containerTypeToken)
+            return {};
+        ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, settings, true); // TODO: set isCpp
+        return {std::move(vtParent)};
+    }
+    if (Token::Match(tok->astParent(), "return|(|{|%assign%") && parent) {
+        *parent = tok->astParent();
+    }
+    if (tok->astParent()->valueType())
+        return {*tok->astParent()->valueType()};
+    return {};
+}
+
 bool astIsLHS(const Token* tok)
 {
     if (!tok)
@@ -1261,69 +1344,39 @@ static bool isForLoopCondition(const Token * const tok)
            parent->astParent()->astParent()->astOperand1()->str() == "for";
 }
 
-static bool isZeroConstant(const Token *tok)
-{
-    while (tok && tok->isCast())
-        tok = tok->astOperand2() ? tok->astOperand2() : tok->astOperand1();
-    return Token::simpleMatch(tok, "0") && !tok->isExpandedMacro();
-}
-
-/**
- * Is token used a boolean (cast to a bool, or used as a condition somewhere)
- * @param tok the token to check
- * @param checkingParent true if we are checking a parent. This is used to know
- * what we are checking. For instance in `if (i == 2)`, isUsedAsBool("==") is
- * true whereas isUsedAsBool("i") is false, but it might call
- * isUsedAsBool_internal("==") which must not return true
- */
-static bool isUsedAsBool_internal(const Token * const tok, bool checkingParent)
+bool isUsedAsBool(const Token* const tok)
 {
     if (!tok)
         return false;
-    const Token::Type type = tok->tokType();
-    if (type == Token::eBitOp || type == Token::eIncDecOp || (type == Token::eArithmeticalOp && !tok->isUnaryOp("*")))
-        // those operators don't return a bool
-        return false;
-    if (type == Token::eComparisonOp) {
-        if (!checkingParent)
-            // this operator returns a bool
-            return true;
-        if (Token::Match(tok, "==|!="))
-            return isZeroConstant(tok->astOperand1()) || isZeroConstant(tok->astOperand2());
-        return false;
-    }
-    if (type == Token::eLogicalOp)
-        return true;
     if (astIsBool(tok))
         return true;
-
-    const Token * const parent = tok->astParent();
+    if (Token::Match(tok, "!|&&|%oror%|%comp%"))
+        return true;
+    const Token* parent = tok->astParent();
     if (!parent)
         return false;
-    if (parent->str() == "(" && parent->astOperand2() == tok) {
-        if (Token::Match(parent->astOperand1(), "if|while"))
-            return true;
-
-        if (!parent->isCast()) { // casts are handled via the recursive call, as astIsBool will be true
-            // is it a call to a function ?
-            int argnr;
-            const Token *const func = getTokenArgumentFunction(tok, argnr);
-            if (!func || !func->function())
-                return false;
-            const Variable *var = func->function()->getArgumentVar(argnr);
-            return var && (var->getTypeName() == "bool");
-        }
-    } else if (isForLoopCondition(tok))
+    if (Token::Match(parent, "&&|!|%oror%"))
         return true;
-    else if (Token::simpleMatch(parent, "?") && astIsLHS(tok))
+    if (parent->isCast())
+        return isUsedAsBool(parent);
+    if (parent->isUnaryOp("*"))
+        return isUsedAsBool(parent);
+    if (Token::Match(parent, "==|!=") && tok->astSibling()->hasKnownIntValue() &&
+        tok->astSibling()->values().front().intvalue == 0)
         return true;
-
-    return isUsedAsBool_internal(parent, true);
-}
-
-bool isUsedAsBool(const Token * const tok)
-{
-    return isUsedAsBool_internal(tok, false);
+    if (parent->str() == "(" && astIsRHS(tok) && Token::Match(parent->astOperand1(), "if|while"))
+        return true;
+    if (Token::simpleMatch(parent, "?") && astIsLHS(tok))
+        return true;
+    if (isForLoopCondition(tok))
+        return true;
+    if (!Token::Match(parent, "%cop%")) {
+        std::vector<ValueType> vtParents = getParentValueTypes(tok);
+        return std::any_of(vtParents.begin(), vtParents.end(), [&](const ValueType& vt) {
+            return vt.pointer == 0 && vt.type == ValueType::BOOL;
+        });
+    }
+    return false;
 }
 
 static bool astIsBoolLike(const Token* tok)
diff --git a/lib/astutils.h b/lib/astutils.h
index d65bf05a8..9c4fc4e60 100644
--- a/lib/astutils.h
+++ b/lib/astutils.h
@@ -180,6 +180,10 @@ const Token* getParentMember(const Token * tok);
 const Token* getParentLifetime(const Token* tok);
 const Token* getParentLifetime(bool cpp, const Token* tok, const Library* library);
 
+std::vector<ValueType> getParentValueTypes(const Token* tok,
+                                           const Settings* settings = nullptr,
+                                           const Token** parent = nullptr);
+
 bool astIsLHS(const Token* tok);
 bool astIsRHS(const Token* tok);
 
diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp
index 6dbba4d7d..b0466994b 100644
--- a/lib/checkcondition.cpp
+++ b/lib/checkcondition.cpp
@@ -1467,6 +1467,7 @@ void CheckCondition::alwaysTrueFalse()
                 if (f->functionScope && Token::Match(f->functionScope->bodyStart, "{ return true|false ;"))
                     continue;
             }
+            const Token* condition = nullptr;
             {
                 // is this a condition..
                 const Token *parent = tok->astParent();
@@ -1474,29 +1475,43 @@ void CheckCondition::alwaysTrueFalse()
                     parent = parent->astParent();
                 if (!parent)
                     continue;
-                const Token *condition = nullptr;
                 if (parent->str() == "?" && precedes(tok, parent))
-                    condition = parent->astOperand1();
+                    condition = parent;
                 else if (Token::Match(parent->previous(), "if|while ("))
-                    condition = parent->astOperand2();
+                    condition = parent->previous();
                 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();
+                    condition = parent;
+                else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() &&
+                         Token::simpleMatch(parent->astParent()->astParent()->previous(), "for ("))
+                    condition = parent->astParent()->astParent()->previous();
                 else
                     continue;
-                (void)condition;
             }
             // Skip already diagnosed values
             if (diag(tok, false))
                 continue;
+            if (condition->isConstexpr())
+                continue;
+            if (!isUsedAsBool(tok))
+                continue;
+            if (Token::simpleMatch(tok->astParent(), "return") && Token::Match(tok, ".|%var%|%assign%"))
+                continue;
             if (Token::Match(tok, "%num%|%bool%|%char%"))
                 continue;
             if (Token::Match(tok, "! %num%|%bool%|%char%"))
                 continue;
-            if (Token::Match(tok, "%oror%|&&|:"))
+            if (Token::Match(tok, "%oror%|&&") &&
+                (tok->astOperand1()->hasKnownIntValue() || tok->astOperand2()->hasKnownIntValue()))
                 continue;
-            if (tok->isComparisonOp() && isSameExpression(mTokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true))
+            if (Token::simpleMatch(tok, ":"))
+                continue;
+            if (tok->isComparisonOp() && 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, "( )");
@@ -1508,32 +1523,11 @@ void CheckCondition::alwaysTrueFalse()
             {
                 const ValueFlow::Value *zeroValue = nullptr;
                 const Token *nonZeroExpr = nullptr;
-                if (CheckOther::comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr) || CheckOther::testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr))
+                if (CheckOther::comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr) ||
+                    CheckOther::testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr))
                     continue;
             }
 
-            const Token* astTop = tok->astTop();
-            const bool constIfWhileExpression =
-                tok->astParent() && Token::Match(astTop->astOperand1(), "if|while") && !astTop->astOperand1()->isConstexpr() &&
-                (Token::Match(tok->astParent(), "%oror%|&&") || Token::Match(tok->astParent()->astOperand1(), "if|while"));
-            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 isReturn = Token::simpleMatch(astTop, "return");
-            const bool returnExpression = isReturn && (tok->isComparisonOp() || Token::Match(tok, "&&|%oror%"));
-            const bool callExpression = Token::simpleMatch(tok, "(");
-            if (!(constIfWhileExpression || constValExpr || compExpr || ternaryExpression || returnExpression || callExpression))
-                continue;
-            // only warn for functions returning bool
-            if (isReturn && callExpression && !(scope->function && scope->function->retDef && scope->function->retDef->str() == "bool"))
-                continue;
-
-            const Token* expr1 = tok->astOperand1(), *expr2 = tok->astOperand2();
-            const bool isUnknown = (expr1 && expr1->valueType() && expr1->valueType()->type == ValueType::UNKNOWN_TYPE) ||
-                                   (expr2 && expr2->valueType() && expr2->valueType()->type == ValueType::UNKNOWN_TYPE);
-            if (isUnknown)
-                continue;
-
             // Don't warn when there are expanded macros..
             bool isExpandedMacro = false;
             visitAstNodes(tok, [&](const Token * tok2) {
@@ -1575,9 +1569,6 @@ void CheckCondition::alwaysTrueFalse()
             if (hasSizeof)
                 continue;
 
-            if (isIfConstexpr(tok))
-                continue;
-
             alwaysTrueFalseError(tok, &tok->values().front());
         }
     }
diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
index ab1b0e5b1..7b37a4315 100644
--- a/lib/valueflow.cpp
+++ b/lib/valueflow.cpp
@@ -378,91 +378,6 @@ const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, Val
     });
 }
 
-static bool isInConstructorList(const Token* tok)
-{
-    if (!tok)
-        return false;
-    if (!astIsRHS(tok))
-        return false;
-    const Token* parent = tok->astParent();
-    if (!Token::Match(parent, "{|("))
-        return false;
-    if (!Token::Match(parent->previous(), "%var% {|("))
-        return false;
-    if (!parent->astOperand1() || !parent->astOperand2())
-        return false;
-    do {
-        parent = parent->astParent();
-    } while (Token::simpleMatch(parent, ","));
-    return Token::simpleMatch(parent, ":") && !Token::simpleMatch(parent->astParent(), "?");
-}
-
-static std::vector<ValueType> getParentValueTypes(const Token* tok,
-                                                  const Settings* settings = nullptr,
-                                                  const Token** parent = nullptr)
-{
-    if (!tok)
-        return {};
-    if (!tok->astParent())
-        return {};
-    if (isInConstructorList(tok)) {
-        if (parent)
-            *parent = tok->astParent()->astOperand1();
-        if (tok->astParent()->astOperand1()->valueType())
-            return {*tok->astParent()->astOperand1()->valueType()};
-        return {};
-    } else if (Token::Match(tok->astParent(), "(|{|,")) {
-        int argn = -1;
-        const Token* ftok = getTokenArgumentFunction(tok, argn);
-        const Token* typeTok = nullptr;
-        if (ftok && argn >= 0) {
-            if (ftok->function()) {
-                std::vector<ValueType> result;
-                const Token* nameTok = nullptr;
-                for (const Variable* var : getArgumentVars(ftok, argn)) {
-                    if (!var)
-                        continue;
-                    if (!var->valueType())
-                        continue;
-                    nameTok = var->nameToken();
-                    result.push_back(*var->valueType());
-                }
-                if (result.size() == 1 && nameTok && parent) {
-                    *parent = nameTok;
-                }
-                return result;
-            } else if (const Type* t = Token::typeOf(ftok, &typeTok)) {
-                if (astIsPointer(typeTok))
-                    return {*typeTok->valueType()};
-                const Scope* scope = t->classScope;
-                // Check for aggregate constructors
-                if (scope && scope->numConstructors == 0 && t->derivedFrom.empty() &&
-                    (t->isClassType() || t->isStructType()) && numberOfArguments(ftok) < scope->varlist.size()) {
-                    assert(argn < scope->varlist.size());
-                    auto it = std::next(scope->varlist.begin(), argn);
-                    if (it->valueType())
-                        return {*it->valueType()};
-                }
-            }
-        }
-    }
-    if (settings && Token::Match(tok->astParent()->tokAt(-2), ". push_back|push_front|insert|push (") &&
-        astIsContainer(tok->astParent()->tokAt(-2)->astOperand1())) {
-        const Token* contTok = tok->astParent()->tokAt(-2)->astOperand1();
-        const ValueType* vtCont = contTok->valueType();
-        if (!vtCont->containerTypeToken)
-            return {};
-        ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, settings, true); // TODO: set isCpp
-        return {std::move(vtParent)};
-    }
-    if (Token::Match(tok->astParent(), "return|(|{|%assign%") && parent) {
-        *parent = tok->astParent();
-    }
-    if (tok->astParent()->valueType())
-        return {*tok->astParent()->valueType()};
-    return {};
-}
-
 static bool isEscapeScope(const Token* tok, TokenList * tokenlist, bool unknown = false)
 {
     if (!Token::simpleMatch(tok, "{"))
@@ -6434,6 +6349,28 @@ struct StartIteratorInferModel : IteratorInferModel {
     }
 };
 
+static bool isIntegralOnlyOperator(const Token* tok) {
+    return Token::Match(tok, "%|<<|>>|&|^|~|%or%");
+}
+
+static bool isIntegral(const Token* tok)
+{
+    if (!tok)
+        return false;
+    if (astIsIntegral(tok, false))
+        return true;
+    if (tok->valueType())
+        return false;
+    // These operators only work on integers
+    if (isIntegralOnlyOperator(tok))
+        return true;
+    if (isIntegralOnlyOperator(tok->astParent()))
+        return true;
+    if (Token::Match(tok, "+|-|*|/") && tok->isBinaryOp())
+        return isIntegral(tok->astOperand1()) && isIntegral(tok->astOperand2());
+    return false;
+}
+
 static void valueFlowInferCondition(TokenList* tokenlist,
                                     const Settings* settings)
 {
@@ -6463,7 +6400,7 @@ static void valueFlowInferCondition(TokenList* tokenlist,
                         setTokenValue(tok, value, settings);
                     }
                 }
-            } else {
+            } else if (isIntegral(tok->astOperand1()) && isIntegral(tok->astOperand2())) {
                 std::vector<ValueFlow::Value> result =
                     infer(IntegralInferModel{}, tok->str(), tok->astOperand1()->values(), tok->astOperand2()->values());
                 for (const ValueFlow::Value& value : result) {
diff --git a/test/testcondition.cpp b/test/testcondition.cpp
index 086aa3517..3ac059b7e 100644
--- a/test/testcondition.cpp
+++ b/test/testcondition.cpp
@@ -3336,7 +3336,7 @@ private:
               "    const int b = 52;\n"
               "    return a+b;\n"
               "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (style) Condition 'a+b' is always true\n", errout.str());
 
         check("int f() {\n"
               "    int a = 50;\n"
@@ -4318,7 +4318,8 @@ private:
         ASSERT_EQUALS("[test.cpp:3]: (style) Condition '!s.empty()' is always false\n"
                       "[test.cpp:4]: (style) Condition 's.empty()' is always true\n"
                       "[test.cpp:5]: (style) Condition 's.empty()' is always true\n"
-                      "[test.cpp:6]: (style) Condition '(bool)0' is always false\n",
+                      "[test.cpp:6]: (style) Condition '(bool)0' is always false\n"
+                      "[test.cpp:7]: (style) Condition 's.empty()' is always true\n",
                       errout.str());
 
         check("int f(bool b) {\n"