From b15c35aff878cd59328de80b95ba35ab0399b2c8 Mon Sep 17 00:00:00 2001 From: Brian Gesiak Date: Mon, 25 Mar 2019 00:53:10 +0000 Subject: [PATCH] Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block" 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. Reviewers: GorNishanov, tks2103, rsmith, riccibruno, jbulow Reviewed By: riccibruno Subscribers: EricWF, jdoerfert, lewissbaker, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D59752 llvm-svn: 356865 --- clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 + clang/include/clang/Sema/Scope.h | 3 ++ clang/lib/Parse/ParseStmt.cpp | 3 +- clang/lib/Sema/Scope.cpp | 4 +- clang/lib/Sema/SemaCoroutine.cpp | 64 ++++++++++++++++++------ clang/test/SemaCXX/coroutines.cpp | 63 ++++++++++++++++++++--- clang/test/SemaCXX/exceptions.cpp | 5 ++ 7 files changed, 121 insertions(+), 23 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 5467293..a8776d9 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -9271,6 +9271,8 @@ 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< diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h index a1b451c..7848df8 100644 --- a/clang/include/clang/Sema/Scope.h +++ b/clang/include/clang/Sema/Scope.h @@ -131,6 +131,9 @@ public: /// 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: diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp index 267003b..2db3132 100644 --- a/clang/lib/Parse/ParseStmt.cpp +++ b/clang/lib/Parse/ParseStmt.cpp @@ -2261,7 +2261,8 @@ StmtResult Parser::ParseCXXCatchBlock(bool FnCatch) { // 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)); + Scope::CatchScope | + (FnCatch ? Scope::FnTryCatchScope : 0)); // exception-declaration is equivalent to '...' or a parameter-declaration // without default arguments. diff --git a/clang/lib/Sema/Scope.cpp b/clang/lib/Sema/Scope.cpp index 0994068..51b0b24 100644 --- a/clang/lib/Sema/Scope.cpp +++ b/clang/lib/Sema/Scope.cpp @@ -166,7 +166,9 @@ void Scope::dumpImpl(raw_ostream &OS) const { {SEHExceptScope, "SEHExceptScope"}, {SEHFilterScope, "SEHFilterScope"}, {CompoundStmtScope, "CompoundStmtScope"}, - {ClassInheritanceScope, "ClassInheritanceScope"}}; + {ClassInheritanceScope, "ClassInheritanceScope"}, + {CatchScope, "CatchScope"}, + }; for (auto Info : FlagInfo) { if (Flags & Info.first) { diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index 22a1280..9d328f4 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -185,21 +185,8 @@ static QualType lookupCoroutineHandleType(Sema &S, QualType PromiseType, 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 @@ -668,12 +655,57 @@ bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc, 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(); @@ -771,6 +803,8 @@ ExprResult Sema::ActOnCoyieldExpr(Scope *S, SourceLocation Loc, Expr *E) { return ExprError(); } + checkSuspensionContext(*this, Loc, "co_yield"); + // Build yield_value call. ExprResult Awaitable = buildPromiseCall( *this, getCurFunction()->CoroutinePromise, Loc, "yield_value", E); diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp index 5e1ff34..99964ef 100644 --- a/clang/test/SemaCXX/coroutines.cpp +++ b/clang/test/SemaCXX/coroutines.cpp @@ -314,13 +314,23 @@ struct CtorDtor { } }; +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." @@ -328,6 +338,47 @@ void unevaluated() { // 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}} diff --git a/clang/test/SemaCXX/exceptions.cpp b/clang/test/SemaCXX/exceptions.cpp index 9e76783..1e786ad 100644 --- a/clang/test/SemaCXX/exceptions.cpp +++ b/clang/test/SemaCXX/exceptions.cpp @@ -7,6 +7,7 @@ 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; @@ -18,6 +19,10 @@ void trys() { } 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'}} } -- 2.7.4