Fix #10980 FN constVariable with range-based for loop (#4144)

* Fix #10980 FN constVariable with range-based for loop

* Format

* nullptr check

* Restrict scopes

* Add const

* Undo

* Add more const

lib/astutils.cpp

@@ -915,14 +915,14 @@ static bool hasUnknownVars(const Token* startTok)
     return result;
 }
 
-static bool isStructuredBindingVariable(const Variable* var)
+bool isStructuredBindingVariable(const Variable* var)
 {
     if (!var)
         return false;
     const Token* tok = var->nameToken();
-    while (Token::Match(tok->astParent(), "[|,"))
+    while (tok && Token::Match(tok->astParent(), "[|,|:"))
         tok = tok->astParent();
-    return Token::simpleMatch(tok, "[");
+    return tok && (tok->str() == "[" || Token::simpleMatch(tok->previous(), "] :")); // TODO: remove workaround when #11105 is fixed
 }
 
 /// This takes a token that refers to a variable and it will return the token

lib/astutils.h

@@ -205,6 +205,8 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
 
 bool isEqualKnownValue(const Token * const tok1, const Token * const tok2);
 
+bool isStructuredBindingVariable(const Variable* var);
+
 /**
  * Is token used a boolean, that is to say cast to a bool, or used as a condition in a if/while/for
  */

lib/checkother.cpp

@@ -1407,13 +1407,13 @@ void CheckOther::checkConstVariable()
             continue;
         if (var->isConst())
             continue;
-        if (!var->scope())
+        const Scope* scope = var->scope();
+        if (!scope)
             continue;
-        const Scope *scope = var->scope();
+        const Function* function = scope->function;
-        if (!scope->function)
+        if (!function && !scope->isLocal())
             continue;
-        const Function *function = scope->function;
+        if (function && var->isArgument()) {
-        if (var->isArgument()) {
             if (function->isImplicitlyVirtual() || function->templateDef)
                 continue;
             if (isUnusedVariable(var))
@@ -1433,9 +1433,18 @@ void CheckOther::checkConstVariable()
             continue;
         if (isAliased(var))
             continue;
+        if (isStructuredBindingVariable(var)) // TODO: check all bound variables
+            continue;
         if (isVariableChanged(var, mSettings, mTokenizer->isCPP()))
             continue;
-        if (Function::returnsReference(function) && !Function::returnsConst(function)) {
+        const bool hasFunction = function != nullptr;
+        if (!hasFunction) {
+            const Scope* functionScope = scope;
+            do {
+                functionScope = functionScope->nestedIn;
+            } while (functionScope && !(function = functionScope->function));
+        }
+        if (function && Function::returnsReference(function) && !Function::returnsConst(function)) {
             std::vector<const Token*> returns = Function::findReturns(function);
             if (std::any_of(returns.begin(), returns.end(), [&](const Token* retTok) {
                 if (retTok->varId() == var->declarationId())
@@ -1531,7 +1540,7 @@ void CheckOther::checkConstVariable()
                 continue;
         }
 
-        constVariableError(var, function);
+        constVariableError(var, hasFunction ? function : nullptr);
     }
 }
 

lib/clangimport.cpp

@@ -1519,7 +1519,7 @@ static void setValues(Tokenizer *tokenizer, SymbolDatabase *symbolDatabase)
 {
     const Settings * const settings = tokenizer->getSettings();
 
-    for (Scope &scope: symbolDatabase->scopeList) {
+    for (const Scope& scope : symbolDatabase->scopeList) {
         if (!scope.definedType)
             continue;
 

lib/preprocessor.cpp

@@ -71,7 +71,7 @@ Preprocessor::Preprocessor(Settings& settings, ErrorLogger *errorLogger) : mSett
 
 Preprocessor::~Preprocessor()
 {
-    for (std::pair<const std::string, simplecpp::TokenList *>& tokenList : mTokenLists)
+    for (const std::pair<const std::string, simplecpp::TokenList*>& tokenList : mTokenLists)
         delete tokenList.second;
 }
 
@@ -1043,10 +1043,10 @@ unsigned int Preprocessor::calculateChecksum(const simplecpp::TokenList &tokens1
     return crc32(ostr.str());
 }
 
-void Preprocessor::simplifyPragmaAsm(simplecpp::TokenList *tokenList)
+void Preprocessor::simplifyPragmaAsm(simplecpp::TokenList *tokenList) const
 {
     Preprocessor::simplifyPragmaAsmPrivate(tokenList);
-    for (std::pair<const std::string, simplecpp::TokenList *>& list : mTokenLists) {
+    for (const std::pair<const std::string, simplecpp::TokenList*>& list : mTokenLists) {
         Preprocessor::simplifyPragmaAsmPrivate(list.second);
     }
 }

lib/preprocessor.h

@@ -172,7 +172,7 @@ public:
      */
     unsigned int calculateChecksum(const simplecpp::TokenList &tokens1, const std::string &toolinfo) const;
 
-    void simplifyPragmaAsm(simplecpp::TokenList *tokenList);
+    void simplifyPragmaAsm(simplecpp::TokenList *tokenList) const;
 
 private:
 

lib/symboldatabase.cpp

@@ -863,7 +863,7 @@ void SymbolDatabase::createSymbolDatabaseNeedInitialization()
 {
     if (mTokenizer->isC()) {
         // For C code it is easy, as there are no constructors and no default values
-        for (Scope& scope : scopeList) {
+        for (const Scope& scope : scopeList) {
             if (scope.definedType)
                 scope.definedType->needInitialization = Type::NeedInitialization::True;
         }
@@ -888,7 +888,7 @@ void SymbolDatabase::createSymbolDatabaseNeedInitialization()
                     // check for default constructor
                     bool hasDefaultConstructor = false;
 
-                    for (Function& func: scope.functionList) {
+                    for (const Function& func : scope.functionList) {
                         if (func.type == Function::eConstructor) {
                             // check for no arguments: func ( )
                             if (func.argCount() == 0) {
@@ -1452,7 +1452,7 @@ void SymbolDatabase::createSymbolDatabaseIncompleteVars()
 
 void SymbolDatabase::createSymbolDatabaseEscapeFunctions()
 {
-    for (Scope & scope : scopeList) {
+    for (const Scope& scope : scopeList) {
         if (scope.type != Scope::eFunction)
             continue;
         Function * function = scope.function;

lib/templatesimplifier.cpp

@@ -3374,10 +3374,10 @@ static bool specMatch(
 void TemplateSimplifier::getSpecializations()
 {
     // try to locate a matching declaration for each user defined specialization
-    for (auto & spec : mTemplateDeclarations) {
+    for (const auto& spec : mTemplateDeclarations) {
         if (spec.isSpecialization()) {
             bool found = false;
-            for (auto & decl : mTemplateDeclarations) {
+            for (const auto& decl : mTemplateDeclarations) {
                 if (specMatch(spec, decl)) {
                     mTemplateSpecializationMap[spec.token()] = decl.token();
                     found = true;
@@ -3386,7 +3386,7 @@ void TemplateSimplifier::getSpecializations()
             }
 
             if (!found) {
-                for (auto & decl : mTemplateForwardDeclarations) {
+                for (const auto& decl : mTemplateForwardDeclarations) {
                     if (specMatch(spec, decl)) {
                         mTemplateSpecializationMap[spec.token()] = decl.token();
                         break;
@@ -3400,10 +3400,10 @@ void TemplateSimplifier::getSpecializations()
 void TemplateSimplifier::getPartialSpecializations()
 {
     // try to locate a matching declaration for each user defined partial specialization
-    for (auto & spec : mTemplateDeclarations) {
+    for (const auto& spec : mTemplateDeclarations) {
         if (spec.isPartialSpecialization()) {
             bool found = false;
-            for (auto & decl : mTemplateDeclarations) {
+            for (const auto& decl : mTemplateDeclarations) {
                 if (specMatch(spec, decl)) {
                     mTemplatePartialSpecializationMap[spec.token()] = decl.token();
                     found = true;
@@ -3412,7 +3412,7 @@ void TemplateSimplifier::getPartialSpecializations()
             }
 
             if (!found) {
-                for (auto & decl : mTemplateForwardDeclarations) {
+                for (const auto& decl : mTemplateForwardDeclarations) {
                     if (specMatch(spec, decl)) {
                         mTemplatePartialSpecializationMap[spec.token()] = decl.token();
                         break;
@@ -3841,7 +3841,7 @@ void TemplateSimplifier::simplifyTemplates(
         }
 
         // remove explicit instantiations
-        for (TokenAndName & j : mExplicitInstantiationsToDelete) {
+        for (const TokenAndName& j : mExplicitInstantiationsToDelete) {
             Token * start = j.token();
             if (start) {
                 Token * end = start->next();

test/testother.cpp

@@ -2181,7 +2181,24 @@ private:
         check("void f(std::vector<int>& v) {\n"
               "    for(auto& x:v) {}\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'v' can be declared as reference to const\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'v' can be declared as reference to const\n"
+                      "[test.cpp:2]: (style) Variable 'x' can be declared as reference to const\n",
+                      errout.str());
+
+        check("void f(std::vector<int>& v) {\n" // #10980
+              "    for (int& i : v)\n"
+              "        if (i == 0) {}\n"
+              "    for (const int& i : v)\n"
+              "        if (i == 0) {}\n"
+              "    for (auto& i : v)\n"
+              "        if (i == 0) {}\n"
+              "    for (const auto& i : v)\n"
+              "        if (i == 0) {}\n"
+              "    v.clear();\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:2]: (style) Variable 'i' can be declared as reference to const\n"
+                      "[test.cpp:6]: (style) Variable 'i' can be declared as reference to const\n",
+                      errout.str());
 
         check("void f(std::vector<int>& v) {\n"
               "    for(const auto& x:v) {}\n"