From c2a164d8377e0856f3dee69d85075ee9898dbf52 Mon Sep 17 00:00:00 2001 From: flovent Date: Tue, 22 Jul 2025 22:10:40 +0800 Subject: [PATCH] [CherryPick][clang-tidy] Improve `bugprone-infinite-loop` check by adding handing for structured bindings (#144213) 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 --- .../clang-tidy/bugprone/InfiniteLoopCheck.cpp | 58 +++-- .../clang-tidy/utils/Aliasing.cpp | 14 +- clang-tools-extra/clang-tidy/utils/Aliasing.h | 2 +- clang-tools-extra/docs/ReleaseNotes.rst | 4 + .../checkers/bugprone/infinite-loop.cpp | 202 ++++++++++++++++++ 5 files changed, 254 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp index e329588290cd4..03af38e7c3de1 100644 --- a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp @@ -50,7 +50,7 @@ loopEndingStmt(internal::Matcher 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(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(VD)) { + Var = VarD; + } else if (const auto *BD = dyn_cast(VD)) { + if (const auto *DD = dyn_cast(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(Cond)) { - if (const auto *Var = dyn_cast(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(Cond)) { + if (const auto *VD = dyn_cast(DRE->getDecl())) + return isVarPossiblyChanged(Func, LoopStmt, VD, Context); + } else if (isa(Cond)) { // FIXME: Handle MemberExpr. return true; } else if (const auto *CE = dyn_cast(Cond)) { @@ -124,6 +135,10 @@ static std::string getCondVarNames(const Stmt *Cond) { if (const auto *DRE = dyn_cast(Cond)) { if (const auto *Var = dyn_cast(DRE->getDecl())) return std::string(Var->getName()); + + if (const auto *BD = dyn_cast(DRE->getDecl())) { + return std::string(BD->getName()); + } } std::string Result; @@ -215,10 +230,17 @@ static bool overlap(ArrayRef SCC, /// returns true iff `Cond` involves at least one static local variable. static bool hasStaticLocalVariable(const Stmt *Cond) { - if (const auto *DRE = dyn_cast(Cond)) + if (const auto *DRE = dyn_cast(Cond)) { if (const auto *VD = dyn_cast(DRE->getDecl())) if (VD->isStaticLocal()) return true; + + if (const auto *BD = dyn_cast(DRE->getDecl())) + if (const auto *DD = dyn_cast(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(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(Var); VD && VD->isEscapingByref()) return true; if (const auto *DS = dyn_cast(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(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 ebcdeca8c2ee5..54692fb7f7cee 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/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 + ` check by adding detection for + variables introduced by structured bindings. + Removed checks ^^^^^^^^^^^^^^ 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..9a58a7ae2f2ab 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,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 +struct pair { + T first; + U second; + + pair(T a, U b) : first(a), second(b) {} +}; +} + +template +void structured_binding_in_template_byval(T a, U b) { + auto [c, d] = std::pair(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 +void structured_binding_in_template_bylref(T a, U b) { + auto p = std::pair(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 +void structured_binding_in_template_byrref(T a, U b) { + auto p = std::pair(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 struct tuple_size; + template struct tuple_element; + template class tuple; + +namespace { + template + struct size_helper { static const T value = v; }; +} // namespace + +template +struct tuple_size> : size_helper {}; + +template +struct tuple_element> { + using type = __type_pack_element; +}; + +template class tuple {}; + +template +typename tuple_element>::type get(tuple); +} // namespace std + +std::tuple &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; + } +}