[clang] [clang-tools-extra] [clang-tidy] bugprone-infinite-loop: Add support for tuple structured bindings (PR #147410)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 16 12:11:25 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Aviral Goel (aviralg)
<details>
<summary>Changes</summary>
Tuple structured bindings can introduce new variable declarations under `BindingDecl` nodes which are currently ignored by the infinite loop checker. This PR implements support for extracting variables from these nodes. Subsequent logic for checking if these variables have changed has been reused.
---
Full diff: https://github.com/llvm/llvm-project/pull/147410.diff
4 Files Affected:
- (modified) clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp (+49-12)
- (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp (+47)
- (modified) clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h (+2)
- (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+28-11)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
index 3c3024d538785..a0011eab4251b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ b/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;
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..0895b91f77d9b 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
@@ -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;) {
diff --git a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
index 3344959072c22..441bc56b29de8 100644
--- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
+++ b/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);
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 823d7543f085f..fb36d9a4cea64 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/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;
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/147410
More information about the cfe-commits
mailing list