[ASTMatcher] Add coroutineBodyStmt matcher

[ccotter]

Summary

The coroutineBodyStmt matcher matches CoroutineBodyStmt AST nodes.

[ccotter]

• Add hasBody matcher support for coroutineBodyStmt

[aaron.ballman]

Thank you for this! FWIW, it looks like precommit CI found some issues that need to be resolved.

clang/include/clang/ASTMatchers/ASTMatchers.h

5498

I'm not certain it makes sense to me to add CoroutineBodyStmt to hasBody -- in this case, it doesn't *have* a body, it *is* the body.

clang/lib/ASTMatchers/Dynamic/Registry.cpp

171

Please keep this sorted alphabetically.

[ccotter]

clang/include/clang/ASTMatchers/ASTMatchers.h

5498

With respect to hasBody(), my intent was to treat the CoroutineBodyStmt node as analogous to a FunctionDecl or WhileStmt. WhileStmts have information like the loop condition expression, as CoroutineBodyStmts contain the synthesized expressions such as the initial suspend Stmt. Both WhileStmts and CoroutineBodyStmts then have the getBody() methods, usually a CompoundStmt for WhileStmts and either a CompoundStmt or TryStmt for CoroutineBodyStmts.

Ultimately, I wanted to be able to match the CoroutineBodyStmt's function-body (using the grammar) from the standard, e.g., coroutineBodyStmt(hasBody(compoundStmt().bind(...))). If there is a different approach you'd recommend that's in line with the AST matcher design strategy, I can use that instead.

[ccotter]

• Add hasBody matcher support for coroutineBodyStmt
• update doc
• Sort list
• clang-format

[ccotter]

• Fix ParserTest.Errors

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

5498

What concerns me about the approach you have is that it doesn't model the AST. A coroutine body statement is a special kind of function body, so it does not itself *have* a body. So this is a bit like adding CompoundStmt to the hasBody matcher in that regard.

To me, the correct way to surface this would be to write the matcher: functionDecl(hasBody(coroutineBodyStmt())) to get to the coroutine body, and if you want to bind to the compound statement, I'd imagine functionDecl(hasBody(coroutineBodyStmt(has(compoundStmt().bind(...))))) would be the correct approach. My thinking there is that we'd use the has() traversal matcher to go from the coroutine body to any of the extra information it tracks (the compound statement, the promise, allocate/deallocate, etc).

Pinging @klimek and @sammccall for additional opinions.

[ccotter]

clang/include/clang/ASTMatchers/ASTMatchers.h

5498

I think I see. With my proposal, the match would be functionDecl(hasBody(coroutineBodyStmt(hasBody(stmt().bind(...))))). For completeness, your suggestion would need functionDecl(hasBody(coroutineBodyStmt(has(stmt(anyOf(cxxTryStmt(), compoundStmt()).bind(...)))))).

I am a bit hung up though on two things, and clarifications on both would help solidify my understanding of the design philosophy of the matchers. First, since CoroutineBodyStmt has a getBody() method, I assumed it would make it eligible for the hasBody matcher, although perhaps I am making an incorrect assumption/generalization here?

Second, the has() approach seems less accurate since we would be relying on the fact that the other children nodes of CoroutineBodyStmt (initial or final suspend point, etc) would never be a CompoundStmt or CXXTryStmt, else we might get unintentional matches. Or, one might miss the CXXTryStmt corner case.

Follow up question - should a need arise (say, authoring many clang-tidy checks that need extensive coroutine matcher support on the initial suspend point etc), would we see the matchers supporting things like coroutineBodyStmt(hasInitialSuspendPoint(...), hasFinalSuspendPoint(..)), or rely on a combination of has approach / non-ASTMatcher logic (or locally defined ASTMatchers within the clang-tidy library)?

It looks like this phab can be broken down into two changes - first, the addition of a coroutineBodyStmt matcher, and second, a hasBody traversal matcher for coroutineBodyStmt. Happy to separate these out depending on the direction of the discussion.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

5498

I am a bit hung up though on two things, and clarifications on both would help solidify my understanding of the design philosophy of the matchers. First, since CoroutineBodyStmt has a getBody() method, I assumed it would make it eligible for the hasBody matcher, although perhaps I am making an incorrect assumption/generalization here?

As I understand the evolution here, we started out with hasBody() to match on function declaration AST nodes with a body. It then evolved to include do/while/for (and eventually range-based for) loops, which is a bit of an odd decision (why not anything with a substatement, like if or switch?). Now we're looking to add support for the coroutine body node, but that has a half dozen+ different things we can traverse to, one of which is the compound statement for the coroutine body. All of these cases are kind of different from one another, so the design is a bit confusing (at least to me).

Second, the has() approach seems less accurate since we would be relying on the fact that the other children nodes of CoroutineBodyStmt (initial or final suspend point, etc) would never be a CompoundStmt or CXXTryStmt, else we might get unintentional matches. Or, one might miss the CXXTryStmt corner case.

That's a fantastic point that I hadn't considered. We probably will want named traversal methods to go from the coroutine body statement to the other kinds of AST nodes it supports. And based on that, hasBody() is a quite reasonable name because that's one of the other kinds of AST nodes.

Follow up question - should a need arise (say, authoring many clang-tidy checks that need extensive coroutine matcher support on the initial suspend point etc), would we see the matchers supporting things like coroutineBodyStmt(hasInitialSuspendPoint(...), hasFinalSuspendPoint(..)), or rely on a combination of has approach / non-ASTMatcher logic (or locally defined ASTMatchers within the clang-tidy library)?

I'm leaning more towards introducing new traversal matchers. In terms of what goes into ASTMatchers.h, we usually only add matchers once there are multiple in-tree needs for the matcher, otherwise we recommend using a locally defined matcher. ASTMatchers.h is quite expensive to compile due to all the template shenanigans, and so we've resisted adding matchers unless they're "needed" to try to keep compile times reasonable.

Hmmm, I'm coming around to reusing hasBody() like you've done here. I think it's pretty weird to read: functionDecl(hasBody(coroutineBodyStmt(hasBody(stmt().bind(...))) because of the duplicated calls to hasBody, but I think it's more defensible now than I was originally thinking. I'm still curious if other folks who care about AST matchers have an opinion given that this is a bit trickier than usual.

[ccotter]

Bump - anyone else have any thoughts here?

[aaron.ballman]

Hmmm, I'm coming around to reusing hasBody() like you've done here. I think it's pretty weird to read: functionDecl(hasBody(coroutineBodyStmt(hasBody(stmt().bind(...))) because of the duplicated calls to hasBody, but I think it's more defensible now than I was originally thinking. I'm still curious if other folks who care about AST matchers have an opinion given that this is a bit trickier than usual.

I think I'm okay with this direction, but one last try at pinging @klimek and @sammccall in case they've got an opinion. If we don't hear from them by Thur of this week, I'd say it's fine to move forward with the patch as-is.

clang/docs/ReleaseNotes.rst

261

You should also mention that the hasBody matcher now works with coroutine bodies.

[sammccall]

It seems pretty weird to me that the two child edges from functiondecl -> coroutinebodystmt -> compoundstmt are both called "body".

However that *is* what they're called in the AST today, and having the matchers correspond to the AST's names seems important.
So this seems OK to me (renaming the inner "body" edge to something else seems like a good idea, but is out of scope here).

[ccotter]

• update release note, rebase

[ccotter]

fix bad 'arc diff'

[aaron.ballman]

LGTM! Thanks @sammccall for weighing in, I appreciate it. :-)

[ccotter]

Thanks both! Assuming this passes build, would one of you mind committing this for me? Chris Cotter <[e-mail]>

[aaron.ballman]

In D140794#4160358, @ccotter wrote:

Thanks both! Assuming this passes build, would one of you mind committing this for me? Chris Cotter <[e-mail]>

Happy to do so! I've landed it on your behalf in dcf5ad89dcc5cc69b9df3e5dd9be71c65642f519

clang/docs/ReleaseNotes.rst

 -------------------------------
 - Add ``__builtin_elementwise_log`` builtin for floating point types only.
 - Add ``__builtin_elementwise_log10`` builtin for floating point types only.
 - Add ``__builtin_elementwise_log2`` builtin for floating point types only.
 
 AST Matchers
 ------------
 
+- Add ``coroutineBodyStmt`` matcher.
+
+- The ``hasBody`` matcher now matches coroutine body nodes in
+  ``CoroutineBodyStmts``.
+
 clang-format
 ------------
 
 - Add ``NextLineOnly`` style to option ``PackConstructorInitializers``.
   Compared to ``NextLine`` style, ``NextLineOnly`` style will not try to
   put the initializers on the current line first, instead, it will try to
   put the initializers on the next line only.
 

clang/include/clang/ASTMatchers/ASTMatchers.h

 /// \code
 ///   co_yield 1;
 /// \endcode
 /// coyieldExpr()
 ///   matches 'co_yield 1'
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, CoyieldExpr>
     coyieldExpr;
 
+/// Matches coroutine body statements.
+///
+/// coroutineBodyStmt() matches the coroutine below
+/// \code
+///   generator<int> gen() {
+///     co_return;
+///   }
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, CoroutineBodyStmt>
+    coroutineBodyStmt;
+
 /// Matches nullptr literal.
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, CXXNullPtrLiteralExpr>
     cxxNullPtrLiteralExpr;
 
 /// Matches GNU __builtin_choose_expr.
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, ChooseExpr>
     chooseExpr;
 

 ///   matches \c i[1] with the \c declRefExpr() matching \c i
 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' statement or a function 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.
+/// Matches a 'for', 'while', 'while' statement or a function or coroutine
+/// definition that has a given body. Note that in case of functions or
+/// coroutines this matcher only matches the definition itself and not the
+/// other declarations of the same function or coroutine.
 ///
 /// Given
 /// \code
 ///   for (;;) {}
 /// \endcode
 /// forStmt(hasBody(compoundStmt()))
 ///   matches 'for (;;) {}'
 /// with compoundStmt()
 ///   matching '{}'
 ///
 /// Given
 /// \code
 ///   void f();
 ///   void f() {}
 /// \endcode
 /// functionDecl(hasBody(compoundStmt()))
 ///   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) {
+AST_POLYMORPHIC_MATCHER_P(
+    hasBody,
+    AST_POLYMORPHIC_SUPPORTED_TYPES(DoStmt, ForStmt, WhileStmt, CXXForRangeStmt,
+                                    FunctionDecl, CoroutineBodyStmt),
   if (Finder->isTraversalIgnoringImplicitNodes() && isDefaultedHelper(&Node))
     return false;
   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.

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

 const internal::VariadicDynCastAllOfMatcher<Stmt, GotoStmt> gotoStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, LabelStmt> labelStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, AddrLabelExpr> addrLabelExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, SwitchStmt> switchStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, SwitchCase> switchCase;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CaseStmt> caseStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, DefaultStmt> defaultStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CompoundStmt> compoundStmt;
+const internal::VariadicDynCastAllOfMatcher<Stmt, CoroutineBodyStmt>
+    coroutineBodyStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXCatchStmt> cxxCatchStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXTryStmt> cxxTryStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXThrowExpr> cxxThrowExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, NullStmt> nullStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, AsmStmt> asmStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXBoolLiteralExpr>
     cxxBoolLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, StringLiteral> stringLiteral;

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(characterLiteral);
   REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplatePartialSpecializationDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
   REGISTER_MATCHER(compoundLiteralExpr);
   REGISTER_MATCHER(compoundStmt);
   REGISTER_MATCHER(coawaitExpr);
   REGISTER_MATCHER(conditionalOperator);
   REGISTER_MATCHER(constantArrayType);
   REGISTER_MATCHER(constantExpr);
   REGISTER_MATCHER(containsDeclaration);
   REGISTER_MATCHER(continueStmt);
   REGISTER_MATCHER(coreturnStmt);
+  REGISTER_MATCHER(coroutineBodyStmt);
   REGISTER_MATCHER(coyieldExpr);
   REGISTER_MATCHER(cudaKernelCallExpr);
   REGISTER_MATCHER(cxxBaseSpecifier);
   REGISTER_MATCHER(cxxBindTemporaryExpr);
   REGISTER_MATCHER(cxxBoolLiteral);
   REGISTER_MATCHER(cxxCatchStmt);
   REGISTER_MATCHER(cxxConstCastExpr);
   REGISTER_MATCHER(cxxConstructExpr);

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

 #include <coro_header>
 void check_match_co_yield() {
   co_yield 1.0;
 }
 )cpp";
   EXPECT_TRUE(matchesConditionally(CoYieldCode, 
                                    coyieldExpr(isExpansionInMainFile()), 
                                    true, {"-std=c++20", "-I/"}, M));
+
+  StringRef NonCoroCode = R"cpp(
+#include <coro_header>
+void non_coro_function() {
+}
+)cpp";
+
+  EXPECT_TRUE(matchesConditionally(CoReturnCode, coroutineBodyStmt(), true,
+                                   {"-std=c++20", "-I/"}, M));
+  EXPECT_TRUE(matchesConditionally(CoAwaitCode, coroutineBodyStmt(), true,
+                                   {"-std=c++20", "-I/"}, M));
+  EXPECT_TRUE(matchesConditionally(CoYieldCode, coroutineBodyStmt(), true,
+                                   {"-std=c++20", "-I/"}, M));
+
+  EXPECT_FALSE(matchesConditionally(NonCoroCode, coroutineBodyStmt(), true,
+                                    {"-std=c++20", "-I/"}, M));
+
+  StringRef CoroWithDeclCode = R"cpp(
+#include <coro_header>
+void coro() {
+  int thevar;
+  co_return 1;
+}
+)cpp";
+  EXPECT_TRUE(matchesConditionally(
+      CoroWithDeclCode,
+      coroutineBodyStmt(hasBody(compoundStmt(
+          has(declStmt(containsDeclaration(0, varDecl(hasName("thevar")))))))),
+      true, {"-std=c++20", "-I/"}, M));
+
+  StringRef CoroWithTryCatchDeclCode = R"cpp(
+#include <coro_header>
+void coro() try {
+  int thevar;
+  co_return 1;
+} catch (...) {}
+)cpp";
+  EXPECT_TRUE(matchesConditionally(
+      CoroWithTryCatchDeclCode,
+      coroutineBodyStmt(hasBody(cxxTryStmt(has(compoundStmt(has(
+          declStmt(containsDeclaration(0, varDecl(hasName("thevar")))))))))),
+      true, {"-std=c++20", "-I/"}, M));
 }
 
 TEST(Matcher, isClassMessage) {
   EXPECT_TRUE(matchesObjC(
       "@interface NSString +(NSString *) stringWithFormat; @end "
       "void f() { [NSString stringWithFormat]; }",
       objcMessageExpr(isClassMessage())));
 

clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

EXPECT_EQ(
       "1:22: Error parsing matcher. Found end-of-code while looking for ')'.",
       ParseWithError("mapAnyOf(ifStmt).with("));
   EXPECT_EQ("1:1: Failed to build matcher: mapAnyOf.",
             ParseWithError("mapAnyOf()"));
   EXPECT_EQ("1:1: Error parsing argument 1 for matcher mapAnyOf.\n1:1: Failed "
             "to build matcher: mapAnyOf.",
             ParseWithError("mapAnyOf(\"foo\")"));
   EXPECT_EQ("Input value has unresolved overloaded type: "
-            "Matcher<DoStmt|ForStmt|WhileStmt|CXXForRangeStmt|FunctionDecl>",
+            "Matcher<DoStmt|ForStmt|WhileStmt|CXXForRangeStmt|FunctionDecl|"
+            "CoroutineBodyStmt>",
             ParseMatcherWithError("hasBody(stmt())"));
   EXPECT_EQ(
       "1:1: Error parsing argument 1 for matcher decl.\n"
       "1:6: Error building matcher hasAttr.\n"
       "1:14: Unknown value 'attr::Fnal' for arg 1; did you mean 'attr::Final'",
       ParseMatcherWithError(R"query(decl(hasAttr("attr::Fnal")))query"));
   EXPECT_EQ("1:1: Error parsing argument 1 for matcher decl.\n"
             "1:6: Error building matcher hasAttr.\n"