Fix #14471: Improve check: shadowVariable for function argument (#8303)

Author: ludviggunne

.selfcheck_suppressions

@@ -78,3 +78,5 @@ knownConditionTrueFalse:externals/tinyxml2/tinyxml2.cpp
 useStlAlgorithm:externals/simplecpp/simplecpp.cpp
 funcArgNamesDifferentUnnamed:externals/simplecpp/simplecpp.h
 missingMemberCopy:externals/simplecpp/simplecpp.h
+shadowFunction:externals/simplecpp/simplecpp.cpp
+shadowFunction:externals/simplecpp/simplecpp.h

gui/mainwindow.cpp

@@ -562,7 +562,7 @@ void MainWindow::saveSettings() const
     mUI->mResults->saveSettings(mSettings);
 }
 
-void MainWindow::doAnalyzeProject(ImportProject p, const bool checkLibrary, const bool checkConfiguration)
+void MainWindow::doAnalyzeProject(ImportProject p, const bool checkLib, const bool checkConfig)
 {
     Settings checkSettings;
     auto supprs = std::make_shared<Suppressions>();
@@ -606,8 +606,8 @@ void MainWindow::doAnalyzeProject(ImportProject p, const bool checkLibrary, cons
 
     mUI->mResults->setCheckDirectory(checkPath);
     checkSettings.force = false;
-    checkSettings.checkLibrary = checkLibrary;
-    checkSettings.checkConfiguration = checkConfiguration;
+    checkSettings.checkLibrary = checkLib;
+    checkSettings.checkConfiguration = checkConfig;
 
     if (mProjectFile)
         qDebug() << "Checking project file" << mProjectFile->getFilename();
@@ -634,7 +634,7 @@ void MainWindow::doAnalyzeProject(ImportProject p, const bool checkLibrary, cons
     mUI->mResults->setCheckSettings(checkSettings);
 }
 
-void MainWindow::doAnalyzeFiles(const QStringList &files, const bool checkLibrary, const bool checkConfiguration)
+void MainWindow::doAnalyzeFiles(const QStringList &files, const bool checkLib, const bool checkConfig)
 {
     if (files.isEmpty())
         return;
@@ -675,7 +675,7 @@ void MainWindow::doAnalyzeFiles(const QStringList &files, const bool checkLibrar
     // TODO: lock UI here?
     mUI->mResults->checkingStarted(fdetails.size());
     mThread->setFiles(std::move(fdetails));
-    if (mProjectFile && !checkConfiguration)
+    if (mProjectFile && !checkConfig)
         mThread->setAddonsAndTools(mProjectFile->getAddonsAndTools());
     mThread->setSuppressions(mProjectFile ? mProjectFile->getCheckingSuppressions() : QList<SuppressionList::Suppression>());
     QDir inf(mCurrentDirectory);
@@ -685,8 +685,8 @@ void MainWindow::doAnalyzeFiles(const QStringList &files, const bool checkLibrar
     checkLockDownUI(); // lock UI while checking
 
     mUI->mResults->setCheckDirectory(checkPath);
-    checkSettings.checkLibrary = checkLibrary;
-    checkSettings.checkConfiguration = checkConfiguration;
+    checkSettings.checkLibrary = checkLib;
+    checkSettings.checkConfiguration = checkConfig;
 
     if (mProjectFile)
         qDebug() << "Checking project file" << mProjectFile->getFilename();
@@ -1856,7 +1856,7 @@ bool MainWindow::loadLastResults()
     return true;
 }
 
-void MainWindow::analyzeProject(const ProjectFile *projectFile, const QStringList& recheckFiles, const bool checkLibrary, const bool checkConfiguration)
+void MainWindow::analyzeProject(const ProjectFile *projectFile, const QStringList& recheckFiles, const bool checkLib, const bool checkConfig)
 {
     Settings::terminate(false);
 
@@ -1961,7 +1961,7 @@ void MainWindow::analyzeProject(const ProjectFile *projectFile, const QStringLis
             msg.exec();
             return;
         }
-        doAnalyzeProject(p, checkLibrary, checkConfiguration);  // TODO: avoid copy
+        doAnalyzeProject(p, checkLib, checkConfig);  // TODO: avoid copy
         return;
     }
 
@@ -1974,7 +1974,7 @@ void MainWindow::analyzeProject(const ProjectFile *projectFile, const QStringLis
     if (paths.isEmpty()) {
         paths << mCurrentDirectory;
     }
-    doAnalyzeFiles(paths, checkLibrary, checkConfiguration);
+    doAnalyzeFiles(paths, checkLib, checkConfig);
 }
 
 void MainWindow::newProjectFile()

gui/mainwindow.h

@@ -267,10 +267,10 @@ private slots:
      * @brief Analyze the project.
      * @param projectFile Pointer to the project to analyze.
      * @param recheckFiles files to recheck, empty => check all files
-     * @param checkLibrary Flag to indicate if the library should be checked.
-     * @param checkConfiguration Flag to indicate if the configuration should be checked.
+     * @param checkLib Flag to indicate if the library should be checked.
+     * @param checkConfig Flag to indicate if the configuration should be checked.
      */
-    void analyzeProject(const ProjectFile *projectFile, const QStringList& recheckFiles, bool checkLibrary = false, bool checkConfiguration = false);
+    void analyzeProject(const ProjectFile *projectFile, const QStringList& recheckFiles, bool checkLib = false, bool checkConfig = false);
 
     /**
      * @brief Set current language
@@ -306,19 +306,19 @@ private slots:
     /**
      * @brief Analyze project
      * @param p imported project
-     * @param checkLibrary Flag to indicate if library should be checked
-     * @param checkConfiguration Flag to indicate if the configuration should be checked.
+     * @param checkLib Flag to indicate if library should be checked
+     * @param checkConfig Flag to indicate if the configuration should be checked.
      */
-    void doAnalyzeProject(ImportProject p, bool checkLibrary = false, bool checkConfiguration = false);
+    void doAnalyzeProject(ImportProject p, bool checkLib = false, bool checkConfig = false);
 
     /**
      * @brief Analyze all files specified in parameter files
      *
      * @param files List of files and/or directories to analyze
-     * @param checkLibrary Flag to indicate if library should be checked
-     * @param checkConfiguration Flag to indicate if the configuration should be checked.
+     * @param checkLib Flag to indicate if library should be checked
+     * @param checkConfig Flag to indicate if the configuration should be checked.
      */
-    void doAnalyzeFiles(const QStringList &files, bool checkLibrary = false, bool checkConfiguration = false);
+    void doAnalyzeFiles(const QStringList &files, bool checkLib = false, bool checkConfig = false);
 
     /**
      * @brief Get our default cppcheck settings and read project file.

lib/checkcondition.cpp

@@ -2085,10 +2085,10 @@ void CheckCondition::checkCompareValueOutOfTypeRange()
     }
 }
 
-void CheckCondition::compareValueOutOfTypeRangeError(const Token *comparison, const std::string &type, MathLib::bigint value, bool result)
+void CheckCondition::compareValueOutOfTypeRangeError(const Token *comparisonTok, const std::string &type, MathLib::bigint value, bool result)
 {
     reportError(
-        comparison,
+        comparisonTok,
         Severity::style,
         "compareValueOutOfTypeRangeError",
         "Comparing expression of type '" + type + "' against value " + MathLib::toString(value) + ". Condition is always " + bool_to_string(result) + ".",

lib/checkcondition.h

@@ -156,7 +156,7 @@ class CPPCHECKLIB CheckCondition : public Check {
     void assignmentInCondition(const Token *eq);
 
     void checkCompareValueOutOfTypeRange();
-    void compareValueOutOfTypeRangeError(const Token *comparison, const std::string &type, MathLib::bigint value, bool result);
+    void compareValueOutOfTypeRangeError(const Token *comparisonTok, const std::string &type, MathLib::bigint value, bool result);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override;
 

lib/checkother.cpp

@@ -4168,46 +4168,65 @@ void CheckOther::checkShadowVariables()
         const Scope *functionScope = &scope;
         while (functionScope && functionScope->type != ScopeType::eFunction && functionScope->type != ScopeType::eLambda)
             functionScope = functionScope->nestedIn;
-        for (const Variable &var : scope.varlist) {
-            if (var.nameToken() && var.nameToken()->isExpandedMacro()) // #8903
-                continue;
+        const auto checkVar = [&](const Variable &var) {
+            if (!var.nameToken())
+                return;
+
+            if (var.nameToken()->isExpandedMacro()) // #8903
+                return;
 
-            if (functionScope && functionScope->type == ScopeType::eFunction && functionScope->function) {
+            if (!var.isArgument() && functionScope && functionScope->type == ScopeType::eFunction && functionScope->function) {
                 const auto & argList = functionScope->function->argumentList;
                 auto it = std::find_if(argList.cbegin(), argList.cend(), [&](const Variable& arg) {
                     return arg.nameToken() && var.name() == arg.name();
                 });
                 if (it != argList.end()) {
-                    shadowError(var.nameToken(), it->nameToken(), "argument");
-                    continue;
+                    shadowError(var.nameToken(), "local variable", it->nameToken(), "argument");
+                    return;
                 }
             }
 
             const Token *shadowed = findShadowed(scope.nestedIn, var, var.nameToken()->linenr());
             if (!shadowed)
                 shadowed = findShadowed(scope.functionOf, var, var.nameToken()->linenr());
             if (!shadowed)
-                continue;
+                return;
             if (scope.type == ScopeType::eFunction && scope.className == var.name())
-                continue;
+                return;
             if (functionScope->functionOf && functionScope->functionOf->isClassOrStructOrUnion() && functionScope->function &&
                 (functionScope->function->isStatic() || functionScope->function->isFriend()) &&
                 shadowed->variable() && !shadowed->variable()->isLocal())
-                continue;
-            shadowError(var.nameToken(), shadowed, (shadowed->varId() != 0) ? "variable" : "function");
-        }
+                return;
+            if (var.scope() && var.scope()->function && var.scope()->function->isConstructor()) {
+                if (shadowed->variable() && shadowed->variable()->isMember())
+                    return;
+                if (shadowed->function() && shadowed->function()->nestedIn &&
+                    shadowed->function()->nestedIn->isClassOrStruct())
+                    return;
+            }
+            shadowError(var.nameToken(), var.isArgument() ? "argument" : "local variable",
+                        shadowed, (shadowed->varId() != 0) ?
+                        (shadowed->variable()->isMember() ? "member" : "variable") : "function");
+        };
+        for (const Variable &var : scope.varlist)
+            checkVar(var);
+        if (functionScope && functionScope->type == ScopeType::eFunction && functionScope->function)
+            for (const Variable &arg: functionScope->function->argumentList)
+                checkVar(arg);
     }
 }
 
-void CheckOther::shadowError(const Token *var, const Token *shadowed, const std::string& type)
+void CheckOther::shadowError(const Token *shadows, const std::string &shadowsType,
+                             const Token *shadowed, const std::string &shadowedType)
 {
     ErrorPath errorPath;
-    errorPath.emplace_back(shadowed, "Shadowed declaration");
-    errorPath.emplace_back(var, "Shadow variable");
-    const std::string &varname = var ? var->str() : type;
-    const std::string Type = char(std::toupper(type[0])) + type.substr(1);
-    const std::string id = "shadow" + Type;
-    const std::string message = "$symbol:" + varname + "\nLocal variable \'$symbol\' shadows outer " + type;
+    errorPath.emplace_back(shadowed, "Shadowed " + shadowedType);
+    errorPath.emplace_back(shadows, "Shadow " + shadowsType);
+    const std::string &varname = shadows ? shadows->str() : shadowsType;
+    const std::string ShadowsType = char(std::toupper(shadowsType[0])) + shadowsType.substr(1);
+    const std::string ShadowedType = char(std::toupper(shadowedType[0])) + shadowedType.substr(1);
+    const std::string id = "shadow" + ShadowedType;
+    const std::string message = "$symbol:" + varname + "\n" + ShadowsType + " \'$symbol\' shadows outer " + shadowedType;
     reportError(std::move(errorPath), Severity::style, id.c_str(), message, CWE398, Certainty::normal);
 }
 
@@ -4889,9 +4908,10 @@ void CheckOther::getErrorMessages(ErrorLogger *errorLogger, const Settings *sett
     c.accessMovedError(nullptr, "v", nullptr, false);
     c.funcArgNamesDifferent("function", 1, nullptr, nullptr);
     c.redundantBitwiseOperationInSwitchError(nullptr, "varname");
-    c.shadowError(nullptr, nullptr, "variable");
-    c.shadowError(nullptr, nullptr, "function");
-    c.shadowError(nullptr, nullptr, "argument");
+    c.shadowError(nullptr, "local variable", nullptr, "variable");
+    c.shadowError(nullptr, "local variable", nullptr, "argument");
+    c.shadowError(nullptr, "local variable", nullptr, "function");
+    c.shadowError(nullptr, "local variable", nullptr, "member");
     c.knownArgumentError(nullptr, nullptr, nullptr, "x", false);
     c.knownPointerToBoolError(nullptr, nullptr);
     c.comparePointersError(nullptr, nullptr, nullptr);

lib/checkother.h

@@ -258,7 +258,7 @@ class CPPCHECKLIB CheckOther : public Check {
     void accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive);
     void funcArgNamesDifferent(const std::string & functionName, nonneg int 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 shadowError(const Token *var, const Token *shadowed, const std::string& type);
+    void shadowError(const Token *shadows, const std::string &shadowsType, const Token *shadowed, const std::string &shadowedType);
     void knownArgumentError(const Token *tok, const Token *ftok, const ValueFlow::Value *value, const std::string &varexpr, bool isVariableExpressionHidden);
     void knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value);
     void comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2);

lib/checkunusedfunctions.cpp

@@ -505,9 +505,9 @@ void CheckUnusedFunctions::analyseWholeProgram(const Settings &settings, ErrorLo
     }
 }
 
-void CheckUnusedFunctions::updateFunctionData(const CheckUnusedFunctions& check)
+void CheckUnusedFunctions::updateFunctionData(const CheckUnusedFunctions& checkUnusedFunctions)
 {
-    for (const auto& entry : check.mFunctions)
+    for (const auto& entry : checkUnusedFunctions.mFunctions)
     {
         FunctionUsage &usage = mFunctions[entry.first];
         if (!usage.lineNumber) {
@@ -521,6 +521,6 @@ void CheckUnusedFunctions::updateFunctionData(const CheckUnusedFunctions& check)
         usage.usedOtherFile |= entry.second.usedOtherFile;
         usage.usedSameFile |= entry.second.usedSameFile;
     }
-    mFunctionDecl.insert(mFunctionDecl.cend(), check.mFunctionDecl.cbegin(), check.mFunctionDecl.cend());
-    mFunctionCalls.insert(check.mFunctionCalls.cbegin(), check.mFunctionCalls.cend());
+    mFunctionDecl.insert(mFunctionDecl.cend(), checkUnusedFunctions.mFunctionDecl.cbegin(), checkUnusedFunctions.mFunctionDecl.cend());
+    mFunctionCalls.insert(checkUnusedFunctions.mFunctionCalls.cbegin(), checkUnusedFunctions.mFunctionCalls.cend());
 }

lib/checkunusedfunctions.h

@@ -58,7 +58,7 @@ class CPPCHECKLIB CheckUnusedFunctions {
     // Return true if an error is reported.
     bool check(const Settings& settings, ErrorLogger& errorLogger) const;
 
-    void updateFunctionData(const CheckUnusedFunctions& check);
+    void updateFunctionData(const CheckUnusedFunctions& checkUnusedFunctions);
 
 private:
     static void unusedFunctionError(ErrorLogger& errorLogger,

lib/filesettings.h

@@ -91,9 +91,9 @@ class FileWithDetails
         return mFsFileId;
     }
 
-    void setFsFileId(std::size_t fsFileId)
+    void setFsFileId(std::size_t id)
     {
-        mFsFileId = fsFileId;
+        mFsFileId = id;
     }
 private:
     std::string mPath;