Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][ASTMatcher] fix hasAnyBase not binding submatchers #67939

Merged

Conversation

5chmidti
Copy link
Contributor

@5chmidti 5chmidti commented Oct 1, 2023

The BoundNodesTreeBuilder used in the BaseSpecMatcher was the original
and was reset to its original state if a match occurred.
The matcher now uses the local copy in the inner matcher.

Fixes #65421

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Oct 1, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 1, 2023

@llvm/pr-subscribers-clang

Changes

The BoundNodesTreeBuilder used in the BaseSpecMatcher was the original
and was reset to its original state if a match occurred.
The matcher now uses the local copy in the inner matcher.

Fixes #65421


Full diff: https://github.com/llvm/llvm-project/pull/67939.diff

2 Files Affected:

  • (modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+1-1)
  • (modified) clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp (+13)
diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 40688107215f287..435bbdeda22066e 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -87,7 +87,7 @@ bool matchesAnyBase(const CXXRecordDecl &Node,
       [Finder, Builder, &BaseSpecMatcher](const CXXBaseSpecifier *BaseSpec,
                                           CXXBasePath &IgnoredParam) {
         BoundNodesTreeBuilder Result(*Builder);
-        if (BaseSpecMatcher.matches(*BaseSpec, Finder, Builder)) {
+        if (BaseSpecMatcher.matches(*BaseSpec, Finder, &Result)) {
           *Builder = std::move(Result);
           return true;
         }
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 89954711804aa57..d4a695b974bf0e5 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -8,6 +8,7 @@
 
 #include "ASTMatchersTest.h"
 #include "clang/AST/Attrs.inc"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
@@ -5457,6 +5458,18 @@ TEST(HasParent, NoDuplicateParents) {
     stmt().bind("node"), std::make_unique<HasDuplicateParents>()));
 }
 
+TEST(HasAnyBase, BindsInnerBoundNodes) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "struct Inner {}; struct Proxy : Inner {}; struct Main : public "
+      "Proxy {};",
+      cxxRecordDecl(hasName("Main"),
+                    hasAnyBase(cxxBaseSpecifier(hasType(
+                        cxxRecordDecl(hasName("Inner")).bind("base-class")))))
+          .bind("class"),
+      std::make_unique<VerifyIdIsBoundTo<CXXRecordDecl>>("base-class",
+                                                         "Inner")));
+}
+
 TEST(TypeMatching, PointeeTypes) {
   EXPECT_TRUE(matches("int b; int &a = b;",
                       referenceType(pointee(builtinType()))));

The BoundNodesTreeBuilder used in the BaseSpecMatcher was the original
and was reset to its original state if a match occurred.
The matcher now uses the local copy in the inner matcher.

Fixes llvm#65421
@5chmidti 5chmidti force-pushed the clang_ast_matcher_hasAnyBase_inner_binds branch from 8a57465 to d0ac0cd Compare October 3, 2023 21:48
@5chmidti
Copy link
Contributor Author

5chmidti commented Oct 3, 2023

Added a comment to the release notes

@shafik
Copy link
Collaborator

shafik commented Oct 4, 2023

I think you should also add [ASTMatcher] to the subject after [Clang] at least that is what past PRs seem to do.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@5chmidti 5chmidti changed the title [clang] fix hasAnyBase not binding submatchers [clang][ASTMatcher] fix hasAnyBase not binding submatchers Oct 6, 2023
@5chmidti
Copy link
Contributor Author

I think this can be merged, but I don't have write access.

@AaronBallman AaronBallman merged commit 5b07de1 into llvm:main Oct 16, 2023
@AaronBallman
Copy link
Collaborator

I think this can be merged, but I don't have write access.

Ah, thank you for letting me know!

@5chmidti 5chmidti deleted the clang_ast_matcher_hasAnyBase_inner_binds branch November 2, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bindings enclosed within hasAnyBase() don't work
4 participants