[ASTMatcher] Add AST Matcher support for C++20 coroutine keywords

[ChuanqiXu]

Summary

It seems like we can't match co_* keywords introduced from C++20 directly. So I try to enable the support for that.

[sammccall]

This looks good, as far as it goes - it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

You'll need to run clang/docs/tools/dump_ast_matchers.py to update the docs for the new matchers.

clang/include/clang/ASTMatchers/ASTMatchers.h

2414

dependend ->dependent

[njames93]

Small stylistic nit, is it better to have these matchers named coAwaitExpr, coYieldExpr etc. I know the types in clang aren't capitalized but they are internal to clang. Matchers are user facing so having a nicer style is more important.

clang/lib/ASTMatchers/Dynamic/Registry.cpp

195–197

Please keep this in alphabetical order.

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

559–572

This is a little unsatisfactory for a test. You aren't ensuring that coReturnStmt() is matching the co_return etc.
A better test could creating a FileContentMappings object and use it to create a header file with all the boilerplate.
Then you can have test code like this:

#include "my_header.h"
void check_match_co_return() {
  co_return 1;
}

And test that code against coreturnStmt(isExpansionInMainFile()).
This will be a very precise test case.
You'd have to use matchesConditionally instead of matches in order to pass the headers.

[ChuanqiXu]

Fix typos and update test cases.

[ChuanqiXu]

update clang/docs/LibASTMatchersReference.html

[ChuanqiXu]

In D96316#2551227, @sammccall wrote:

it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

I am confused about the statements? What does it mean?

[njames93]

In D96316#2570425, @ChuanqiXu wrote:

In D96316#2551227, @sammccall wrote:

it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

I am confused about the statements? What does it mean?

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

[ChuanqiXu]

In D96316#2577545, @njames93 wrote:

In D96316#2570425, @ChuanqiXu wrote:

In D96316#2551227, @sammccall wrote:

it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

I am confused about the statements? What does it mean?

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

I am not familiar with ASTMatchers. And from the link you give, it looks like that coyieldExpr, coreturnStmt and coawaitExpr should be Node Matchers instead of Traversal Matchers.

[njames93]

In D96316#2578039, @ChuanqiXu wrote:

In D96316#2577545, @njames93 wrote:

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

I am not familiar with ASTMatchers. And from the link you give, it looks like that coyieldExpr, coreturnStmt and coawaitExpr should be Node Matchers instead of Traversal Matchers.

Those should be Node matchers, the traversal matchers would be the yields(...), returns(...) and hasOperand(...) matchers.

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

569

nit: use c++20, same for the tests below.

[ChuanqiXu]

update c++20 with c++2a

[ChuanqiXu]

In D96316#2578640, @njames93 wrote:

In D96316#2578039, @ChuanqiXu wrote:

In D96316#2577545, @njames93 wrote:

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

I am not familiar with ASTMatchers. And from the link you give, it looks like that coyieldExpr, coreturnStmt and coawaitExpr should be Node Matchers instead of Traversal Matchers.

Those should be Node matchers, the traversal matchers would be the yields(...), returns(...) and hasOperand(...) matchers.

So is it ok to omit the implementation like yields ?

[njames93]

In D96316#2587180, @ChuanqiXu wrote:>

So is it ok to omit the implementation like yields ?

Yes, that can be done in a follow up and there's no onus on you to do it.

[ChuanqiXu]

In D96316#2587529, @njames93 wrote:

In D96316#2587180, @ChuanqiXu wrote:>

So is it ok to omit the implementation like yields ?

Yes, that can be done in a follow up and there's no onus on you to do it.

Thanks for reviewing! Do you think is it ok to commit this patch?

[ChuanqiXu]

@njames93 @sammccall gental ping~

[njames93]

I'm still of the opinion we should be capitalizing these, coYieldExpr etc, but I'd prefer feedback from other reviewers before making that change.
Also some small changes to the matchers documentation while will require re running the documentation script.

clang/include/clang/ASTMatchers/ASTMatchers.h

2403–2408

nit:

2411–2412

nit:

2415–2420

nit:

[steveire]

In D96316#2606661, @njames93 wrote:

I'm still of the opinion we should be capitalizing these, coYieldExpr etc, but I'd prefer feedback from other reviewers before making that change.
Also some small changes to the matchers documentation while will require re running the documentation script.

The convention which is almost completely followed currently is to use the name of the AST class and lowercase the prefix (ie CXX to cxx). In the AST class, the Co is not a prefix, because the AST class is not called CoYieldExpr but CoyieldExpr.

So, I think the matchers should continue to follow the convention and be called coyieldExpr.

[sammccall]

LGTM with the doc nits from njames93 (and the doc generator script re-run)

In D96316#2614784, @steveire wrote:

The convention which is almost completely followed currently is to use the name of the AST class and lowercase the prefix (ie CXX to cxx). In the AST class, the Co is not a prefix, because the AST class is not called CoYieldExpr but CoyieldExpr.

So, I think the matchers should continue to follow the convention and be called coyieldExpr.

+1, it's more important (and realistic) to match the names of the AST nodes than any other names.

[njames93]

In D96316#2615079, @sammccall wrote:

+1, it's more important (and realistic) to match the names of the AST nodes than any other names.

I think my gripe is that in my opinion the ast nodes should have be named CoYieldExpr etc.

[ChuanqiXu]

Re-run documentation with the nit suggested.

Thanks for reviewing!

[aaron.ballman]

LGTM

clang/include/clang/ASTMatchers/ASTMatchers.h

2413

clang/include/clang/ASTMatchers/ASTMatchers.h

 /// \code
 ///   while (true) { continue; }
 /// \endcode
 /// continueStmt()
 ///   matches 'continue'
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, ContinueStmt>
     continueStmt;
 
+/// Matches co_return statements.
+///
+/// Given
+/// \code
+///   while (true) { co_return; }
+/// \endcode
+/// coreturnStmt()
+///   matches 'co_return'
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, CoreturnStmt>
+    coreturnStmt;
+
 /// Matches return statements.
 ///
 /// Given
 /// \code
 ///   return 1;
 /// \endcode
 /// returnStmt()
 ///   matches 'return 1'

 /// Example match: {1}, (1, 2)
 /// \code
 ///   int array[4] = {1};
 ///   vector int myvec = (vector int)(1, 2);
 /// \endcode
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, CompoundLiteralExpr>
     compoundLiteralExpr;
 
+/// Matches co_await expression
+///
+/// Example match: co_await 1
+/// \code
+///   co_await 1;
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, CoawaitExpr>
+    coawaitExpr;
+/// Matches dependent co_await expression while the type of the promise
+/// is dependent
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, DependentCoawaitExpr>
+    dependendCoawaitExpr;
+/// Matches co_yield expression
+///
+/// Example match: co_yield 1
+/// \code
+///   co_yield 1;
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Stmt, CoyieldExpr>
+    coyieldExpr;
+
 /// Matches nullptr literal.
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, CXXNullPtrLiteralExpr>
     cxxNullPtrLiteralExpr;
 
 /// Matches GNU __builtin_choose_expr.
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, ChooseExpr>
     chooseExpr;
 

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

 const internal::VariadicDynCastAllOfMatcher<Stmt, IfStmt> ifStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, ForStmt> forStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXForRangeStmt>
     cxxForRangeStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, WhileStmt> whileStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, DoStmt> doStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, BreakStmt> breakStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, ContinueStmt> continueStmt;
+const internal::VariadicDynCastAllOfMatcher<Stmt, CoreturnStmt> coreturnStmt;
 const internal::VariadicDynCastAllOfMatcher<Stmt, ReturnStmt> returnStmt;
 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, FixedPointLiteral>
     fixedPointLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, UserDefinedLiteral>
     userDefinedLiteral;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CompoundLiteralExpr>
     compoundLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, CXXNullPtrLiteralExpr>
     cxxNullPtrLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, ChooseExpr> chooseExpr;
+const internal::VariadicDynCastAllOfMatcher<Stmt, CoawaitExpr>
+    coawaitExpr;
+const internal::VariadicDynCastAllOfMatcher<Stmt, DependentCoawaitExpr>
+    dependendCoawaitExpr;
+const internal::VariadicDynCastAllOfMatcher<Stmt, CoyieldExpr>
+    coyieldExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, GNUNullExpr> gnuNullExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, GenericSelectionExpr>
     genericSelectionExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, AtomicExpr> atomicExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, StmtExpr> stmtExpr;
 const internal::VariadicDynCastAllOfMatcher<Stmt, BinaryOperator>
     binaryOperator;
 const internal::MapAnyOfMatcher<BinaryOperator, CXXOperatorCallExpr,

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(complexType);
   REGISTER_MATCHER(compoundLiteralExpr);
   REGISTER_MATCHER(compoundStmt);
   REGISTER_MATCHER(conditionalOperator);
   REGISTER_MATCHER(constantArrayType);
   REGISTER_MATCHER(constantExpr);
   REGISTER_MATCHER(containsDeclaration);
   REGISTER_MATCHER(continueStmt);
+  REGISTER_MATCHER(coreturnStmt);
   REGISTER_MATCHER(cudaKernelCallExpr);
   REGISTER_MATCHER(cxxBindTemporaryExpr);
   REGISTER_MATCHER(cxxBoolLiteral);
   REGISTER_MATCHER(cxxCatchStmt);
   REGISTER_MATCHER(cxxConstCastExpr);
   REGISTER_MATCHER(cxxConstructExpr);
   REGISTER_MATCHER(cxxConstructorDecl);
   REGISTER_MATCHER(cxxConversionDecl);
   REGISTER_MATCHER(cxxCtorInitializer);
   REGISTER_MATCHER(cxxDeductionGuideDecl);
   REGISTER_MATCHER(cxxDefaultArgExpr);
   REGISTER_MATCHER(cxxDeleteExpr);
   REGISTER_MATCHER(cxxDependentScopeMemberExpr);
   REGISTER_MATCHER(cxxDestructorDecl);
   REGISTER_MATCHER(cxxDynamicCastExpr);
   REGISTER_MATCHER(cxxForRangeStmt);
   REGISTER_MATCHER(cxxFunctionalCastExpr);
   REGISTER_MATCHER(cxxMemberCallExpr);
   REGISTER_MATCHER(cxxMethodDecl);
   REGISTER_MATCHER(cxxNewExpr);
+  REGISTER_MATCHER(coawaitExpr);
+  REGISTER_MATCHER(dependendCoawaitExpr);
+  REGISTER_MATCHER(coyieldExpr);
   REGISTER_MATCHER(cxxNoexceptExpr);
   REGISTER_MATCHER(cxxNullPtrLiteralExpr);
   REGISTER_MATCHER(cxxOperatorCallExpr);
   REGISTER_MATCHER(cxxRecordDecl);
   REGISTER_MATCHER(cxxReinterpretCastExpr);
   REGISTER_MATCHER(cxxRewrittenBinaryOperator);
   REGISTER_MATCHER(cxxStaticCastExpr);
   REGISTER_MATCHER(cxxStdInitializerListExpr);

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

EXPECT_TRUE(matches(
                 hasAncestor(lambdaExpr(hasAncestor(varDecl(hasName("l"))))))));
   EXPECT_TRUE(matches(
       Code, cxxConversionDecl(hasBody(compoundStmt(has(returnStmt(
                                   hasReturnValue(implicitCastExpr()))))),
                               hasAncestor(lambdaExpr(hasAncestor(
                                   varDecl(hasName("lPtrDecay"))))))));
 }
 
+TEST(Matcher, MatchesCoroutine) {
+  StringRef Code = R"cpp(
+namespace std {
+namespace experimental {
+
+template <class... Args>
+struct void_t_imp {
+  using type = void;
+};
+template <class... Args>
+using void_t = typename void_t_imp<Args...>::type;
+
+template <class T, class = void>
+struct traits_sfinae_base {};
+
+template <class T>
+struct traits_sfinae_base<T, void_t<typename T::promise_type>> {
+  using promise_type = typename T::promise_type;
+};
+
+template <class Ret, class... Args>
+struct coroutine_traits : public traits_sfinae_base<Ret> {};
+}}  // namespace std::experimental
+struct awaitable {
+  bool await_ready() noexcept;
+  template <typename F>
+  void await_suspend(F) noexcept;
+  void await_resume() noexcept;
+} a;
+struct promise {
+  void get_return_object();
+  awaitable initial_suspend();
+  awaitable final_suspend() noexcept;
+  awaitable yield_value(int); // expected-note 2{{candidate}}
+  void return_value(int); // expected-note 2{{here}}
+  void unhandled_exception();
+};
+template <typename... T>
+struct std::experimental::coroutine_traits<void, T...> { using promise_type = promise; };
+namespace std {
+namespace experimental {
+template <class PromiseType = void>
+struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+};
+}} // namespace std::experimental
+void check_match_co_await() {
+  co_await a;
+}
+void check_match_co_yield() {
+  co_yield 1.0;
+}
+void check_match_co_return() {
+  co_return 1;
+}
+)cpp";
+
+  EXPECT_TRUE(matches(Code, coawaitExpr(), Lang_CXX20));
+  EXPECT_TRUE(matches(Code, coyieldExpr(), Lang_CXX20));
+  EXPECT_TRUE(matches(Code, coreturnStmt(), Lang_CXX20));
+}
+
 TEST(Matcher, isClassMessage) {
   EXPECT_TRUE(matchesObjC(
       "@interface NSString +(NSString *) stringWithFormat; @end "
       "void f() { [NSString stringWithFormat]; }",
       objcMessageExpr(isClassMessage())));
 
   EXPECT_FALSE(matchesObjC(
       "@interface NSString @end "