Fixed cppcheck-opensource#3593 (New Check: Check for assignment within conditional expression)

danmar · danmar · commit 72802554c928 · 2021-04-07T17:21:34.000+02:00
diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp
@@ -1698,3 +1698,46 @@ void CheckCondition::duplicateConditionalAssignError(const Token *condTok, const
     reportError(
         errors, Severity::style, "duplicateConditionalAssign", msg, CWE398, Certainty::normal);
 }
+
+
+void CheckCondition::checkAssignmentInCondition()
+{
+    if (!mSettings->severity.isEnabled(Severity::style))
+        return;
+
+    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
+    for (const Scope * scope : symbolDatabase->functionScopes) {
+        for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) {
+            if (tok->str() != "=")
+                continue;
+            if (!tok->astParent())
+                continue;
+
+            // Is this assignment of container/iterator?
+            if (!tok->valueType())
+                continue;
+            if (tok->valueType()->type != ValueType::Type::CONTAINER && tok->valueType()->type != ValueType::Type::ITERATOR)
+                continue;
+
+            // warn if this is a conditional expression..
+            if (Token::Match(tok->astParent()->previous(), "if|while ("))
+                assignmentInCondition(tok);
+            else if (Token::Match(tok->astParent(), "%oror%|&&"))
+                assignmentInCondition(tok);
+            else if (Token::simpleMatch(tok->astParent(), "?") && tok == tok->astParent()->astOperand1())
+                assignmentInCondition(tok);
+        }
+    }
+}
+
+void CheckCondition::assignmentInCondition(const Token *eq)
+{
+    reportError(
+        eq,
+        Severity::style,
+        "assignmentInCondition",
+        "Assignment in condition should probably be comparison.",
+        CWE571,
+        Certainty::normal);
+}
+
diff --git a/lib/checkcondition.h b/lib/checkcondition.h
@@ -70,6 +70,7 @@ class CPPCHECKLIB CheckCondition : public Check {
         checkCondition.checkBadBitmaskCheck();
         checkCondition.comparison();
         checkCondition.checkModuloAlwaysTrueFalse();
+        checkCondition.checkAssignmentInCondition();
     }
 
     /** mismatching assignment / comparison */
@@ -122,6 +123,9 @@ class CPPCHECKLIB CheckCondition : public Check {
 
     void checkDuplicateConditionalAssign();
 
+    /** @brief Assignment in condition */
+    void checkAssignmentInCondition();
+
 private:
     // The conditions that have been diagnosed
     std::set<const Token*> mCondDiags;
@@ -161,6 +165,8 @@ class CPPCHECKLIB CheckCondition : public Check {
 
     void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok);
 
+    void assignmentInCondition(const Token *eq);
+
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
         CheckCondition c(nullptr, settings, errorLogger);
 
@@ -183,6 +189,7 @@ class CPPCHECKLIB CheckCondition : public Check {
         c.invalidTestForOverflow(nullptr, nullptr, "false");
         c.pointerAdditionResultNotNullError(nullptr, nullptr);
         c.duplicateConditionalAssignError(nullptr, nullptr);
+        c.assignmentInCondition(nullptr);
     }
 
     static std::string myName() {
@@ -203,7 +210,8 @@ class CPPCHECKLIB CheckCondition : public Check {
                "- Mutual exclusion over || always evaluating to true\n"
                "- Comparisons of modulo results that are always true/false.\n"
                "- Known variable values => condition is always true/false\n"
-               "- Invalid test for overflow. Some mainstream compilers remove such overflow tests when optimising code.\n";
+               "- Invalid test for overflow. Some mainstream compilers remove such overflow tests when optimising code.\n"
+               "- Assignment of container/iterator in condition should probably be comparison.\n";
     }
 };
 /// @}
diff --git a/test/testcondition.cpp b/test/testcondition.cpp
@@ -120,6 +120,8 @@ class TestCondition : public TestFixture {
         TEST_CASE(alwaysTrueFalseInLogicalOperators);
         TEST_CASE(pointerAdditionResultNotNull);
         TEST_CASE(duplicateConditionalAssign);
+
+        TEST_CASE(checkAssignmentInCondition);
     }
 
     void check(const char code[], const char* filename = "test.cpp", bool inconclusive = false) {
@@ -4153,6 +4155,13 @@ class TestCondition : public TestFixture {
               "}");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void checkAssignmentInCondition() {
+        check("void f(std::string s) {\n"
+              "    if (s=\"123\"){}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) Assignment in condition should probably be comparison.\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestCondition)
