Add WarnOnModificationOfCopiedLoopVariable to

performance-for-range-copy.

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.

Author: flowerhack

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,10 +85,15 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
   } else if (!LoopVar.getType().isConstQualified()) {
     return false;
   }
+
+  // TODO: Check if this is actually needed for some cases? It seems redundant
+  // with the Expensive check in handleCopyIsOnlyConstReferenced but maybe
+  // I'm missing something
   std::optional<bool> Expensive =
       utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (!Expensive || !*Expensive)
     return false;
+
   auto Diagnostic =
       diag(LoopVar.getLocation(),
            "the loop variable's type is not a reference type; this creates a "
@@ -105,13 +116,35 @@ 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) {
   std::optional<bool> Expensive =
       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 +163,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;

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

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

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();
+  }
+}
+