[clang-tidy] bugprone-infinite-loop: Add support for tuple structured bindings #147410
+126
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
aviralg
force-pushed
the
users/aviralg/infinite-loop-structured-bindings
branch
from
4978c1a to
674f686
Compare
… 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`
aviralg
force-pushed
the
users/aviralg/infinite-loop-structured-bindings
branch
from
674f686 to
be9fc41
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Aviral Goel (aviralg) ChangesTuple structured bindings can introduce new variable declarations under Full diff: https://github.com/llvm/llvm-project/pull/147410.diff 4 Files Affected:
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;
}
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Aviral Goel (aviralg) ChangesTuple structured bindings can introduce new variable declarations under Full diff: https://github.com/llvm/llvm-project/pull/147410.diff 4 Files Affected:
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;
}
|
|
We are proceeding with #144213. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tuple structured bindings introduce implicit variable declarations under
BindingDeclnodes which are currently ignored by the infinite loop checker.This PR adds support for these bindings to this checker through the following
changes:
The pattern matcher in
ExprMutationAnalyzerhas been updated to matchagainst
DeclRefExprnodes that refer to these implicit variables viaBindingDeclnodes.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.
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