Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block"

[modocache]

Summary

https://reviews.llvm.org/D59076 added a new coroutine error that
prevented users from using 'co_await' or 'co_yield' within a exception
handler. However, it was reverted in https://reviews.llvm.org/rC356774
because it caused a regression in nested scopes in C++ catch statements,
as documented by https://bugs.llvm.org/show_bug.cgi?id=41171.

The issue was due to an incorrect use of a clang::ParseScope. To fix:

1. Add a regression test for catch statement parsing that mimics the bug report from https://bugs.llvm.org/show_bug.cgi?id=41171.
2. Re-apply the coroutines error patch from https://reviews.llvm.org/D59076, but this time with the correct ParseScope behavior.

[riccibruno]

lib/Parse/ParseStmt.cpp

2293

Just to make sure I understood the problem correctly, the issue was that you passed ScopeFlags to ParseCompoundStatement(). This override the default flags which are Scope::DeclScope | Scope::CompoundStmtScope. In particular now ParseCompoundStatement() was done as if in the controlling scope of an if statement. Now as per [basic.scope.block]/p4:

Names declared in the init-statement, the for-range-declaration, and in the condition of if, while, for, and switch statements are local to the if, while, for, or switch statement (including the controlled statement), and shall not be redeclared in a subsequent condition of that statement nor in the outermost block (or, for the if statement, any of the outermost blocks) of the controlled statement; see 9.4.

This caused the declaration in the compound statement to be detected as erroneous. Indeed the following worked just fine.

void f() {
    try {}
    catch (...) {
        int i;
        {
            {
                int i;
            }
        }
    }
}

Does this make sense or I am completely off base here ?

[modocache]

lib/Parse/ParseStmt.cpp

2293

That's exactly my understanding, yes! In fact thank you for the clear explanation.

[riccibruno]

Great! Since this already received one round of reviews I guess this looks okay.

test/SemaCXX/exceptions.cpp

25

I am wondering if there is already a test which checks systematically that a declaration which shadows another declaration is valid/invalid. I am not seeing such a systematic test, but it might be a nice addition (though that it clearly out of scope for this patch)

[modocache]

Thank you for the review!

test/SemaCXX/exceptions.cpp

25

That's a good point. I was very surprised when I learned that my original patch had broken the lookup behavior described in https://bugs.llvm.org/show_bug.cgi?id=41171 but still passed all the tests in check-clang. I'll try to follow up this patch with some more comprehensive tests.

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 |
+                                  Scope::CatchScope |
                                   (FnCatch ? Scope::FnTryCatchScope : 0));
 
   // 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());
   if (Block.isInvalid())
     return Block;
 
   return Actions.ActOnCXXCatchBlock(CatchLoc, ExceptionDecl, Block.get());
 }
 
 void Parser::ParseMicrosoftIfExistsStatement(StmtVector &Stmts) {
   IfExistsCondition Result;

lib/Sema/Scope.cpp

std::pair<unsigned, const char *> FlagInfo[] = {
       {OpenMPDirectiveScope, "OpenMPDirectiveScope"},
       {OpenMPLoopDirectiveScope, "OpenMPLoopDirectiveScope"},
       {OpenMPSimdDirectiveScope, "OpenMPSimdDirectiveScope"},
       {EnumScope, "EnumScope"},
       {SEHTryScope, "SEHTryScope"},
       {SEHExceptScope, "SEHExceptScope"},
       {SEHFilterScope, "SEHFilterScope"},
       {CompoundStmtScope, "CompoundStmtScope"},
-      {ClassInheritanceScope, "ClassInheritanceScope"}};
+      {ClassInheritanceScope, "ClassInheritanceScope"},
+      {CatchScope, "CatchScope"},
+  };
 
   for (auto Info : FlagInfo) {
     if (Flags & Info.first) {
       OS << Info.second;
       Flags &= ~Info.first;
       if (Flags)
         OS << " | ";
     }

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;
 }
 
+// Recursively walks up the scope hierarchy until either a 'catch' or a function
+// scope is found, whichever comes first.
+static bool isWithinCatchScope(Scope *S) {
+  // 'co_await' and 'co_yield' keywords are disallowed within catch blocks, but
+  // lambdas that use 'co_await' are allowed. The loop below ends when a
+  // function scope is found in order to ensure the following behavior:
+  //
+  // void foo() {      // <- function scope
+  //   try {           //
+  //     co_await x;   // <- 'co_await' is OK within a function scope
+  //   } catch {       // <- catch scope
+  //     co_await x;   // <- 'co_await' is not OK within a catch scope
+  //     []() {        // <- function scope
+  //       co_await x; // <- 'co_await' is OK within a function scope
+  //     }();
+  //   }
+  // }
+  while (S && !(S->getFlags() & Scope::FnScope)) {
+    if (S->getFlags() & Scope::CatchScope)
+      return true;
+    S = S->getParent();
+  }
+  return false;
+}
+
+// [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 void checkSuspensionContext(Sema &S, SourceLocation Loc,
+                                   StringRef Keyword) {
+  // 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;
+
+  // Second emphasis of [expr.await]p2: must be outside of an exception handler.
+  if (isWithinCatchScope(S.getCurScope()))
+    S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;
+}
+
 ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
   if (!ActOnCoroutineBodyStart(S, Loc, "co_await")) {
     CorrectDelayedTyposInExpr(E);
     return ExprError();
   }
 
+  checkSuspensionContext(*this, Loc, "co_await");
+
   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();

 }
 
 ExprResult Sema::ActOnCoyieldExpr(Scope *S, SourceLocation Loc, Expr *E) {
   if (!ActOnCoroutineBodyStart(S, Loc, "co_yield")) {
     CorrectDelayedTyposInExpr(E);
     return ExprError();
   }
 
+  checkSuspensionContext(*this, Loc, "co_yield");
+
   // Build yield_value call.
   ExprResult Awaitable = buildPromiseCall(
       *this, getCurFunction()->CoroutinePromise, Loc, "yield_value", E);
   if (Awaitable.isInvalid())
     return ExprError();
 
   // Build 'operator co_await' call.
   Awaitable = buildOperatorCoawaitCall(*this, S, Loc, Awaitable.get());

test/SemaCXX/coroutines.cpp

struct CtorDtor {
   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
   }
 };
 
+namespace std { class type_info; }
+
 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}}
+  decltype(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+                        // expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+                      // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+                      // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+                      // expected-warning@-2 {{expression result unused}}
+  decltype(co_yield 1); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+                        // expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_yield 2); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+                      // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_yield 3); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+                      // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+                      // expected-warning@-2 {{expression result unused}}
 }
 
 // [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 (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+    []() -> void { co_await a; }(); // OK
+    co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void await_nested_in_catch_coroutine() {
+  try {
+  } catch (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+    try {
+      co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+      []() -> void { co_await a; }(); // OK
+    } catch (...) {
+      co_return 123;
+    }
+  }
+}
+
+void await_in_lambda_in_catch_coroutine() {
+  try {
+  } catch (...) {
+    []() -> void { co_await a; }(); // OK
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+    co_yield 1; // 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}}
 }

test/SemaCXX/exceptions.cpp

 // RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
 // RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
 // RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
 
 struct A; // expected-note 4 {{forward declaration of 'A'}}
 
 struct Abstract { virtual void f() = 0; }; // expected-note {{unimplemented pure virtual method 'f'}}
 
 void trys() {
+  int k = 42;
   try {
   } catch(int i) { // expected-note {{previous definition}}
     int j = i;
     int i; // expected-error {{redefinition of 'i'}}
   } catch(float i) {
   } catch(void v) { // expected-error {{cannot catch incomplete type 'void'}}
   } catch(A a) { // expected-error {{cannot catch incomplete type 'A'}}
   } catch(A *a) { // expected-error {{cannot catch pointer to incomplete type 'A'}}
   } catch(A &a) { // expected-error {{cannot catch reference to incomplete type 'A'}}
   } catch(Abstract) { // expected-error {{variable type 'Abstract' is an abstract class}}
   } catch(...) {
+    int ref = k;
+    {
+      int ref = k;
+    }
     int j = i; // expected-error {{use of undeclared identifier 'i'}}
   }
 
   try {
   } catch(...) { // expected-error {{catch-all handler must come last}}
   } catch(int) {
   }
 }