[coroutines] Diagnose when promise types fail to declare either return_void or return_value.

[EricWF]

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.

[GorNishanov]

LGTM

include/clang/Basic/DiagnosticSemaKinds.td

 def err_ambiguous_suitable_delete_member_function_found : Error<
   "multiple suitable %0 functions in %1">;
 def warn_ambiguous_suitable_delete_function_found : Warning<
   "multiple suitable %0 functions for %1; no 'operator delete' function "
   "will be invoked if initialization throws an exception">,
   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 "
   "incompatible with C++1z">, InGroup<DeprecatedIncrementBool>;
 def ext_increment_bool : ExtWarn<
   "ISO C++1z does not allow incrementing expression of type bool">,
   DefaultError, InGroup<IncrementBool>;
 def err_increment_decrement_enum : Error<

   "'%0' included multiple times, additional include site here">;
 }
 
 let CategoryName = "Coroutines Issue" in {
 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<
   "'%0' cannot be used in an unevaluated context">;
 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"

   "this function cannot be a coroutine: %q0 has no member named 'promise_type'">;
 def err_implied_std_coroutine_traits_promise_type_not_class : Error<
   "this function cannot be a coroutine: %0 is not a class">;
 def err_coroutine_promise_type_incomplete : Error<
   "this function cannot be a coroutine: %0 is an incomplete type">;
 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<
   "call to '%select{initial_suspend|final_suspend}0' implicitly "
   "required by the %select{initial suspend point|final suspend point}0">;
 def err_coroutine_promise_unhandled_exception_required : Error<
   "%0 is required to declare the member 'unhandled_exception()'">;
 def warn_coroutine_promise_unhandled_exception_required_with_exceptions : Warning<

lib/Sema/SemaCoroutine.cpp

 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 
 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
 /// function type.
 static QualType lookupPromiseType(Sema &S, const FunctionProtoType *FnType,
                                   SourceLocation KwLoc,
                                   SourceLocation FuncLoc) {
   // FIXME: Cache std::coroutine_traits once we've found it.
bool CoroutineStmtBuilder::makeReturnOnAllocFailure() {
   ExprResult ReturnObjectOnAllocationFailure =
       S.ActOnCallExpr(nullptr, DeclNameExpr.get(), Loc, {}, Loc);
   if (ReturnObjectOnAllocationFailure.isInvalid())
     return false;
 
   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;
   }
 
   this->ReturnStmtOnAllocFailure = ReturnStmt.get();
   return true;
 }

 
 bool CoroutineStmtBuilder::makeOnFallthrough() {
   assert(!IsPromiseDependentType &&
          "cannot make statement while the promise type is dependent");
 
   // [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) {
     // If the unqualified-id return_void is found, flowing off the end of a
     // coroutine is equivalent to a co_return with no operand. Otherwise,
     // flowing off the end of a coroutine results in undefined behavior.
     Fallthrough = S.BuildCoreturnStmt(FD.getLocation(), nullptr,
                                       /*IsImplicit*/false);
bool CoroutineStmtBuilder::makeReturnObject() {
 
   this->ReturnValue = ReturnObject.get();
   return true;
 }
 
 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();
 }
 
 bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
   assert(!IsPromiseDependentType &&
          "cannot make statement while the promise type is dependent");

test/SemaCXX/coreturn.cpp

 VoidTagReturnValue test10() {
   co_await a;
 } // expected-warning {{control reaches end of coroutine}}
 
 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}}

test/SemaCXX/coroutines.cpp

namespace dependent_operator_co_await_lookup {
 
   struct transform_promise {
     typedef transform_awaitable await_arg;
     coro<transform_promise> get_return_object();
     transformed initial_suspend();
     ::adl_ns::coawait_arg_type final_suspend();
     transformed await_transform(transform_awaitable);
     void unhandled_exception();
+    void return_void();
   };
   template <class AwaitArg>
   struct basic_promise {
     typedef AwaitArg await_arg;
     coro<basic_promise> get_return_object();
     awaitable initial_suspend();
     awaitable final_suspend();
     void unhandled_exception();
+    void return_void();
   };
 
   awaitable operator co_await(await_arg_1);
 
   template <typename T, typename U>
   coro<T> await_template_3(U t) {
     co_await t;
   }
void z(yield_fn_tag) {
     co_return g;
   }
 }
 
 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;
 }
 
 struct bad_promise_2 {
   coro<bad_promise_2> get_return_object();
   // 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'}}
   co_await a;
 }
 
 struct bad_promise_3 {
   coro<bad_promise_3> get_return_object();
   // 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;
 }
 
 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'}}
   // expected-note@-1 {{call to 'initial_suspend' implicitly required by the initial suspend point}}
   co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
 }
 
 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'}}
   // expected-note@-1 {{call to 'final_suspend' implicitly required by the final suspend point}}
   co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
 }
 
 struct bad_promise_6 {
   coro<bad_promise_6> get_return_object();
   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'}}
   co_await a;
 }
 
 template <class T>
 coro<T> bad_implicit_return_dependent(T) { // expected-error {{'bad_promise_6' declares both 'return_value' and 'return_void'}}
coro<T> dependent_uses_nothrow_new(T) {
    co_return;
 }
 template coro<good_promise_13> dependent_uses_nothrow_new(good_promise_13);
 
 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() {}
     void unhandled_exception();
   };
 };
 
 extern "C" int f(mismatch_gro_type_tag1) { 
   // expected-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'void'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
 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() {}
     void unhandled_exception();
   };
 };
 
 extern "C" int f(mismatch_gro_type_tag2) { 
   // expected-error@-1 {{cannot initialize return object of type 'int' with an lvalue of type 'void *'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
 struct mismatch_gro_type_tag3 {};
 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() {}
     void unhandled_exception();
   };
 };
 
 extern "C" int f(mismatch_gro_type_tag3) { 
   // expected-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'void'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
 
 struct mismatch_gro_type_tag4 {};
 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() {}
     void unhandled_exception();
   };
 };
 
 extern "C" int f(mismatch_gro_type_tag4) { 
   // expected-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'char *'}}
   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;
 }