[AST][Coroutine] Fix CoyieldExpr missing end loc

[danix800]

Summary

Coroutine co_yield/co_await/co_return are implemented by a serious
of synthesized CXXMemberExpr which have no lexical right-side parenthesis.

This fix uses the end loc of inner expr as the hypothetical RParenLoc of CXXMemberExpr.
For tools this might be an issue since the RParen token doesn't exist (but has a valid location).

For future improvement, we might:

1. mark those inner (generated) exprs as implict (tools have chances to skip these nodes) (by @aaron.ballman)
2. borrow the idea from InitListExpr, there are two forms, one is for semantic, the other one is for syntactic, having these two split can make everything easier (by @hokein)

Fixes https://github.com/llvm/llvm-project/issues/64483

[tbaeder]

clang/lib/Sema/SemaCoroutine.cpp

321–322

Why getBeginLoc() and not getEndLoc()?

[danix800]

clang/lib/Sema/SemaCoroutine.cpp

321–322

Aha! Thinko I guess.

Previously I noticed that expr of single ID only tracks its begin loc (end loc == begin loc). So the getBeginLoc().

But for composite expr it's not the case, end loc == last single ID's begin loc.

Here last arg could be a composite expr too and getEndLoc() should be used.

[danix800]

Use getEndLoc() instead of getBeginLoc().

[aaron.ballman]

LGTM!

[hokein]

clang/lib/Sema/SemaCoroutine.cpp

322

Thanks for the fast fix.

Reading the source code here, I'm not sure this is a correct fix (and put the heuristic here). I assume the Loc points the the co_yield token here, passing the Loc to the LParenLoc and RParenLoc parameters seems wrong to me, there is no ( and ) written in the source code here (because the member call expression is an implicit generated node in the clang AST), we should pass an invalid source location (SourceLocation()).

The CallExpr::getEndLoc() has implemented similar heuristic to handle the case where the RParenLoc is invalid.

[aaron.ballman]

clang/lib/Sema/SemaCoroutine.cpp

322

passing the Loc to the LParenLoc and RParenLoc parameters seems wrong to me

FWIW, I thought so as well, but I thought we have the same behavior for CoreturnExpr and CoawaitExpr. These statements are not really call expressions as far as the AST is concerned, so it's a bit hard to say whether there is a right or wrong answer for where the l- and r-paren locations are for the call.

[danix800]

1. Use invalid loc: SourceLocation()

ExprWithCleanups 0x55d1067162c8 <line:68:3, col:12> 'void'
`-CoyieldExpr 0x55d106716280 <col:3, col:12> 'void'
  |-CXXMemberCallExpr 0x55d106715ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  | |-MemberExpr 0x55d106715ea8 <col:3> '<bound member function type>' .yield_value 0x55d10670b220
  | | `-DeclRefExpr 0x55d106715e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55d106714218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  | `-ImplicitCastExpr 0x55d106715f00 <col:12> 'int' <LValueToRValue>
  |   `-DeclRefExpr 0x55d1067118e8 <col:12> 'int' lvalue ParmVar 0x55d1067116c0 'sss' 'int'
  |-MaterializeTemporaryExpr 0x55d106715f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | `-CXXMemberCallExpr 0x55d106715ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  |   |-MemberExpr 0x55d106715ea8 <col:3> '<bound member function type>' .yield_value 0x55d10670b220
  |   | `-DeclRefExpr 0x55d106715e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55d106714218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  |   `-ImplicitCastExpr 0x55d106715f00 <col:12> 'int' <LValueToRValue>
  |     `-DeclRefExpr 0x55d1067118e8 <col:12> 'int' lvalue ParmVar 0x55d1067116c0 'sss' 'int'
  |-ExprWithCleanups 0x55d106715fd8 <col:3, <invalid sloc>> 'bool'
  | `-CXXMemberCallExpr 0x55d106715fb8 <col:3, <invalid sloc>> 'bool'
  |   `-MemberExpr 0x55d106715f88 <col:3> '<bound member function type>' .await_ready 0x55d106708858
  |     `-OpaqueValueExpr 0x55d106715f70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  |       `-MaterializeTemporaryExpr 0x55d106715f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  |         `-CXXMemberCallExpr 0x55d106715ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  |           |-MemberExpr 0x55d106715ea8 <col:3> '<bound member function type>' .yield_value 0x55d10670b220
  |           | `-DeclRefExpr 0x55d106715e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55d106714218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  |           `-ImplicitCastExpr 0x55d106715f00 <col:12> 'int' <LValueToRValue>
  |             `-DeclRefExpr 0x55d1067118e8 <col:12> 'int' lvalue ParmVar 0x55d1067116c0 'sss' 'int'
  |-CXXMemberCallExpr 0x55d106716160 <col:3> 'void'
  | |-MemberExpr 0x55d106716130 <col:3> '<bound member function type>' .await_suspend 0x55d106708ad8
  | | `-OpaqueValueExpr 0x55d106715f70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | |   `-MaterializeTemporaryExpr 0x55d106715f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | |     `-CXXMemberCallExpr 0x55d106715ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  | |       |-MemberExpr 0x55d106715ea8 <col:3> '<bound member function type>' .yield_value 0x55d10670b220
  | |       | `-DeclRefExpr 0x55d106715e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55d106714218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  | |       `-ImplicitCastExpr 0x55d106715f00 <col:12> 'int' <LValueToRValue>
  | |         `-DeclRefExpr 0x55d1067118e8 <col:12> 'int' lvalue ParmVar 0x55d1067116c0 'sss' 'int'
  | `-ImplicitCastExpr 0x55d106716218 <col:3> 'coroutine_handle<>':'std::coroutine_handle<void>' <ConstructorConversion>
  |   `-CXXConstructExpr 0x55d1067161e8 <col:3> 'coroutine_handle<>':'std::coroutine_handle<void>' 'void (coroutine_handle<promise_type>) noexcept'
  |     `-CallExpr 0x55d1067160d0 <col:3> 'coroutine_handle<promise_type>':'std::coroutine_handle<Chat::promise_type>'
  |       |-ImplicitCastExpr 0x55d1067160b8 <col:3> 'coroutine_handle<promise_type> (*)(void *) noexcept' <FunctionToPointerDecay>
  |       | `-DeclRefExpr 0x55d106716098 <col:3> 'coroutine_handle<promise_type> (void *) noexcept' lvalue CXXMethod 0x55d10670dd98 'from_address' 'coroutine_handle<promise_type> (void *) noexcept'
  |       `-CallExpr 0x55d106716078 <col:3> 'void *'
  |         `-ImplicitCastExpr 0x55d106716060 <col:3> 'void *(*)() noexcept' <FunctionToPointerDecay>
  |           `-DeclRefExpr 0x55d106716040 <col:3> 'void *() noexcept' lvalue Function 0x55d106714c88 '__builtin_coro_frame' 'void *() noexcept'
  `-CXXMemberCallExpr 0x55d106716260 <col:3, <invalid sloc>> 'void'
    `-MemberExpr 0x55d106716230 <col:3> '<bound member function type>' .await_resume 0x55d106708bf8
      `-OpaqueValueExpr 0x55d106715f70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
        `-MaterializeTemporaryExpr 0x55d106715f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
          `-CXXMemberCallExpr 0x55d106715ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
            |-MemberExpr 0x55d106715ea8 <col:3> '<bound member function type>' .yield_value 0x55d10670b220
            | `-DeclRefExpr 0x55d106715e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55d106714218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
            `-ImplicitCastExpr 0x55d106715f00 <col:12> 'int' <LValueToRValue>
              `-DeclRefExpr 0x55d1067118e8 <col:12> 'int' lvalue ParmVar 0x55d1067116c0 'sss' 'int'

Note that CXXMemberCallExpr 0x55d106716260 <col:3, <invalid sloc>> relies on RParenLoc directly. Coroutine is implemented as member call so I think RParen is intended and should exist and be correct.

2. Use last arg's end loc if exists: auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();

ExprWithCleanups 0x55dcfa09f2c8 <line:68:3, col:12> 'void'
`-CoyieldExpr 0x55dcfa09f280 <col:3, col:12> 'void'
  |-CXXMemberCallExpr 0x55dcfa09eed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  | |-MemberExpr 0x55dcfa09eea8 <col:3> '<bound member function type>' .yield_value 0x55dcfa094220
  | | `-DeclRefExpr 0x55dcfa09ee88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55dcfa09d218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  | `-ImplicitCastExpr 0x55dcfa09ef00 <col:12> 'int' <LValueToRValue>
  |   `-DeclRefExpr 0x55dcfa09a8e8 <col:12> 'int' lvalue ParmVar 0x55dcfa09a6c0 'sss' 'int'
  |-MaterializeTemporaryExpr 0x55dcfa09ef58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | `-CXXMemberCallExpr 0x55dcfa09eed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  |   |-MemberExpr 0x55dcfa09eea8 <col:3> '<bound member function type>' .yield_value 0x55dcfa094220
  |   | `-DeclRefExpr 0x55dcfa09ee88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55dcfa09d218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  |   `-ImplicitCastExpr 0x55dcfa09ef00 <col:12> 'int' <LValueToRValue>
  |     `-DeclRefExpr 0x55dcfa09a8e8 <col:12> 'int' lvalue ParmVar 0x55dcfa09a6c0 'sss' 'int'
  |-ExprWithCleanups 0x55dcfa09efd8 <col:3> 'bool'
  | `-CXXMemberCallExpr 0x55dcfa09efb8 <col:3> 'bool'
  |   `-MemberExpr 0x55dcfa09ef88 <col:3> '<bound member function type>' .await_ready 0x55dcfa091858
  |     `-OpaqueValueExpr 0x55dcfa09ef70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  |       `-MaterializeTemporaryExpr 0x55dcfa09ef58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  |         `-CXXMemberCallExpr 0x55dcfa09eed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  |           |-MemberExpr 0x55dcfa09eea8 <col:3> '<bound member function type>' .yield_value 0x55dcfa094220
  |           | `-DeclRefExpr 0x55dcfa09ee88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55dcfa09d218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  |           `-ImplicitCastExpr 0x55dcfa09ef00 <col:12> 'int' <LValueToRValue>
  |             `-DeclRefExpr 0x55dcfa09a8e8 <col:12> 'int' lvalue ParmVar 0x55dcfa09a6c0 'sss' 'int'
  |-CXXMemberCallExpr 0x55dcfa09f160 <col:3> 'void'
  | |-MemberExpr 0x55dcfa09f130 <col:3> '<bound member function type>' .await_suspend 0x55dcfa091ad8
  | | `-OpaqueValueExpr 0x55dcfa09ef70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | |   `-MaterializeTemporaryExpr 0x55dcfa09ef58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | |     `-CXXMemberCallExpr 0x55dcfa09eed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  | |       |-MemberExpr 0x55dcfa09eea8 <col:3> '<bound member function type>' .yield_value 0x55dcfa094220
  | |       | `-DeclRefExpr 0x55dcfa09ee88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55dcfa09d218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
  | |       `-ImplicitCastExpr 0x55dcfa09ef00 <col:12> 'int' <LValueToRValue>
  | |         `-DeclRefExpr 0x55dcfa09a8e8 <col:12> 'int' lvalue ParmVar 0x55dcfa09a6c0 'sss' 'int'
  | `-ImplicitCastExpr 0x55dcfa09f218 <col:3> 'coroutine_handle<>':'std::coroutine_handle<void>' <ConstructorConversion>
  |   `-CXXConstructExpr 0x55dcfa09f1e8 <col:3> 'coroutine_handle<>':'std::coroutine_handle<void>' 'void (coroutine_handle<promise_type>) noexcept'
  |     `-CallExpr 0x55dcfa09f0d0 <col:3> 'coroutine_handle<promise_type>':'std::coroutine_handle<Chat::promise_type>'
  |       |-ImplicitCastExpr 0x55dcfa09f0b8 <col:3> 'coroutine_handle<promise_type> (*)(void *) noexcept' <FunctionToPointerDecay>
  |       | `-DeclRefExpr 0x55dcfa09f098 <col:3> 'coroutine_handle<promise_type> (void *) noexcept' lvalue CXXMethod 0x55dcfa096d98 'from_address' 'coroutine_handle<promise_type> (void *) noexcept'
  |       `-CallExpr 0x55dcfa09f078 <col:3> 'void *'
  |         `-ImplicitCastExpr 0x55dcfa09f060 <col:3> 'void *(*)() noexcept' <FunctionToPointerDecay>
  |           `-DeclRefExpr 0x55dcfa09f040 <col:3> 'void *() noexcept' lvalue Function 0x55dcfa09dc88 '__builtin_coro_frame' 'void *() noexcept'
  `-CXXMemberCallExpr 0x55dcfa09f260 <col:3> 'void'
    `-MemberExpr 0x55dcfa09f230 <col:3> '<bound member function type>' .await_resume 0x55dcfa091bf8
      `-OpaqueValueExpr 0x55dcfa09ef70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
        `-MaterializeTemporaryExpr 0x55dcfa09ef58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
          `-CXXMemberCallExpr 0x55dcfa09eed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
            |-MemberExpr 0x55dcfa09eea8 <col:3> '<bound member function type>' .yield_value 0x55dcfa094220
            | `-DeclRefExpr 0x55dcfa09ee88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x55dcfa09d218 '__promise' 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type'
            `-ImplicitCastExpr 0x55dcfa09ef00 <col:12> 'int' <LValueToRValue>
              `-DeclRefExpr 0x55dcfa09a8e8 <col:12> 'int' lvalue ParmVar 0x55dcfa09a6c0 'sss' 'int'

Note that CXXMemberCallExpr 0x55dcfa09f260 <col:3> cannot get end loc because no arg exists (but covered by its base).

3. Use Base end loc instead of Loc if no args: auto EndLoc = Args.empty() ? Base->getEndLoc() : Args.back()->getEndLoc();

ExprWithCleanups 0x5650003822c8 <line:68:3, col:12> 'void'
`-CoyieldExpr 0x565000382280 <col:3, col:12> 'void'
  |-CXXMemberCallExpr 0x565000381ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  | |-MemberExpr 0x565000381ea8 <col:3> '<bound member function type>' .yield_value 0x565000377220
  | | `-DeclRefExpr 0x565000381e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' ...
  | `-ImplicitCastExpr 0x565000381f00 <col:12> 'int' <LValueToRValue>
  |   `-DeclRefExpr 0x56500037d8e8 <col:12> 'int' lvalue ParmVar 0x56500037d6c0 'sss' 'int'
  |-MaterializeTemporaryExpr 0x565000381f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | `-CXXMemberCallExpr 0x565000381ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  |   |-MemberExpr 0x565000381ea8 <col:3> '<bound member function type>' .yield_value 0x565000377220
  |   | `-DeclRefExpr 0x565000381e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' ...
  |   `-ImplicitCastExpr 0x565000381f00 <col:12> 'int' <LValueToRValue>
  |     `-DeclRefExpr 0x56500037d8e8 <col:12> 'int' lvalue ParmVar 0x56500037d6c0 'sss' 'int'
  |-ExprWithCleanups 0x565000381fd8 <col:3, col:12> 'bool'
  | `-CXXMemberCallExpr 0x565000381fb8 <col:3, col:12> 'bool'
  |   `-MemberExpr 0x565000381f88 <col:3> '<bound member function type>' .await_ready 0x565000374858
  |     `-OpaqueValueExpr 0x565000381f70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  |       `-MaterializeTemporaryExpr 0x565000381f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  |         `-CXXMemberCallExpr 0x565000381ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  |           |-MemberExpr 0x565000381ea8 <col:3> '<bound member function type>' .yield_value 0x565000377220
  |           | `-DeclRefExpr 0x565000381e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' ...
  |           `-ImplicitCastExpr 0x565000381f00 <col:12> 'int' <LValueToRValue>
  |             `-DeclRefExpr 0x56500037d8e8 <col:12> 'int' lvalue ParmVar 0x56500037d6c0 'sss' 'int'
  |-CXXMemberCallExpr 0x565000382160 <col:3> 'void'
  | |-MemberExpr 0x565000382130 <col:3> '<bound member function type>' .await_suspend 0x565000374ad8
  | | `-OpaqueValueExpr 0x565000381f70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | |   `-MaterializeTemporaryExpr 0x565000381f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
  | |     `-CXXMemberCallExpr 0x565000381ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
  | |       |-MemberExpr 0x565000381ea8 <col:3> '<bound member function type>' .yield_value 0x565000377220
  | |       | `-DeclRefExpr 0x565000381e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' ...
  | |       `-ImplicitCastExpr 0x565000381f00 <col:12> 'int' <LValueToRValue>
  | |         `-DeclRefExpr 0x56500037d8e8 <col:12> 'int' lvalue ParmVar 0x56500037d6c0 'sss' 'int'
  | `-ImplicitCastExpr 0x565000382218 <col:3> 'coroutine_handle<>':'std::coroutine_handle<void>' <ConstructorConversion>
  |   `-CXXConstructExpr 0x5650003821e8 <col:3> 'coroutine_handle<>':'std::coroutine_handle<void>' 'void (coroutine_handle<promise_type>) noexcept'
  |     `-CallExpr 0x5650003820d0 <col:3> 'coroutine_handle<promise_type>':'std::coroutine_handle<Chat::promise_type>'
  |       |-ImplicitCastExpr 0x5650003820b8 <col:3> 'coroutine_handle<promise_type> (*)(void *) noexcept' <FunctionToPointerDecay>
  |       | `-DeclRefExpr 0x565000382098 <col:3> 'coroutine_handle<promise_type> (void *) noexcept' lvalue CXXMethod 0x565000379d98 'from_address' ...
  |       `-CallExpr 0x565000382078 <col:3> 'void *'
  |         `-ImplicitCastExpr 0x565000382060 <col:3> 'void *(*)() noexcept' <FunctionToPointerDecay>
  |           `-DeclRefExpr 0x565000382040 <col:3> 'void *() noexcept' lvalue Function 0x565000380c88 '__builtin_coro_frame' 'void *() noexcept'
  `-CXXMemberCallExpr 0x565000382260 <col:3, col:12> 'void'
    `-MemberExpr 0x565000382230 <col:3> '<bound member function type>' .await_resume 0x565000374bf8
      `-OpaqueValueExpr 0x565000381f70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
        `-MaterializeTemporaryExpr 0x565000381f58 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue
          `-CXXMemberCallExpr 0x565000381ed8 <col:3, col:12> 'std::suspend_always':'std::suspend_always'
            |-MemberExpr 0x565000381ea8 <col:3> '<bound member function type>' .yield_value 0x565000377220
            | `-DeclRefExpr 0x565000381e88 <col:3> 'std::coroutine_traits<Chat, int>::promise_type':'Chat::promise_type' lvalue Var 0x565000380218 '__promise' ...
            `-ImplicitCastExpr 0x565000381f00 <col:12> 'int' <LValueToRValue>
              `-DeclRefExpr 0x56500037d8e8 <col:12> 'int' lvalue ParmVar 0x56500037d6c0 'sss' 'int'

This is better, but CXXMemberCallExpr 0x565000382160 <col:3> 'void' still cannot get proper end loc, its base spans <col:3, col:12>, arg also exists but this arg is irrevelant.

It's really hard to tell which is correct or not.

[hokein]

Thanks for exploring all the options.

For case 1)

Note that CXXMemberCallExpr 0x55d106716260 <col:3, <invalid sloc>> relies on RParenLoc directly. Coroutine is implemented as member call so I think RParen is intended and should exist and be correct.

Yeah, I think this is because we don't have argument, so the getEndLoc heuristic failed, and it returned the RParenLoc which is invalid. This could be fixed by adjusting the heuristic there (we fallback to getBeginLoc if the endLoc is invalid), but no sure whether it will has any side effect.

for case 2) and 3)

Note that CXXMemberCallExpr 0x55dcfa09f260 <col:3> cannot get end loc because no arg exists (but covered by its base).

This is better, but CXXMemberCallExpr 0x565000382160 <col:3> 'void' still cannot get proper end loc, its base spans <col:3, col:12>, arg also exists but this arg is irrevelant.

Looks like the source range of the MemberExpr is not correct in both case 2) and 3), I guess this is the culprit, but I think it is a different bug in MemberExpr::getEndLoc .

`-MemberExpr 0x55dcfa09f230 <col:3> '<bound member function type>' .await_resume 0x55dcfa091bf8
     `-OpaqueValueExpr 0x55dcfa09ef70 <col:3, col:12> 'std::suspend_always':'std::suspend_always' lvalue

clang/lib/Sema/SemaCoroutine.cpp

322

Agree, the coroutine is a hard case. I was concerned about the MemberCallExpr::getRParenLoc() API, it returns a valid source location but the token it points to is not a ), this could lead to subtle bugs in tooling.

since this change is an improvement, I'm fine with the current change.

clang/test/AST/coroutine-co_yield-source-range.cpp

5

nit: use the test/AST/Inputs/std-coroutine.h header to avoid repeating the boilerplate code.

[aaron.ballman]

clang/lib/Sema/SemaCoroutine.cpp

322

Agree, the coroutine is a hard case. I was concerned about the MemberCallExpr::getRParenLoc() API, it returns a valid source location but the token it points to is not a ), this could lead to subtle bugs in tooling.

Yeah, that was actually my concern as well -- I'm glad we're both on the same page. :-)

I eventually convinced myself that folks are generally interested in the right paren location because they want to know when the argument list is complete, not because they specifically want the right paren token itself. But the reason they want to do that is because they want to insert a qualifier, a semi-colon, an attribute, etc (something that follows the closing paren) and in all of those cases, they'll be surprised here. Perhaps we should be marking this call expression as an implicit AST node so that it's more clear to users "this wasn't from source, don't expect fix-its to be a good idea?"

clang/test/AST/coroutine-co_yield-source-range.cpp

5

Good call, I always forget that helper code exists!

[hokein]

clang/lib/Sema/SemaCoroutine.cpp

322

I eventually convinced myself that folks are generally interested in the right paren location because they want to know when the argument list is complete, not because they specifically want the right paren token itself. But the reason they want to do that is because they want to insert a qualifier, a semi-colon, an attribute, etc (something that follows the closing paren) and in all of those cases, they'll be surprised here.

Yes, exactly.

Perhaps we should be marking this call expression as an implicit AST node so that it's more clear to users "this wasn't from source, don't expect fix-its to be a good idea?"

Possibly. My understanding of this expression is that it represents the written operand of the co_yield expression to some degree (see https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L2901). If we marked it implicit, then there is no "visible" expression in the CoyieldExpr, which means the operand will not be visited.

Overall, it looks like the current AST node for co_yield, co_await, co_await expressions are mostly modeling the language semantics, thus they are complicated, I guess this is the reason why it feels hard to improve the support for the syntactic. Probably, we can borrow the idea from InitListExpr, there are two forms, one is for semantic, the other one is for syntactic, having these two split can make everything easier.

[aaron.ballman]

Aside from some changes to the test coverage, I think @hokein and I are in agreement on moving forward with the patch as-is (feel free to correct me if I'm wrong though!).

clang/lib/Sema/SemaCoroutine.cpp

322

Possibly. My understanding of this expression is that it represents the written operand of the co_yield expression to some degree (see https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L2901). If we marked it implicit, then there is no "visible" expression in the CoyieldExpr, which means the operand will not be visited.

Agreed, but it's also pretty weird to have a CallExpr AST node where no call expression exists in the source code. For example, when we do an -ast-print for the test case below, we get some fun behavior:

Chat f(int s)
    {
        co_yield __promise.yield_value(s);
co_return s;        co_await s;
    }

https://godbolt.org/z/KxzPoPz7T

and this spills over into AST matchers that try to ignore code not spelled in source: https://godbolt.org/z/fKhjMEs5P (note match #4, for example).

Overall, it looks like the current AST node for co_yield, co_await, co_await expressions are mostly modeling the language semantics, thus they are complicated, I guess this is the reason why it feels hard to improve the support for the syntactic. Probably, we can borrow the idea from InitListExpr, there are two forms, one is for semantic, the other one is for syntactic, having these two split can make everything easier.

That's a pretty good idea -- having a semantic and syntactic form does seem like it would be a good approach in the future.

[danix800]

Cleanup duplicated boilerplate testcase code. Append a few extra child nodes of CoyieldExpr dumped in testcase.

[danix800]

I'll finish this patch when CI build succeeds.

For future improvement I might start with the idea
of marking those generated inner exprs as implicit,
this seems to be easier.

[shafik]

clang/lib/Sema/SemaCoroutine.cpp

322

We should use bugprone-argument-comment when using literals for arguments.

clang/lib/Sema/SemaCoroutine.cpp

static ExprResult buildMemberCall(Sema &S, Expr *Base, SourceLocation Loc,
   if (auto *TE = dyn_cast<TypoExpr>(Result.get())) {
     S.clearDelayedTypo(TE);
     S.Diag(Loc, diag::err_no_member)
         << NameInfo.getName() << Base->getType()->getAsCXXRecordDecl()
         << Base->getSourceRange();
     return ExprError();
   }
 
-  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, Loc, nullptr);
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }
 
 // See if return type is coroutine-handle and if so, invoke builtin coro-resume
 // on its address. This is to enable the support for coroutine-handle
 // returning await_suspend that results in a guaranteed tail call to the target
 // coroutine.
 static Expr *maybeTailCall(Sema &S, QualType RetType, Expr *E,
                            SourceLocation Loc) {

clang/test/AST/Inputs/std-coroutine.h

template <typename Promise> struct coroutine_handle : coroutine_handle<> {
   static coroutine_handle from_promise(Promise &promise) {
     coroutine_handle p;
     p.ptr = __builtin_coro_promise(&promise, alignof(Promise), true);
     return p;
   }
 };
 
 struct suspend_always {
-  bool await_ready() { return false; }
-  void await_suspend(coroutine_handle<>) {}
-  void await_resume() {}
+  bool await_ready() noexcept { return false; }
+  void await_suspend(coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
 };
 
 struct suspend_never {
   bool await_ready() noexcept { return true; }
   void await_suspend(coroutine_handle<>) noexcept {}
   void await_resume() noexcept {}
 };
 
 } // namespace std
 
 #endif // STD_COROUTINE_H

clang/test/AST/coroutine-co_yield-source-range.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++20 \
+// RUN:    -fsyntax-only -ast-dump | FileCheck %s
+
+#include "Inputs/std-coroutine.h"
+
+using namespace std;
+
+struct Chat {
+  struct promise_type {
+    std::suspend_always initial_suspend() { return {}; }
+    Chat get_return_object() {
+      return std::coroutine_handle<promise_type>::from_promise(*this);
+    }
+    std::suspend_always yield_value(int m) { return {}; }
+    std::suspend_always final_suspend() noexcept { return {}; }
+    std::suspend_always return_value(int) { return {}; }
+    void unhandled_exception() {}
+
+    auto await_transform(int s) {
+      struct awaiter {
+        promise_type *promise;
+        bool await_ready() { return true; }
+        int await_resume() { return promise->message; }
+        void await_suspend(std::coroutine_handle<>) {}
+      };
+
+      return awaiter{this};
+    }
+    int message;
+  };
+
+  Chat(std::coroutine_handle<promise_type> promise);
+
+  std::coroutine_handle<promise_type> handle;
+};
+
+Chat f(int s)  {
+  // CHECK:      CoyieldExpr {{.*}} <col:3, col:12>
+  // CHECK-NEXT:   CXXMemberCallExpr {{.*}} <col:3, col:12> {{.*}}
+  // CHECK-NEXT:     MemberExpr {{.*}} <col:3> {{.*}}
+  // CHECK-NEXT:       DeclRefExpr {{.*}} <col:3> {{.*}}
+  // CHECK-NEXT:     ImplicitCastExpr {{.*}} <col:12> {{.*}}
+  // CHECK-NEXT:       DeclRefExpr {{.*}} <col:12> {{.*}}
+  co_yield s;
+  // CHECK:      CoreturnStmt {{.*}} <line:{{.*}}:3, col:13>
+  co_return s;
+  // CHECK:      CoawaitExpr {{.*}} <col:3, col:12> 'int'
+  co_await s;
+}