[coroutines][PR40978] Emit error for co_yield within catch block

[modocache]

Summary

As reported in https://bugs.llvm.org/show_bug.cgi?id=40978, it's an
error to use the co_yield or co_await keywords outside of a valid
"suspension context" as defined by [expr.await]p2 of
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4775.pdf.

Whether or not the current scope was in a function-try-block's
(https://en.cppreference.com/w/cpp/language/function-try-block) handler
could be determined using scope flag Scope::FnTryCatchScope. No
such flag existed for a simple C++ catch statement, so this commit adds
one.

[EricWF]

lib/Sema/SemaCoroutine.cpp

675

We can still build a valid AST after encountering this error, no?

[modocache]

lib/Sema/SemaCoroutine.cpp

675

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

[lewissbaker]

lib/Sema/SemaCoroutine.cpp

197–200

Does removing this check now mean that we're not checking that co_return statements don't appear in unevaluated contexts?

Or is that already handled elsewhere by the fact that co_return statements are not expressions and are therefore detected earlier as a grammar violation when parsing sizeof() expression?

675

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(); }();
}

test/SemaCXX/coroutines.cpp

299–311

Not related to this diff, but...

I don't think that these should be ill-formed.

According to N4775 there are only exclusions added for [class.ctor] and [class.dtor].
I can't see anything in the spec that says that assignment special member functions cannot be coroutines.

[modocache]

lib/Sema/SemaCoroutine.cpp

197–200

That's right! If one were to attempt to use a co_return within a subexpression of sizeof, they'd see error: expected expression before they'd ever have seen this error message. So I believe there's no need to perform this check for co_return, and I believe that's why the revisions to the standard included in the Coroutines TS don't make any special mention of disallowing co_return in those contexts.

675

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.

test/SemaCXX/coroutines.cpp

299–311

That's a great point. Could you create a Bugzilla for this work? And maybe we can get @GorNishanov's opinion?

[modocache]

Add test case for executing a lambda using co_yield within a catch block.

[modocache]

lib/Sema/SemaCoroutine.cpp

675

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?

[GorNishanov]

test/SemaCXX/coroutines.cpp

299–311

In 2015, such as https://wg21.link/N4499 there was a blank prohibition:
"A special member function shall not be a coroutine."

Later, at @rsmith suggestion, we relaxed it a little and banned only constructors and destructors.

I am not yet aware of any use cases where such coroutines would be useful.

[lewissbaker]

test/SemaCXX/coroutines.cpp

299–311

See https://bugs.llvm.org/show_bug.cgi?id=40997

[lewissbaker]

lib/Sema/SemaCoroutine.cpp

675

Yes, I believe that co_await within a try-block within a catch-block should not be allowed.

[GorNishanov]

lib/Sema/SemaCoroutine.cpp

675

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.

[modocache]

Thank you for the reviews! This revision fixes the nested try/catch behavior. I still need to modify this such that semantic analysis continues for the rest of the function body.

[modocache]

Remove unused function parameter.

[modocache]

OK, I've responded to all your review comments -- thank you! Please give this another look when you get a chance. I would like to emit note diagnostics pointing to the catch blocks, but I've left that as a FIXME for now.

[GorNishanov]

LGTM

[modocache]

Great, thanks for the reviews, everyone!

[riccibruno]

This is causing https://bugs.llvm.org/show_bug.cgi?id=41171.

[riccibruno]

In D59076#1438479, @riccibruno wrote:

This is causing https://bugs.llvm.org/show_bug.cgi?id=41171.

@modocache Could you take a look please ? Nested scopes in a catch statement are completely broken because of this patch.

[modocache]

Sorry for the late response, I'm looking now. Should I revert this for now?

[modocache]

I'm pushing a revert. Sorry for the trouble!

[modocache]

Reverted in rC356774. I'll rework this change along with a test confirming https://bugs.llvm.org/show_bug.cgi?id=41171 doesn't occur. Apologies!

include/clang/Basic/DiagnosticSemaKinds.td

 def err_return_in_coroutine : Error<
   "return statement not allowed in coroutine; did you mean 'co_return'?">;
 def note_declared_coroutine_here : Note<
   "function is a coroutine due to use of '%0' here">;
 def err_coroutine_objc_method : Error<
   "Objective-C methods as coroutines are not yet supported">;
 def err_coroutine_unevaluated_context : Error<
   "'%0' cannot be used in an unevaluated context">;
+def err_coroutine_within_handler : Error<
+  "'%0' cannot be used in the handler of a try block">;
 def err_coroutine_outside_function : Error<
   "'%0' cannot be used outside a function">;
 def err_coroutine_invalid_func_context : Error<
   "'%1' cannot be used in %select{a constructor|a destructor"
   "|a copy assignment operator|a move assignment operator|the 'main' function"
   "|a constexpr function|a function with a deduced return type"
   "|a varargs function}0">;
 def err_implied_coroutine_type_not_found : Error<

include/clang/Sema/Scope.h

enum ScopeFlags {
     /// We are currently in the filter expression of an SEH except block.
     SEHFilterScope = 0x200000,
 
     /// This is a compound statement scope.
     CompoundStmtScope = 0x400000,
 
     /// We are between inheritance colon and the real class/struct definition scope.
     ClassInheritanceScope = 0x800000,
+
+    /// This is the scope of a C++ catch statement.
+    CatchScope = 0x1000000,
   };
 
 private:
   /// The parent scope for this scope.  This is null for the translation-unit
   /// scope.
   Scope *AnyParent;
 
   /// Flags - This contains a set of ScopeFlags, which indicates how the scope

lib/Parse/ParseStmt.cpp

StmtResult Parser::ParseCXXCatchBlock(bool FnCatch) {
 
   BalancedDelimiterTracker T(*this, tok::l_paren);
   if (T.expectAndConsume())
     return StmtError();
 
   // C++ 3.3.2p3:
   // The name in a catch exception-declaration is local to the handler and
   // shall not be redeclared in the outermost block of the handler.
-  ParseScope CatchScope(this, Scope::DeclScope | Scope::ControlScope |
-                          (FnCatch ? Scope::FnTryCatchScope : 0));
+  unsigned ScopeFlags = Scope::DeclScope | Scope::ControlScope |
+                        Scope::CatchScope |
+                        (FnCatch ? Scope::FnTryCatchScope : 0);
+  ParseScope CatchScope(this, ScopeFlags);
 
   // exception-declaration is equivalent to '...' or a parameter-declaration
   // without default arguments.
   Decl *ExceptionDecl = nullptr;
   if (Tok.isNot(tok::ellipsis)) {
     ParsedAttributesWithRange Attributes(AttrFactory);
     MaybeParseCXX11Attributes(Attributes);
 
StmtResult Parser::ParseCXXCatchBlock(bool FnCatch) {
   T.consumeClose();
   if (T.getCloseLocation().isInvalid())
     return StmtError();
 
   if (Tok.isNot(tok::l_brace))
     return StmtError(Diag(Tok, diag::err_expected) << tok::l_brace);
 
   // FIXME: Possible draft standard bug: attribute-specifier should be allowed?
-  StmtResult Block(ParseCompoundStatement());
+  StmtResult Block(ParseCompoundStatement(/*isStmtExpr=*/false, ScopeFlags));
   if (Block.isInvalid())
     return Block;
 
   return Actions.ActOnCXXCatchBlock(CatchLoc, ExceptionDecl, Block.get());
 }
 
 void Parser::ParseMicrosoftIfExistsStatement(StmtVector &Stmts) {
   IfExistsCondition Result;

lib/Sema/SemaCoroutine.cpp

if (S.RequireCompleteType(Loc, CoroHandleType,
                             diag::err_coroutine_type_missing_specialization))
     return QualType();
 
   return CoroHandleType;
 }
 
 static bool isValidCoroutineContext(Sema &S, SourceLocation Loc,
                                     StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within a function where
-  // an await-expression can appear is called a suspension context of the
-  // function." And per [expr.yield]p1: "A yield-expression shall appear only
-  // within a suspension context of a function."
-  if (S.isUnevaluatedContext()) {
-    S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-    return false;
-  }
-
-  // Per [expr.await]p2, any other usage must be within a function.
+  // [expr.await]p2 dictates that 'co_await' and 'co_yield' must be used within
+  // a function body.
   // FIXME: This also covers [expr.await]p2: "An await-expression shall not
   // appear in a default argument." But the diagnostic QoI here could be
   // improved to inform the user that default arguments specifically are not
   // allowed.
   auto *FD = dyn_cast<FunctionDecl>(S.CurContext);
   if (!FD) {
     S.Diag(Loc, isa<ObjCMethodDecl>(S.CurContext)
                     ? diag::err_coroutine_objc_method
bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
   if (FinalSuspend.isInvalid())
     return true;
 
   ScopeInfo->setCoroutineSuspends(InitSuspend.get(), FinalSuspend.get());
 
   return true;
 }
 
+// [expr.await]p2, emphasis added: "An await-expression shall appear only in
+// a *potentially evaluated* expression within the compound-statement of a
+// function-body *outside of a handler* [...] A context within a function
+// where an await-expression can appear is called a suspension context of the
+// function."
+static bool isValidSuspensionContext(Sema &S, SourceLocation Loc,
+                                     StringRef Keyword,
+                                     bool IsImplicit = false) {
+  // First emphasis of [expr.await]p2: must be a potentially evaluated context.
+  // That is, 'co_await' and 'co_yield' cannot appear in subexpressions of
+  // \c sizeof.
+  if (S.isUnevaluatedContext()) {
+    S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
+    return false;
+  }
+
+  // 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;
+    return false;
+  }
+
+  return true;
+}
+
 ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!ActOnCoroutineBodyStart(S, Loc, "co_await")) {
+  if (!ActOnCoroutineBodyStart(S, Loc, "co_await") ||
+      !isValidSuspensionContext(*this, Loc, "co_await")) {
     CorrectDelayedTyposInExpr(E);
     return ExprError();
   }
 
   if (E->getType()->isPlaceholderType()) {
     ExprResult R = CheckPlaceholderExpr(E);
     if (R.isInvalid()) return ExprError();
     E = R.get();
   }
   ExprResult Lookup = buildOperatorCoawaitLookupExpr(*this, S, Loc);
   if (Lookup.isInvalid())
     return ExprError();
   return BuildUnresolvedCoawaitExpr(Loc, E,
                                    cast<UnresolvedLookupExpr>(Lookup.get()));
 }
 
 ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *E,
                                             UnresolvedLookupExpr *Lookup) {
   auto *FSI = checkCoroutineContext(*this, Loc, "co_await");
-  if (!FSI)
+  if (!FSI || !isValidSuspensionContext(*this, Loc, "co_await"))
     return ExprError();
 
   if (E->getType()->isPlaceholderType()) {
     ExprResult R = CheckPlaceholderExpr(E);
     if (R.isInvalid())
       return ExprError();
     E = R.get();
   }
ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *E,
   Expr *Res =
       new (Context) CoawaitExpr(Loc, E, RSS.Results[0], RSS.Results[1],
                                 RSS.Results[2], RSS.OpaqueValue, IsImplicit);
 
   return Res;
 }
 
 ExprResult Sema::ActOnCoyieldExpr(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!ActOnCoroutineBodyStart(S, Loc, "co_yield")) {
+  if (!ActOnCoroutineBodyStart(S, Loc, "co_yield") ||
+      !isValidSuspensionContext(*this, Loc, "co_yield")) {
     CorrectDelayedTyposInExpr(E);
     return ExprError();
   }
 
   // Build yield_value call.
   ExprResult Awaitable = buildPromiseCall(
       *this, getCurFunction()->CoroutinePromise, Loc, "yield_value", E);
   if (Awaitable.isInvalid())

test/SemaCXX/coroutines.cpp

struct CtorDtor {
   }
   CtorDtor(awaitable a) {
     // The spec doesn't say this is ill-formed, but it must be.
     co_await a; // expected-error {{'co_await' cannot be used in a constructor}}
   }
   ~CtorDtor() {
     co_return 0; // expected-error {{'co_return' cannot be used in a destructor}}
   }
   // FIXME: The spec says this is ill-formed.
   void operator=(CtorDtor&) {
     co_yield 0; // expected-error {{'co_yield' cannot be used in a copy assignment operator}}
   }
   void operator=(CtorDtor const &) {
     co_yield 0; // expected-error {{'co_yield' cannot be used in a copy assignment operator}}
   }
   void operator=(CtorDtor &&) {
     co_await a; // expected-error {{'co_await' cannot be used in a move assignment operator}}
   }
   void operator=(CtorDtor const &&) {
     co_await a; // expected-error {{'co_await' cannot be used in a move assignment operator}}
   }
   void operator=(int) {
     co_await a; // OK. Not a special member
   }
 };
 
 void unevaluated() {
   decltype(co_await a); // expected-error {{cannot be used in an unevaluated context}}
   sizeof(co_await a); // expected-error {{cannot be used in an unevaluated context}}
   typeid(co_await a); // expected-error {{cannot be used in an unevaluated context}}
   decltype(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
   sizeof(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
   typeid(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
 }
 
 // [expr.await]p2: "An await-expression shall not appear in a default argument."
 // FIXME: A better diagnostic would explicitly state that default arguments are
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try {
+  } catch (...) {
+    co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+    co_yield a; // expected-error {{'co_yield' cannot be used in the handler of a try block}}
+  }
+}
+
+void return_in_catch_coroutine() {
+  try {
+  } catch (...) {
+    co_return 123; // OK
+  }
+}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used in a constexpr function}}
   // expected-error@-1 {{'co_yield' cannot be used in a function with a deduced return type}}
 }
 
 void varargs_coroutine(const char *, ...) {
   co_await a; // expected-error {{'co_await' cannot be used in a varargs function}}
 }