[clang-tidy] bugprone-infinite-loop: Add support for tuple structured bindings

Tuple structured bindings introduce implicit variable declarations under
`BindingDecl` nodes which are currently ignored by the infinite loop checker.
This PR adds support for these bindings to this checker through the following
changes:

1. The pattern matcher in `ExprMutationAnalyzer` has been updated to match
against `DeclRefExpr` nodes that refer to these implicit variables via
`BindingDecl` nodes.

2. Enumeration of a loop's condition's variables for mutation analysis has been
updated to recognize these implicit variables so they can be checked for
mutation.

3. Enumeration of the names of a loop's condition's variables for error
reporting has been similarly updated.

The changes have been tested against a mock tuple implementation lifted from
`clang/unittests/Analysis/FlowSensitive/TransferTest.cpp`

Author: aviralg

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

@@ -64,24 +64,53 @@ static bool isChanged(const Stmt *LoopStmt, const VarDecl *Var,
   return ExprMutationAnalyzer(*LoopStmt, *Context).isMutated(Var);
 }
 
+bool isVolatileOrNonIntegerType(QualType QT) {
+
+  if (QT.isVolatileQualified())
+    return true;
+
+  const Type *T = QT.getTypePtr();
+
+  if (T->isIntegerType())
+    return false;
+
+  if (T->isRValueReferenceType()) {
+    QT = dyn_cast<RValueReferenceType>(T)->getPointeeType();
+    return isVolatileOrNonIntegerType(QT);
+  }
+
+  return true;
+}
+
+static bool isVarDeclPossiblyChanged(const Decl *Func, const Stmt *LoopStmt,
+                                     const VarDecl *Var, ASTContext *Context) {
+  if (!Var->isLocalVarDeclOrParm())
+    return true;
+
+  if (isVolatileOrNonIntegerType(Var->getType()))
+    return true;
+
+  return hasPtrOrReferenceInFunc(Func, Var) ||
+         isChanged(LoopStmt, Var, Context);
+}
+
 /// 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;
+    const ValueDecl *VD = DRE->getDecl();
 
-      if (Var->getType().isVolatileQualified())
-        return true;
-
-      if (!Var->getType().getTypePtr()->isIntegerType())
-        return true;
+    if (const auto *Var = dyn_cast<VarDecl>(VD)) {
+      return isVarDeclPossiblyChanged(Func, LoopStmt, Var, Context);
+    }
 
-      return hasPtrOrReferenceInFunc(Func, Var) ||
-             isChanged(LoopStmt, Var, Context);
-      // FIXME: Track references.
+    if (const auto *BD = dyn_cast<BindingDecl>(VD)) {
+      if (const auto *Var = BD->getHoldingVar()) {
+        return isVarDeclPossiblyChanged(Func, LoopStmt, Var, Context);
+      }
     }
+
+    // FIXME: Track references.
   } else if (isa<MemberExpr, CallExpr, ObjCIvarRefExpr, ObjCPropertyRefExpr,
                  ObjCMessageExpr>(Cond)) {
     // FIXME: Handle MemberExpr.
@@ -121,8 +150,16 @@ static bool isAtLeastOneCondVarChanged(const Decl *Func, const Stmt *LoopStmt,
 /// Return the variable names in `Cond`.
 static std::string getCondVarNames(const Stmt *Cond) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) {
-    if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl()))
+    const ValueDecl *VD = DRE->getDecl();
+
+    if (const auto *Var = dyn_cast<VarDecl>(VD))
       return std::string(Var->getName());
+
+    if (const auto *BD = dyn_cast<BindingDecl>(VD)) {
+      if (const auto *Var = BD->getHoldingVar()) {
+        return Var->getName().str();
+      }
+    }
   }
 
   std::string Result;

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

@@ -592,6 +592,53 @@ void test_structured_bindings_bad() {
   }
 }
 
+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;
+  }
+}
+
 void test_volatile_cast() {
   // This is a no-op cast. Clang ignores the qualifier, we should too.
   for (int i = 0; (volatile int)i < 10;) {

clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h

@@ -54,6 +54,8 @@ class ExprMutationAnalyzer {
     const Stmt *findMutationMemoized(const Expr *Exp,
                                      llvm::ArrayRef<MutationFinder> Finders,
                                      Memoized::ResultMap &MemoizedResults);
+    const ast_matchers::internal::BindableMatcher<Stmt>
+    makeDeclRefExprMatcher(const Decl *Dec);
     const Stmt *tryEachDeclRef(const Decl *Dec, MutationFinder Finder);
 
     const Stmt *findExprMutation(ArrayRef<ast_matchers::BoundNodes> Matches);

clang/lib/Analysis/ExprMutationAnalyzer.cpp

@@ -113,6 +113,14 @@ AST_MATCHER_P(Stmt, canResolveToExpr, const Stmt *, Inner) {
   return canExprResolveTo(Exp, Target);
 }
 
+AST_MATCHER_P(BindingDecl, hasHoldingVar,
+              ast_matchers::internal::Matcher<VarDecl>, InnerMatcher) {
+  if (const VarDecl *HoldingVar = Node.getHoldingVar()) {
+    return InnerMatcher.matches(*HoldingVar, Finder, Builder);
+  }
+  return false;
+}
+
 // use class member to store data can reduce stack usage to avoid stack overflow
 // when recursive call.
 class ExprPointeeResolve {
@@ -310,21 +318,30 @@ const Stmt *ExprMutationAnalyzer::Analyzer::findMutationMemoized(
   return nullptr;
 }
 
+const ast_matchers::internal::BindableMatcher<Stmt>
+ExprMutationAnalyzer::Analyzer::makeDeclRefExprMatcher(const Decl *Dec) {
+
+  // For VarDecl created implicitly via structured bindings, create a matcher
+  // for DeclRefExpr nodes which refer to this VarDecl via BindingDecl nodes.
+  if (const auto *VD = dyn_cast<VarDecl>(Dec)) {
+    if (VD->isImplicit()) {
+      return declRefExpr(to(bindingDecl(hasHoldingVar(equalsNode(VD)))));
+    }
+  }
+
+  return declRefExpr(to(
+      // `Dec` or a binding if `Dec` is a decomposition.
+      anyOf(equalsNode(Dec), bindingDecl(forDecomposition(equalsNode(Dec))))));
+}
+
 const Stmt *
 ExprMutationAnalyzer::Analyzer::tryEachDeclRef(const Decl *Dec,
                                                MutationFinder Finder) {
-  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);
+  const auto matcher = makeDeclRefExprMatcher(Dec);
+  const auto nodeId = NodeID<Expr>::value;
+  const auto Refs = match(findAll(matcher.bind(nodeId)), Stm, Context);
   for (const auto &RefNodes : Refs) {
-    const auto *E = RefNodes.getNodeAs<Expr>(NodeID<Expr>::value);
+    const auto *E = RefNodes.getNodeAs<Expr>(nodeId);
     if ((this->*Finder)(E))
       return E;
   }