Add check for return value of boolean function (#1451)

* Add check for return value of boolean function

The rule for converting an integer to a boolean is that 0 is mapped to
false and everything else is mapped to true. There is nothing wrong with
the following code (according to the standards):

    bool f()
    {
        return -1;
    }

and neither gcc nor clang will warn about it. However, it's a bit
confusing. This commit adds a check that warns when a value other than 0
or 1 is returned from a boolean function (similar to the existing check
that functions with boolean arguments are only passed 0 or 1). Since the
code is perfectly legal, set the severity to "Style".

* Use early continue and remove some braces

* Add testcase with multiple returns

* Avoid null pointer dereference in case of return without operand

* Skip lambdas

Add TODO-test cases that shows FPs when the return type of lambdas are
specified explicitly (this is a problem with findLambdaEndToken).

* Enable testcases

lib/checkbool.cpp

@@ -27,6 +27,7 @@
 #include "symboldatabase.h"
 #include "token.h"
 #include "tokenize.h"
+#include "valueflow.h"
 
 #include <cstddef>
 #include <list>
@@ -454,3 +455,31 @@ void CheckBool::assignBoolToFloatError(const Token *tok)
     reportError(tok, Severity::style, "assignBoolToFloat",
                 "Boolean value assigned to floating point variable.", CWE704, false);
 }
+
+void CheckBool::returnValueOfFunctionReturningBool(void)
+{
+    if (!mSettings->isEnabled(Settings::STYLE))
+        return;
+
+    const SymbolDatabase * const symbolDatabase = mTokenizer->getSymbolDatabase();
+
+    for (const Scope * scope : symbolDatabase->functionScopes) {
+        if (!(scope->function && Token::Match(scope->function->retDef, "bool|_Bool")))
+            continue;
+
+        for (const Token* tok = scope->bodyStart->next(); tok && (tok != scope->bodyEnd); tok = tok->next()) {
+            // Skip lambdas
+            const Token* tok2 = findLambdaEndToken(tok);
+            if (tok2)
+                tok = tok2;
+            else if (Token::simpleMatch(tok, "return") && tok->astOperand1() &&
+                     (tok->astOperand1()->getValueGE(2, mSettings) || tok->astOperand1()->getValueLE(-1, mSettings)))
+                returnValueBoolError(tok);
+        }
+    }
+}
+
+void CheckBool::returnValueBoolError(const Token *tok)
+{
+    reportError(tok, Severity::style, "returnNonBoolInBooleanFunction", "Non-boolean value returned from function returning bool");
+}

lib/checkbool.h

@@ -58,6 +58,7 @@ public:
         checkBool.checkComparisonOfBoolWithInt();
         checkBool.checkAssignBoolToFloat();
         checkBool.pointerArithBool();
+        checkBool.returnValueOfFunctionReturningBool();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -100,6 +101,9 @@ public:
     void pointerArithBool();
     void pointerArithBoolCond(const Token *tok);
 
+    /** @brief %Check if a function returning bool returns an integer other than 0 or 1 */
+    void returnValueOfFunctionReturningBool();
+
 private:
     // Error messages..
     void comparisonOfFuncReturningBoolError(const Token *tok, const std::string &expression);
@@ -112,6 +116,7 @@ private:
     void bitwiseOnBooleanError(const Token *tok, const std::string &varname, const std::string &op);
     void comparisonOfBoolExpressionWithIntError(const Token *tok, bool n0o1);
     void pointerArithBoolError(const Token *tok);
+    void returnValueBoolError(const Token *tok);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override {
         CheckBool c(nullptr, settings, errorLogger);
@@ -126,6 +131,7 @@ private:
         c.comparisonOfBoolExpressionWithIntError(nullptr, true);
         c.pointerArithBoolError(nullptr);
         c.comparisonOfBoolWithInvalidComparator(nullptr, "expression");
+        c.returnValueBoolError(nullptr);
     }
 
     static std::string myName() {
@@ -140,7 +146,8 @@ private:
                "- comparison of a boolean value with boolean value using relational operator\n"
                "- using bool in bitwise expression\n"
                "- pointer addition in condition (either dereference is forgot or pointer overflow is required to make the condition false)\n"
-               "- Assigning bool value to pointer or float\n";
+               "- Assigning bool value to pointer or float\n"
+               "- Returning an integer other than 0 or 1 from a function with boolean return value\n";
     }
 };
 /// @}

test/testbool.cpp

@@ -64,6 +64,8 @@ private:
 
         // Converting pointer addition result to bool
         TEST_CASE(pointerArithBool1);
+
+        TEST_CASE(returnNonBool);
     }
 
     void check(const char code[], bool experimental = false, const char filename[] = "test.cpp") {
@@ -1008,6 +1010,103 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:2]: (error) Converting pointer arithmetic result to bool. The bool is always true unless there is undefined behaviour.\n", errout.str());
     }
+
+    void returnNonBool() {
+        check("bool f(void) {\n"
+              "    return 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "    return 1;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "    return 2;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+
+        check("bool f(void) {\n"
+              "    return -1;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+
+        check("bool f(void) {\n"
+              "    return 1 + 1;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+
+        check("bool f(void) {\n"
+              "    int x = 0;\n"
+              "    return x;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "    int x = 10;\n"
+              "    return x;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+
+        check("bool f(void) {\n"
+              "    return 2 < 1;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "    int ret = 0;\n"
+              "    if (a)\n"
+              "        ret = 1;\n"
+              "    return ret;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "    int ret = 0;\n"
+              "    if (a)\n"
+              "        ret = 3;\n"
+              "    return ret;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:5]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+
+        check("bool f(void) {\n"
+              "    if (a)\n"
+              "        return 3;\n"
+              "    return 4;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Non-boolean value returned from function returning bool\n"
+                      "[test.cpp:4]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+
+        check("bool f(void) {\n"
+              "    return;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "auto x = [](void) { return -1; };\n"
+              "return false;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "auto x = [](void) { return -1; };\n"
+              "return 2;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+
+        check("bool f(void) {\n"
+              "auto x = [](void) -> int { return -1; };\n"
+              "return false;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(void) {\n"
+              "auto x = [](void) -> int { return -1; };\n"
+              "return 2;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Non-boolean value returned from function returning bool\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestBool)