Fix #11642 FN redundantAssignment with mutual assignment (#6536)

danmar · Jun 28, 2024 · a8ee10a · a8ee10a
1 parent 16ebe0b
commit a8ee10a
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 4 deletions.
diff --git a/lib/checkother.cpp b/lib/checkother.cpp
@@ -519,6 +519,10 @@ void CheckOther::checkRedundantAssignment()
                         tokenToCheck = tempToken;
                 }
 
+                if (start->hasKnownSymbolicValue(tokenToCheck) && Token::simpleMatch(start->astParent(), "=") && !diag(tok)) {
+                    redundantAssignmentSameValueError(start, tokenToCheck, tok->astOperand1()->expressionString());
+                }
+
                 // Get next assignment..
                 const Token *nextAssign = fwdAnalysis.reassign(tokenToCheck, start, scope->bodyEnd);
 
@@ -541,8 +545,10 @@ void CheckOther::checkRedundantAssignment()
                     redundantAssignmentInSwitchError(tok, nextAssign, tok->astOperand1()->expressionString());
                 else if (isInitialization)
                     redundantInitializationError(tok, nextAssign, tok->astOperand1()->expressionString(), inconclusive);
-                else
+                else {
+                    diag(nextAssign);
                     redundantAssignmentError(tok, nextAssign, tok->astOperand1()->expressionString(), inconclusive);
+                }
             }
         }
     }
@@ -587,6 +593,16 @@ void CheckOther::redundantAssignmentInSwitchError(const Token *tok1, const Token
                 "Variable '$symbol' is reassigned a value before the old one has been used. 'break;' missing?", CWE563, Certainty::normal);
 }
 
+void CheckOther::redundantAssignmentSameValueError(const Token *tok1, const Token* tok2, const std::string &var)
+{
+    const ValueFlow::Value* val = tok1->getKnownValue(ValueFlow::Value::ValueType::SYMBOLIC);
+    auto errorPath = val->errorPath;
+    errorPath.emplace_back(tok2, "");
+    reportError(errorPath, Severity::style, "redundantAssignment",
+                "$symbol:" + var + "\n"
+                "Variable '$symbol' is assigned an expression that holds the same value.", CWE563, Certainty::normal);
+}
+
 
 //---------------------------------------------------------------------------
 //    switch (x)
@@ -2485,7 +2501,8 @@ void CheckOther::checkDuplicateExpression()
                         tok->astOperand2()->expressionString() == nextAssign->astOperand2()->expressionString()) {
                         bool differentDomain = false;
                         const Scope * varScope = var1->scope() ? var1->scope() : scope;
-                        for (const Token *assignTok = Token::findsimplematch(var2, ";"); assignTok && assignTok != varScope->bodyEnd; assignTok = assignTok->next()) {
+                        const Token* assignTok = Token::findsimplematch(var2, ";");
+                        for (; assignTok && assignTok != varScope->bodyEnd; assignTok = assignTok->next()) {
                             if (!Token::Match(assignTok, "%assign%|%comp%"))
                                 continue;
                             if (!assignTok->astOperand1())
@@ -2516,8 +2533,10 @@ void CheckOther::checkDuplicateExpression()
                         }
                         if (!differentDomain && !isUniqueExpression(tok->astOperand2()))
                             duplicateAssignExpressionError(var1, var2, false);
-                        else if (mSettings->certainty.isEnabled(Certainty::inconclusive))
+                        else if (mSettings->certainty.isEnabled(Certainty::inconclusive)) {
+                            diag(assignTok);
                             duplicateAssignExpressionError(var1, var2, true);
+                        }
                     }
                 }
             }

diff --git a/lib/checkother.h b/lib/checkother.h
@@ -27,6 +27,7 @@
 #include "errortypes.h"
 #include "tokenize.h"
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -75,11 +76,11 @@ class CPPCHECKLIB CheckOther : public Check {
         checkOther.warningOldStylePointerCast();
         checkOther.invalidPointerCast();
         checkOther.checkCharVariable();
-        checkOther.checkRedundantAssignment();
         checkOther.redundantBitwiseOperationInSwitchError();
         checkOther.checkSuspiciousCaseInSwitch();
         checkOther.checkDuplicateBranch();
         checkOther.checkDuplicateExpression();
+        checkOther.checkRedundantAssignment();
         checkOther.checkUnreachableCode();
         checkOther.checkSuspiciousSemicolon();
         checkOther.checkVariableScope();
@@ -253,6 +254,7 @@ class CPPCHECKLIB CheckOther : public Check {
     void redundantAssignmentError(const Token *tok1, const Token* tok2, const std::string& var, bool inconclusive);
     void redundantInitializationError(const Token *tok1, const Token* tok2, const std::string& var, bool inconclusive);
     void redundantAssignmentInSwitchError(const Token *tok1, const Token *tok2, const std::string &var);
+    void redundantAssignmentSameValueError(const Token *tok1, const Token *tok2, const std::string &var);
     void redundantCopyError(const Token *tok1, const Token* tok2, const std::string& var);
     void redundantBitwiseOperationInSwitchError(const Token *tok, const std::string &varname);
     void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString);
@@ -426,6 +428,11 @@ class CPPCHECKLIB CheckOther : public Check {
                "- calculating modulo of one.\n"
                "- known function argument, suspicious calculation.\n";
     }
+
+    bool diag(const Token* tok) {
+        return !mRedundantAssignmentDiag.emplace(tok).second;
+    }
+    std::set<const Token*> mRedundantAssignmentDiag;
 };
 /// @}
 //---------------------------------------------------------------------------

diff --git a/test/testother.cpp b/test/testother.cpp
@@ -215,6 +215,7 @@ class TestOther : public TestFixture {
         TEST_CASE(redundantVarAssignment_switch_break);
         TEST_CASE(redundantInitialization);
         TEST_CASE(redundantMemWrite);
+        TEST_CASE(redundantAssignmentSameValue);
 
         TEST_CASE(varFuncNullUB);
 
@@ -10189,6 +10190,35 @@ class TestOther : public TestFixture {
         TODO_ASSERT_EQUALS("error", "", errout_str());
     }
 
+    void redundantAssignmentSameValue() {
+        check("int main() {\n" // #11642
+              "    int a = 0;\n"
+              "    int b = a;\n"
+              "    int c = 1;\n"
+              "    a = b;\n"
+              "    return a * b * c;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (style) Variable 'a' is assigned an expression that holds the same value.\n", errout_str());
+
+        check("int main() {\n"
+              "    int a = 0;\n"
+              "    int b = a;\n"
+              "    int c = 1;\n"
+              "    a = b + 1;\n"
+              "    return a * b * c;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        check("int main() {\n"
+              "    int a = 0;\n"
+              "    int b = a;\n"
+              "    int c = 1;\n"
+              "    a = b = 5;\n"
+              "    return a * b * c;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (style) Redundant initialization for 'b'. The initialized value is overwritten before it is read.\n", errout_str());
+    }
+
     void varFuncNullUB() { // #4482
         check("void a(...);\n"
               "void b() { a(NULL); }");