[clang] 04a02c1 - [ASTMatchers] forCallable should not erase binding on success (#89657)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 5 01:20:40 PDT 2024
Author: Marco Borgeaud
Date: 2024-06-05T10:20:37+02:00
New Revision: 04a02c1b88cfb8445ce962ba5ce496401a3e27fe
URL: https://github.com/llvm/llvm-project/commit/04a02c1b88cfb8445ce962ba5ce496401a3e27fe
DIFF: https://github.com/llvm/llvm-project/commit/04a02c1b88cfb8445ce962ba5ce496401a3e27fe.diff
LOG: [ASTMatchers] forCallable should not erase binding on success (#89657)
Do not erase Builder when the first check fails because it could succeed
on the second stack frame.
The problem was that `InnerMatcher.matches` erases the bindings when it
returns false. The appropriate solution is to pass a copy of the
bindings, similar to what `matchesFirstInRange` does.
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 39a9013c75a41..deb1560b781ee 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -996,6 +996,7 @@ AST Matchers
- Fixed ``forEachArgumentWithParam`` and ``forEachArgumentWithParamType`` to
not skip the explicit object parameter for operator calls.
- Fixed captureVars assertion failure if not capturesVariables. (#GH76425)
+- ``forCallable`` now properly preserves binding on successful match. (#GH89657)
clang-format
------------
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 0f3257db6f415..ca44c3ee08565 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -8371,20 +8371,28 @@ AST_MATCHER_P(Stmt, forCallable, internal::Matcher<Decl>, InnerMatcher) {
const auto &CurNode = Stack.back();
Stack.pop_back();
if (const auto *FuncDeclNode = CurNode.get<FunctionDecl>()) {
- if (InnerMatcher.matches(*FuncDeclNode, Finder, Builder)) {
+ BoundNodesTreeBuilder B = *Builder;
+ if (InnerMatcher.matches(*FuncDeclNode, Finder, &B)) {
+ *Builder = std::move(B);
return true;
}
} else if (const auto *LambdaExprNode = CurNode.get<LambdaExpr>()) {
+ BoundNodesTreeBuilder B = *Builder;
if (InnerMatcher.matches(*LambdaExprNode->getCallOperator(), Finder,
- Builder)) {
+ &B)) {
+ *Builder = std::move(B);
return true;
}
} else if (const auto *ObjCMethodDeclNode = CurNode.get<ObjCMethodDecl>()) {
- if (InnerMatcher.matches(*ObjCMethodDeclNode, Finder, Builder)) {
+ BoundNodesTreeBuilder B = *Builder;
+ if (InnerMatcher.matches(*ObjCMethodDeclNode, Finder, &B)) {
+ *Builder = std::move(B);
return true;
}
} else if (const auto *BlockDeclNode = CurNode.get<BlockDecl>()) {
- if (InnerMatcher.matches(*BlockDeclNode, Finder, Builder)) {
+ BoundNodesTreeBuilder B = *Builder;
+ if (InnerMatcher.matches(*BlockDeclNode, Finder, &B)) {
+ *Builder = std::move(B);
return true;
}
} else {
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index af99c73f1945f..47a71134d5027 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -5973,6 +5973,37 @@ TEST(StatementMatcher, ForCallable) {
EXPECT_TRUE(notMatches(CppString2,
returnStmt(forCallable(functionDecl(hasName("F"))))));
+ StringRef CodeWithDeepCallExpr = R"cpp(
+void Other();
+void Function() {
+ {
+ (
+ Other()
+ );
+ }
+}
+)cpp";
+ auto ForCallableFirst =
+ callExpr(forCallable(functionDecl(hasName("Function"))),
+ callee(functionDecl(hasName("Other")).bind("callee")))
+ .bind("call");
+ auto ForCallableSecond =
+ callExpr(callee(functionDecl(hasName("Other")).bind("callee")),
+ forCallable(functionDecl(hasName("Function"))))
+ .bind("call");
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ CodeWithDeepCallExpr, ForCallableFirst,
+ std::make_unique<VerifyIdIsBoundTo<CallExpr>>("call")));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ CodeWithDeepCallExpr, ForCallableFirst,
+ std::make_unique<VerifyIdIsBoundTo<FunctionDecl>>("callee")));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ CodeWithDeepCallExpr, ForCallableSecond,
+ std::make_unique<VerifyIdIsBoundTo<CallExpr>>("call")));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ CodeWithDeepCallExpr, ForCallableSecond,
+ std::make_unique<VerifyIdIsBoundTo<FunctionDecl>>("callee")));
+
// These tests are specific to forCallable().
StringRef ObjCString1 = "@interface I"
"-(void) foo;"
@@ -6014,6 +6045,105 @@ TEST(StatementMatcher, ForCallable) {
binaryOperator(forCallable(blockDecl()))));
}
+namespace {
+class ForCallablePreservesBindingWithMultipleParentsTestCallback
+ : public BoundNodesCallback {
+public:
+ bool run(const BoundNodes *BoundNodes) override {
+ FunctionDecl const *FunDecl =
+ BoundNodes->getNodeAs<FunctionDecl>("funDecl");
+ // Validate test assumptions. This would be expressed as ASSERT_* in
+ // a TEST().
+ if (!FunDecl) {
+ EXPECT_TRUE(false && "Incorrect test setup");
+ return false;
+ }
+ auto const *FunDef = FunDecl->getDefinition();
+ if (!FunDef || !FunDef->getBody() ||
+ FunDef->getNameAsString() != "Function") {
+ EXPECT_TRUE(false && "Incorrect test setup");
+ return false;
+ }
+
+ ExpectCorrectResult(
+ "Baseline",
+ callExpr(callee(cxxMethodDecl().bind("callee"))).bind("call"), //
+ FunDecl);
+
+ ExpectCorrectResult("ForCallable first",
+ callExpr(forCallable(equalsNode(FunDecl)),
+ callee(cxxMethodDecl().bind("callee")))
+ .bind("call"),
+ FunDecl);
+
+ ExpectCorrectResult("ForCallable second",
+ callExpr(callee(cxxMethodDecl().bind("callee")),
+ forCallable(equalsNode(FunDecl)))
+ .bind("call"),
+ FunDecl);
+
+ // This value does not really matter: the EXPECT_* will set the exit code.
+ return true;
+ }
+
+ bool run(const BoundNodes *BoundNodes, ASTContext *Context) override {
+ return run(BoundNodes);
+ }
+
+private:
+ void ExpectCorrectResult(StringRef LogInfo,
+ ArrayRef<BoundNodes> Results) const {
+ EXPECT_EQ(Results.size(), 1u) << LogInfo;
+ if (Results.empty())
+ return;
+ auto const &R = Results.front();
+ EXPECT_TRUE(R.getNodeAs<CallExpr>("call")) << LogInfo;
+ EXPECT_TRUE(R.getNodeAs<CXXMethodDecl>("callee")) << LogInfo;
+ }
+
+ template <typename MatcherT>
+ void ExpectCorrectResult(StringRef LogInfo, MatcherT Matcher,
+ FunctionDecl const *FunDef) const {
+ auto &Context = FunDef->getASTContext();
+ auto const &Results = match(findAll(Matcher), *FunDef->getBody(), Context);
+ ExpectCorrectResult(LogInfo, Results);
+ }
+};
+} // namespace
+
+TEST(StatementMatcher, ForCallablePreservesBindingWithMultipleParents) {
+ // Tests in this file are fairly simple and therefore can rely on matches,
+ // matchAndVerifyResultTrue, etc. This test, however, needs a FunctionDecl* in
+ // order to call equalsNode in order to reproduce the observed issue (bindings
+ // being removed despite forCallable matching the node).
+ //
+ // Because of this and because the machinery to compile the code into an
+ // ASTUnit is not exposed outside matchAndVerifyResultConditionally, it is
+ // cheaper to have a custom BoundNodesCallback for the purpose of this test.
+ StringRef codeWithTemplateFunction = R"cpp(
+struct Klass {
+ void Method();
+ template <typename T>
+ void Function(T t); // Declaration
+};
+
+void Instantiate(Klass k) {
+ k.Function(0);
+}
+
+template <typename T>
+void Klass::Function(T t) { // Definition
+ // Compound statement has two parents: the declaration and the definition.
+ Method();
+}
+)cpp";
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ codeWithTemplateFunction,
+ callExpr(callee(functionDecl(hasName("Function")).bind("funDecl"))),
+ std::make_unique<
+ ForCallablePreservesBindingWithMultipleParentsTestCallback>()));
+}
+
TEST(Matcher, ForEachOverriden) {
const auto ForEachOverriddenInClass = [](const char *ClassName) {
return cxxMethodDecl(ofClass(hasName(ClassName)), isVirtual(),
More information about the cfe-commits
mailing list