[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 <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 <https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1663> 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



================
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 <class Ret, typename... T>
----------------
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