[clang-tidy] cppcoreguidelines-declare-loop-variable-in-the-initializer: a new check

[DNS320]

Summary

This check implements the ES.74 rule of the C++ Core Guidelines.

The goal of this check is to find for-statements which do not declare their loop variable in the initializer part.
This limits the loop variables visibility to the scope of the loop and prevents using the loop variable for other purposes after the loop.

Thank you for the review.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp

29

It'll be reasonable to add add note with actual variable declaration.

31

I think should be for statement.

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h

2

I may be mistaken, but proper code is -*- C++ -*-. Please remove dash at left side.

25

Please add isLanguageVersionSupported. Older versions of C doesn't support variable declarations inside for loops.

clang-tools-extra/docs/ReleaseNotes.rst

95

I think should be for statement and for highlighted with double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.rst

7

Please synchronize with statement in Release Notes.

9

Such references are usually placed at the end. See other checks documentation as example.

[njames93]

I'd argue this check isn't following the enforcement specified in the relevant guideline:

Enforcement:
Warn when a variable modified inside the for-statement is declared outside the loop and not being used outside the loop.

All this is checking is that there is a declStmt in the for stmt, it doesn't even have to be a relevant declaration, this code would be considered ok by the implementation in here.

int I = 0;
for (int Unused = 0; I < 10; ++I) {}

Ideally you want to check for variables modified in the condition or iteration expression that aren't declared in the init statement.
To be fully compliant with the wording you would also need to ensure the variable isn't referenced outside the loop.

[DNS320]

Thank you for your reviews.
I will work on your comments and write back soon.

[DNS320]

I updated the check according to the last review.
The check should now fully implement the defined enforcement chapter from the ES.74 C++ Core Guideline.

About searching for declRefExpr outside the for statement:
I limited the search to the next ancestor compoundStmt of the varDecl, so the Visitor must not traverse the whole translation unit.

[DNS320]

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp

29

The hole check was refactored. A diagnostic hint points now to the statement which modifies the variable.

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h

2

Thanks. I checked other header files with long names. I correct it this way, to match the other ones:

//===--- DeclareLoopVariableInTheInitializerCheck.h - clang-tidy-*- C++ -*-===//
//===--- ProBoundsArrayToPointerDecayCheck.h - clang-tidy--------*- C++ -*-===//

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp

37

const auto* could be used because type is spelled in same statement.

[steveire]

I implemented something like this recently too. The code I was trying to refactor was something like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}

ie, the variable was used repeatedly, but always initialized in the loop.

I also had code like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
ii = getNumber();
doSomething(ii);

ie, the next statement referring to ii outside the loop was an assignment, so I could just change it to

for (auto ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
auto ii = getNumber();
doSomething(ii);

You don't necessarily have to handle these cases in the initial version of this check, but you could consider them in the future.

[DNS320]

Renamed the IsInsideMatchedForStmt() function according to a comment from the build system.
Changed a data type to "auto" according to a comment from Eugene.Zelenko.

[DNS320]

In D100092#2726808, @steveire wrote:

I implemented something like this recently too. The code I was trying to refactor was something like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}

ie, the variable was used repeatedly, but always initialized in the loop.

I also had code like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
ii = getNumber();
doSomething(ii);

ie, the next statement referring to ii outside the loop was an assignment, so I could just change it to

for (auto ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
auto ii = getNumber();
doSomething(ii);

You don't necessarily have to handle these cases in the initial version of this check, but you could consider them in the future.

Thank you for your comment!
I agree with you and I can see the need to cover such code with a style check.
The initial check was contributed to implement the ES.74 C++ Core Guideline. As I think, your code examples should be covered by a check that implements the ES.26 rule of the C++ Core Guidelines, which could be implemented in the future.
Due to your comment, I noticed the link to the ES.26 rule in the description of this check is not accurate, so I will remove it.

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp

37

Thanks!

[DNS320]

Removed a link to the ES.26 C++ Core Guideline in the documentation part.

[DNS320]

Dear reviewers,

Since this work was conducted as part of a bachelor's thesis, which has to be
handed in on the 18th of June 2021, we wanted to add a friendly ping to this
patch.

It would make us, the thesis team, proud to state that some of our work was
accepted and included into the LLVM project.

Therefore, we wanted to ask if we can further improve anything in this patch
for it to get a LGTM?

Thanks a lot in advance!
Yours faithfully,
Marco & Fabian

[njames93]

Sorry its been so long, How does this handle cases where the variable contains resources. Like a vector being used in a loop where you don't want to release its memory between each iteration.

void foo(){
  std::vector<char> Buffer;
  for (int i = 0; i < limit;++i) {
    Buffer.clear();
    // Do something with buffer
  }
}

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidGotoCheck.cpp
   AvoidNonConstGlobalVariablesCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
+  DeclareLoopVariableInTheInitializerCheck.cpp
   InitVariablesCheck.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
   PreferMemberInitializerCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "AvoidNonConstGlobalVariablesCheck.h"
+#include "DeclareLoopVariableInTheInitializerCheck.h"
 #include "InitVariablesCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "PreferMemberInitializerCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<AvoidNonConstGlobalVariablesCheck>(
         "cppcoreguidelines-avoid-non-const-global-variables");
+    CheckFactories.registerCheck<DeclareLoopVariableInTheInitializerCheck>(
+        "cppcoreguidelines-declare-loop-variable-in-the-initializer");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");
     CheckFactories.registerCheck<InitVariablesCheck>(
         "cppcoreguidelines-init-variables");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h

+//===--- DeclareLoopVariableInTheInitializerCheck.h - clang-tidy-*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_DECLARELOOPVARIABLEINTHEINITIALIZERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_DECLARELOOPVARIABLEINTHEINITIALIZERCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Finds variables that are modified inside a ``for`` statement, are not used
+/// elsewhere and are declared outside the ``for`` statement.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.html
+class DeclareLoopVariableInTheInitializerCheck : public ClangTidyCheck {
+public:
+  DeclareLoopVariableInTheInitializerCheck(StringRef Name,
+                                           ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_DECLARELOOPVARIABLEINTHEINITIALIZERCHECK_H

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp

+//===--- DeclareLoopVariableInTheInitializerCheck.cpp - clang-tidy --------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DeclareLoopVariableInTheInitializerCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+class OutsideForStmtVisitor
+    : public RecursiveASTVisitor<OutsideForStmtVisitor> {
+  friend class RecursiveASTVisitor<OutsideForStmtVisitor>;
+
+public:
+  OutsideForStmtVisitor(const ForStmt *ForStmt, const VarDecl *VarDecl,
+                        const MatchFinder::MatchResult &Result)
+      : MatchedForStmt(ForStmt), MatchedDecl(VarDecl), MatchedResult(Result),
+        IsOutsideMatchedForStmt(false) {}
+
+  bool hasDeclRefExpOutside(const CompoundStmt *Compound) {
+    TraverseStmt(const_cast<CompoundStmt *>(Compound));
+    return IsOutsideMatchedForStmt;
+  }
+
+private:
+  bool VisitDeclRefExpr(DeclRefExpr *D) {
+    if (const VarDecl *To = dyn_cast<VarDecl>(D->getDecl())) {
+      if (To == MatchedDecl &&
+          !IsInsideMatchedForStmt(MatchedResult, DynTypedNode::create(*D))) {
+        IsOutsideMatchedForStmt = true;
+        return false;
+      }
+    }
+    return true;
+  }
+
+  bool IsInsideMatchedForStmt(const MatchFinder::MatchResult &Result,
+                              const DynTypedNode &Node) {
+    const auto *PossibleForStmt = Node.get<ForStmt>();
+
+    if (PossibleForStmt && PossibleForStmt == MatchedForStmt) {
+      return true;
+    }
+
+    return llvm::any_of(Result.Context->getParents(Node),
+                        [&](const DynTypedNode &Parent) {
+                          return IsInsideMatchedForStmt(Result, Parent);
+                        });
+  }
+
+  const ForStmt *MatchedForStmt;
+  const VarDecl *MatchedDecl;
+  const MatchFinder::MatchResult &MatchedResult;
+  bool IsOutsideMatchedForStmt;
+};
+
+void DeclareLoopVariableInTheInitializerCheck::registerMatchers(
+    MatchFinder *Finder) {
+  Finder->addMatcher(
+      declRefExpr(
+          hasAncestor(forStmt().bind("ForStmt")),
+          anyOf(hasAncestor(unaryOperator().bind("Operator")),
+                hasAncestor(
+                    binaryOperator(isAssignmentOperator()).bind("Operator"))),
+          to(varDecl(hasAncestor(compoundStmt().bind("Compound")),
+                     unless(hasAncestor(forStmt(equalsBoundNode("ForStmt")))))
+                 .bind("VarDecl"))),
+      this);
+}
+
+void DeclareLoopVariableInTheInitializerCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedForStmt = Result.Nodes.getNodeAs<ForStmt>("ForStmt");
+  const auto *MatchedExprOperator = Result.Nodes.getNodeAs<Expr>("Operator");
+  const auto *MatchedVarDecl = Result.Nodes.getNodeAs<VarDecl>("VarDecl");
+  const auto *MatchedCompoundStmt =
+      Result.Nodes.getNodeAs<CompoundStmt>("Compound");
+
+  if (OutsideForStmtVisitor(MatchedForStmt, MatchedVarDecl, Result)
+          .hasDeclRefExpOutside(MatchedCompoundStmt)) {
+    return;
+  }
+
+  diag(MatchedVarDecl->getLocation(),
+       "Variable %0 is only modified in a for statement and not used "
+       "elsewhere. Consider declaring it inside the for statement.")
+      << MatchedVarDecl;
+  diag(MatchedExprOperator->getBeginLoc(), "Variable gets modified here",
+       DiagnosticIDs::Note);
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`concurrency-thread-canceltype-asynchronous
   type is set to asynchronous.
 
 - New :doc:`altera-unroll-loops
   <clang-tidy/checks/altera-unroll-loops>` check.
 
   Finds inner loops that have not been unrolled, as well as fully unrolled
   loops with unknown loops bounds or a large number of iterations.
 
+- New :doc:`cppcoreguidelines-declare-loop-variable-in-the-initializer
+  <clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer>` check.
+
+  Finds variables that are modified inside a ``for`` statement, are not used elsewhere
+  and are declared outside the ``for`` statement.
+
 - New :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
 
   Finds member initializations in the constructor body which can be placed into
   the initialization list instead.
 
 - New :doc:`misc-std-stream-objects-outside-main
   <clang-tidy/checks/misc-std-stream-objects-outside-main>` check.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.rst

+.. title:: clang-tidy - cppcoreguidelines-declare-loop-variable-in-the-initializer
+
+cppcoreguidelines-declare-loop-variable-in-the-initializer
+==========================================================
+
+Finds variables that are modified inside a ``for`` statement, are not used elsewhere
+and are declared outside the ``for`` statement.
+
+.. code-block:: c++
+
+  const int Limit{10};
+
+  void func1() {
+    // Should print a warning
+    int A{0};
+
+    for(int I{0}; I < Limit; I++) {
+      A = 3;
+    }
+  }
+
+  void func2() {
+    // Should print a warning
+    int I{0};
+
+    for(I = 1; I < Limit; I++) {}
+  }
+
+  void func3() {
+    // Should print a warning
+    int I{0};
+
+    for(int Unused{0}; I < Limit; I++) {}
+  }
+
+  void func4() {
+    // OK
+    int A{0};
+
+    for(int I{0}; I < Limit; I++) {
+      const int B{A};
+    }
+  }
+
+This check implements the rule `ES.74 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es74-prefer-to-declare-a-loop-variable-in-the-initializer-part-of-a-for-statement>`_ of the C++ Core Guidelines.
+
+It does also cover parts of:
+    - `ES.26 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es26-dont-use-a-variable-for-two-unrelated-purposes>`_
+    - `ES.5 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es5-keep-scopes-small>`_
+    - `ES.6 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es6-declare-names-in-for-statement-initializers-and-conditions-to-limit-scope>`_

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak <clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak.html>`_,
    `clang-analyzer-valist.CopyToSelf <clang-analyzer-valist.CopyToSelf.html>`_,
    `clang-analyzer-valist.Uninitialized <clang-analyzer-valist.Uninitialized.html>`_,
    `clang-analyzer-valist.Unterminated <clang-analyzer-valist.Unterminated.html>`_,
    `concurrency-mt-unsafe <concurrency-mt-unsafe.html>`_,
    `concurrency-thread-canceltype-asynchronous <concurrency-thread-canceltype-asynchronous.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines-avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_,
+   `cppcoreguidelines-declare-loop-variable-in-the-initializer <cppcoreguidelines-declare-loop-variable-in-the-initializer.html>`_,
    `cppcoreguidelines-init-variables <cppcoreguidelines-init-variables.html>`_, "Yes"
    `cppcoreguidelines-interfaces-global-init <cppcoreguidelines-interfaces-global-init.html>`_,
    `cppcoreguidelines-macro-usage <cppcoreguidelines-macro-usage.html>`_,
    `cppcoreguidelines-narrowing-conversions <cppcoreguidelines-narrowing-conversions.html>`_,
    `cppcoreguidelines-no-malloc <cppcoreguidelines-no-malloc.html>`_,
    `cppcoreguidelines-owning-memory <cppcoreguidelines-owning-memory.html>`_,
    `cppcoreguidelines-prefer-member-initializer <cppcoreguidelines-prefer-member-initializer.html>`_, "Yes"
    `cppcoreguidelines-pro-bounds-array-to-pointer-decay <cppcoreguidelines-pro-bounds-array-to-pointer-decay.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-declare-loop-variable-in-the-initializer.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-declare-loop-variable-in-the-initializer %t
+
+const int Limit{10};
+
+void func1() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'A' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer]
+  // CHECK-MESSAGES: :11:5: note: Variable gets modified here
+  int A{0};
+
+  for (int I{0}; I < Limit; I++) {
+    A = 3;
+  }
+}
+
+void func2() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'A' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer]
+  // CHECK-MESSAGES: :21:5: note: Variable gets modified here
+  int A{0};
+
+  for (int I{0}; I < Limit; I++) {
+    A += 3;
+  }
+}
+
+void func3() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer]
+  // CHECK-MESSAGES: :30:8: note: Variable gets modified here
+  int I{0};
+
+  for (I = 1; I < Limit; I++) {
+  }
+}
+
+void func4() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer]
+  // CHECK-MESSAGES: :39:21: note: Variable gets modified here
+  int I{0};
+
+  for (; I < Limit; I++) {
+  }
+}
+
+void func5() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer]
+  // CHECK-MESSAGES: :48:34: note: Variable gets modified here
+  int I{0};
+
+  for (int Unused{0}; I < Limit; I++) {
+  }
+}
+
+void func6() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer]
+  // CHECK-MESSAGES: :58:8: note: Variable gets modified here
+  int I{0};
+  int Z{0};
+
+  for (I = 3; I < Limit; I++) {
+  }
+  Z = 3;
+}
+
+void func7() {
+  // OK
+  int A{0};
+
+  for (int I{0}; I < Limit; I++) {
+    const int B{A};
+  }
+}
+
+void func8() {
+  // OK
+  int I{0};
+
+  for (I = 0; I < Limit; I++) {
+  }
+  const int A{I};
+}