[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov requested changes to this revision.
GorNishanov added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Sema/SemaCoroutine.cpp:675
+  // Second emphasis of [expr.await]p2: must be outside of an exception handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+    S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;
----------------
lewissbaker wrote:
> modocache wrote:
> > modocache wrote:
> > > lewissbaker wrote:
> > > > modocache wrote:
> > > > > EricWF wrote:
> > > > > > We can still build a valid AST after encountering this error, no?
> > > > > > 
> > > > > > 
> > > > > I believe so. Just to be clear: you'd like me to continue building the AST even after emitting this error diagnostic? My understanding is that most of this file bails soon after any error is encountered (correct me if that's wrong). I'm happy to change that, but I wonder if it'd be better to do that in a separate diff...?
> > > > Do the scope flags reset when a lambda occurs within a catch-block?
> > > > 
> > > > eg. The following should still be valid.
> > > > ```
> > > > try { ... }
> > > > catch (...) {
> > > >   []() -> task { co_await foo(); }();
> > > > }
> > > > ```
> > > I believe they're reset, the example you posted compiles fine with this patch. I'll add a test case specifically for this and confirm exactly where the scope flags are reset or ignored in the lambda case.
> > When the parser encounters a lambda, it takes the path `Parser::ParseLambdaExpression -> Parser::ParseLambdaExpressionAfterIntroducer -> Parser::ParseCompoundStatementBody`, which creates an inner scope for the body of the lambda. This inner scope does not have the `Scope::CatchScope` flag, so it doesn't result in the error.
> > 
> > Although, your question did make me realize that this compiles just fine, even with this patch:
> > 
> > ```
> > void example() {
> >   try {
> >     throw;
> >   } catch (...) {
> >     try {
> >       co_await a;
> >     }
> >   }
> > }
> > ```
> > 
> > But I believe this above case should also be treated as an error, right?
> Yes, I believe that co_await within a try-block within a catch-block should not be allowed.
Yes. That will result in suspension of the coroutine and we don't yet know how to suspend in catch blocks.

Also, I agree with @EricWF, this error should not prevent us from continuing semantic analysis with the rest of the function body.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59076