Fix a bug with the hasAncestor AST matcher when a node has several parents without pointer identity

[loic-joly-sonarsource]

Before the change, getMemoizationData is used as a key to stop visiting the parents, but if a node has no identity, this is nullptr, which means that the visit will stop as soon as a second node with no identity is visited.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Loïc Joly (loic-joly-sonarsource)

Changes

Before the change, getMemoizationData is used as a key to stop visiting the parents, but if a node has no identity, this is nullptr, which means that the visit will stop as soon as a second node with no identity is visited.

---

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

2 Files Affected:

• (modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+2-1)
• (modified) clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp (+18)

diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 3d01a70395a9bb..a7ef5ae6949537 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -1237,7 +1237,8 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
           // Make sure we do not visit the same node twice.
           // Otherwise, we'll visit the common ancestors as often as there
           // are splits on the way down.
-          if (Visited.insert(Parent.getMemoizationData()).second)
+          if (Parent.getMemoizationData() == nullptr ||
+            Visited.insert(Parent.getMemoizationData()).second)
             Queue.push_back(Parent);
         }
         Queue.pop_front();
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 1d18869a6b8afc..17b058e4783018 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -5536,6 +5536,24 @@ TEST(HasAncestor, MatchesInImplicitCode) {
         hasAncestor(recordDecl(hasName("A")))))))));
 }
 
+TEST(HasAncestor, MatchesWithMultipleParentsWithoutPointerIdentity) {
+    EXPECT_TRUE(matches(
+    R"cpp(
+template <int i> class Fact {};
+template <class T> class W {};
+template <class T> struct A
+{
+    static void f() {
+        W<Fact<12>> fact12;
+    }
+};
+void f() {
+    A<int>::f();
+    A<double>::f();
+})cpp",
+    integerLiteral(hasAncestor(functionDecl()))));
+}
+
 TEST(HasParent, MatchesOnlyParent) {
   EXPECT_TRUE(matches(
     "void f() { if (true) { int x = 42; } }",

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[PiotrZSL]

In general change looks reasonable.

[PiotrZSL]

Note: Avoid double calls to getMemoizationData (assign to some local variable and use or write), but still call to getMemoizationData should be in theory optimized.

[loic-joly-sonarsource]

Since this is simply an if and taking an address, I assumed it would be optimized. But I applied the suggested change.

[loic-joly-sonarsource]

Hi there. I do not have commit access, but this PR had one approval. Could someone with commit access merge it?