[ASTMatchers] Added hasDirectBase Matcher

[njames93]

Summary

Adds a matcher called hasDirectBase for matching the CXXBaseSpecifier of a class that directly derives from another class.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

2886–2888

It seems like these aren't really part of the example?

3541

This is undoing a change that was just added less than two weeks ago, so I think the potential for breaking code is small. That said, can you explain why you think hasClass is a better approach than hasType?

[njames93]

clang/include/clang/ASTMatchers/ASTMatchers.h

2886–2888

They are, just not directly. Shows it won't match any old base specifier.

3541

Yeah, as that change hasn't reached landed onto a release branch breaking code shouldn't be an issue, If it was I'd leave it in.

• hasType is very generic, whereas hasClass is specific to what a CXXBaseSpecifier supports.
• It makes the matchers marginally simpler. hasDirectBase(hasType(cxxRecordDecl(hasName("Base")))) vs hasDirectBase(hasClass(hasName("Base")))
• In the documentation it also specifies that hasClass takes a `Matcher<CXXRecordDecl>, making it more user friendly.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

2886–2888

Ah, might be good to put comments on there that say // doesn't match to make it more clear.

3541

FWIW, I prefer hasType to hasClass. You can inherit from things which are not a class, such as a struct (so the name is a bit of a misnomer, but not too awful), a class template (which you can't match with this interface), or a template type (which you also can't match with this interface).

[jkorous]

@njames93 hasDirectBase seems like a useful matcher to me! OTOH I am not totally convinced about hasType -> hasClass. Anyway, don't you want to land hasDirectBase as a separate patch first and then discuss the rest?

One more thing - I'm just thinking if there isn't some corner case where a base class could be interpreted as both direct and indirect. The most ugly case I came up with is virtual inheritance. I admit I don't know what the standard says about this so it might be a non-issue. Also, it'd still probably make sense to match on such base. WDYT?

struct Base {};
struct Proxy : virtual Base {};
struct Derived : Base, Proxy {};

clang/include/clang/ASTMatchers/ASTMatchers.h

3541

I don't feel super strongly about this but I also slightly prefer hasType.

To be fair - I didn't really have things like inheritance from template parameters on my mind when working on hasAnyBase (it's definitely not tested) so I'd rather not assume it works.

3557

Nit: while "[base specifier] hasType" sounds natural to me for some reason hasClass doesn't. English is not my first language though.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

3168

Is this (Base != BAse) a typo or a way how to tease the asserts?

[njames93]

• Added back hasType overload for CXXBaseSpecifier
• Added hasClassTemplate and hasClassOrClassTemplate matcher for CXXBaseSpecifier
• Added hasTemplatedDecl for ClassTemplateDecl

[njames93]

In D81552#2086420, @jkorous wrote:

@njames93 hasDirectBase seems like a useful matcher to me! OTOH I am not totally convinced about hasType -> hasClass. Anyway, don't you want to land hasDirectBase as a separate patch first and then discuss the rest?

There more I add to this, the more splitting it up sounds like a good idea.

One more thing - I'm just thinking if there isn't some corner case where a base class could be interpreted as both direct and indirect. The most ugly case I came up with is virtual inheritance. I admit I don't know what the standard says about this so it might be a non-issue. Also, it'd still probably make sense to match on such base. WDYT?

struct Base {};
struct Proxy : virtual Base {};
struct Derived : Base, Proxy {};

In that case it would probably register as a direct and indirect base. However there is no matcher(nor much need for one) that will solely match indirect bases so its mostly a non issue.

clang/include/clang/ASTMatchers/ASTMatchers.h

3541

I have decided to put hasType back in there as it does have some general uses. However I have added more class and class template specific matchers as I feel these are slightly more user friendly.

LMK what you think of this approach.

Side note what is the correct collective term for classes and structs. I'd be tempted to refer to them how clang does, records, but hasRecord seems wrong.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

3168

It was to tease the assers, but I removed it.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3541

Side note what is the correct collective term for classes and structs. I'd be tempted to refer to them how clang does, records, but hasRecord seems wrong.

We use the term "record", but I'm not certain how widely used that is.

3557

I agree that hasClass seems unnatural here. Out of curiosity, could we modify the hasName matcher to work on base specifiers so you can write: cxxRecordDecl(hasAnyBase(hasName("Base"))) as shorthand for the more wordy version cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")))))?

[jkorous]

clang/include/clang/ASTMatchers/ASTMatchers.h

3604

I think we should just use eachOf matcher for this kind of composition.

https://clang.llvm.org/docs/LibASTMatchersReference.html#traversal-matchers

[jkorous]

clang/include/clang/ASTMatchers/ASTMatchers.h

3557

Wouldn't it be strange to treat hasName differently than all the other narrowing matchers? Honest question - I feel that hasName might be the most commonly used, just don't know if that's enough to justify this.
https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers

[njames93]

clang/include/clang/ASTMatchers/ASTMatchers.h

3541

https://en.cppreference.com/w/cpp/language/class - Going of what that says, it states that a class declaration starts with a keyword either class or struct. Nowhere on the page does it mention record.
Continuing on from this point, we have many more matchers with class in the name but work on structs too:
ofClass, hasInClassInitializer and injectedClassNameType. If you're being pedantic there is also classTemplateDecl, classTemplatePartialSpecializationDecl and classTemplateSpecializationDecl.

Having said all of that I'm still not a huge fan of hasClass, but I'm less of a fan of hasType. I'd thought of forClass but that could be misinterpreted as the derived class of the CXXBaseSpecifier Kind of like the behaviour of forFunction.

class Base {};
class Derived : Base {};

Does cxxBaseSpecifier(forClass(cxxRecordDecl().bind("X")) bind to Derived` or Base?

3557

Repurposing hasName would be a pain especially considering 99% of its use cases wont be for base class matching.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3557

Wouldn't it be strange to treat hasName differently than all the other narrowing matchers? Honest question - I feel that hasName might be the most commonly used, just don't know if that's enough to justify this. https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers

Different how? I'm suggesting to overload hasName to work on a CXXBaseSpecifier since those have a name.

Repurposing hasName would be a pain especially considering 99% of its use cases wont be for base class matching.

I'm asking what the right API is for users, though, which is a bit different. Base specifiers have names (there are no unnamed base specifiers), so to me, it makes more sense for hasName to work with them directly since that is the thing that does name matching.

I think you can accomplish this by using a PolymorphicMatcherWithParam1 like we do for hasOverloadedOperatorName which can narrow to either a CXXOperatorCallExpr or a FunctionDecl.

3604

+1

[jkorous]

clang/include/clang/ASTMatchers/ASTMatchers.h

3557

Wouldn't it be strange to treat hasName differently than all the other narrowing matchers? Honest question - I feel that hasName might be the most commonly used, just don't know if that's enough to justify this. https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers

Different how? I'm suggesting to overload hasName to work on a CXXBaseSpecifier since those have a name.

What I meant is that technically we can overload any Matcher<CXXRecordDecl> matcher in the same fashion so having the overloaded version of hasName only makes it somewhat special (and someone might argue that it'd impact consistency of matchers composability). Anyway, I'm fine with your suggestion!

[aaron.ballman]

Pinging @klimek , @sammccall , and @dblaikie to see if there are some opinions about overloading hasName (and possibly other naming related questions).

clang/include/clang/ASTMatchers/ASTMatchers.h

3557

Ah, I see what you mean -- thanks for explaining. I'm on the fence about this. One the one hand, base specifiers *in the AST* do not have names, so it seems defensible to say that hasName should not handle a base specifier. On the other hand, base specifiers *in the language* are identifiers that always have a name, so it seems defensible to say that hashName should handle a base specifier.

Pulling in some more folks to see if a wider audience brings consensus.

[klimek]

We have some precedent for overloading has* matchers, but I'll defer to Sam's judgement whether that's a good idea here.

[sammccall]

clang/include/clang/ASTMatchers/ASTMatchers.h

3557

I think I agree that it's reasonable/consistent to do this, but I'm not sure it's a good idea.
Base specifier -> type -> class -> name seems like the most *regular* traversal that reflects the shape/concepts of the AST, shortcutting steps makes the model (IMO) more complicated to make it more terse.
Is matching a base specifier with a hardcoded name really common enough in absolute terms that people should keep this special case in their heads?

I think hasType(cxxRecordDecl(hasName("Base"))) a bit better, but i think of *expressions* having types, and would prefer specifiesType or just specifies at the outside.
But just my 2c and I'm not deeply familiar with the conventions here - I can live with any of the options.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3557

Is matching a base specifier with a hardcoded name really common enough in absolute terms that people should keep this special case in their heads?

Thanks for the perspective. I don't think base specifiers will be matched all that often, but I'm also not certain it's a special case because base specifiers are conceptually things with names even if they're not exposed as such via our AST. However, you bring up a good point about modelling the shape of the AST and that does suggest base specifier -> type -> class -> name as the right way to go.

I think hasType(cxxRecordDecl(hasName("Base"))) a bit better, but i think of *expressions* having types, and would prefer specifiesType or just specifies at the outside.

If hasType only worked on expressions, I would be okay with adding specifies or specifiesType for this functionality, but since hasType works on expressions and declarations, I think it's more natural to reuse hasType for the specifier than to introduce specifiesType (or hasClass) for base specifiers (which name a class type).

[njames93]

Removed all hasClass related changes.

[njames93]

I've removed the hasClass changes as they dont entirely belong in here and weren't well received. Now with just the hasDirectBase this should be in a good state.

[aaron.ballman]

LGTM with a whitespace nit.

clang/include/clang/ASTMatchers/ASTMatchers.h

2895

Can remove the newline.

clang/docs/LibASTMatchersReference.html

cxxNewExpr(hasPlacementArg(1, integerLiteral(equals(16))))
   matches the expression 'new (Storage, 16) MyClass()'.
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html">CXXRecordDecl</a>&gt;</td><td class="name" onclick="toggle('hasAnyBase0')"><a name="hasAnyBase0Anchor">hasAnyBase</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXBaseSpecifier.html">CXXBaseSpecifier</a>&gt; BaseSpecMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="hasAnyBase0"><pre>Matches C++ classes that have a direct or indirect base matching BaseSpecMatcher.
 
 Example:
-matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase")))))
+matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase"))))
   class Foo;
   class Bar : Foo {};
   class Baz : Bar {};
   class SpecialBase;
   class Proxy : SpecialBase {};  // matches Proxy
   class IndirectlyDerived : Proxy {};  //matches IndirectlyDerived
 
 FIXME: Refactor this and isDerivedFrom to reuse implementation.
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html">CXXRecordDecl</a>&gt;</td><td class="name" onclick="toggle('hasDirectBase0')"><a name="hasDirectBase0Anchor">hasDirectBase</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXBaseSpecifier.html">CXXBaseSpecifier</a>&gt; BaseSpecMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="hasDirectBase0"><pre>Matches C++ classes that have a direct base matching BaseSpecMatcher.
+
+Example:
+matcher hasDirectBase(hasType(cxxRecordDecl(hasName("SpecialBase"))))
+  class Foo;
+  class Bar : Foo {};
+  class Baz : Bar {};
+  class SpecialBase;
+  class Proxy : SpecialBase {};  // matches Proxy
+  class IndirectlyDerived : Proxy {};  // doesn't match
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html">CXXRecordDecl</a>&gt;</td><td class="name" onclick="toggle('hasMethod0')"><a name="hasMethod0Anchor">hasMethod</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html">CXXMethodDecl</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="hasMethod0"><pre>Matches the first method of a class or struct that satisfies InnerMatcher.
 
 Given:
   class A { void func(); };
   class B { void member(); };
 
 cxxRecordDecl(hasMethod(hasName("func"))) matches the declaration of

clang/include/clang/ASTMatchers/ASTMatchers.h

AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
   const auto *InterfaceDecl = cast<ObjCInterfaceDecl>(&Node);
   return Matcher<ObjCInterfaceDecl>(M).matches(*InterfaceDecl, Finder, Builder);
 }
 
 /// Matches C++ classes that have a direct or indirect base matching \p
 /// BaseSpecMatcher.
 ///
 /// Example:
-/// matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase")))))
+/// matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase"))))
 /// \code
 ///   class Foo;
 ///   class Bar : Foo {};
 ///   class Baz : Bar {};
 ///   class SpecialBase;
 ///   class Proxy : SpecialBase {};  // matches Proxy
 ///   class IndirectlyDerived : Proxy {};  //matches IndirectlyDerived
 /// \endcode
 ///
 // FIXME: Refactor this and isDerivedFrom to reuse implementation.
 AST_MATCHER_P(CXXRecordDecl, hasAnyBase, internal::Matcher<CXXBaseSpecifier>,
               BaseSpecMatcher) {
   return internal::matchesAnyBase(Node, BaseSpecMatcher, Finder, Builder);
 }
 
+/// Matches C++ classes that have a direct base matching \p BaseSpecMatcher.
+///
+/// Example:
+/// matcher hasDirectBase(hasType(cxxRecordDecl(hasName("SpecialBase"))))
+/// \code
+///   class Foo;
+///   class Bar : Foo {};
+///   class Baz : Bar {};
+///   class SpecialBase;
+///   class Proxy : SpecialBase {};  // matches Proxy
+///   class IndirectlyDerived : Proxy {};  // doesn't match
+/// \endcode
+AST_MATCHER_P(CXXRecordDecl, hasDirectBase, internal::Matcher<CXXBaseSpecifier>,
+              BaseSpecMatcher) {
+  return Node.hasDefinition() &&
+         llvm::any_of(Node.bases(), [&](const CXXBaseSpecifier &Base) {
+           return BaseSpecMatcher.matches(Base, Finder, Builder);
+         });
+}
+
 /// Similar to \c isDerivedFrom(), but also matches classes that directly
 /// match \c Base.
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
     isSameOrDerivedFrom,
     AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
     internal::Matcher<NamedDecl>, Base, 0) {
   const auto M = anyOf(Base, isDerivedFrom(Base));
 

 /// class Derived : Base {};
 /// \endcode
 ///
 /// Usable as: Matcher<Expr>, Matcher<FriendDecl>, Matcher<ValueDecl>,
 /// Matcher<CXXBaseSpecifier>
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
     hasType,
     AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
                                     CXXBaseSpecifier),
     internal::Matcher<Decl>, InnerMatcher, 1) {
   QualType QT = internal::getUnderlyingType(Node);
   if (!QT.isNull())
     return qualType(hasDeclaration(InnerMatcher)).matches(QT, Finder, Builder);
   return false;
 }
 
 /// Matches if the type location of the declarator decl's type matches
 /// the inner matcher.
 ///
 /// Given
 /// \code
 ///   int x;
 /// \endcode
 /// declaratorDecl(hasTypeLoc(loc(asString("int"))))
 ///   matches int x
 AST_MATCHER_P(DeclaratorDecl, hasTypeLoc, internal::Matcher<TypeLoc>, Inner) {
   if (!Node.getTypeSourceInfo())
     // This happens for example for implicit destructors.
     return false;
   return Inner.matches(Node.getTypeSourceInfo()->getTypeLoc(), Finder, Builder);
 }
 
 /// Matches if the matched type is represented by the given string.

 AST_MATCHER_P_OVERLOAD(QualType, pointsTo, internal::Matcher<Decl>,
                        InnerMatcher, 1) {
   return pointsTo(qualType(hasDeclaration(InnerMatcher)))
       .matches(Node, Finder, Builder);
 }
 
 /// Matches if the matched type matches the unqualified desugared
 /// type of the matched node.
 ///
 /// For example, in:
 /// \code
 ///   class A {};
 ///   using B = A;
 /// \endcode
 /// The matcher type(hasUnqualifiedDesugaredType(recordType())) matches
 /// both B and A.
 AST_MATCHER_P(Type, hasUnqualifiedDesugaredType, internal::Matcher<Type>,

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(hasDecayedType);
   REGISTER_MATCHER(hasDeclContext);
   REGISTER_MATCHER(hasDeclaration);
   REGISTER_MATCHER(hasDeducedType);
   REGISTER_MATCHER(hasDefaultArgument);
   REGISTER_MATCHER(hasDefinition);
   REGISTER_MATCHER(hasDescendant);
   REGISTER_MATCHER(hasDestinationType);
+  REGISTER_MATCHER(hasDirectBase);
   REGISTER_MATCHER(hasDynamicExceptionSpec);
   REGISTER_MATCHER(hasEitherOperand);
   REGISTER_MATCHER(hasElementType);
   REGISTER_MATCHER(hasElse);
   REGISTER_MATCHER(hasExplicitSpecifier);
   REGISTER_MATCHER(hasExternalFormalLinkage);
   REGISTER_MATCHER(hasFalseExpression);
   REGISTER_MATCHER(hasGlobalStorage);

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

 }
 
 TEST(IsVirtual, NoVirtualBase) {
   EXPECT_TRUE(notMatches("class Base {};"
                          "class Derived : Base {};",
                          cxxRecordDecl(hasAnyBase(isVirtual()))));
 }
 
+TEST(BaseSpecifier, hasDirectBase) {
+  EXPECT_TRUE(matches(
+      R"cc(
+    class Base {};
+    class Derived : Base{};
+    )cc",
+      cxxRecordDecl(hasName("Derived"),
+                    hasDirectBase(hasType(cxxRecordDecl(hasName("Base")))))));
+
+  StringRef MultiDerived = R"cc(
+    class Base {};
+    class Base2 {};
+    class Derived : Base, Base2{};
+    )cc";
+
+  EXPECT_TRUE(matches(
+      MultiDerived,
+      cxxRecordDecl(hasName("Derived"),
+                    hasDirectBase(hasType(cxxRecordDecl(hasName("Base")))))));
+  EXPECT_TRUE(matches(
+      MultiDerived,
+      cxxRecordDecl(hasName("Derived"),
+                    hasDirectBase(hasType(cxxRecordDecl(hasName("Base2")))))));
+
+  StringRef Indirect = R"cc(
+    class Base {};
+    class Intermediate : Base {};
+    class Derived : Intermediate{};
+    )cc";
+
+  EXPECT_TRUE(
+      matches(Indirect, cxxRecordDecl(hasName("Derived"),
+                                      hasDirectBase(hasType(cxxRecordDecl(
+                                          hasName("Intermediate")))))));
+  EXPECT_TRUE(notMatches(
+      Indirect,
+      cxxRecordDecl(hasName("Derived"),
+                    hasDirectBase(hasType(cxxRecordDecl(hasName("Base")))))));
+}
 } // namespace ast_matchers
 } // namespace clang