Fix hasName matcher assertion with inline namespaces

[njames93]

Fix handling of nodes which can be skipped in the fast path for the hasName matcher

#100973

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nathan James (njames93)

Changes

Fix handling of nodes which can be skipped in the fast path for the hasName matcher

#100973

---

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

2 Files Affected:

• (modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+14-7)
• (modified) clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (+4)

diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index bf87b1aa0992a..0556ae7ffae2f 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -537,14 +537,21 @@ class PatternSet {
   /// that didn't match.
   /// Return true if there are still any patterns left.
   bool consumeNameSuffix(StringRef NodeName, bool CanSkip) {
-    for (size_t I = 0; I < Patterns.size();) {
-      if (::clang::ast_matchers::internal::consumeNameSuffix(Patterns[I].P,
-                                                             NodeName) ||
-          CanSkip) {
-        ++I;
-      } else {
-        Patterns.erase(Patterns.begin() + I);
+    if (CanSkip) {
+      // If we can skip the node, then we need to handle the case where a
+      // skipped node has the same name as its parent.
+      // namespace a { inline namespace a { class A; } }
+      // cxxRecordDecl(hasName("::a::A")) 
+      // To do this, any patterns that match should be duplicated in our set, one of them with the tail removed.
+      for (size_t I = 0, E = Patterns.size(); I != E; ++I) {
+        StringRef Pattern = Patterns[I].P;
+        if (ast_matchers::internal::consumeNameSuffix(Patterns[I].P, NodeName))
+          Patterns.push_back({Pattern, Patterns[I].IsFullyQualified});
       }
+    } else {
+      llvm::erase_if(Patterns, [&NodeName](auto &Pattern) {
+        return !::clang::ast_matchers::internal::consumeNameSuffix(Pattern.P, NodeName);
+      });
     }
     return !Patterns.empty();
   }
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
index f26140675fd46..611e1f9ba5327 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2552,6 +2552,10 @@ TEST_P(ASTMatchersTest, HasName_MatchesNamespaces) {
                          recordDecl(hasName("a+b::C"))));
   EXPECT_TRUE(notMatches("namespace a { namespace b { class AC; } }",
                          recordDecl(hasName("C"))));
+  EXPECT_TRUE(matches("namespace a { inline namespace a { class C; } }",
+                      recordDecl(hasName("::a::C"))));
+  EXPECT_TRUE(matches("namespace a { inline namespace a { class C; } }",
+                      recordDecl(hasName("::a::a::C"))));
 }
 
 TEST_P(ASTMatchersTest, HasName_MatchesOuterClasses) {

[AaronBallman]

I can't get the assertion to reproduce, but I can see the behavioral change with the first matcher: https://godbolt.org/z/TcsYPoGEh

So this should probably come with a release note for the fix?

[njames93]

I can't get the assertion to reproduce, but I can see the behavioral change with the first matcher: https://godbolt.org/z/TcsYPoGEh

So this should probably come with a release note for the fix?

The assertions are only shown on debug build, but it comes from here

Which section in the release notes do changes to AST matchers belong in

[AaronBallman]

I can't get the assertion to reproduce, but I can see the behavioral change with the first matcher: https://godbolt.org/z/TcsYPoGEh
So this should probably come with a release note for the fix?

The assertions are only shown on debug build, but it comes from here

That should still trigger with an assertions build though, right?

Which section in the release notes do changes to AST matchers belong in

Somewhere around here:

llvm-project/clang/docs/ReleaseNotes.rst

Line 247 in 77655f4

AST Matchers

[AaronBallman]

LGTM!