Check conditions in return statements (#1411)

* Identify return conditions in multiconditions

* Improve error messages

* Check return statements are always true or false

* Add more tests for FPs

* Fix FP when returning const like variables

* Fix FP when returning pointers or classes

* Fix FP with member variable access

* Check non-local variables

* Use simplematch

* Check for null

lib/astutils.cpp

@@ -878,6 +878,20 @@ bool isVariableChanged(const Token *start, const Token *end, const unsigned int
     return false;
 }
 
+bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp)
+{
+    if(!var)
+        return false;
+    if(!var->scope())
+        return false;
+    const Token * start = var->declEndToken();
+    if(!start)
+        return false;
+    if(Token::Match(start, "; %varid% =", var->declarationId()))
+        start = start->tokAt(2);
+    return isVariableChanged(start->next(), var->scope()->bodyEnd, var->declarationId(), var->isGlobal(), settings, cpp);
+}
+
 int numberOfArguments(const Token *start)
 {
     int arguments=0;

lib/astutils.h

@@ -30,6 +30,7 @@
 class Library;
 class Settings;
 class Token;
+class Variable;
 
 /** Is expression a 'signed char' if no promotion is used */
 bool astIsSignedChar(const Token *tok);
@@ -110,6 +111,8 @@ bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings,
 /** Is variable changed in block of code? */
 bool isVariableChanged(const Token *start, const Token *end, const unsigned int varid, bool globalvar, const Settings *settings, bool cpp);
 
+bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp);
+
 /** Determines the number of arguments - if token is a function call or macro
  * @param start token which is supposed to be the function/macro name.
  * \return Number of arguments

lib/checkcondition.cpp

@@ -535,21 +535,27 @@ void CheckCondition::multiCondition2()
             continue;
 
         // parse until second condition is reached..
-        enum MULTICONDITIONTYPE { INNER, AFTER } type;
+        enum MULTICONDITIONTYPE { INNER, AFTER };
         const Token *tok;
-        if (Token::Match(scope.bodyStart, "{ return|throw|continue|break")) {
+
+        // Parse inner condition first and then early return condition
+        std::vector<MULTICONDITIONTYPE> types = {MULTICONDITIONTYPE::INNER};
+        if (Token::Match(scope.bodyStart, "{ return|throw|continue|break"))
+            types.push_back(MULTICONDITIONTYPE::AFTER);
+        for(MULTICONDITIONTYPE type:types) {
+            if(type == MULTICONDITIONTYPE::AFTER) {
                 tok = scope.bodyEnd->next();
-            type = MULTICONDITIONTYPE::AFTER;
             } else {
                 tok = scope.bodyStart;
-            type = MULTICONDITIONTYPE::INNER;
             }
             const Token * const endToken = tok->scope()->bodyEnd;
 
             for (; tok && tok != endToken; tok = tok->next()) {
-            if (Token::simpleMatch(tok, "if (")) {
+                if (Token::Match(tok, "if|return")) {
+                    const Token * condStartToken = tok->str() == "if" ? tok->next() : tok;
+                    const Token * condEndToken = tok->str() == "if" ? condStartToken->link() : Token::findsimplematch(condStartToken, ";");
                     // Does condition modify tracked variables?
-                if (const Token *op = Token::findmatch(tok, "++|--", tok->linkAt(1))) {
+                    if (const Token *op = Token::findmatch(tok, "++|--", condEndToken)) {
                         bool bailout = false;
                         while (op) {
                             if (vars.find(op->astOperand1()->varId()) != vars.end()) {
@@ -560,14 +566,24 @@ void CheckCondition::multiCondition2()
                                 bailout = true;
                                 break;
                             }
-                        op = Token::findmatch(op->next(), "++|--", tok->linkAt(1));
+                            op = Token::findmatch(op->next(), "++|--", condEndToken);
                         }
                         if (bailout)
                             break;
                     }
 
                     // Condition..
-                const Token *cond2 = tok->next()->astOperand2();
+                    const Token *cond2 = tok->str() == "if" ? condStartToken->astOperand2() : condStartToken->astOperand1();
+                    // Check if returning boolean values
+                    if (tok->str() == "return") {
+                        const Variable * condVar = nullptr;
+                        if (Token::Match(cond2, "%var% ;"))
+                            condVar = cond2->variable();
+                        else if(Token::Match(cond2, ". %var% ;"))
+                            condVar = cond2->next()->variable();
+                        if (condVar && (condVar->isClass() || condVar->isPointer()))
+                            break;
+                    }
 
                     ErrorPath errorPath;
 
@@ -682,16 +698,30 @@ void CheckCondition::multiCondition2()
             }
         }
     }
+}
+
+static std::string innerSmtString(const Token * tok)
+{
+    if(!tok)
+        return "if";
+    if(!tok->astTop())
+        return "if";
+    const Token * top = tok->astTop();
+    if(top->str() == "(" && top->astOperand1())
+        return top->astOperand1()->str();
+    return top->str();
+}
 
 void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath)
 {
     const std::string s1(tok1 ? tok1->expressionString() : "x");
     const std::string s2(tok2 ? tok2->expressionString() : "!x");
+    const std::string innerSmt = innerSmtString(tok2);
     errorPath.emplace_back(ErrorPathItem(tok1, "outer condition: " + s1));
     errorPath.emplace_back(ErrorPathItem(tok2, "opposite inner condition: " + s2));
 
-    const std::string msg("Opposite inner 'if' condition leads to a dead code block.\n"
+    const std::string msg("Opposite inner '" + innerSmt + "' condition leads to a dead code block.\n"
-                          "Opposite inner 'if' condition leads to a dead code block (outer condition is '" + s1 + "' and inner condition is '" + s2 + "').");
+                          "Opposite inner '" + innerSmt + "' condition leads to a dead code block (outer condition is '" + s1 + "' and inner condition is '" + s2 + "').");
     reportError(errorPath, Severity::warning, "oppositeInnerCondition", msg, CWE398, false);
 }
 
@@ -699,11 +729,12 @@ void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token
 {
     const std::string s1(tok1 ? tok1->expressionString() : "x");
     const std::string s2(tok2 ? tok2->expressionString() : "x");
+    const std::string innerSmt = innerSmtString(tok2);
     errorPath.emplace_back(ErrorPathItem(tok1, "outer condition: " + s1));
     errorPath.emplace_back(ErrorPathItem(tok2, "identical inner condition: " + s2));
 
-    const std::string msg("Identical inner 'if' condition is always true.\n"
+    const std::string msg("Identical inner '" + innerSmt + "' condition is always true.\n"
-                          "Identical inner 'if' condition is always true (outer condition is '" + s1 + "' and inner condition is '" + s2 + "').");
+                          "Identical inner '" + innerSmt + "' condition is always true (outer condition is '" + s1 + "' and inner condition is '" + s2 + "').");
     reportError(errorPath, Severity::warning, "identicalInnerCondition", msg, CWE398, false);
 }
 
@@ -1222,7 +1253,7 @@ void CheckCondition::alwaysTrueFalse()
                 continue;
             if (Token::Match(tok, "! %num%|%bool%|%char%"))
                 continue;
-            if (Token::Match(tok, "%oror%|&&"))
+            if (Token::Match(tok, "%oror%|&&|:"))
                 continue;
             if (Token::Match(tok, "%comp%") && isSameExpression(mTokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true))
                 continue;
@@ -1232,8 +1263,20 @@ void CheckCondition::alwaysTrueFalse()
                 (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 returnStatement = Token::simpleMatch(tok->astTop(), "return") &&
+                Token::Match(tok->astParent(), "%oror%|&&|return");
 
-            if (!(constIfWhileExpression || constValExpr || compExpr))
+            if (!(constIfWhileExpression || constValExpr || compExpr || returnStatement))
+                continue;
+
+            if(returnStatement && (tok->isEnumerator() || Token::Match(tok, "nullptr|NULL")))
+                continue;
+
+            if(returnStatement && Token::simpleMatch(tok->astParent(), "return") && tok->variable() && (
+                !tok->variable()->isLocal() ||
+                tok->variable()->isReference() ||
+                tok->variable()->isConst() || 
+                !isVariableChanged(tok->variable(), mSettings, mTokenizer->isCPP())))
                 continue;
 
             // Don't warn in assertions. Condition is often 'always true' by intention.

test/testcondition.cpp

@@ -1441,6 +1441,13 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
 
+        check("bool foo(int a, int b) {\n"
+              "    if(a==b)\n"
+              "        return a!=b;\n"
+              "    return false;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite inner 'return' condition leads to a dead code block.\n", errout.str());
+
         check("void foo(int a, int b) {\n"
               "    if(a==b)\n"
               "        if(b!=a)\n"
@@ -2049,6 +2056,25 @@ private:
               "}\n");
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Identical inner 'if' condition is always true.\n", errout.str());
 
+        check("bool f(bool a) {\n"
+              "    if(a) { return a; }\n"
+              "    return false;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (warning) Identical inner 'return' condition is always true.\n", errout.str());
+
+        check("int* f(int* a, int * b) {\n"
+              "    if(a) { return a; }\n"
+              "    return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct A { int * x; };\n"
+              "int* f(A a, int * b) {\n"
+              "    if(a.x) { return a.x; }\n"
+              "    return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
         check("void f() {\n"
               "    uint32_t value;\n"
               "    get_value(&value);\n"
@@ -2069,6 +2095,12 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Identical condition 'x>100', second condition is always false\n", errout.str());
 
+        check("bool f(int x) {\n"
+              "  if (x > 100) { return false; }\n"
+              "  return x > 100;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Identical condition 'x>100', second condition is always false\n", errout.str());
+
         check("void f(int x) {\n"
               "  if (x > 100) { return; }\n"
               "  if (x > 100 || y > 100) {}\n"
@@ -2444,6 +2476,12 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:4]: (style) Condition '!x' is always true\n", errout.str());
 
+        check("bool f(int x) {\n"
+              "  if(x == 0) { x++; return x == 0; } \n"
+              "  return false;\n"
+              "}");
+        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"
               "  A(x++ == 1);\n"
@@ -2546,6 +2584,42 @@ private:
               "    {}\n"
               "}\n");
         ASSERT_EQUALS("[test.cpp:4]: (style) Condition '!b' is always true\n", errout.str());
+
+        check("bool f() { return nullptr; }");
+        ASSERT_EQUALS("", errout.str());
+
+        check("enum E { A };\n"
+              "bool f() { return A; }\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f() { \n"
+              "    const int x = 0;\n"
+              "    return x; \n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f() { \n"
+              "    int x = 0;\n"
+              "    return x; \n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool& g();\n"
+              "bool f() {\n"
+              "    bool & b = g();\n"
+              "    b = false;\n"
+              "    return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct A {\n"
+              "    bool b;\n"
+              "    bool f() {\n"
+              "        b = false;\n"
+              "        return b;\n"
+              "    }\n"
+              "};\n");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void multiConditionAlwaysTrue() {