Fix FP with duplicate assignments by checking if the expression is unique (#1223)

* Fix FP with duplicate assignments by checking if the expression is unique

* Use array of pointers

* Reorder scope condition

lib/astutils.cpp

@@ -28,6 +28,7 @@
 #include "valueflow.h"
 
 #include <list>
+#include <functional>
 
 static bool astIsCharWithSign(const Token *tok, ValueType::Sign sign)
 {
@@ -423,6 +424,61 @@ bool isWithoutSideEffects(bool cpp, const Token* tok)
     return true;
 }
 
+bool isUniqueExpression(const Token* tok)
+{
+    if(!tok)
+        return true;
+    if(tok->function()) {
+        const Function * fun = tok->function();
+        const Scope * scope = fun->nestedIn;
+        if(!scope)
+            return true;
+        for(const Function& f:scope->functionList) {
+            if(f.argumentList.size() == fun->argumentList.size() && f.name() != fun->name()) {
+                return false;
+            }
+        }
+    } else if(tok->variable()) {
+        const Variable * var = tok->variable();
+        const Scope * scope = var->scope();
+        if(!scope)
+            return true;
+        const Type * varType = var->type();
+        // Iterate over the variables in scope and the parameters of the function if possible
+        const Function * fun = scope->function;
+        const std::list<Variable>* setOfVars[] = {&scope->varlist, fun ? &fun->argumentList : nullptr};
+        if (varType) {
+            for(const std::list<Variable>* vars:setOfVars) {
+                if(!vars)
+                    continue;
+                for(const Variable& v:*vars) {
+                    if (v.type() && v.type()->name() == varType->name() && v.name() != var->name()) {
+                        return false;
+                    }
+                }
+            }
+        } else {
+            for(const std::list<Variable>* vars:setOfVars) {
+                if(!vars)
+                    continue;
+                for(const Variable& v:*vars) {
+                    if (v.isFloatingType() == var->isFloatingType() &&
+                        v.isEnumType() == var->isEnumType() &&
+                        v.isClass() == var->isClass() &&
+                        v.isArray() == var->isArray() &&
+                        v.isPointer() == var->isPointer() &&
+                        v.name() != var->name())
+                        return false;
+                }
+            }
+        }
+    } else if(!isUniqueExpression(tok->astOperand1())) {
+        return false;
+    }
+
+    return isUniqueExpression(tok->astOperand2());
+}
+
 bool isReturnScope(const Token * const endToken)
 {
     if (!endToken || endToken->str() != "}")

lib/astutils.h

@@ -77,6 +77,8 @@ bool isConstExpression(const Token *tok, const Library& library, bool pure);
 
 bool isWithoutSideEffects(bool cpp, const Token* tok);
 
+bool isUniqueExpression(const Token* tok);
+
 /** Is scope a return scope (scope will unconditionally return) */
 bool isReturnScope(const Token *endToken);
 

lib/checkother.cpp

@@ -1947,7 +1947,8 @@ void CheckOther::checkDuplicateExpression()
                         tok->next()->tokType() != Token::eType &&
                         tok->next()->tokType() != Token::eName &&
                         isSameExpression(_tokenizer->isCPP(), true, tok->next(), nextAssign->next(), _settings->library, true) &&
-                        isSameExpression(_tokenizer->isCPP(), true, tok->astOperand2(), nextAssign->astOperand2(), _settings->library, false)) {
+                        isSameExpression(_tokenizer->isCPP(), true, tok->astOperand2(), nextAssign->astOperand2(), _settings->library, false) &&
+                        !isUniqueExpression(tok->astOperand2())) {
                         duplicateAssignExpressionError(var1, var2);
                     }
                 }

test/testother.cpp

@@ -133,6 +133,7 @@ private:
         TEST_CASE(duplicateExpressionTemplate); // #6930
         TEST_CASE(oppositeExpression);
         TEST_CASE(duplicateVarExpression);
+        TEST_CASE(duplicateVarExpressionUnique);
 
         TEST_CASE(checkSignOfUnsignedVariable);
         TEST_CASE(checkSignOfPointer);
@@ -3971,7 +3972,7 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str());
 
-        check("struct Foo { int f() const; };\n"
+        check("struct Foo { int f() const; int g() const; };\n"
               "void test() {\n"
               "    Foo f = Foo{};\n"
               "    int i = f.f();\n"
@@ -3979,6 +3980,15 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str());
 
+        check("struct Foo { int f() const; int g() const; };\n"
+              "void test() {\n"
+              "    Foo f = Foo{};\n"
+              "    Foo f2 = Foo{};\n"
+              "    int i = f.f();\n"
+              "    int j = f.f();\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:5]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str());
+
         check("int f() __attribute__((pure));\n"
               "void test() {\n"
               "    int i = 1 + f();\n"
@@ -3994,19 +4004,20 @@ private:
         ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str());
 
         check("int f(int) __attribute__((pure));\n"
+              "int g(int) __attribute__((pure));\n"
               "void test() {\n"
               "    int i = f(0);\n"
               "    int j = f(0);\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str());
 
-        check("void test(int * p) {\n"
+        check("void test(int * p, int * q) {\n"
               "    int i = *p;\n"
               "    int j = *p;\n"
               "}");
         ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str());
 
-        check("struct A { int x; };"
+        check("struct A { int x; int y; };"
               "void test(A a) {\n"
               "    int i = a.x;\n"
               "    int j = a.x;\n"
@@ -4084,6 +4095,50 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void duplicateVarExpressionUnique() {
+        check("struct SW { int first; };\n"
+              "void foo(SW* x) {\n"
+              "    int start = x->first;\n"
+              "    int end   = x->first;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+        check("struct SW { int first; };\n"
+              "void foo(SW* x, int i, int j) {\n"
+              "    int start = x->first;\n"
+              "    int end   = x->first;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+        check("struct Foo { int f() const; };\n"
+              "void test() {\n"
+              "    Foo f = Foo{};\n"
+              "    int i = f.f();\n"
+              "    int j = f.f();\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void test(int * p) {\n"
+              "    int i = *p;\n"
+              "    int j = *p;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct Foo { int f() const; int g(int) const; };\n"
+              "void test() {\n"
+              "    Foo f = Foo{};\n"
+              "    int i = f.f();\n"
+              "    int j = f.f();\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct Foo { int f() const; };\n"
+              "void test() {\n"
+              "    Foo f = Foo{};\n"
+              "    int i = f.f();\n"
+              "    int j = f.f();\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void checkSignOfUnsignedVariable() {
         check(
             "void foo() {\n"