[CherryPick][clang-tidy] Improve bugprone-infinite-loop check by adding handing for structured bindings (llvm#144213) (#11104)

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.

rdar://152083639

Co-authored-by: flovent <[email protected]>

Author: aviralg

clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp

@@ -50,7 +50,7 @@ loopEndingStmt(internal::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() &&
@@ -65,26 +65,37 @@ static bool isChanged(const Stmt *LoopStmt, const VarDecl *Var,
   return ExprMutationAnalyzer(*LoopStmt, *Context).isMutated(Var);
 }
 
+static bool isVarPossiblyChanged(const Decl *Func, const Stmt *LoopStmt,
+                                 const ValueDecl *VD, ASTContext *Context) {
+  const VarDecl *Var = nullptr;
+  if (const auto *VarD = dyn_cast<VarDecl>(VD)) {
+    Var = VarD;
+  } else if (const auto *BD = dyn_cast<BindingDecl>(VD)) {
+    if (const auto *DD = dyn_cast<DecompositionDecl>(BD->getDecomposedDecl()))
+      Var = DD;
+  }
+
+  if (!Var)
+    return false;
+
+  if (!Var->isLocalVarDeclOrParm() || Var->getType().isVolatileQualified())
+    return true;
+
+  if (!VD->getType().getTypePtr()->isIntegerType())
+    return true;
+
+  return hasPtrOrReferenceInFunc(Func, VD) || isChanged(LoopStmt, VD, Context);
+  // FIXME: Track references.
+}
+
 /// Return whether `Cond` is a variable that is possibly changed in `LoopStmt`.
 static bool isVarThatIsPossiblyChanged(const Decl *Func, const Stmt *LoopStmt,
                                        const Stmt *Cond, ASTContext *Context) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) {
-    if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl())) {
-      if (!Var->isLocalVarDeclOrParm())
-        return true;
-
-      if (Var->getType().isVolatileQualified())
-        return true;
-
-      if (!Var->getType().getTypePtr()->isIntegerType())
-        return true;
-
-      return hasPtrOrReferenceInFunc(Func, Var) ||
-             isChanged(LoopStmt, Var, Context);
-      // FIXME: Track references.
-    }
-  } else if (isa<MemberExpr, CallExpr,
-                 ObjCIvarRefExpr, ObjCPropertyRefExpr, ObjCMessageExpr>(Cond)) {
+    if (const auto *VD = dyn_cast<ValueDecl>(DRE->getDecl()))
+      return isVarPossiblyChanged(Func, LoopStmt, VD, Context);
+  } else if (isa<MemberExpr, CallExpr, ObjCIvarRefExpr, ObjCPropertyRefExpr,
+                 ObjCMessageExpr>(Cond)) {
     // FIXME: Handle MemberExpr.
     return true;
   } else if (const auto *CE = dyn_cast<CastExpr>(Cond)) {
@@ -124,6 +135,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 +230,17 @@ 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<DecompositionDecl>(BD->getDecomposedDecl()))
+        if (DD->isStaticLocal())
+          return true;
+  }
+
   for (const Stmt *Child : Cond->children())
     if (Child && hasStaticLocalVariable(Child))
       return true;

clang-tools-extra/clang-tidy/utils/Aliasing.cpp

@@ -14,24 +14,24 @@
 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;
   });
 }
 
 /// 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);
 }

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
 

clang-tools-extra/docs/ReleaseNotes.rst

@@ -564,6 +564,10 @@ Changes in existing checks
   usages of ``std::string_view::compare``. Added a `StringLikeClasses` option
   to detect usages of ``compare`` method in custom string-like classes.
 
+- Improved :doc:`bugprone-infinite-loop
+  <clang-tidy/checks/bugprone/infinite-loop>` check by adding detection for
+  variables introduced by structured bindings.
+
 Removed checks
 ^^^^^^^^^^^^^^
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp

@@ -711,3 +711,205 @@ 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++;
+    }
+}
+
+namespace std {
+template <typename T, typename U>
+struct pair {
+  T first;
+  U second;
+
+  pair(T a, U b) : first(a), second(b) {}
+};
+}
+
+template <typename T, typename U>
+void structured_binding_in_template_byval(T a, U b) {
+  auto [c, d] = std::pair<T, U>(a,b);
+
+  while (c < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (c) are updated in the loop body [bugprone-infinite-loop]
+    d++;
+  }
+
+  while (c < 10) {
+    c++; // no warning
+  }
+}
+
+template <typename T, typename U>
+void structured_binding_in_template_bylref(T a, U b) {
+  auto p = std::pair<T, U>(a,b);
+  auto& [c, d] = p;
+
+  while (c < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (c) are updated in the loop body [bugprone-infinite-loop]
+    d++;
+  }
+
+  while (c < 10) {
+    c++; // no warning
+  }
+}
+
+template <typename T, typename U>
+void structured_binding_in_template_byrref(T a, U b) {
+  auto p = std::pair<T, U>(a,b);
+  auto&& [c, d] = p;
+
+  while (c < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (c) are updated in the loop body [bugprone-infinite-loop]
+    d++;
+  }
+
+  while (c < 10) {
+    c++; // no warning
+  }
+}
+
+void structured_binding_in_template_instantiation(int b) {
+  structured_binding_in_template_byval(b, 0);
+  structured_binding_in_template_bylref(b, 0);
+  structured_binding_in_template_byrref(b, 0);
+}
+
+void array_structured_binding() {
+  int arr[2] = {0, 0};
+  auto [x, y] = arr;
+
+  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) {
+    y++; // no warning
+  }
+}
+
+namespace std {
+    using size_t = int;
+    template <class> struct tuple_size;
+    template <std::size_t, class> struct tuple_element;
+    template <class...> class tuple;
+
+namespace {
+    template <class T, T v>
+    struct size_helper { static const T value = v; };
+} // namespace
+
+template <class... T>
+struct tuple_size<tuple<T...>> : size_helper<std::size_t, sizeof...(T)> {};
+
+template <std::size_t I, class... T>
+struct tuple_element<I, tuple<T...>> {
+    using type =  __type_pack_element<I, T...>;
+};
+
+template <class...> class tuple {};
+
+template <std::size_t I, class... T>
+typename tuple_element<I, tuple<T...>>::type get(tuple<T...>);
+} // namespace std
+
+std::tuple<int*, int> &get_chunk();
+
+void test_structured_bindings_tuple() {
+  auto [buffer, size ] = get_chunk();
+  int maxLen = 8;
+
+  while (size < maxLen) {
+    // No warning. The loop is finite because 'size' is being incremented in each iteration and compared against 'maxLen' for termination
+    buffer[size++] = 2;
+  }
+}
+
+void test_structured_bindings_tuple_ref() {
+  auto& [buffer, size ] = get_chunk();
+  int maxLen = 8;
+
+  while (size < maxLen) {
+    // No warning. The loop is finite because 'size' is being incremented in each iteration and compared against 'maxLen' for termination
+    buffer[size++] = 2;
+  }
+}