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

Tue Aug 8 11:53:23 PDT 2023

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157296