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] Add a matcher for the name of a DependentScopeDeclRefExpr #121656

Conversation

AmrDeveloper
Copy link
Member

Add the hasDependentName matcher to match the name of DependentScopeDeclRefExpr

Example: dependentScopeDeclRefExpr(hasDependentName("name"))

Fixes: #121610

@AmrDeveloper AmrDeveloper added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 4, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 4, 2025

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

Add the hasDependentName matcher to match the name of DependentScopeDeclRefExpr

Example: dependentScopeDeclRefExpr(hasDependentName("name"))

Fixes: #121610


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

5 Files Affected:

  • (modified) clang/docs/LibASTMatchersReference.html (+13)
  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+11)
  • (modified) clang/lib/ASTMatchers/Dynamic/Registry.cpp (+1)
  • (modified) clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (+15)
diff --git a/clang/docs/LibASTMatchersReference.html b/clang/docs/LibASTMatchersReference.html
index fc557888013254..6a03aeb5eec2a1 100644
--- a/clang/docs/LibASTMatchersReference.html
+++ b/clang/docs/LibASTMatchersReference.html
@@ -3449,6 +3449,19 @@ <h2 id="narrowing-matchers">Narrowing Matchers</h2>
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1DependentScopeDeclRefExpr.html">DependentScopeDeclRefExpr</a>&gt;</td><td class="name" onclick="toggle('hasDependentName0')"><a name="hasDependentName0Anchor">hasDependentName</a></td><td>std::string N</td></tr>
+<tr><td colspan="4" class="doc" id="hasDependentName0"><pre>Matches the dependent name of a dependent scope decl ref expr.
+
+Matches the dependent name of a dependent scope decl ref expr
+
+Given:
+
+  template &lt;class T&lt; class X : T { void f() { T::v; } };
+
+dependentScopeDeclRefExpr(hasDependentName("v")) matches `T::v`
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXDependentScopeMemberExpr.html">CXXDependentScopeMemberExpr</a>&gt;</td><td class="name" onclick="toggle('memberHasSameNameAsBoundNode0')"><a name="memberHasSameNameAsBoundNode0Anchor">memberHasSameNameAsBoundNode</a></td><td>std::string BindingID</td></tr>
 <tr><td colspan="4" class="doc" id="memberHasSameNameAsBoundNode0"><pre>Matches template-dependent, but known, member names against an already-bound
 node
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5e75fc447636e0..4ef69ca4c743ed 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1116,6 +1116,8 @@ AST Matchers
 
 - Add ``dependentTemplateSpecializationType`` matcher to match a dependent template specialization type.
 
+- Add ``hasDependentName`` matcher to match the dependent name of a dependent scope decl ref expr.
+
 clang-format
 ------------
 
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index dd0fedb2cda2d4..6828fc6da1d5cf 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -3257,6 +3257,17 @@ AST_MATCHER_P(CXXDependentScopeMemberExpr, memberHasSameNameAsBoundNode,
       });
 }
 
+/// Matches the dependent name of a dependent scope decl ref expr
+///
+/// Given:
+/// \code
+///  template <class T> class X : T { void f() { T::v; } };
+/// \endcode
+/// \c dependentScopeDeclRefExpr(hasDependentName("v")) matches `T::v`
+AST_MATCHER_P(DependentScopeDeclRefExpr, hasDependentName, std::string, N) {
+  return Node.getDeclName().getAsString() == N;
+}
+
 /// Matches C++ classes that are directly or indirectly derived from a class
 /// matching \c Base, or Objective-C classes that directly or indirectly
 /// subclass a class matching \c Base.
diff --git a/clang/lib/ASTMatchers/Dynamic/Registry.cpp b/clang/lib/ASTMatchers/Dynamic/Registry.cpp
index 97e6bbc093fe46..336d3a14f79559 100644
--- a/clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ b/clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -314,6 +314,7 @@ RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(hasDeducedType);
   REGISTER_MATCHER(hasDefaultArgument);
   REGISTER_MATCHER(hasDefinition);
+  REGISTER_MATCHER(hasDependentName);
   REGISTER_MATCHER(hasDescendant);
   REGISTER_MATCHER(hasDestinationType);
   REGISTER_MATCHER(hasDirectBase);
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
index 056b7c7b571ef4..4278e3d4fe5959 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2235,6 +2235,21 @@ TEST_P(ASTMatchersTest, ArgumentCountIs_CXXConstructExpr) {
                  Constructor1Arg));
 }
 
+TEST_P(ASTMatchersTest, hasDependentName_DependentScopeDeclRefExpr) {
+  if (!GetParam().isCXX() || GetParam().hasDelayedTemplateParsing()) {
+    // FIXME: Fix this test to work with delayed template parsing.
+    return;
+  }
+
+  EXPECT_TRUE(matches("template <class T> class X : T { void f() { T::v; } };",
+                      dependentScopeDeclRefExpr(hasDependentName("v"))));
+
+  EXPECT_TRUE(
+      matches("template <typename T> struct S { static T Foo; };"
+              "template <typename T> void declToImport() { (void)S<T>::Foo; }",
+              dependentScopeDeclRefExpr(hasDependentName("Foo"))));
+}
+
 TEST(ASTMatchersTest, NamesMember_CXXDependentScopeMemberExpr) {
 
   // Member functions:

Copy link
Collaborator

@HighCommander4 HighCommander4 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this!

The patch looks pretty good, I just have some minor comments and a suggestion for a new test case.

@@ -2235,6 +2235,21 @@ TEST_P(ASTMatchersTest, ArgumentCountIs_CXXConstructExpr) {
Constructor1Arg));
}

TEST_P(ASTMatchersTest, hasDependentName_DependentScopeDeclRefExpr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: looking at other tests in this file, the test names start with a capital letter (e.g. HasSize_CXX) even if the matcher they are testing starts with lowercase (e.g. hasSize)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

EXPECT_TRUE(matches("template <class T> class X : T { void f() { T::v; } };",
dependentScopeDeclRefExpr(hasDependentName("v"))));

EXPECT_TRUE(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have a test case where the dependent name refers to a function (specifically a static member function, since I think that will get a DependentScopeDeclRefExpr rather than a CXXDepdendentScopeMemberExpr).

Come to think of it, this applies for the test for dependentScopeDeclRefExpr itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thank you for suggesting it :D

@@ -3257,6 +3257,17 @@ AST_MATCHER_P(CXXDependentScopeMemberExpr, memberHasSameNameAsBoundNode,
});
}

/// Matches the dependent name of a dependent scope decl ref expr
Copy link
Collaborator

Choose a reason for hiding this comment

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

likewise here, "of a DependentScopeDeclRefExpr"

@@ -1116,6 +1116,8 @@ AST Matchers

- Add ``dependentTemplateSpecializationType`` matcher to match a dependent template specialization type.

- Add ``hasDependentName`` matcher to match the dependent name of a dependent scope decl ref expr.
Copy link
Collaborator

Choose a reason for hiding this comment

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

likewise here, "of a DependentScopeDeclRefExpr"

@@ -3449,6 +3449,19 @@ <h2 id="narrowing-matchers">Narrowing Matchers</h2>
</pre></td></tr>


<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1DependentScopeDeclRefExpr.html">DependentScopeDeclRefExpr</a>&gt;</td><td class="name" onclick="toggle('hasDependentName0')"><a name="hasDependentName0Anchor">hasDependentName</a></td><td>std::string N</td></tr>
<tr><td colspan="4" class="doc" id="hasDependentName0"><pre>Matches the dependent name of a dependent scope decl ref expr.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "of a DependentScopeDeclRefExpr" would read better to me

Copy link
Collaborator

@HighCommander4 HighCommander4 left a comment

Choose a reason for hiding this comment

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

Thanks! The update looks good, I have one more minor suggestion for a change which I'll just go ahead and make before merging.

dependentScopeDeclRefExpr(hasDependentName("Foo"))));

EXPECT_TRUE(matches("template <typename T> struct S { static T foo(); };"
"template <typename T> void x() { S<T>::foo; }",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: it would make the example more realistic if the function is actually called, as in S<T>::foo();

@HighCommander4 HighCommander4 merged commit f3590c1 into llvm:main Jan 6, 2025
9 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jan 6, 2025

LLVM Buildbot has detected a new failure on builder clang-ppc64le-linux-multistage running on ppc64le-clang-multistage-test while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/5845

Here is the relevant piece of the build log for the reference
Step 5 (ninja check 1) failure: stage 1 checked (failure)
******************** TEST 'LLVM :: ExecutionEngine/JITLink/x86-64/ELF_R_X86_64_SIZE.s' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 2: /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-mc -triple=x86_64-unknown-linux -position-independent      -filetype=obj -o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.1.o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/llvm/llvm/test/ExecutionEngine/JITLink/x86-64/ELF_R_X86_64_SIZE.s
+ /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-mc -triple=x86_64-unknown-linux -position-independent -filetype=obj -o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.1.o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/llvm/llvm/test/ExecutionEngine/JITLink/x86-64/ELF_R_X86_64_SIZE.s
RUN: at line 4: /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-jitlink -noexec /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.1.o
+ /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-jitlink -noexec /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.1.o
RUN: at line 7: /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-mc -triple=x86_64-unknown-linux -position-independent --defsym=OVERFLOW=1      -filetype=obj -o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.2.o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/llvm/llvm/test/ExecutionEngine/JITLink/x86-64/ELF_R_X86_64_SIZE.s
+ /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-mc -triple=x86_64-unknown-linux -position-independent --defsym=OVERFLOW=1 -filetype=obj -o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.2.o /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/llvm/llvm/test/ExecutionEngine/JITLink/x86-64/ELF_R_X86_64_SIZE.s
RUN: at line 9: not /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-jitlink -noexec /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.2.o 2>&1 | /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/FileCheck /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/llvm/llvm/test/ExecutionEngine/JITLink/x86-64/ELF_R_X86_64_SIZE.s
+ not /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/llvm-jitlink -noexec /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/test/ExecutionEngine/JITLink/x86-64/Output/ELF_R_X86_64_SIZE.s.tmp.2.o
+ /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/bin/FileCheck /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/llvm/llvm/test/ExecutionEngine/JITLink/x86-64/ELF_R_X86_64_SIZE.s

--

********************


@AmrDeveloper
Copy link
Member Author

Thanks! The update looks good, I have one more minor suggestion for a change which I'll just go ahead and make before merging.

Thank you

@AmrDeveloper
Copy link
Member Author

@HighCommander4

We can also create a new matcher for dependentNameType so we can use it like dependentNameType(hasDependentType(builtinType())), which is similar to Complex and Array types.

If confirmed I can work on it too :D

Thank you

@HighCommander4
Copy link
Collaborator

We can also create a new matcher for dependentNameType so we can use it like dependentNameType(hasDependentType(builtinType())), which is similar to Complex and Array types.

I don't think DependentNameType has a Type property that we could match in this way; the idea behind DependentNameType is that resolving it to a concrete Type needs to be deferred until after instantiation when the dependent name can be looked up.

That said, it would be useful to have a matcher for the name of a DependentNameType, something like dependentNameType(hasDependentName("type")) for typename T::type. I don't know how straightforward it is to overload a matcher name (hasDependentName in this case) in this way, but if you're interested in playing around with it, I think that would make for a useful addition!

@AmrDeveloper
Copy link
Member Author

We can also create a new matcher for dependentNameType so we can use it like dependentNameType(hasDependentType(builtinType())), which is similar to Complex and Array types.

I don't think DependentNameType has a Type property that we could match in this way; the idea behind DependentNameType is that resolving it to a concrete Type needs to be deferred until after instantiation when the dependent name can be looked up.

That said, it would be useful to have a matcher for the name of a DependentNameType, something like dependentNameType(hasDependentName("type")) for typename T::type. I don't know how straightforward it is to overload a matcher name (hasDependentName in this case) in this way, but if you're interested in playing around with it, I think that would make for a useful addition!

Thanks for the explanation, I am interested to try to play around with it and will update you with result,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a matcher for the name of a DependentScopeDeclRefExpr
4 participants