Add check for shadow variables

.travis.yml

@@ -45,9 +45,11 @@ script:
 # fail the entire job as soon as one of the subcommands exits non-zero to save time and resources
   - set -e
 # download clang git, compile cppcheck, run cppcheck on clang code to look for crashes in cppcheck. if this is done, terminate build
-  - if [[ "$CHECK_CLANG" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/clang/archive/a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip" & make -j 4 & wait; unzip a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip > /dev/null; touch /tmp/clang.cppcheck; cd ./clang-a1f8bd3778cc5a53236a53500c6ab184e945eefa ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" -iINPUTS -itest/Driver/Inputs/gen-response.c -itest/Index/index-many-logical-ops.c -itest/Sema/many-logical-ops.c --suppressions-list=../.travis_llvmcheck_suppressions -j 2 |& grep -v ".* files checked.*done" |& tee /tmp/clang.cppcheck; cd ../ ; echo "CLANG" ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/clang.cppcheck; exit; fi
+# DM: I can't make it work. Let's rely on daca@home instead.
+#  - if [[ "$CHECK_CLANG" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/clang/archive/a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip" & make -j 4 & wait; unzip a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip > /dev/null; touch /tmp/clang.cppcheck; cd ./clang-a1f8bd3778cc5a53236a53500c6ab184e945eefa ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" -q -iINPUTS -itest/Driver/Inputs/gen-response.c -itest/Index/index-many-logical-ops.c -itest/Sema/many-logical-ops.c -j >& /tmp/clang.cppcheck ; cd ../ ; echo "CLANG" ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/clang.cppcheck; exit; fi
 # check llvm as well
-  - if [[  "$CHECK_LLVM" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/llvm/archive/6fa6453210fa34c1c28bd73a431d04734549f0d6.zip"  & make -j 4 & wait; unzip 6fa6453210fa34c1c28bd73a431d04734549f0d6.zip > /dev/null;  touch /tmp/llvm.cppcheck;  cd ./llvm-6fa6453210fa34c1c28bd73a431d04734549f0d6 ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" --suppressions-list=../.travis_llvmcheck_suppressions -j 2 |& grep -v ".* files checked.*done" |& tee /tmp/llvm.cppcheck; cd ../ ; echo "LLVM"  ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/llvm.cppcheck; exit; fi
+# DM: I can't make it work. Let's rely on daca@home instead.
+#  - if [[  "$CHECK_LLVM" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/llvm/archive/6fa6453210fa34c1c28bd73a431d04734549f0d6.zip"  & make -j 4 & wait; unzip 6fa6453210fa34c1c28bd73a431d04734549f0d6.zip > /dev/null;  touch /tmp/llvm.cppcheck;  cd ./llvm-6fa6453210fa34c1c28bd73a431d04734549f0d6 ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" --suppressions-list=../.travis_llvmcheck_suppressions -j 2 -q >& /tmp/llvm.cppcheck; cd ../ ; echo "LLVM"  ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/llvm.cppcheck; exit; fi
 # check if dmake needs to be rerun but if yes, don't fail the build but notify us.
 # to update dmake: "make dmake; ./dmake; and commit
   - echo "If the following command fails, run 'make dmake; make run-dmake' and commit the resulting change."

.travis_suppressions

@@ -6,6 +6,7 @@ nullPointer:build/checkother.cpp
 missingOverride
 noValidConfiguration
 useStlAlgorithm
+shadowLocal
 
 *:gui/test/*
 *:test/test.cxx

lib/checkother.cpp

@@ -2937,3 +2937,47 @@ void CheckOther::funcArgOrderDifferent(const std::string & functionName,
     reportError(tokens, Severity::warning, "funcArgOrderDifferent", msg, CWE683, false);
 }
 
+static const Token *findShadowed(const Scope *scope, const std::string &varname, int linenr)
+{
+    if (!scope)
+        return nullptr;
+    for (const Variable &var : scope->varlist) {
+        if (scope->isExecutable() && var.nameToken()->linenr() > linenr)
+            continue;
+        if (var.name() == varname)
+            return var.nameToken();
+    }
+    for (const Function &f : scope->functionList) {
+        if (f.name() == varname)
+            return f.tokenDef;
+    }
+    return findShadowed(scope->nestedIn, varname, linenr);
+}
+
+void CheckOther::checkShadowVariables()
+{
+    if (!mSettings->isEnabled(Settings::STYLE))
+        return;
+    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
+    for (const Scope & scope : symbolDatabase->scopeList) {
+        if (!scope.isExecutable())
+            continue;
+        for (const Variable &var : scope.varlist) {
+            const Token *shadowed = findShadowed(scope.nestedIn, var.name(), var.nameToken()->linenr());
+            if (!shadowed)
+                continue;
+            if (scope.type == Scope::eFunction && scope.className == var.name())
+                continue;
+            shadowVariablesError(var.nameToken(), shadowed);
+        }
+    }
+}
+
+void CheckOther::shadowVariablesError(const Token *var, const Token *shadowed)
+{
+    ErrorPath errorPath;
+    errorPath.push_back(ErrorPathItem(shadowed, "Shadowed declaration"));
+    errorPath.push_back(ErrorPathItem(var, "Shadow variable"));
+    const std::string &varname = var ? var->str() : "var";
+    reportError(errorPath, Severity::style, "shadowLocal", "$symbol:" + varname + "\nLocal variable $symbol shadows outer symbol", CWE398, false);
+}

lib/checkother.h

@@ -81,6 +81,7 @@ public:
         checkOther.checkUnusedLabel();
         checkOther.checkEvaluationOrder();
         checkOther.checkFuncArgNamesDifferent();
+        checkOther.checkShadowVariables();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -211,6 +212,9 @@ public:
     /** @brief %Check if function declaration and definition argument names different */
     void checkFuncArgNamesDifferent();
 
+    /** @brief %Check for shadow variables. Less noisy than gcc/clang -Wshadow. */
+    void checkShadowVariables();
+
 private:
     // Error messages..
     void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result);
@@ -263,6 +267,7 @@ private:
     void accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive);
     void funcArgNamesDifferent(const std::string & functionName, size_t index, const Token* declaration, const Token* definition);
     void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector<const Token*> & declarations, const std::vector<const Token*> & definitions);
+    void shadowVariablesError(const Token *var, const Token *shadowed);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override {
         CheckOther c(nullptr, settings, errorLogger);
@@ -324,6 +329,7 @@ private:
         c.accessMovedError(nullptr, "v", nullptr, false);
         c.funcArgNamesDifferent("function", 1, nullptr, nullptr);
         c.redundantBitwiseOperationInSwitchError(nullptr, "varname");
+        c.shadowVariablesError(nullptr, nullptr);
 
         const std::vector<const Token *> nullvec;
         c.funcArgOrderDifferent("function", nullptr, nullptr, nullvec, nullvec);
@@ -385,7 +391,8 @@ private:
                "- redundant pointer operation on pointer like &\\*some_ptr.\n"
                "- find unused 'goto' labels.\n"
                "- function declaration and definition argument names different.\n"
-               "- function declaration and definition argument order different.\n";
+               "- function declaration and definition argument order different.\n"
+               "- shadow variable.\n";
     }
 };
 /// @}

test/testother.cpp

@@ -214,6 +214,8 @@ private:
         TEST_CASE(funcArgNamesDifferent);
         TEST_CASE(funcArgOrderDifferent);
         TEST_CASE(cpp11FunctionArgInit); // #7846 - "void foo(int declaration = {}) {"
+
+        TEST_CASE(shadowLocal);
     }
 
     void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) {
@@ -2990,11 +2992,11 @@ private:
 
         // #6406 - designated initializer doing bogus self assignment
         check("struct callbacks {\n"
-              "    void (*something)(void);\n"
+              "    void (*s)(void);\n"
               "};\n"
               "void something(void) {}\n"
               "void f() {\n"
-              "    struct callbacks ops = { .something = ops.something };\n"
+              "    struct callbacks ops = { .s = ops.s };\n"
               "}\n");
         TODO_ASSERT_EQUALS("[test.cpp:6]: (warning) Redundant assignment of 'something' to itself.\n", "", errout.str());
 
@@ -4621,7 +4623,7 @@ private:
 
         check("template <typename T>\n"
               "T f() {\n"
-              "  T a = T();\n"
+              "  T x = T();\n"
               "}\n"
               "int &a = f<int&>();\n");
         ASSERT_EQUALS("", errout.str());
@@ -5340,7 +5342,7 @@ private:
               "    const int a = getA + 3;\n"
               "    return 0;\n"
               "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:4]: (style) Local variable getA shadows outer symbol\n", errout.str());
 
         check("class A{public:A(){}};\n"
               "const A& getA(){static A a;return a;}\n"
@@ -7384,6 +7386,27 @@ private:
                         ));
         ASSERT_EQUALS("", errout.str());
     }
+
+    void shadowLocal() {
+        check("int x;\n"
+              "void f() { int x; }\n");
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Local variable x shadows outer symbol\n", errout.str());
+
+        check("int x();\n"
+              "void f() { int x; }\n");
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Local variable x shadows outer symbol\n", errout.str());
+
+        check("void f() {\n"
+              "  if (cond) {int x;}\n" // <- not a shadow variable
+              "  int x;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("int size() {\n"
+              "  int size;\n" // <- not a shadow variable
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)