Add a mode 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.

Author: flowerhack

clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp

@@ -79,10 +79,15 @@ 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)
-    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 "
@@ -108,10 +113,16 @@ static bool isReferenced(const VarDecl &LoopVar, const Stmt &Stmt,
 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;
+
+  // TODO: Add some toggle here for caring about Expensive. Legacy checks
+  // care about this but our new check does not.
+  // Simply disabling the Expensive check altogether for now so we can test the
+  // new check.
+  //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) {
@@ -133,6 +144,16 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
 
     return true;
   }
+
+  // If it's copied and mutated, there's a high chance that's a bug.
+  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;
 }
 

playground/test.cpp

@@ -0,0 +1,88 @@
+#include <iostream>
+#include <vector>
+#include <string.h>
+
+void ModifyString(std::string *s) {
+	s->append("!");
+}
+
+class DataCache {
+public:
+  // This member function declaration will also be matched.
+  // Okay my code ONLY matched this one.
+  // But God that's so much better than nothing.
+  const std::vector<int>& get_cached_items() const;
+
+private:
+  std::vector<int> items_;
+};
+
+const std::vector<std::string>& get_global_strings();
+
+const std::vector<double>& get_default_values() {
+    static std::vector<double> defaults = {3.14, 2.71, 1.61};
+    return defaults;
+}
+
+int main() {
+	std::cout << "Initializing some_strings.\n";
+	std::vector<std::string> some_strings;
+	some_strings.push_back("one");
+	some_strings.push_back("two");
+	some_strings.push_back("three");
+	// Three tools:
+	// * suspicious-copy-in-range-loop (as written right now)
+	// * performance-for-range-copy  with no options
+	// * performance-for-range-copy with WarnOnAllAutoCopies enabled
+
+	std::cout << "Modify copies; print them as you modify them.\n";
+	// Case 1.
+	// These strings are being COPIED and then MODIFIED, so:
+	// SHOULD WARN? YES
+	// DOES WARN?
+	// suspicious-copy-in-range-loop: YES
+	// ~ performance-for-range-copy with no options: NO
+	// performance-for-range-copy with WarnOnAllAutoCopies enabled: YES
+	for (auto x : some_strings) {
+		ModifyString(&x);
+		std::cout << x << "\n";
+		//std::cout << &x << "\n";
+	}
+
+	// Case 2.
+	// These strings are being COPIED but left UNMODIFIED, so:
+	// SHOULD WARN? NO
+	// DOES WARN?
+	// ~ suspicious-copy-in-range-loop: YES
+	// performance-for-range-copy with no options: NO
+	// ~ performance-for-range-copy with WarnOnAllAutoCopies enabled: YES
+	for (auto x : some_strings) {
+		std::cout << "hi\n";
+		std::cout << x << "\n";
+		//std::cout << &x << "\n";
+	}
+
+	// Case 3.
+	// These strings are being NEITHER COPIED, NOR MODIFIED, so
+	// SHOULD WARN? NO
+	// DOES WARN?
+	// suspicious-copy-in-range-loop: NO
+	// performance-for-range-copy with no options: NO
+	// performance-for-range-copy with WarnOnAllAutoCopies enabled: NO
+	for (auto& x : some_strings) {
+		std::cout << x << "\n";
+		std::cout << &x << "\n";
+	}
+
+	// Case 4.
+	// These strings are being NOT COPIED but referenced, and MODIFIED, so
+	// SHOULD WARN? NO
+	// DOES WARN?
+	// suspicious-copy-in-range-loop: NO
+	// performance-for-range-copy with no options: NO
+	// performance-for-range-copy with WarnOnAllAutoCopies enabled: NO
+	for (auto& x : some_strings) {
+		x = "modd'd!";
+		std::cout << x << "\n";
+	}
+}