[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

We should use 
[bugprone-argument-comment](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
 when using literals for arguments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8d60e10ce4bd: [AST][Coroutine] Fix CoyieldExpr missing end 
loc (authored by dingfei fd...@feysh.com).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/AST/Inputs/std-coroutine.h
  clang/test/AST/coroutine-co_yield-source-range.cpp


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,49 @@
+// 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::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);
+
+  std::coroutine_handle handle;
+};
+
+Chat f(int s)  {
+  // CHECK:  CoyieldExpr {{.*}} 
+  // CHECK-NEXT:   CXXMemberCallExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: MemberExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: ImplicitCastExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  co_yield s;
+  // CHECK:  CoreturnStmt {{.*}} 
+  co_return s;
+  // CHECK:  CoawaitExpr {{.*}}  'int'
+  co_await s;
+}
Index: clang/test/AST/Inputs/std-coroutine.h
===
--- clang/test/AST/Inputs/std-coroutine.h
+++ clang/test/AST/Inputs/std-coroutine.h
@@ -55,9 +55,9 @@
 };
 
 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 {
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -318,7 +318,8 @@
 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


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,49 @@
+// 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::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);
+
+  std::coroutine_handle handle;
+};
+
+Chat f(int s)  {
+  // CHECK:  CoyieldExpr {{.*}} 
+  // CHECK-NEXT:   CXXMemberCallExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: MemberExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: ImplicitCastExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  co_yield s;
+  // CHECK:  CoreturnStmt {{.*}} 
+  co_return s;
+  // CHECK:  CoawaitExpr {{.*}}  'int'
+  co_await s;
+}
Index: clang/test/AST/Inputs/std-coroutine.h
===
--- 

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Ding Fei via Phabricator via cfe-commits]

danix800 added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Ding Fei via Phabricator via cfe-commits]

danix800 updated this revision to Diff 548434.
danix800 edited the summary of this revision.
danix800 added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/AST/Inputs/std-coroutine.h
  clang/test/AST/coroutine-co_yield-source-range.cpp


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,49 @@
+// 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::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);
+
+  std::coroutine_handle handle;
+};
+
+Chat f(int s)  {
+  // CHECK:  CoyieldExpr {{.*}} 
+  // CHECK-NEXT:   CXXMemberCallExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: MemberExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: ImplicitCastExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  co_yield s;
+  // CHECK:  CoreturnStmt {{.*}} 
+  co_return s;
+  // CHECK:  CoawaitExpr {{.*}}  'int'
+  co_await s;
+}
Index: clang/test/AST/Inputs/std-coroutine.h
===
--- clang/test/AST/Inputs/std-coroutine.h
+++ clang/test/AST/Inputs/std-coroutine.h
@@ -55,9 +55,9 @@
 };
 
 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 {
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -318,7 +318,8 @@
 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


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,49 @@
+// 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::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);
+
+  std::coroutine_handle handle;
+};
+
+Chat f(int s)  {
+  // CHECK:  CoyieldExpr {{.*}} 
+  // CHECK-NEXT:   CXXMemberCallExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: MemberExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  // CHECK-NEXT: ImplicitCastExpr {{.*}}  {{.*}}
+  // CHECK-NEXT:   DeclRefExpr {{.*}}  {{.*}}
+  co_yield s;
+  // CHECK:  CoreturnStmt {{.*}} 
+  co_return s;
+  // CHECK:  CoawaitExpr {{.*}}  'int'
+  co_await s;
+}
Index: clang/test/AST/Inputs/std-coroutine.h
===
--- 

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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!).




Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

hokein wrote:
> aaron.ballman wrote:
> > hokein wrote:
> > > aaron.ballman wrote:
> > > > hokein wrote:
> > > > > 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](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1662-L1664)
> > > > >  to handle the case where the RParenLoc is invalid.
> > > > >  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.
> > > 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.
> > > 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?"
> > 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.
> 
> 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 

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

aaron.ballman wrote:
> hokein wrote:
> > aaron.ballman wrote:
> > > hokein wrote:
> > > > 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](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1662-L1664)
> > > >  to handle the case where the RParenLoc is invalid.
> > > >  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.
> > 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.
> > 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?"
> 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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

hokein wrote:
> aaron.ballman wrote:
> > hokein wrote:
> > > 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](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1662-L1664)
> > >  to handle the case where the RParenLoc is invalid.
> > >  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.
> 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.
> 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?"



Comment at: clang/test/AST/coroutine-co_yield-source-range.cpp:4
+
+namespace std {
+template 

hokein wrote:
> nit: use the `test/AST/Inputs/std-coroutine.h` header to avoid repeating the 
> boilerplate code. 
Good call, I always forget that helper code exists!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for exploring all the options.

For case 1)

> Note that CXXMemberCallExpr 0x55d106716260 > 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  cannot get end loc because 
> no arg exists (but covered by its base).



> This is better, but CXXMemberCallExpr 0x565000382160  'void' still 
> cannot get proper end loc, its base spans , 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  '' 
.await_resume 0x55dcfa091bf8
   `-OpaqueValueExpr 0x55dcfa09ef70  
'std::suspend_always':'std::suspend_always' lvalue




Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

aaron.ballman wrote:
> hokein wrote:
> > 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](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1662-L1664)
> >  to handle the case where the RParenLoc is invalid.
> >  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.
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.



Comment at: clang/test/AST/coroutine-co_yield-source-range.cpp:4
+
+namespace std {
+template 

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Ding Fei via Phabricator via cfe-commits]

danix800 added a comment.

1. Use invalid loc: `SourceLocation()`

  ExprWithCleanups 0x55d1067162c8  'void'
  `-CoyieldExpr 0x55d106716280  'void'
|-CXXMemberCallExpr 0x55d106715ed8  
'std::suspend_always':'std::suspend_always'
| |-MemberExpr 0x55d106715ea8  '' 
.yield_value 0x55d10670b220
| | `-DeclRefExpr 0x55d106715e88  'std::coroutine_traits::promise_type':'Chat::promise_type' lvalue Var 0x55d106714218 '__promise' 
'std::coroutine_traits::promise_type':'Chat::promise_type'
| `-ImplicitCastExpr 0x55d106715f00  'int' 
|   `-DeclRefExpr 0x55d1067118e8  'int' lvalue ParmVar 
0x55d1067116c0 'sss' 'int'
|-MaterializeTemporaryExpr 0x55d106715f58  
'std::suspend_always':'std::suspend_always' lvalue
| `-CXXMemberCallExpr 0x55d106715ed8  
'std::suspend_always':'std::suspend_always'
|   |-MemberExpr 0x55d106715ea8  '' 
.yield_value 0x55d10670b220
|   | `-DeclRefExpr 0x55d106715e88  'std::coroutine_traits::promise_type':'Chat::promise_type' lvalue Var 0x55d106714218 '__promise' 
'std::coroutine_traits::promise_type':'Chat::promise_type'
|   `-ImplicitCastExpr 0x55d106715f00  'int' 
| `-DeclRefExpr 0x55d1067118e8  'int' lvalue ParmVar 
0x55d1067116c0 'sss' 'int'
|-ExprWithCleanups 0x55d106715fd8 > 'bool'
| `-CXXMemberCallExpr 0x55d106715fb8 > 'bool'
|   `-MemberExpr 0x55d106715f88  '' 
.await_ready 0x55d106708858
| `-OpaqueValueExpr 0x55d106715f70  
'std::suspend_always':'std::suspend_always' lvalue
|   `-MaterializeTemporaryExpr 0x55d106715f58  
'std::suspend_always':'std::suspend_always' lvalue
| `-CXXMemberCallExpr 0x55d106715ed8  
'std::suspend_always':'std::suspend_always'
|   |-MemberExpr 0x55d106715ea8  '' .yield_value 0x55d10670b220
|   | `-DeclRefExpr 0x55d106715e88  
'std::coroutine_traits::promise_type':'Chat::promise_type' lvalue 
Var 0x55d106714218 '__promise' 'std::coroutine_traits::promise_type':'Chat::promise_type'
|   `-ImplicitCastExpr 0x55d106715f00  'int' 

| `-DeclRefExpr 0x55d1067118e8  'int' lvalue ParmVar 
0x55d1067116c0 'sss' 'int'
|-CXXMemberCallExpr 0x55d106716160  'void'
| |-MemberExpr 0x55d106716130  '' 
.await_suspend 0x55d106708ad8
| | `-OpaqueValueExpr 0x55d106715f70  
'std::suspend_always':'std::suspend_always' lvalue
| |   `-MaterializeTemporaryExpr 0x55d106715f58  
'std::suspend_always':'std::suspend_always' lvalue
| | `-CXXMemberCallExpr 0x55d106715ed8  
'std::suspend_always':'std::suspend_always'
| |   |-MemberExpr 0x55d106715ea8  '' .yield_value 0x55d10670b220
| |   | `-DeclRefExpr 0x55d106715e88  
'std::coroutine_traits::promise_type':'Chat::promise_type' lvalue 
Var 0x55d106714218 '__promise' 'std::coroutine_traits::promise_type':'Chat::promise_type'
| |   `-ImplicitCastExpr 0x55d106715f00  'int' 
| | `-DeclRefExpr 0x55d1067118e8  'int' lvalue ParmVar 
0x55d1067116c0 'sss' 'int'
| `-ImplicitCastExpr 0x55d106716218  
'coroutine_handle<>':'std::coroutine_handle' 
|   `-CXXConstructExpr 0x55d1067161e8  
'coroutine_handle<>':'std::coroutine_handle' 'void 
(coroutine_handle) noexcept'
| `-CallExpr 0x55d1067160d0  
'coroutine_handle':'std::coroutine_handle'
|   |-ImplicitCastExpr 0x55d1067160b8  
'coroutine_handle (*)(void *) noexcept' 
|   | `-DeclRefExpr 0x55d106716098  
'coroutine_handle (void *) noexcept' lvalue CXXMethod 
0x55d10670dd98 'from_address' 'coroutine_handle (void *) noexcept'
|   `-CallExpr 0x55d106716078  'void *'
| `-ImplicitCastExpr 0x55d106716060  'void *(*)() noexcept' 

|   `-DeclRefExpr 0x55d106716040  'void *() noexcept' lvalue 
Function 0x55d106714c88 '__builtin_coro_frame' 'void *() noexcept'
`-CXXMemberCallExpr 0x55d106716260 > 'void'
  `-MemberExpr 0x55d106716230  '' 
.await_resume 0x55d106708bf8
`-OpaqueValueExpr 0x55d106715f70  
'std::suspend_always':'std::suspend_always' lvalue
  `-MaterializeTemporaryExpr 0x55d106715f58  
'std::suspend_always':'std::suspend_always' lvalue
`-CXXMemberCallExpr 0x55d106715ed8  
'std::suspend_always':'std::suspend_always'
  |-MemberExpr 0x55d106715ea8  '' .yield_value 0x55d10670b220
  | `-DeclRefExpr 0x55d106715e88  
'std::coroutine_traits::promise_type':'Chat::promise_type' lvalue 
Var 0x55d106714218 '__promise' 'std::coroutine_traits::promise_type':'Chat::promise_type'
  `-ImplicitCastExpr 0x55d106715f00  'int' 
`-DeclRefExpr 0x55d1067118e8  'int' lvalue ParmVar 
0x55d1067116c0 'sss' 'int'

Note that `CXXMemberCallExpr 0x55d106716260 >` 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  'void'
  `-CoyieldExpr 0x55dcfa09f280  'void'

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

hokein wrote:
> 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](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1662-L1664)
>  to handle the case where the RParenLoc is invalid.
>  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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

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](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1662-L1664)
 to handle the case where the RParenLoc is invalid.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Ding Fei via Phabricator via cfe-commits]

danix800 updated this revision to Diff 547843.
danix800 added a comment.

Use `getEndLoc()` instead of `getBeginLoc()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/AST/coroutine-co_yield-source-range.cpp


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++20 \
+// RUN:-fsyntax-only -ast-dump | FileCheck %s
+
+namespace std {
+template 
+struct coroutine_traits { using promise_type = typename Ret::promise_type; };
+
+template 
+struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  static coroutine_handle from_promise(Promise );
+  constexpr void* address() const noexcept;
+};
+template <>
+struct coroutine_handle {
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+  static coroutine_handle from_address(void *);
+  constexpr void* address() const noexcept;
+};
+
+struct suspend_always {
+  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
+
+struct Chat {
+  struct promise_type {
+std::suspend_always initial_suspend() { return {}; }
+Chat get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+std::suspend_always yield_value(int m) {
+  return {};
+}
+std::suspend_always final_suspend() noexcept { return {}; }
+std::suspend_always return_value(int) noexcept { return {}; }
+void unhandled_exception() noexcept {}
+
+auto await_transform(int s) noexcept {
+  struct awaiter {
+promise_type *promise;
+bool await_ready() const {
+  return true;
+}
+int await_resume() const {
+  return promise->message;
+}
+void await_suspend(std::coroutine_handle<>) {
+}
+  };
+  return awaiter{this};
+}
+int message;
+  };
+
+  Chat(std::coroutine_handle promise);
+
+  std::coroutine_handle handle;
+};
+
+Chat f(int s) {
+  // CHECK: CoyieldExpr {{.*}} 
+  co_yield s;
+  // CHECK: CoreturnStmt {{.*}} 
+  co_return s;
+  // CHECK: CoawaitExpr {{.*}} 
+  co_await s;
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -318,7 +318,8 @@
 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


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++20 \
+// RUN:-fsyntax-only -ast-dump | FileCheck %s
+
+namespace std {
+template 
+struct coroutine_traits { using promise_type = typename Ret::promise_type; };
+
+template 
+struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  static coroutine_handle from_promise(Promise );
+  constexpr void* address() const noexcept;
+};
+template <>
+struct coroutine_handle {
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+  static coroutine_handle from_address(void *);
+  constexpr void* address() const noexcept;
+};
+
+struct suspend_always {
+  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
+
+struct Chat {
+  struct promise_type {
+std::suspend_always initial_suspend() { return {}; }
+Chat get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+std::suspend_always yield_value(int m) {
+  return {};
+}
+std::suspend_always final_suspend() noexcept { return {}; }
+std::suspend_always return_value(int) noexcept { return {}; }
+void unhandled_exception() noexcept {}
+
+auto await_transform(int s) noexcept {
+  struct awaiter {
+promise_type *promise;
+bool await_ready() const {
+  return true;
+}
+int 

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Ding Fei via Phabricator via cfe-commits]

danix800 added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:321
 
-  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, Loc, nullptr);
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getBeginLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);

tbaeder wrote:
> Why `getBeginLoc()` and not `getEndLoc()`?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:321
 
-  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, Loc, nullptr);
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getBeginLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);

Why `getBeginLoc()` and not `getEndLoc()`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157296/new/

https://reviews.llvm.org/D157296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Ding Fei via Phabricator via cfe-commits]

danix800 created this revision.
danix800 added a reviewer: aaron.ballman.
danix800 added a project: clang.
Herald added a subscriber: ChuanqiXu.
Herald added a project: All.
danix800 requested review of this revision.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157296

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/AST/coroutine-co_yield-source-range.cpp


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++20 \
+// RUN:-fsyntax-only -ast-dump | FileCheck %s
+
+namespace std {
+template 
+struct coroutine_traits { using promise_type = typename Ret::promise_type; };
+
+template 
+struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  static coroutine_handle from_promise(Promise );
+  constexpr void* address() const noexcept;
+};
+template <>
+struct coroutine_handle {
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+  static coroutine_handle from_address(void *);
+  constexpr void* address() const noexcept;
+};
+
+struct suspend_always {
+  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
+
+struct Chat {
+  struct promise_type {
+std::suspend_always initial_suspend() { return {}; }
+Chat get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+std::suspend_always yield_value(int m) {
+  return {};
+}
+std::suspend_always final_suspend() noexcept { return {}; }
+std::suspend_always return_value(int) noexcept { return {}; }
+void unhandled_exception() noexcept {}
+
+auto await_transform(int s) noexcept {
+  struct awaiter {
+promise_type *promise;
+bool await_ready() const {
+  return true;
+}
+int await_resume() const {
+  return promise->message;
+}
+void await_suspend(std::coroutine_handle<>) {
+}
+  };
+  return awaiter{this};
+}
+int message;
+  };
+
+  Chat(std::coroutine_handle promise);
+
+  std::coroutine_handle handle;
+};
+
+Chat f(int s) {
+  // CHECK: CoyieldExpr {{.*}} 
+  co_yield s;
+  // CHECK: CoreturnStmt {{.*}} 
+  co_return s;
+  // CHECK: CoawaitExpr {{.*}} 
+  co_await s;
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -318,7 +318,8 @@
 return ExprError();
   }
 
-  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, Loc, nullptr);
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getBeginLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }
 
 // See if return type is coroutine-handle and if so, invoke builtin coro-resume


Index: clang/test/AST/coroutine-co_yield-source-range.cpp
===
--- /dev/null
+++ clang/test/AST/coroutine-co_yield-source-range.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++20 \
+// RUN:-fsyntax-only -ast-dump | FileCheck %s
+
+namespace std {
+template 
+struct coroutine_traits { using promise_type = typename Ret::promise_type; };
+
+template 
+struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  static coroutine_handle from_promise(Promise );
+  constexpr void* address() const noexcept;
+};
+template <>
+struct coroutine_handle {
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+  static coroutine_handle from_address(void *);
+  constexpr void* address() const noexcept;
+};
+
+struct suspend_always {
+  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
+
+struct Chat {
+  struct promise_type {
+std::suspend_always initial_suspend() { return {}; }
+Chat get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+std::suspend_always yield_value(int m) {
+  return {};
+}
+std::suspend_always final_suspend() noexcept { return {}; }
+std::suspend_always return_value(int) noexcept { return {}; }
+void unhandled_exception() noexcept {}
+
+auto await_transform(int s) noexcept {
+  struct