Author: ericwf Date: Thu May 25 09:59:39 2017 New Revision: 303868 URL: http://llvm.org/viewvc/llvm-project?rev=303868&view=rev Log: [coroutines] Diagnose when promise types fail to declare either return_void or return_value.
Summary: According to the PDTS it's perfectly legal to have a promise type that defines neither `return_value` nor `return_void`. However a coroutine that uses such a promise type will almost always have UB, because it can never `co_return`. This patch changes Clang to diagnose such cases as an error. It also cleans up some of the diagnostic messages relating to member lookup in the promise type. Reviewers: GorNishanov, rsmith Reviewed By: GorNishanov Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D33534 Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/lib/Sema/SemaCoroutine.cpp cfe/trunk/test/SemaCXX/coreturn.cpp cfe/trunk/test/SemaCXX/coroutines.cpp Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=303868&r1=303867&r2=303868&view=diff ============================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu May 25 09:59:39 2017 @@ -6303,6 +6303,8 @@ def warn_ambiguous_suitable_delete_funct InGroup<DiagGroup<"ambiguous-delete">>; def note_member_declared_here : Note< "member %0 declared here">; +def note_member_first_declared_here : Note< + "member %0 first declared here">; def err_decrement_bool : Error<"cannot decrement expression of type bool">; def warn_increment_bool : Warning< "incrementing expression of type bool is deprecated and " @@ -8918,8 +8920,6 @@ 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 note_promise_member_declared_here : Note< - "'%0' is declared here">; def err_coroutine_objc_method : Error< "Objective-C methods as coroutines are not yet supported">; def err_coroutine_unevaluated_context : Error< @@ -8954,8 +8954,10 @@ def err_coroutine_promise_type_incomplet def err_coroutine_type_missing_specialization : Error< "this function cannot be a coroutine: missing definition of " "specialization %q0">; -def err_coroutine_promise_return_ill_formed : Error< - "%0 declares both 'return_value' and 'return_void'">; +def err_coroutine_promise_incompatible_return_functions : Error< + "the coroutine promise type %0 declares both 'return_value' and 'return_void'">; +def err_coroutine_promise_requires_return_function : Error< + "the coroutine promise type %0 must declare either 'return_value' or 'return_void'">; def note_coroutine_promise_implicit_await_transform_required_here : Note< "call to 'await_transform' implicitly required by 'co_await' here">; def note_coroutine_promise_suspend_implicitly_required : Note< Modified: cfe/trunk/lib/Sema/SemaCoroutine.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCoroutine.cpp?rev=303868&r1=303867&r2=303868&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaCoroutine.cpp (original) +++ cfe/trunk/lib/Sema/SemaCoroutine.cpp Thu May 25 09:59:39 2017 @@ -23,14 +23,22 @@ using namespace clang; using namespace sema; -static bool lookupMember(Sema &S, const char *Name, CXXRecordDecl *RD, - SourceLocation Loc) { +static LookupResult lookupMember(Sema &S, const char *Name, CXXRecordDecl *RD, + SourceLocation Loc, bool &Res) { DeclarationName DN = S.PP.getIdentifierInfo(Name); LookupResult LR(S, DN, Loc, Sema::LookupMemberName); // Suppress diagnostics when a private member is selected. The same warnings // will be produced again when building the call. LR.suppressDiagnostics(); - return S.LookupQualifiedName(LR, RD); + Res = S.LookupQualifiedName(LR, RD); + return LR; +} + +static bool lookupMember(Sema &S, const char *Name, CXXRecordDecl *RD, + SourceLocation Loc) { + bool Res; + lookupMember(S, Name, RD, Loc, Res); + return Res; } /// Look up the std::coroutine_traits<...>::promise_type for the given @@ -861,9 +869,8 @@ bool CoroutineStmtBuilder::makeReturnOnA StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnObjectOnAllocationFailure.get()); if (ReturnStmt.isInvalid()) { - S.Diag(Found.getFoundDecl()->getLocation(), - diag::note_promise_member_declared_here) - << DN.getAsString(); + S.Diag(Found.getFoundDecl()->getLocation(), diag::note_member_declared_here) + << DN; S.Diag(Fn.FirstCoroutineStmtLoc, diag::note_declared_coroutine_here) << Fn.getFirstCoroutineStmtKeyword(); return false; @@ -993,13 +1000,32 @@ bool CoroutineStmtBuilder::makeOnFallthr // [dcl.fct.def.coroutine]/4 // The unqualified-ids 'return_void' and 'return_value' are looked up in // the scope of class P. If both are found, the program is ill-formed. - const bool HasRVoid = lookupMember(S, "return_void", PromiseRecordDecl, Loc); - const bool HasRValue = lookupMember(S, "return_value", PromiseRecordDecl, Loc); + bool HasRVoid, HasRValue; + LookupResult LRVoid = + lookupMember(S, "return_void", PromiseRecordDecl, Loc, HasRVoid); + LookupResult LRValue = + lookupMember(S, "return_value", PromiseRecordDecl, Loc, HasRValue); StmtResult Fallthrough; if (HasRVoid && HasRValue) { // FIXME Improve this diagnostic - S.Diag(FD.getLocation(), diag::err_coroutine_promise_return_ill_formed) + S.Diag(FD.getLocation(), + diag::err_coroutine_promise_incompatible_return_functions) + << PromiseRecordDecl; + S.Diag(LRVoid.getRepresentativeDecl()->getLocation(), + diag::note_member_first_declared_here) + << LRVoid.getLookupName(); + S.Diag(LRValue.getRepresentativeDecl()->getLocation(), + diag::note_member_first_declared_here) + << LRValue.getLookupName(); + return false; + } else if (!HasRVoid && !HasRValue) { + // FIXME: The PDTS currently specifies this case as UB, not ill-formed. + // However we still diagnose this as an error since until the PDTS is fixed. + S.Diag(FD.getLocation(), + diag::err_coroutine_promise_requires_return_function) + << PromiseRecordDecl; + S.Diag(PromiseRecordDecl->getLocation(), diag::note_defined_here) << PromiseRecordDecl; return false; } else if (HasRVoid) { @@ -1074,8 +1100,8 @@ bool CoroutineStmtBuilder::makeReturnObj static void noteMemberDeclaredHere(Sema &S, Expr *E, FunctionScopeInfo &Fn) { if (auto *MbrRef = dyn_cast<CXXMemberCallExpr>(E)) { auto *MethodDecl = MbrRef->getMethodDecl(); - S.Diag(MethodDecl->getLocation(), diag::note_promise_member_declared_here) - << MethodDecl->getName(); + S.Diag(MethodDecl->getLocation(), diag::note_member_declared_here) + << MethodDecl; } S.Diag(Fn.FirstCoroutineStmtLoc, diag::note_declared_coroutine_here) << Fn.getFirstCoroutineStmtKeyword(); Modified: cfe/trunk/test/SemaCXX/coreturn.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/coreturn.cpp?rev=303868&r1=303867&r2=303868&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/coreturn.cpp (original) +++ cfe/trunk/test/SemaCXX/coreturn.cpp Thu May 25 09:59:39 2017 @@ -138,16 +138,3 @@ VoidTagReturnValue test11(bool b) { if (b) co_return 42; } // expected-warning {{control may reach end of coroutine}} - -// FIXME: Promise types that declare neither return_value nor return_void -// should be ill-formed. This test should be removed when that is fixed. -VoidTagNoReturn test12() { - co_await a; -} // expected-warning {{control reaches end of coroutine}} - -// FIXME: Promise types that declare neither return_value nor return_void -// should be ill-formed. This test should be removed when that is fixed. -VoidTagNoReturn test13(bool b) { - if (b) - co_await a; -} // expected-warning {{control reaches end of coroutine}} Modified: cfe/trunk/test/SemaCXX/coroutines.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/coroutines.cpp?rev=303868&r1=303867&r2=303868&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/coroutines.cpp (original) +++ cfe/trunk/test/SemaCXX/coroutines.cpp Thu May 25 09:59:39 2017 @@ -347,6 +347,7 @@ namespace dependent_operator_co_await_lo ::adl_ns::coawait_arg_type final_suspend(); transformed await_transform(transform_awaitable); void unhandled_exception(); + void return_void(); }; template <class AwaitArg> struct basic_promise { @@ -355,6 +356,7 @@ namespace dependent_operator_co_await_lo awaitable initial_suspend(); awaitable final_suspend(); void unhandled_exception(); + void return_void(); }; awaitable operator co_await(await_arg_1); @@ -473,6 +475,7 @@ struct bad_promise_1 { suspend_always initial_suspend(); suspend_always final_suspend(); void unhandled_exception(); + void return_void(); }; coro<bad_promise_1> missing_get_return_object() { // expected-error {{no member named 'get_return_object' in 'bad_promise_1'}} co_await a; @@ -483,6 +486,7 @@ struct bad_promise_2 { // FIXME: We shouldn't offer a typo-correction here! suspend_always final_suspend(); // expected-note {{here}} void unhandled_exception(); + void return_void(); }; // FIXME: This shouldn't happen twice coro<bad_promise_2> missing_initial_suspend() { // expected-error {{no member named 'initial_suspend' in 'bad_promise_2'}} @@ -494,6 +498,7 @@ struct bad_promise_3 { // FIXME: We shouldn't offer a typo-correction here! suspend_always initial_suspend(); // expected-note {{here}} void unhandled_exception(); + void return_void(); }; coro<bad_promise_3> missing_final_suspend() { // expected-error {{no member named 'final_suspend' in 'bad_promise_3'}} co_await a; @@ -503,6 +508,7 @@ struct bad_promise_4 { coro<bad_promise_4> get_return_object(); not_awaitable initial_suspend(); suspend_always final_suspend(); + void return_void(); }; // FIXME: This diagnostic is terrible. coro<bad_promise_4> bad_initial_suspend() { // expected-error {{no member named 'await_ready' in 'not_awaitable'}} @@ -514,6 +520,7 @@ struct bad_promise_5 { coro<bad_promise_5> get_return_object(); suspend_always initial_suspend(); not_awaitable final_suspend(); + void return_void(); }; // FIXME: This diagnostic is terrible. coro<bad_promise_5> bad_final_suspend() { // expected-error {{no member named 'await_ready' in 'not_awaitable'}} @@ -526,8 +533,8 @@ struct bad_promise_6 { suspend_always initial_suspend(); suspend_always final_suspend(); void unhandled_exception(); - void return_void(); - void return_value(int) const; + void return_void(); // expected-note 2 {{member 'return_void' first declared here}} + void return_value(int) const; // expected-note 2 {{member 'return_value' first declared here}} void return_value(int); }; coro<bad_promise_6> bad_implicit_return() { // expected-error {{'bad_promise_6' declares both 'return_value' and 'return_void'}} @@ -751,7 +758,7 @@ struct mismatch_gro_type_tag1 {}; template<> struct std::experimental::coroutine_traits<int, mismatch_gro_type_tag1> { struct promise_type { - void get_return_object() {} //expected-note {{'get_return_object' is declared here}} + void get_return_object() {} //expected-note {{member 'get_return_object' declared here}} suspend_always initial_suspend() { return {}; } suspend_always final_suspend() { return {}; } void return_void() {} @@ -768,7 +775,7 @@ struct mismatch_gro_type_tag2 {}; template<> struct std::experimental::coroutine_traits<int, mismatch_gro_type_tag2> { struct promise_type { - void* get_return_object() {} //expected-note {{'get_return_object' is declared here}} + void *get_return_object() {} //expected-note {{member 'get_return_object' declared here}} suspend_always initial_suspend() { return {}; } suspend_always final_suspend() { return {}; } void return_void() {} @@ -786,7 +793,7 @@ template<> struct std::experimental::coroutine_traits<int, mismatch_gro_type_tag3> { struct promise_type { int get_return_object() {} - static void get_return_object_on_allocation_failure() {} //expected-note {{'get_return_object_on_allocation_failure' is declared here}} + static void get_return_object_on_allocation_failure() {} //expected-note {{member 'get_return_object_on_allocation_failure' declared here}} suspend_always initial_suspend() { return {}; } suspend_always final_suspend() { return {}; } void return_void() {} @@ -805,7 +812,7 @@ template<> struct std::experimental::coroutine_traits<int, mismatch_gro_type_tag4> { struct promise_type { int get_return_object() {} - static char* get_return_object_on_allocation_failure() {} //expected-note {{'get_return_object_on_allocation_failure' is declared here}} + static char *get_return_object_on_allocation_failure() {} //expected-note {{member 'get_return_object_on_allocation_failure' declared}} suspend_always initial_suspend() { return {}; } suspend_always final_suspend() { return {}; } void return_void() {} @@ -818,14 +825,15 @@ extern "C" int f(mismatch_gro_type_tag4) co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}} } -struct bad_promise_no_return_func { +struct bad_promise_no_return_func { // expected-note {{'bad_promise_no_return_func' defined here}} coro<bad_promise_no_return_func> get_return_object(); suspend_always initial_suspend(); suspend_always final_suspend(); void unhandled_exception(); }; -// FIXME: Make this ill-formed because the promise type declares -// neither return_value(...) or return_void(). +// FIXME: The PDTS currently specifies this as UB, technically forbidding a +// diagnostic. coro<bad_promise_no_return_func> no_return_value_or_return_void() { + // expected-error@-1 {{'bad_promise_no_return_func' must declare either 'return_value' or 'return_void'}} co_await a; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
