Skip to content

[clang-tidy] Improve bugprone-infinite-loop check by adding handing for structured bindings #144213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

flovent
Copy link
Contributor

@flovent flovent commented Jun 14, 2025

Before this patch, this check only handles VarDecl as varaibles' declaration in statement, but this will ignore variables in structured bindings (BindingDecl in AST), which leads to false positives.

Closes #138842.

@llvmbot
Copy link
Member

llvmbot commented Jun 14, 2025

@llvm/pr-subscribers-clang-tidy

Author: None (flovent)

Changes

Before this patch, this check only handles VarDecl as varaibles' declaration in statement, but this will ignore variables in structured bindings (BindingDecl in AST), which leads to false positives.

Closes #138842.


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

5 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp (+31-2)
  • (modified) clang-tools-extra/clang-tidy/utils/Aliasing.cpp (+7-7)
  • (modified) clang-tools-extra/clang-tidy/utils/Aliasing.h (+1-1)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp (+81)
diff --git a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
index 07116a7ff15f5..4ceca53e2f7f3 100644
--- a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -50,7 +50,7 @@ static Matcher<Stmt> loopEndingStmt(Matcher<Stmt> Internal) {
 }
 
 /// Return whether `Var` was changed in `LoopStmt`.
-static bool isChanged(const Stmt *LoopStmt, const VarDecl *Var,
+static bool isChanged(const Stmt *LoopStmt, const ValueDecl *Var,
                       ASTContext *Context) {
   if (const auto *ForLoop = dyn_cast<ForStmt>(LoopStmt))
     return (ForLoop->getInc() &&
@@ -83,6 +83,23 @@ static bool isVarThatIsPossiblyChanged(const Decl *Func, const Stmt *LoopStmt,
              isChanged(LoopStmt, Var, Context);
       // FIXME: Track references.
     }
+
+    if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) {
+      if (const auto *DD =
+              dyn_cast_if_present<DecompositionDecl>(BD->getDecomposedDecl())) {
+        if (!DD->isLocalVarDeclOrParm())
+          return true;
+
+        if (DD->getType().isVolatileQualified())
+          return true;
+
+        if (!BD->getType().getTypePtr()->isIntegerType())
+          return true;
+
+        return hasPtrOrReferenceInFunc(Func, BD) ||
+               isChanged(LoopStmt, BD, Context);
+      }
+    }
   } else if (isa<MemberExpr, CallExpr, ObjCIvarRefExpr, ObjCPropertyRefExpr,
                  ObjCMessageExpr>(Cond)) {
     // FIXME: Handle MemberExpr.
@@ -124,6 +141,10 @@ static std::string getCondVarNames(const Stmt *Cond) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) {
     if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl()))
       return std::string(Var->getName());
+
+    if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) {
+      return std::string(BD->getName());
+    }
   }
 
   std::string Result;
@@ -215,10 +236,18 @@ static bool overlap(ArrayRef<CallGraphNode *> SCC,
 
 /// returns true iff `Cond` involves at least one static local variable.
 static bool hasStaticLocalVariable(const Stmt *Cond) {
-  if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond))
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) {
     if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
       if (VD->isStaticLocal())
         return true;
+
+    if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl()))
+      if (const auto *DD =
+              dyn_cast_if_present<DecompositionDecl>(BD->getDecomposedDecl()))
+        if (DD->isStaticLocal())
+          return true;
+  }
+
   for (const Stmt *Child : Cond->children())
     if (Child && hasStaticLocalVariable(Child))
       return true;
diff --git a/clang-tools-extra/clang-tidy/utils/Aliasing.cpp b/clang-tools-extra/clang-tidy/utils/Aliasing.cpp
index 2facf0625605e..cbe4873b5c022 100644
--- a/clang-tools-extra/clang-tidy/utils/Aliasing.cpp
+++ b/clang-tools-extra/clang-tidy/utils/Aliasing.cpp
@@ -14,14 +14,14 @@
 namespace clang::tidy::utils {
 
 /// Return whether \p S is a reference to the declaration of \p Var.
-static bool isAccessForVar(const Stmt *S, const VarDecl *Var) {
+static bool isAccessForVar(const Stmt *S, const ValueDecl *Var) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(S))
     return DRE->getDecl() == Var;
 
   return false;
 }
 
-static bool capturesByRef(const CXXRecordDecl *RD, const VarDecl *Var) {
+static bool capturesByRef(const CXXRecordDecl *RD, const ValueDecl *Var) {
   return llvm::any_of(RD->captures(), [Var](const LambdaCapture &C) {
     return C.capturesVariable() && C.getCaptureKind() == LCK_ByRef &&
            C.getCapturedVar() == Var;
@@ -29,9 +29,9 @@ static bool capturesByRef(const CXXRecordDecl *RD, const VarDecl *Var) {
 }
 
 /// Return whether \p Var has a pointer or reference in \p S.
-static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) {
+static bool isPtrOrReferenceForVar(const Stmt *S, const ValueDecl *Var) {
   // Treat block capture by reference as a form of taking a reference.
-  if (Var->isEscapingByref())
+  if (const auto *VD = dyn_cast<VarDecl>(Var); VD && VD->isEscapingByref())
     return true;
 
   if (const auto *DS = dyn_cast<DeclStmt>(S)) {
@@ -61,7 +61,7 @@ static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) {
 }
 
 /// Return whether \p Var has a pointer or reference in \p S.
-static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) {
+static bool hasPtrOrReferenceInStmt(const Stmt *S, const ValueDecl *Var) {
   if (isPtrOrReferenceForVar(S, Var))
     return true;
 
@@ -77,7 +77,7 @@ static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) {
 }
 
 static bool refersToEnclosingLambdaCaptureByRef(const Decl *Func,
-                                                const VarDecl *Var) {
+                                                const ValueDecl *Var) {
   const auto *MD = dyn_cast<CXXMethodDecl>(Func);
   if (!MD)
     return false;
@@ -89,7 +89,7 @@ static bool refersToEnclosingLambdaCaptureByRef(const Decl *Func,
   return capturesByRef(RD, Var);
 }
 
-bool hasPtrOrReferenceInFunc(const Decl *Func, const VarDecl *Var) {
+bool hasPtrOrReferenceInFunc(const Decl *Func, const ValueDecl *Var) {
   return hasPtrOrReferenceInStmt(Func->getBody(), Var) ||
          refersToEnclosingLambdaCaptureByRef(Func, Var);
 }
diff --git a/clang-tools-extra/clang-tidy/utils/Aliasing.h b/clang-tools-extra/clang-tidy/utils/Aliasing.h
index 7dad16fc57f1e..6c0763b766805 100644
--- a/clang-tools-extra/clang-tidy/utils/Aliasing.h
+++ b/clang-tools-extra/clang-tidy/utils/Aliasing.h
@@ -25,7 +25,7 @@ namespace clang::tidy::utils {
 /// For `f()` and `n` the function returns ``true`` because `p` is a
 /// pointer to `n` created in `f()`.
 
-bool hasPtrOrReferenceInFunc(const Decl *Func, const VarDecl *Var);
+bool hasPtrOrReferenceInFunc(const Decl *Func, const ValueDecl *Var);
 
 } // namespace clang::tidy::utils
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19ccd1790e757..790ddfaef9103 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -159,6 +159,10 @@ Changes in existing checks
   false positives on deleted constructors that cannot be used to construct
   objects, even if they have public or protected access.
 
+- Improved :doc:`bugprone-infinite-loop
+  <clang-tidy/checks/bugprone/infinite-loop>` check by adding detection for
+  variables introduced by structured bindings.
+
 - Added an option to :doc:`bugprone-multi-level-implicit-pointer-conversion
   <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` to
   choose whether to enable the check in C code or not.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp
index bc14ece3f332c..a8787593da6fe 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp
@@ -711,3 +711,84 @@ void test_local_static_recursion() {
   while (i >= 0)
     p(0); // we don't know what p points to so no warning
 }
+
+struct PairVal {
+  int a;
+  int b;
+  PairVal(int a, int b) : a(a), b(b) {}
+};
+
+void structured_binding_infinite_loop1() {
+  auto [x, y] = PairVal(0, 0);
+  while (x < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop]
+    y++;
+  }
+  while (y < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (y) are updated in the loop body [bugprone-infinite-loop]
+    x++;
+  }
+}
+
+void structured_binding_infinite_loop2() {
+  auto [x, y] = PairVal(0, 0);
+  while (x < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop]
+    // No update to x or y
+  }
+  while (y < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (y) are updated in the loop body [bugprone-infinite-loop]
+    // No update to x or y
+  }
+}
+
+void structured_binding_not_infinite1() {
+  auto [x, y] = PairVal(0, 0);
+  while (x < 10) {
+    x++;
+  }
+  while (y < 10) {
+    y++;
+  }
+}
+
+void volatile_structured_binding_in_condition() {
+  volatile auto [x, y] = PairVal(0, 0);
+  while (!x) {}
+}
+
+void test_local_static_structured_binding_recursion() {
+  static auto [i, _] = PairVal(0, 0);
+  int j = 0;
+
+  i--;
+  while (i >= 0)
+    test_local_static_structured_binding_recursion(); // no warning, recursively decrement i
+  for (; i >= 0;)
+    test_local_static_structured_binding_recursion(); // no warning, recursively decrement i
+  for (; i + j >= 0;)
+    test_local_static_structured_binding_recursion(); // no warning, recursively decrement i
+  for (; i >= 0; i--)
+    ; // no warning, i decrements
+  while (j >= 0)
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (j) are updated in the loop body [bugprone-infinite-loop]
+    test_local_static_structured_binding_recursion();
+
+  int (*p)(int) = 0;
+
+  while (i >= 0)
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+    p = 0;
+  while (i >= 0)
+    p(0); // we don't know what p points to so no warning
+}
+
+struct S { int a; };
+void issue_138842_reduced() {
+    int x = 10;
+    auto [y] = S{1};
+
+    while (y < x) {
+      y++;
+    }
+}

@llvmbot
Copy link
Member

llvmbot commented Jun 14, 2025

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

Author: None (flovent)

Changes

Before this patch, this check only handles VarDecl as varaibles' declaration in statement, but this will ignore variables in structured bindings (BindingDecl in AST), which leads to false positives.

Closes #138842.


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

5 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp (+31-2)
  • (modified) clang-tools-extra/clang-tidy/utils/Aliasing.cpp (+7-7)
  • (modified) clang-tools-extra/clang-tidy/utils/Aliasing.h (+1-1)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp (+81)
diff --git a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
index 07116a7ff15f5..4ceca53e2f7f3 100644
--- a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -50,7 +50,7 @@ static Matcher<Stmt> loopEndingStmt(Matcher<Stmt> Internal) {
 }
 
 /// Return whether `Var` was changed in `LoopStmt`.
-static bool isChanged(const Stmt *LoopStmt, const VarDecl *Var,
+static bool isChanged(const Stmt *LoopStmt, const ValueDecl *Var,
                       ASTContext *Context) {
   if (const auto *ForLoop = dyn_cast<ForStmt>(LoopStmt))
     return (ForLoop->getInc() &&
@@ -83,6 +83,23 @@ static bool isVarThatIsPossiblyChanged(const Decl *Func, const Stmt *LoopStmt,
              isChanged(LoopStmt, Var, Context);
       // FIXME: Track references.
     }
+
+    if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) {
+      if (const auto *DD =
+              dyn_cast_if_present<DecompositionDecl>(BD->getDecomposedDecl())) {
+        if (!DD->isLocalVarDeclOrParm())
+          return true;
+
+        if (DD->getType().isVolatileQualified())
+          return true;
+
+        if (!BD->getType().getTypePtr()->isIntegerType())
+          return true;
+
+        return hasPtrOrReferenceInFunc(Func, BD) ||
+               isChanged(LoopStmt, BD, Context);
+      }
+    }
   } else if (isa<MemberExpr, CallExpr, ObjCIvarRefExpr, ObjCPropertyRefExpr,
                  ObjCMessageExpr>(Cond)) {
     // FIXME: Handle MemberExpr.
@@ -124,6 +141,10 @@ static std::string getCondVarNames(const Stmt *Cond) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) {
     if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl()))
       return std::string(Var->getName());
+
+    if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) {
+      return std::string(BD->getName());
+    }
   }
 
   std::string Result;
@@ -215,10 +236,18 @@ static bool overlap(ArrayRef<CallGraphNode *> SCC,
 
 /// returns true iff `Cond` involves at least one static local variable.
 static bool hasStaticLocalVariable(const Stmt *Cond) {
-  if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond))
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) {
     if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
       if (VD->isStaticLocal())
         return true;
+
+    if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl()))
+      if (const auto *DD =
+              dyn_cast_if_present<DecompositionDecl>(BD->getDecomposedDecl()))
+        if (DD->isStaticLocal())
+          return true;
+  }
+
   for (const Stmt *Child : Cond->children())
     if (Child && hasStaticLocalVariable(Child))
       return true;
diff --git a/clang-tools-extra/clang-tidy/utils/Aliasing.cpp b/clang-tools-extra/clang-tidy/utils/Aliasing.cpp
index 2facf0625605e..cbe4873b5c022 100644
--- a/clang-tools-extra/clang-tidy/utils/Aliasing.cpp
+++ b/clang-tools-extra/clang-tidy/utils/Aliasing.cpp
@@ -14,14 +14,14 @@
 namespace clang::tidy::utils {
 
 /// Return whether \p S is a reference to the declaration of \p Var.
-static bool isAccessForVar(const Stmt *S, const VarDecl *Var) {
+static bool isAccessForVar(const Stmt *S, const ValueDecl *Var) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(S))
     return DRE->getDecl() == Var;
 
   return false;
 }
 
-static bool capturesByRef(const CXXRecordDecl *RD, const VarDecl *Var) {
+static bool capturesByRef(const CXXRecordDecl *RD, const ValueDecl *Var) {
   return llvm::any_of(RD->captures(), [Var](const LambdaCapture &C) {
     return C.capturesVariable() && C.getCaptureKind() == LCK_ByRef &&
            C.getCapturedVar() == Var;
@@ -29,9 +29,9 @@ static bool capturesByRef(const CXXRecordDecl *RD, const VarDecl *Var) {
 }
 
 /// Return whether \p Var has a pointer or reference in \p S.
-static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) {
+static bool isPtrOrReferenceForVar(const Stmt *S, const ValueDecl *Var) {
   // Treat block capture by reference as a form of taking a reference.
-  if (Var->isEscapingByref())
+  if (const auto *VD = dyn_cast<VarDecl>(Var); VD && VD->isEscapingByref())
     return true;
 
   if (const auto *DS = dyn_cast<DeclStmt>(S)) {
@@ -61,7 +61,7 @@ static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) {
 }
 
 /// Return whether \p Var has a pointer or reference in \p S.
-static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) {
+static bool hasPtrOrReferenceInStmt(const Stmt *S, const ValueDecl *Var) {
   if (isPtrOrReferenceForVar(S, Var))
     return true;
 
@@ -77,7 +77,7 @@ static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) {
 }
 
 static bool refersToEnclosingLambdaCaptureByRef(const Decl *Func,
-                                                const VarDecl *Var) {
+                                                const ValueDecl *Var) {
   const auto *MD = dyn_cast<CXXMethodDecl>(Func);
   if (!MD)
     return false;
@@ -89,7 +89,7 @@ static bool refersToEnclosingLambdaCaptureByRef(const Decl *Func,
   return capturesByRef(RD, Var);
 }
 
-bool hasPtrOrReferenceInFunc(const Decl *Func, const VarDecl *Var) {
+bool hasPtrOrReferenceInFunc(const Decl *Func, const ValueDecl *Var) {
   return hasPtrOrReferenceInStmt(Func->getBody(), Var) ||
          refersToEnclosingLambdaCaptureByRef(Func, Var);
 }
diff --git a/clang-tools-extra/clang-tidy/utils/Aliasing.h b/clang-tools-extra/clang-tidy/utils/Aliasing.h
index 7dad16fc57f1e..6c0763b766805 100644
--- a/clang-tools-extra/clang-tidy/utils/Aliasing.h
+++ b/clang-tools-extra/clang-tidy/utils/Aliasing.h
@@ -25,7 +25,7 @@ namespace clang::tidy::utils {
 /// For `f()` and `n` the function returns ``true`` because `p` is a
 /// pointer to `n` created in `f()`.
 
-bool hasPtrOrReferenceInFunc(const Decl *Func, const VarDecl *Var);
+bool hasPtrOrReferenceInFunc(const Decl *Func, const ValueDecl *Var);
 
 } // namespace clang::tidy::utils
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19ccd1790e757..790ddfaef9103 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -159,6 +159,10 @@ Changes in existing checks
   false positives on deleted constructors that cannot be used to construct
   objects, even if they have public or protected access.
 
+- Improved :doc:`bugprone-infinite-loop
+  <clang-tidy/checks/bugprone/infinite-loop>` check by adding detection for
+  variables introduced by structured bindings.
+
 - Added an option to :doc:`bugprone-multi-level-implicit-pointer-conversion
   <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` to
   choose whether to enable the check in C code or not.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp
index bc14ece3f332c..a8787593da6fe 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp
@@ -711,3 +711,84 @@ void test_local_static_recursion() {
   while (i >= 0)
     p(0); // we don't know what p points to so no warning
 }
+
+struct PairVal {
+  int a;
+  int b;
+  PairVal(int a, int b) : a(a), b(b) {}
+};
+
+void structured_binding_infinite_loop1() {
+  auto [x, y] = PairVal(0, 0);
+  while (x < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop]
+    y++;
+  }
+  while (y < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (y) are updated in the loop body [bugprone-infinite-loop]
+    x++;
+  }
+}
+
+void structured_binding_infinite_loop2() {
+  auto [x, y] = PairVal(0, 0);
+  while (x < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop]
+    // No update to x or y
+  }
+  while (y < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (y) are updated in the loop body [bugprone-infinite-loop]
+    // No update to x or y
+  }
+}
+
+void structured_binding_not_infinite1() {
+  auto [x, y] = PairVal(0, 0);
+  while (x < 10) {
+    x++;
+  }
+  while (y < 10) {
+    y++;
+  }
+}
+
+void volatile_structured_binding_in_condition() {
+  volatile auto [x, y] = PairVal(0, 0);
+  while (!x) {}
+}
+
+void test_local_static_structured_binding_recursion() {
+  static auto [i, _] = PairVal(0, 0);
+  int j = 0;
+
+  i--;
+  while (i >= 0)
+    test_local_static_structured_binding_recursion(); // no warning, recursively decrement i
+  for (; i >= 0;)
+    test_local_static_structured_binding_recursion(); // no warning, recursively decrement i
+  for (; i + j >= 0;)
+    test_local_static_structured_binding_recursion(); // no warning, recursively decrement i
+  for (; i >= 0; i--)
+    ; // no warning, i decrements
+  while (j >= 0)
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (j) are updated in the loop body [bugprone-infinite-loop]
+    test_local_static_structured_binding_recursion();
+
+  int (*p)(int) = 0;
+
+  while (i >= 0)
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+    p = 0;
+  while (i >= 0)
+    p(0); // we don't know what p points to so no warning
+}
+
+struct S { int a; };
+void issue_138842_reduced() {
+    int x = 10;
+    auto [y] = S{1};
+
+    while (y < x) {
+      y++;
+    }
+}

@flovent flovent changed the title [clang-tidy] Improve bugprone-infinite-loop check by adding handing for strucuted bindings [clang-tidy] Improve bugprone-infinite-loop check by adding handing for structured bindings Jun 14, 2025
@flovent
Copy link
Contributor Author

flovent commented Jun 14, 2025

New CI failure seems unrelated to the new commit.

@flovent
Copy link
Contributor Author

flovent commented Jun 23, 2025

ping.

Copy link
Contributor

@vbvictor vbvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add tests with:

  • structured binding coming from type-dependent template struct, like
template <typename T, typename U>
void foo(T a, U b) {
    auto [c, d] = std::pair<T, U>(a,b);
}
  • auto& and auto&&

Copy link
Contributor

@vbvictor vbvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like to wait for a second reviewer for some time to double-check if some tests are missing

Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except nit

Comment on lines 91 to 100
return true;

if (DD->getType().isVolatileQualified())
return true;

if (!BD->getType().getTypePtr()->isIntegerType())
return true;

return hasPtrOrReferenceInFunc(Func, BD) ||
isChanged(LoopStmt, BD, Context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid duplicated code with VarDecl part. extract them to separated function

Copy link
Contributor Author

@flovent flovent Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 266d228

Copy link
Contributor

@carlosgalvezp carlosgalvezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please rebase onto latest trunk and address/resolve pending comments.

@flovent flovent force-pushed the tidy-issue-138842 branch from 83bdeeb to 87a68f7 Compare July 17, 2025 13:33
@aviralg
Copy link

aviralg commented Jul 19, 2025

LGTM. I tried to address the specific case of tuple structured bindings in my PR: #147410. Your PR handles the case for all structured bindings. It may be a good idea to add tests for tuple (you can lift from my PR) and array structured bindings too.

@flovent flovent force-pushed the tidy-issue-138842 branch from 87a68f7 to 9db0df2 Compare July 19, 2025 01:16
@flovent
Copy link
Contributor Author

flovent commented Jul 19, 2025

LGTM. I tried to address the specific case of tuple structured bindings in my PR: #147410. Your PR handles the case for all structured bindings. It may be a good idea to add tests for tuple (you can lift from my PR) and array structured bindings too.

Thanks for the review and advice, these testcases have now been added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy] bugprone-infinite-loop false positive with structured binding
6 participants