New check: Warn about constant expression if ( unknown | non_null_constant) (#6519)

lib/checkcondition.cpp

@@ -196,6 +196,32 @@ static void getnumchildren(const Token *tok, std::list<MathLib::bigint> &numchil
         getnumchildren(tok->astOperand2(), numchildren);
 }
 
+void CheckCondition::checkBadBitmaskCheck()
+{
+    if (!_settings->isEnabled("warning"))
+        return;
+
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
+        if (tok->str() == "|" && tok->astOperand1() && tok->astOperand2() && tok->astParent()) {
+            bool isBoolean = Token::Match(tok->astParent(), "&&|%oror%") ||
+                             (tok->astParent()->str() == "?" && tok->astParent()->astOperand1() == tok) ||
+                             (tok->astParent()->str() == "=" && tok->astParent()->astOperand2() == tok && tok->astParent()->astOperand1()->variable() && tok->astParent()->astOperand1()->variable()->typeStartToken()->str() == "bool") ||
+                             (tok->astParent()->str() == "(" && Token::Match(tok->astParent()->astOperand1(), "if|while"));
+
+            bool isTrue = (tok->astOperand1()->values.size() == 1 && tok->astOperand1()->values.front().intvalue != 0 && !tok->astOperand1()->values.front().conditional) ||
+                          (tok->astOperand2()->values.size() == 1 && tok->astOperand2()->values.front().intvalue != 0 && !tok->astOperand2()->values.front().conditional);
+
+            if (isBoolean && isTrue)
+                badBitmaskCheckError(tok);
+        }
+    }
+}
+
+void CheckCondition::badBitmaskCheckError(const Token *tok)
+{
+    reportError(tok, Severity::warning, "badBitmaskCheck", "Result of operator '|' is always true if one operand is non-zero. Did you intend to use '&'?");
+}
+
 void CheckCondition::comparison()
 {
     if (!_settings->isEnabled("style"))

lib/checkcondition.h

@@ -54,6 +54,7 @@ public:
     void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) {
         CheckCondition checkCondition(tokenizer, settings, errorLogger);
         checkCondition.assignIf();
+        checkCondition.checkBadBitmaskCheck();
         checkCondition.comparison();
         checkCondition.oppositeInnerCondition();
         checkCondition.checkIncorrectLogicOperator();
@@ -71,6 +72,9 @@ public:
                             const char bitop,
                             const MathLib::bigint num);
 
+    /** check bitmask using | instead of & */
+    void checkBadBitmaskCheck();
+
     /** mismatching lhs and rhs in comparison */
     void comparison();
 
@@ -93,6 +97,7 @@ private:
 
     void assignIfError(const Token *tok1, const Token *tok2, const std::string &condition, bool result);
     void mismatchingBitAndError(const Token *tok1, const MathLib::bigint num1, const Token *tok2, const MathLib::bigint num2);
+    void badBitmaskCheckError(const Token *tok);
     void comparisonError(const Token *tok,
                          const std::string &bitop,
                          MathLib::bigint value1,
@@ -114,6 +119,7 @@ private:
         CheckCondition c(0, settings, errorLogger);
 
         c.assignIfError(0, 0, "", false);
+        c.badBitmaskCheckError(0);
         c.comparisonError(0, "&", 6, "==", 1, false);
         c.multiConditionError(0,1);
         c.mismatchingBitAndError(0, 0xf0, 0, 1);
@@ -132,6 +138,7 @@ private:
         return "Match conditions with assignments and other conditions:\n"
                "- Mismatching assignment and comparison => comparison is always true/false\n"
                "- Mismatching lhs and rhs in comparison => comparison is always true/false\n"
+               "- Detect usage of | where & should be used\n"
                "- Detect matching 'if' and 'else if' conditions\n"
                "- Mismatching bitand (a &= 0xf0; a &= 1; => a = 0)\n"
                "- Find dead code which is inaccessible due to the counter-conditions check in nested if statements\n"

test/testcondition.cpp

@@ -39,7 +39,8 @@ private:
         TEST_CASE(compare);            // mismatching LHS/RHS in comparison
         TEST_CASE(multicompare);       // mismatching comparisons
         TEST_CASE(duplicateIf);        // duplicate conditions in if and else-if
-        TEST_CASE(invalidMissingSemicolon); // crash as of #5867
+
+        TEST_CASE(checkBadBitmaskCheck);
 
         TEST_CASE(incorrectLogicOperator1);
         TEST_CASE(incorrectLogicOperator2);
@@ -465,11 +466,68 @@ private:
         ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str());
     }
 
-    void invalidMissingSemicolon() {
-        // simply survive - a syntax error would be even better
-        check("void f(int x) {\n"
-              " x = 42\n"
-              "}\n");
+    void checkBadBitmaskCheck() {
+        check("bool f(int x) {\n"
+              "    bool b = x | 0x02;\n"
+              "    return b;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Result of operator '|' is always true if one operand is non-zero. Did you intend to use '&'?\n", errout.str());
+
+        check("bool f(int x) {\n"
+              "    bool b = 0x02 | x;\n"
+              "    return b;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Result of operator '|' is always true if one operand is non-zero. Did you intend to use '&'?\n", errout.str());
+
+        check("int f(int x) {\n"
+              "    int b = x | 0x02;\n"
+              "    return b;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(int x) {\n"
+              "    bool b = x & 0x02;\n"
+              "    return b;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(int x) {\n"
+              "    if(x | 0x02)\n"
+              "        return b;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Result of operator '|' is always true if one operand is non-zero. Did you intend to use '&'?\n", errout.str());
+
+        check("bool f(int x) {\n"
+              "    int y = 0x1;\n"
+              "    if(b) y = 0;\n"
+              "    if(x | y)\n"
+              "        return b;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(int x) {\n"
+              "    foo(a && (x | 0x02));\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Result of operator '|' is always true if one operand is non-zero. Did you intend to use '&'?\n", errout.str());
+
+        check("int f(int x) {\n"
+              "    return (x | 0x02) ? 0 : 5;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Result of operator '|' is always true if one operand is non-zero. Did you intend to use '&'?\n", errout.str());
+
+        check("int f(int x) {\n"
+              "    return x ? (x | 0x02) : 5;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool f(int x) {\n"
+              "    return x | 0x02;\n"
+              "}");
+        TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) Result of operator '|' is always true if one operand is non-zero. Did you intend to use '&'?\n", "", errout.str());
+
+        check("int f(int x) {\n"
+              "    return x | 0x02;\n"
+              "}");
         ASSERT_EQUALS("", errout.str());
     }
 

test/testgarbage.cpp

@@ -70,6 +70,7 @@ private:
         TEST_CASE(garbageCode27);
         TEST_CASE(garbageCode28);
         TEST_CASE(garbageCode29);
+        TEST_CASE(garbageCode30); // #5867
 
         TEST_CASE(garbageValueFlow);
         TEST_CASE(garbageSymbolDatabase);
@@ -385,6 +386,13 @@ private:
         checkCode("|| #if #define <=");
     }
 
+    void garbageCode30() {
+        // simply survive - a syntax error would be even better (#5867)
+        checkCode("void f(int x) {\n"
+                  " x = 42\n"
+                  "}");
+    }
+
     void garbageValueFlow() {
         // #6089
         const char* code = "{} int foo(struct, x1, struct x2, x3, int, x5, x6, x7)\n"