[ASTMatchers] Extend hasName matcher when matching templates #100349

njames93 · 2024-07-24T11:42:10Z

Allow users to match all record instantiations by using <> as a wildcard.

With

template <typename T> struct Foo { void Bar(); };

The following code:

Foo<int>{}.Bar();

Will match against:

callExpr(callee(cxxMethodDecl(hasName("Bar")))) callExpr(callee(cxxMethodDecl(hasName("Foo<int>::Bar")))) callExpr(callee(cxxMethodDecl(hasName("Foo<>::Bar"))))

llvmbot · 2024-07-24T11:42:43Z

@llvm/pr-subscribers-clang

Author: Nathan James (njames93)

Changes

Allow users to match all record instantiations by using <> as a wildcard.

With

template &lt;typename T&gt; struct Foo { void Bar(); };

The following code:

Foo&lt;int&gt;{}.Bar();

Will match against:

callExpr(callee(cxxMethodDecl(hasName("Bar")))) callExpr(callee(cxxMethodDecl(hasName("Foo&lt;int&gt;::Bar")))) callExpr(callee(cxxMethodDecl(hasName("Foo&lt;&gt;::Bar"))))

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

3 Files Affected:

(modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+16)
(modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+27-3)
(modified) clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (+24)

diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h index ca44c3ee08565..4fa04f3af5909 100644 --- a/clang/include/clang/ASTMatchers/ASTMatchers.h +++ b/clang/include/clang/ASTMatchers/ASTMatchers.h @@ -3076,6 +3076,22 @@ inline internal::BindableMatcher<Stmt> sizeOfExpr( /// \code /// namespace a { namespace b { class X; } } /// \endcode +/// +/// Qualified names in templated classes can be matched explicitly or implicity +/// by specifying the template type or using empty angle brackets to match any +/// template. +/// +/// Example matches: +/// - callExpr(callee(functionDecl(hasName("Foo<int>::Bar")))) +/// - callExpr(callee(functionDecl(hasName("Foo<>::Bar")))) +/// \code +/// template<typename T> class Foo{ +/// static void Bar(); +/// }; +/// void Func() { +/// Foo<int>::Bar(); +/// } +/// \endcode inline internal::Matcher<NamedDecl> hasName(StringRef Name) { return internal::Matcher<NamedDecl>( new internal::HasNameMatcher({std::string(Name)})); diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp index bf87b1aa0992a..6d1d2507de4b7 100644 --- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp +++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp @@ -638,6 +638,30 @@ bool HasNameMatcher::matchesNodeFullFast(const NamedDecl &Node) const { return Patterns.foundMatch(/*AllowFullyQualified=*/true); } +static std::optional<StringRef> consumePatternBack(StringRef Pattern, StringRef Target){ + while(!Pattern.empty()){ + auto Index = Pattern.rfind("<>"); + if (Index == StringRef::npos){ + if (Target.consume_back(Pattern)) return Target; + return {}; + } + auto Suffix = Pattern.substr(Index + 1); + if (!Target.consume_back(Suffix)) return {}; + auto BracketCount = 1; + while (BracketCount) { + if (Target.empty()) return {}; + switch(Target.back()){ + case '<': --BracketCount; break; + case '>': ++BracketCount; break; + default: break; + } + Target = Target.drop_back(); + } + Pattern = Pattern.take_front(Index); + } + return Target; +} + bool HasNameMatcher::matchesNodeFullSlow(const NamedDecl &Node) const { const bool SkipUnwrittenCases[] = {false, true}; for (bool SkipUnwritten : SkipUnwrittenCases) { @@ -653,10 +677,10 @@ bool HasNameMatcher::matchesNodeFullSlow(const NamedDecl &Node) const { for (const StringRef Pattern : Names) { if (Pattern.starts_with("::")) { - if (FullName == Pattern) + if (auto Result = consumePatternBack(Pattern, FullName); Result && Result->empty()){ return true; - } else if (FullName.ends_with(Pattern) && - FullName.drop_back(Pattern.size()).ends_with("::")) { + } + } else if (auto Result = consumePatternBack(Pattern, FullName); Result && Result->ends_with("::")) { return true; } } diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp index f26140675fd46..03e5fb3562dca 100644 --- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp +++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp @@ -2664,6 +2664,30 @@ TEST_P(ASTMatchersTest, HasName_QualifiedStringMatchesThroughLinkage) { EXPECT_TRUE(notMatches(code, functionDecl(hasName("::test")))); } +TEST_P(ASTMatchersTest, HasName_TemplateStrip) { + if (!GetParam().isCXX()) { + return; + } + + StringRef Code = "template<typename T> struct Foo{void Bar() const;}; void Func() { Foo<int> Item; Item.Bar(); }"; + + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("Bar")))))); + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("Foo<int>::Bar")))))); + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("::Foo<int>::Bar")))))); + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("::Foo<>::Bar")))))); + EXPECT_TRUE(notMatches(Code, callExpr(callee(cxxMethodDecl(hasName("::Foo<::Bar")))))); + EXPECT_TRUE(notMatches(Code, callExpr(callee(cxxMethodDecl(hasName("::Foo<long>::Bar")))))); + + if (GetParam().isCXX11OrLater()){ + Code = "template<typename T> struct Foo{void Bar() const;}; void Func() { Foo<Foo<int>> Item; Item.Bar(); }"; + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("Bar")))))); + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("Foo<>::Bar")))))); + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("::Foo<>::Bar")))))); + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("Foo<Foo<>>::Bar")))))); + EXPECT_TRUE(matches(Code, callExpr(callee(cxxMethodDecl(hasName("::Foo<Foo<>>::Bar")))))); + } +} + TEST_P(ASTMatchersTest, HasAnyName) { if (!GetParam().isCXX()) { // FIXME: Add a test for `hasAnyName()` that does not depend on C++.

AaronBallman · 2024-07-25T11:52:17Z

Allow users to match all record instantiations by using <> as a wildcard.

I think this will change the behavior of existing matchers, consider:

template <typename Ty = int> struct S { Ty Val; void Call(); }; int main() { S<> s; s.Call(); // Currently matches only this S<float> another; another.Call(); // Will now start to match this as well? }

https://godbolt.org/z/Gr439c9v4

WDYT?

njames93 · 2024-07-25T13:06:19Z

@AaronBallman That's a good point, I didn't account for how defaulted template arguments aren't printed.
Would using <*> ~~or a more rustic approach of <_>~~ be a good alternative?

AaronBallman · 2024-07-25T13:24:09Z

@AaronBallman That's a good point, I didn't account for how defaulted template arguments aren't printed. Would using <*> ~~or a more rustic approach of <_>~~ be a good alternative?

I was thinking about that, but I keep coming around to wondering whether it might make sense to support a regex there instead of a custom syntax. That seems like it gives users a bit more power without requiring them to write additional matchers, but perhaps that's a bad idea for performance reasons...

njames93 · 2024-07-25T13:34:06Z

@AaronBallman That's a good point, I didn't account for how defaulted template arguments aren't printed. Would using <*> ~~or a more rustic approach of <_>~~ be a good alternative?

I was thinking about that, but I keep coming around to wondering whether it might make sense to support a regex there instead of a custom syntax. That seems like it gives users a bit more power without requiring them to write additional matchers, but perhaps that's a bad idea for performance reasons...

We already have a matchesName matcher that would be able to handle any case you want to throw at it, but there is definitely some performance concerns with it.
This is here to simply the basic case where you just want to match any template which should be helpful for most cases.

AaronBallman · 2024-07-25T13:49:12Z

@AaronBallman That's a good point, I didn't account for how defaulted template arguments aren't printed. Would using <*> ~~or a more rustic approach of <_>~~ be a good alternative?

I was thinking about that, but I keep coming around to wondering whether it might make sense to support a regex there instead of a custom syntax. That seems like it gives users a bit more power without requiring them to write additional matchers, but perhaps that's a bad idea for performance reasons...

We already have a matchesName matcher that would be able to handle any case you want to throw at it, but there is definitely some performance concerns with it. This is here to simply the basic case where you just want to match any template which should be helpful for most cases.

Okay, that's a good point! Hmm, then yeah, I think * makes sense as a glob; _ is a loaded term because of https://wg21.link/P2169

Allow users to match all record instantiations by using <> as a wildcard

AaronBallman

The changes should also come with a release note in clang/docs/ReleaseNotes.rst.

AaronBallman · 2024-07-29T13:33:43Z

clang/include/clang/ASTMatchers/ASTMatchers.h

 /// \endcode
+///
+/// Qualified names in templated classes can be matched explicitly or implicity
+/// by specifying the template type or using `<*>` to match any template.


We should probably be clear that <*> is the only syntax we support. e.g., we don't let you search based on arity, as in hasName("Foo<int, *>::Bar"). Might be worth an example as well, definitely worth some test coverage.

5chmidti

I think it does make sense to add this wildcard. The motivation to skip regex sounds like a good one, and it does make sense to special case templates specifically. The implementation looks correct to me. Just some small things to point out regarding the code, and the missing release notes, of course.
Your pr comment should also reflect the change to <*> from <>

5chmidti · 2024-08-24T13:36:44Z

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

+ return {};
+ }
+ auto Suffix = Pattern.substr(Index + 2);
+ if (!Target.consume_back(Suffix))
+ return {};
+ auto BracketCount = 1;
+ while (BracketCount) {
+ if (Target.empty())
+ return {};


IMO returning std::nullopt is better here.

5chmidti · 2024-08-24T13:38:05Z

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

+ auto Index = Pattern.rfind("<*>");
+ if (Index == StringRef::npos) {
+ if (Target.consume_back(Pattern))
+ return Target;
+ return {};
+ }
+ auto Suffix = Pattern.substr(Index + 2);
+ if (!Target.consume_back(Suffix))
+ return {};
+ auto BracketCount = 1;


You're using auto three times here without the type mentioned on the RHS (not sure how strict clang/ is w.r.t. this)

Suffix and Index can be const

5chmidti · 2024-08-24T13:45:12Z

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

+ auto Suffix = Pattern.substr(Index + 2);
+ if (!Target.consume_back(Suffix))


Because I had to check this, could you please add a comment why only + 2 is used?
Something like:

We consume pattern suffix that includes only the > of our <*>, and drop every character from the back until we encounter the corresponding < character to ensure balanced angle brackets.

5chmidti · 2024-08-24T13:48:38Z

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

 if (Pattern.starts_with("::")) {
- if (FullName == Pattern)
+ if (auto Result = consumePatternBack(Pattern, FullName);
+ Result && Result->empty()) {
 return true;
- } else if (FullName.ends_with(Pattern) &&
- FullName.drop_back(Pattern.size()).ends_with("::")) {
+ }
+ } else if (auto Result = consumePatternBack(Pattern, FullName);
+ Result && Result->ends_with("::")) {
 return true;
 }


You could write this loop body as

const std::optional<StringRef> Result = consumePatternBack(Pattern, FullName); if (!Result) continue; if ((Pattern.starts_with("::") && Result->empty()) || Result->ends_with("::")) return true;

which is more readable IMO

llvmbot added the clang Clang issues not falling into any other category label Jul 24, 2024

njames93 force-pushed the has-name-matcher-template branch from ee05cd6 to d623ba5 Compare July 24, 2024 11:43

njames93 requested review from 5chmidti, AaronBallman and kazutakahirata July 25, 2024 07:10

AaronBallman changed the title ~~[ASMMatchers] Extend hasName matcher when matching templates~~ [ASTMatchers] Extend hasName matcher when matching templates Jul 25, 2024

[ASMMatchers] Extend hasName matcher when matching templates

a81c1b2

Allow users to match all record instantiations by using <> as a wildcard

njames93 force-pushed the has-name-matcher-template branch from d623ba5 to a81c1b2 Compare July 27, 2024 06:11

njames93 mentioned this pull request Jul 27, 2024

Create a new check to look for mis-use in calls that take iterators #99917

Open

AaronBallman reviewed Jul 29, 2024

View reviewed changes

5chmidti requested changes Aug 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ASTMatchers] Extend hasName matcher when matching templates #100349

[ASTMatchers] Extend hasName matcher when matching templates #100349

Uh oh!

njames93 commented Jul 24, 2024

llvmbot commented Jul 24, 2024

AaronBallman commented Jul 25, 2024

njames93 commented Jul 25, 2024 •

edited

Loading

AaronBallman commented Jul 25, 2024

njames93 commented Jul 25, 2024

AaronBallman commented Jul 25, 2024

AaronBallman left a comment

AaronBallman Jul 29, 2024

5chmidti left a comment

5chmidti Aug 24, 2024

5chmidti Aug 24, 2024

5chmidti Aug 24, 2024

5chmidti Aug 24, 2024

Labels

4 participants

		auto Suffix = Pattern.substr(Index + 2);
		if (!Target.consume_back(Suffix))

[ASTMatchers] Extend hasName matcher when matching templates #100349

Are you sure you want to change the base?

[ASTMatchers] Extend hasName matcher when matching templates #100349

Uh oh!

Conversation

njames93 commented Jul 24, 2024

llvmbot commented Jul 24, 2024

AaronBallman commented Jul 25, 2024

njames93 commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AaronBallman commented Jul 25, 2024

njames93 commented Jul 25, 2024

AaronBallman commented Jul 25, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Jul 29, 2024

Choose a reason for hiding this comment

5chmidti left a comment

Choose a reason for hiding this comment

5chmidti Aug 24, 2024

Choose a reason for hiding this comment

5chmidti Aug 24, 2024

Choose a reason for hiding this comment

5chmidti Aug 24, 2024

Choose a reason for hiding this comment

5chmidti Aug 24, 2024

Choose a reason for hiding this comment

Labels

4 participants

njames93 commented Jul 25, 2024 •

edited

Loading