[ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`

[baloghadamsoftware]

Summary

AST Matcher hasBody is a polymorphic matcher that behaves differently for loop statements and function declarations. The main difference is the for functions declarations it does not only call FunctionDecl::getBody() but first checks whether the declaration in question is that specific declaration which has the body by calling FunctionDecl::doesThisDeclarationHaveABody(). This is achieved by specialization of the template GetBodyMatcher. Unfortunately template specializations do not catch the descendants of the class for which the template is specialized. Therefore it does not work correcly for the descendants of FunctionDecl, such as CXXMethodDecl, CXXConstructorDecl, CXXDestructorDecl etc. This patch fixes this issue by using a template metaprogram.

[whisperity]

What about http://clang.llvm.org/doxygen/classclang_1_1CXXConversionDecl.html ?

[baloghadamsoftware]

In D87527#2268348, @whisperity wrote:

What about http://clang.llvm.org/doxygen/classclang_1_1CXXConversionDecl.html ?

That is part of "etc.". It works for all current and future descendants of FunctionDecl now.

[lebedev.ri]

Thank you for looking into it.

[martong]

Unfortunately template specializations do not catch the descendants of the class for which the template is specialized. Therefore it does not work correcly for the descendants of FunctionDecl, such as CXXMethodDecl, CXXConstructorDecl, CXXDestructorDecl etc. This patch fixes this issue by using a template metaprogram.

Yes, implicit type conversions are not considered during template argument deduction.

First I was thinking about why don't we just simply specialize for all subclasses of FunctionDecl (CXXMethodDecl, CXXConstructorDecl, CXXDestructorDecl, etc) ?
But then I realized, your solution with enable_if is more generic and will work even if a new subclass is added. Looks good to me.

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

1626

FindsBodyOfFunctionDeclSubclasses ?

1629

"met" -> "method" ?

[ymandel]

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

[baloghadamsoftware]

In D87527#2270939, @ymandel wrote:

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

Yes, exactly. They return 2 findings instead of 1 because the matcher matches both the forward declaration and the definition. The cause for this is that template specializations do not work for the descendants of the class for which is template specialized. Instead, the generic version is used. Thus for the children of FunctionDecl the original GetBodyMatcher is instantiated which does not check whether doesThisDeclarationHaveABody() but returns a body instead which may be bound to another declaration of the same function in the declaration chain. This is obviously wrong.

[ymandel]

In D87527#2271016, @baloghadamsoftware wrote:

In D87527#2270939, @ymandel wrote:

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

Yes, exactly. They return 2 findings instead of 1 because the matcher matches both the forward declaration and the definition. The cause for this is that template specializations do not work for the descendants of the class for which is template specialized. Instead, the generic version is used. Thus for the children of FunctionDecl the original GetBodyMatcher is instantiated which does not check whether doesThisDeclarationHaveABody() but returns a body instead which may be bound to another declaration of the same function in the declaration chain. This is obviously wrong.

Thank you for clarifying. I might nitpick that it's not "obviously wrong": I'm actually writing code now that depends on exactly that behavior -- I don't care where the method decl body is, as long as it's visible in the current TU. That said, I think you're right in wanting FunctionDecl's behavior to align with that of its derived classes and I think it better to behave that way than how it currently behaves. But, we should have an alternative available in case this change breaks anyone else depending on the current behavior. Is there a different matcher with the semantics I've described?

Also, please clarify the comments in the header for this matcher. The current description is quite unclear for functions: "Matches a 'for', 'while', 'do while' statement or a function
definition that has a given body."

[baloghadamsoftware]

Thank you for clarifying. I might nitpick that it's not "obviously wrong": I'm actually writing code now that depends on exactly that behavior -- I don't care where the method decl body is, as long as it's visible in the current TU. That said, I think you're right in wanting FunctionDecl's behavior to align with that of its derived classes and I think it better to behave that way than how it currently behaves. But, we should have an alternative available in case this change breaks anyone else depending on the current behavior. Is there a different matcher with the semantics I've described?

Also, please clarify the comments in the header for this matcher. The current description is quite unclear for functions: "Matches a 'for', 'while', 'do while' statement or a function
definition that has a given body."

We must decide about the namings. If we want to be in sync with the methods in FunctionDecl, then we keep hasBody() as is, but remove the template specialization, and create a new doesThisDeclarationHaveABody(). However, this involves changing existing checks. The other solution is to fix hasBody() like in this patch, and find a new name for the other behavior, such as e.g. hasBodySomewhere() or something similar.

The comments in the header are really poorly written, but there is the word definition which means that the matcher matches function definitions and not declarations. This is surely to be rephrased.

[ymandel]

In D87527#2271059, @baloghadamsoftware wrote:

We must decide about the namings. If we want to be in sync with the methods in FunctionDecl, then we keep hasBody() as is, but remove the template specialization, and create a new doesThisDeclarationHaveABody(). However, this involves changing existing checks. The other solution is to fix hasBody() like in this patch, and find a new name for the other behavior, such as e.g. hasBodySomewhere() or something similar.

Agreed. I think we should stick with your approach, since it involves fewer changes and is consistent with the AST API itself. As for naming, hasBodySomewhere() works, but other options: hasSomeBody, hasAnyBody, hasBodyForSomeDecl.

FWIW, I checked our codebase and we definitely have a handful of uses that rely on current behavior. So, I think its important to add the new matcher before we commit this change.

The comments in the header are really poorly written, but there is the word definition which means that the matcher matches function definitions and not declarations. This is surely to be rephrased.

+1

Thanks!

[baloghadamsoftware]

Created another matcher hasAnyBody, and updated the documentation of the matchers.

[ymandel]

Thanks, this looks great! But, can you also please update https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp#L39, since it depends on the current semantics?

[baloghadamsoftware]

Checker modernize-use-equals-delete adjusted.

clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp

auto PrivateSpecialFn = cxxMethodDecl(
                                      isCopyConstructor(), isMoveConstructor())),
             cxxMethodDecl(
                 anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())),
             cxxDestructorDecl()));
 
   Finder->addMatcher(
       cxxMethodDecl(
           PrivateSpecialFn,
-          unless(anyOf(hasBody(stmt()), isDefaulted(), isDeleted(),
+          unless(anyOf(hasAnyBody(stmt()), isDefaulted(), isDeleted(),
                        ast_matchers::isTemplateInstantiation(),
                        // Ensure that all methods except private special member
                        // functions are defined.
                        hasParent(cxxRecordDecl(hasMethod(unless(
-                           anyOf(PrivateSpecialFn, hasBody(stmt()), isPure(),
+                           anyOf(PrivateSpecialFn, hasAnyBody(stmt()), isPure(),
                                  isDefaulted(), isDeleted()))))))))
           .bind(SpecialFunction),
       this);
 
   Finder->addMatcher(
       cxxMethodDecl(isDeleted(), unless(isPublic())).bind(DeletedNotPublic),
       this);
 }

clang/include/clang/ASTMatchers/ASTMatchers.h

 AST_MATCHER_P(ArraySubscriptExpr, hasBase,
               internal::Matcher<Expr>, InnerMatcher) {
   if (const Expr* Expression = Node.getBase())
     return InnerMatcher.matches(*Expression, Finder, Builder);
   return false;
 }
 
 /// Matches a 'for', 'while', 'do while' statement or a function
-/// definition that has a given body.
+/// definition that has a given body. Note that in case of functions
+/// this matcher only matches the definition itself and not the other
+/// declarations of the same function.
 ///
 /// Given
 /// \code
 ///   for (;;) {}
 /// \endcode
 /// hasBody(compoundStmt())
 ///   matches 'for (;;) {}'
 /// with compoundStmt()
 ///   matching '{}'
+///
+/// Given
+/// \code
+///   void f();
+///   void f() {}
+/// \endcode
+/// hasBody(functionDecl())
+///   matches 'void f() {}'
+/// with compoundStmt()
+///   matching '{}'
+///   but does not match 'void f();'
+
 AST_POLYMORPHIC_MATCHER_P(hasBody,
                           AST_POLYMORPHIC_SUPPORTED_TYPES(DoStmt, ForStmt,
                                                           WhileStmt,
                                                           CXXForRangeStmt,
                                                           FunctionDecl),
                           internal::Matcher<Stmt>, InnerMatcher) {
   const Stmt *const Statement = internal::GetBodyMatcher<NodeType>::get(Node);
   return (Statement != nullptr &&
           InnerMatcher.matches(*Statement, Finder, Builder));
 }
 
+/// Matches a function declaration that has a given body present in the AST.
+/// Note that this matcher matches all the declarations of a function whose
+/// body is present in the AST.
+///
+/// Given
+/// \code
+///   void f();
+///   void f() {}
+///   void g();
+/// \endcode
+/// hasAnyBody(functionDecl())
+///   matches both 'void f();'
+///   and 'void f() {}'
+/// with compoundStmt()
+///   matching '{}'
+///   but does not match 'void g();'
+AST_MATCHER_P(FunctionDecl, hasAnyBody,
+              internal::Matcher<Stmt>, InnerMatcher) {
+  const Stmt *const Statement = Node.getBody();
+  return (Statement != nullptr &&
+          InnerMatcher.matches(*Statement, Finder, Builder));
+}
+
+
 /// Matches compound statements where at least one substatement matches
 /// a given matcher. Also matches StmtExprs that have CompoundStmt as children.
 ///
 /// Given
 /// \code
 ///   { {}; 1+2; }
 /// \endcode
 /// hasAnySubstatement(compoundStmt())

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

struct NotEqualsBoundNodePredicate {
   bool operator()(const internal::BoundNodesMap &Nodes) const {
     return Nodes.getNode(ID) != Node;
   }
 
   std::string ID;
   DynTypedNode Node;
 };
 
-template <typename Ty>
-struct GetBodyMatcher {
-  static const Stmt *get(const Ty &Node) {
-    return Node.getBody();
-  }
+template <typename Ty, typename Enable = void> struct GetBodyMatcher {
+  static const Stmt *get(const Ty &Node) { return Node.getBody(); }
 };
 
-template <>
-inline const Stmt *GetBodyMatcher<FunctionDecl>::get(const FunctionDecl &Node) {
+template <typename Ty>
+struct GetBodyMatcher<Ty, typename std::enable_if<
+                              std::is_base_of<FunctionDecl, Ty>::value>::type> {
+  static const Stmt *get(const Ty &Node) {
     return Node.doesThisDeclarationHaveABody() ? Node.getBody() : nullptr;
   }
+};
 
 template <typename Ty>
 struct HasSizeMatcher {
   static bool hasSize(const Ty &Node, unsigned int N) {
     return Node.getSize() == N;
   }
 };
 

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

TEST(HasBody, FindsBodyOfForWhileDoLoops) {
   EXPECT_TRUE(notMatches("void f() { for(;;); }",
                          forStmt(hasBody(compoundStmt()))));
   EXPECT_TRUE(matches("void f() { while(true) {} }",
                       whileStmt(hasBody(compoundStmt()))));
   EXPECT_TRUE(matches("void f() { do {} while(true); }",
                       doStmt(hasBody(compoundStmt()))));
   EXPECT_TRUE(matches("void f() { int p[2]; for (auto x : p) {} }",
                       cxxForRangeStmt(hasBody(compoundStmt()))));
+}
+
+TEST(HasBody, FindsBodyOfFunctions) {
   EXPECT_TRUE(matches("void f() {}", functionDecl(hasBody(compoundStmt()))));
   EXPECT_TRUE(notMatches("void f();", functionDecl(hasBody(compoundStmt()))));
-  EXPECT_TRUE(matches("void f(); void f() {}",
-                      functionDecl(hasBody(compoundStmt()))));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "void f(); void f() {}",
+      functionDecl(hasBody(compoundStmt())).bind("func"),
+      std::make_unique<VerifyIdIsBoundTo<FunctionDecl>>("func", 1)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "class C { void f(); }; void C::f() {}",
+      cxxMethodDecl(hasBody(compoundStmt())).bind("met"),
+      std::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("met", 1)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "class C { C(); }; C::C() {}",
+      cxxConstructorDecl(hasBody(compoundStmt())).bind("ctr"),
+      std::make_unique<VerifyIdIsBoundTo<CXXConstructorDecl>>("ctr", 1)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "class C { ~C(); }; C::~C() {}",
+      cxxDestructorDecl(hasBody(compoundStmt())).bind("dtr"),
+      std::make_unique<VerifyIdIsBoundTo<CXXDestructorDecl>>("dtr", 1)));
+}
+
+TEST(HasAnyBody, FindsAnyBodyOfFunctions) {
+  EXPECT_TRUE(matches("void f() {}", functionDecl(hasAnyBody(compoundStmt()))));
+  EXPECT_TRUE(notMatches("void f();",
+                         functionDecl(hasAnyBody(compoundStmt()))));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "void f(); void f() {}",
+      functionDecl(hasAnyBody(compoundStmt())).bind("func"),
+      std::make_unique<VerifyIdIsBoundTo<FunctionDecl>>("func", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "class C { void f(); }; void C::f() {}",
+      cxxMethodDecl(hasAnyBody(compoundStmt())).bind("met"),
+      std::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("met", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "class C { C(); }; C::C() {}",
+      cxxConstructorDecl(hasAnyBody(compoundStmt())).bind("ctr"),
+      std::make_unique<VerifyIdIsBoundTo<CXXConstructorDecl>>("ctr", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "class C { ~C(); }; C::~C() {}",
+      cxxDestructorDecl(hasAnyBody(compoundStmt())).bind("dtr"),
+      std::make_unique<VerifyIdIsBoundTo<CXXDestructorDecl>>("dtr", 2)));
 }
 
 TEST(HasAnySubstatement, MatchesForTopLevelCompoundStatement) {
   // The simplest case: every compound statement is in a function
   // definition, and the function body itself must be a compound
   // statement.
   EXPECT_TRUE(matches("void f() { for (;;); }",
                       compoundStmt(hasAnySubstatement(forStmt()))));