Add WarnOnModificationOfCopiedLoopVariable to performance-for-range-c…

[flowerhack]

…opy.

Adds an option to performance-for-range-copy that alerts when a loop variable is copied and modified.

This is a bugprone pattern because the programmer in this case often assumes they are modifying the original value instead of a copy.

The warning can be suppressed by instead explicitly copying the value inside the body of the loop.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Julia Hansbrough (flowerhack)

Changes

…opy.

Adds an option to performance-for-range-copy that alerts when a loop variable is copied and modified.

This is a bugprone pattern because the programmer in this case often assumes they are modifying the original value instead of a copy.

The warning can be suppressed by instead explicitly copying the value inside the body of the loop.

---

Full diff: https://github.com/llvm/llvm-project/pull/155731.diff

4 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp (+30-2)
• (modified) clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h (+8)
• (modified) clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst (+12)
• (added) clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp (+49)

diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
index f545a49dc184b..cbb842927bd57 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -22,11 +22,15 @@ namespace clang::tidy::performance {
 ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", false)),
+      WarnOnModificationOfCopiedLoopVariable(
+          Options.get("WarnOnModificationOfCopiedLoopVariable", false)),
       AllowedTypes(
           utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
 
 void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies);
+  Options.store(Opts, "WarnOnModificationOfCopiedLoopVariable",
+                WarnOnModificationOfCopiedLoopVariable);
   Options.store(Opts, "AllowedTypes",
                 utils::options::serializeStringList(AllowedTypes));
 }
@@ -64,9 +68,11 @@ void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) {
   // Ignore code in macros since we can't place the fixes correctly.
   if (Var->getBeginLoc().isMacroID())
     return;
+  const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange");
+  if (copiedLoopVarIsMutated(*Var, *ForRange, *Result.Context))
+    return;
   if (handleConstValueCopy(*Var, *Result.Context))
     return;
-  const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange");
   handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context);
 }
 
@@ -79,6 +85,7 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
   } else if (!LoopVar.getType().isConstQualified()) {
     return false;
   }
+
   std::optional<bool> Expensive =
       utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (!Expensive || !*Expensive)
@@ -105,6 +112,27 @@ static bool isReferenced(const VarDecl &LoopVar, const Stmt &Stmt,
               .empty();
 }
 
+bool ForRangeCopyCheck::copiedLoopVarIsMutated(const VarDecl &LoopVar,
+                                               const CXXForRangeStmt &ForRange,
+                                               ASTContext &Context) {
+  // If it's copied and mutated, there's a high chance that's a bug.
+  if (WarnOnModificationOfCopiedLoopVariable) {
+    if (ExprMutationAnalyzer(*ForRange.getBody(), Context)
+            .isMutated(&LoopVar)) {
+      auto Diag =
+          diag(LoopVar.getLocation(), "loop variable is copied and then "
+                                      "modified, which is likely a bug; you "
+                                      "probably want to modify the underlying "
+                                      "object and not this copy. If you "
+                                      "*did* intend to modify this copy, "
+                                      "please use an explicit copy inside the "
+                                      "body of the loop");
+      return true;
+    }
+  }
+  return false;
+}
+
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
     ASTContext &Context) {
@@ -112,6 +140,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
       utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
     return false;
+
   // We omit the case where the loop variable is not used in the loop body. E.g.
   //
   // for (auto _ : benchmark_state) {
@@ -130,7 +159,6 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     if (std::optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
             LoopVar, Context, Qualifiers::Const))
       Diag << *Fix << utils::fixit::changeVarDeclToReference(LoopVar, Context);
-
     return true;
   }
   return false;
diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
index 8fabbfa2ae7ba..722ca2f3a2e24 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
@@ -39,8 +39,16 @@ class ForRangeCopyCheck : public ClangTidyCheck {
                                        const CXXForRangeStmt &ForRange,
                                        ASTContext &Context);
 
+  // Checks if the loop variable is copied and then subsequently mutated
+  // in the body of the loop. If so it suggests the copy was unintentional,
+  // or, that the copy would be more clear if done inside the body of the loop.
+  bool copiedLoopVarIsMutated(const VarDecl &LoopVar,
+                              const CXXForRangeStmt &ForRange,
+                              ASTContext &Context);
+
   const bool WarnOnAllAutoCopies;
   const std::vector<StringRef> AllowedTypes;
+  const bool WarnOnModificationOfCopiedLoopVariable;
 };
 
 } // namespace clang::tidy::performance
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst
index d740984029482..a8dbf6e3d435a 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst
@@ -18,6 +18,12 @@ following heuristic is employed:
    invoked on it, or it is used as const reference or value argument in
    constructors or function calls.
 
+There is an additional option to warn any time a loop variable is copied in each
+iteration and subsequently modified in the body of the loop. This is likely to
+be a bug, since the user may believe they are modifying the underlying value
+instead of a copy, and a user that truly intends to modify a copy may do so by
+performing the copy explicitly inside the body of the loop.
+
 Options
 -------
 
@@ -26,6 +32,12 @@ Options
    When `true`, warns on any use of ``auto`` as the type of the range-based for
    loop variable. Default is `false`.
 
+.. option:: WarnOnModificationOfCopiedLoopVariable
+
+   When `true`, warns when the loop variable is copied and subsequently
+   modified. This is likely to be a bug. Moving the copy to an explicit copy
+   inside of the loop will suppress the warning. Default is `false`.
+
 .. option:: AllowedTypes
 
    A semicolon-separated list of names of types allowed to be copied in each
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp
new file mode 100644
index 0000000000000..ed34a4fc586c0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp
@@ -0,0 +1,49 @@
+// RUN: %check_clang_tidy %s performance-for-range-copy %t -- \
+// RUN:     -config="{CheckOptions: {performance-for-range-copy.WarnOnModificationOfCopiedLoopVariable: true}}"
+
+template <typename T>
+struct Iterator {
+  void operator++() {}
+  const T& operator*() {
+    static T* TT = new T();
+    return *TT;
+  }
+  bool operator!=(const Iterator &) { return false; }
+};
+template <typename T>
+struct View {
+  T begin() { return T(); }
+  T begin() const { return T(); }
+  T end() { return T(); }
+  T end() const { return T(); }
+};
+
+struct S {
+  int value;
+
+  S() : value(0) {};
+  S(const S &);
+  ~S();
+  S &operator=(const S &);
+  void modify() {
+    value++;
+  }
+};
+
+void NegativeLoopVariableNotCopied() {
+  for (const S& S1 : View<Iterator<S>>()) {
+  }
+}
+
+void NegativeLoopVariableCopiedButNotModified() {
+  for (S S1 : View<Iterator<S>>()) {
+  }
+}
+
+void PositiveLoopVariableCopiedAndThenModfied() {
+  for (S S1 : View<Iterator<S>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:10: warning: loop variable is copied and then modified, which is likely a bug; you probably want to modify the underlying object and not this copy. If you *did* intend to modify this copy, please use an explicit copy inside the body of the loop
+    S1.modify();
+  }
+}
+

[vbvictor]

I'm against adding this option to this check because performance-for-range-copy is mainly a performance check so we should not mix bugprone patterns into it.

This is a bugprone pattern because the programmer in this case often assumes they are modifying the original value instead of a copy.

I personally highly doubt that modifying a loop variable is a bugprone pattern (more like a general principle of how C++ works)

[vbvictor]

As an alternative solution, your Idea can be a separate check but you should file an issue first to gather some initial feedback.

[EugeneZelenko]

Please mention change in Release Notes.

[EugeneZelenko]

Suggested change

[flowerhack]

Implementing this as a separate check rather than attacking it to performance-for-range-copy sounds fine to me.

I'll go ahead and close this PR out. I've created an issue at #155922 where I offer more evidence/rationale for why this can be counted as a bugprone pattern.