[clang-tidy] Handle C++ structured bindings in performance-for-range-copy

[legrosbuffle]

Right now we are not triggering on:

for (auto [x, y] : container) {
  // const-only access
}

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-tidy

Author: Clement Courbet (legrosbuffle)

Changes

Right now we are not triggering on:

for (auto [x, y] : container) {
  // const-only access
}

---

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

5 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp (+10-3)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp (+26-5)
• (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+10-3)
• (modified) clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp (+20)

diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
index 5bfa6fb0d02d5c..655e480ffa62cb 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -97,6 +97,15 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
   return true;
 }
 
+static bool isReferenced(const VarDecl &LoopVar, const Stmt &Stmt,
+                         ASTContext &Context) {
+  const auto IsLoopVar = varDecl(equalsNode(&LoopVar));
+  return !match(stmt(hasDescendant(declRefExpr(to(valueDecl(anyOf(
+                    IsLoopVar, bindingDecl(forDecomposition(IsLoopVar)))))))),
+                Stmt, Context)
+              .empty();
+}
+
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
     ASTContext &Context) {
@@ -113,9 +122,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
   // compiler warning which can't be suppressed.
   // Since this case is very rare, it is safe to ignore it.
   if (!ExprMutationAnalyzer(*ForRange.getBody(), Context).isMutated(&LoopVar) &&
-      !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
-                                             Context)
-           .empty()) {
+      isReferenced(LoopVar, *ForRange.getBody(), Context)) {
     auto Diag = diag(
         LoopVar.getLocation(),
         "loop variable is copied but only used as const reference; consider "
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4d25e2ebe85f5f..4491d239dc7888 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -419,6 +419,10 @@ Changes in existing checks
   <clang-tidy/checks/performance/faster-string-find>` check to properly escape
   single quotes.
 
+- Improved :doc:`performance-for-range-copy
+  <clang-tidy/checks/performance/for-range-copy>` check to handle cases where
+  the loop variable is a structured binding.
+
 - Improved :doc:`performance-noexcept-move-constructor
   <clang-tidy/checks/performance/noexcept-move-constructor>` to better handle
   conditional noexcept expressions, eliminating false-positives.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
index 1a2eedc9e65c53..e8c967dacfb7d5 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -47,6 +47,11 @@ struct S {
   S &operator=(const S &);
 };
 
+struct Point {
+  ~Point() {}
+  int x, y;
+};
+
 struct Convertible {
   operator S() const {
     return S();
@@ -87,6 +92,10 @@ void instantiated() {
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}}
   // CHECK-FIXES: {{^}}  for (const S& S2 : View<Iterator<S>>()) {}
 
+  for (const auto [X, Y] : View<Iterator<Point>>()) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: the loop variable's type is
+  // CHECK-FIXES: {{^}}  for (const auto& [X, Y] : View<Iterator<Point>>()) {}
+
   for (const T T2 : View<Iterator<T>>()) {}
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}}
   // CHECK-FIXES: {{^}}  for (const T& T2 : View<Iterator<T>>()) {}
@@ -123,11 +132,6 @@ struct Mutable {
   ~Mutable() {}
 };
 
-struct Point {
-  ~Point() {}
-  int x, y;
-};
-
 Mutable& operator<<(Mutable &Out, bool B) {
   Out.setBool(B);
   return Out;
@@ -144,6 +148,7 @@ void useByValue(Mutable M);
 void useByConstValue(const Mutable M);
 void mutate(Mutable *M);
 void mutate(Mutable &M);
+void mutate(int &);
 void onceConstOnceMutated(const Mutable &M1, Mutable &M2);
 
 void negativeVariableIsMutated() {
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
   }
 }
 
+void positiveOnlyAccessedFieldAsConstBinding() {
+  for (auto [X, Y] : View<Iterator<Point>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+    // CHECK-FIXES: for (const auto& [X, Y] : View<Iterator<Point>>()) {
+    use(X);
+    use(Y);
+  }
+}
+
+void negativeOnlyAccessedFieldAsConstBinding() {
+  for (auto [X, Y] : View<Iterator<Point>>()) {
+    use(X);
+    mutate(Y);
+  }
+}
+
 void positiveOnlyUsedInCopyConstructor() {
   for (auto A : View<Iterator<Mutable>>()) {
     // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 624a643cc60e4b..29f05b25bdec2b 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -204,9 +204,16 @@ const Stmt *ExprMutationAnalyzer::findMutationMemoized(
 
 const Stmt *ExprMutationAnalyzer::tryEachDeclRef(const Decl *Dec,
                                                  MutationFinder Finder) {
-  const auto Refs =
-      match(findAll(declRefExpr(to(equalsNode(Dec))).bind(NodeID<Expr>::value)),
-            Stm, Context);
+  const auto Refs = match(
+      findAll(
+          declRefExpr(to(
+                          // `Dec` or a binding if `Dec` is a decomposition.
+                          anyOf(equalsNode(Dec),
+                                bindingDecl(forDecomposition(equalsNode(Dec))))
+                          //
+                          ))
+              .bind(NodeID<Expr>::value)),
+      Stm, Context);
   for (const auto &RefNodes : Refs) {
     const auto *E = RefNodes.getNodeAs<Expr>(NodeID<Expr>::value);
     if ((this->*Finder)(E))
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index f1a1f857a0ee5b..a94f857720b035 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -592,6 +592,26 @@ TEST(ExprMutationAnalyzerTest, ByConstRRefArgument) {
               ElementsAre("static_cast<A &&>(x)"));
 }
 
+// Section: bindings.
+
+TEST(ExprMutationAnalyzerTest, BindingModifies) {
+  const auto AST =
+      buildASTFromCode("struct Point { int x; int y; };"
+                       "void mod(int&);"
+                       "void f(Point p) { auto& [x, y] = p; mod(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x", "mod(x)"));
+}
+
+TEST(ExprMutationAnalyzerTest, BindingDoesNotModify) {
+  const auto AST = buildASTFromCode("struct Point { int x; int y; };"
+                                    "void f(Point p) { auto& [x, y] = p; x; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
 // section: explicit std::move and std::forward testing
 
 TEST(ExprMutationAnalyzerTest, Move) {