[C++20] [Coroutines] Allow promise_type to not define return_void or return_value

[ChuanqiXu]

Summary

According to [[dcl.fct.def.coroutine]p6](http://eel.is/c++draft/dcl.fct.def.coroutine#6), the promise_type is allowed to not define return_void or return_value:

If searches for the names return_­void and return_­value in the scope of the promise type each find any declarations, the program is ill-formed.
[Note 1: If 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 ([stmt.return.coroutine]). — end note]

[urnathan]

clang/lib/Sema/SemaCoroutine.cpp

1475–1476

there's some repetition in the comments. Perhaps a bloc comment before the if-else sequence?

1476–1477

Your testcase doesnt show such a warning. This seems unhelpful.

[ChuanqiXu]

clang/lib/Sema/SemaCoroutine.cpp

1475–1476

Yeah, would do.

1476–1477

Oh, I need emphasize in the comment that the warning is not correct. I want to say that if we don't set FallThrough, the compiler would emit incorrect warning. And in this revision, we set FallThrough so that we could avoid the warning. The edited test case could address this.

[ChuanqiXu]

Address comments.

[urnathan]

looks good! Yay, Halting Problem :)

[• Quuxplusone]

clang/test/SemaCXX/coroutines.cpp

987

It's not UB; it's potential UB.
My understanding is that such a coroutine could still be used:

• if it exits by throwing an exception instead of co_return'ing
• if it suspends and then its caller destroys it instead of resumeing it

I believe it would be useful to have some actual tests for these scenarios, that would actually run and check that the runtime behavior is correct, or at least check the IR that was generated. Is that possible?

[ChuanqiXu]

Address comments

[ChuanqiXu]

clang/test/SemaCXX/coroutines.cpp

987

Yes, such a coroutine could still be used. AFAIK, we couldn't make tests which would run actually. All the test cases I know is about pattern match. So we couldn't make tests in clang to run actually for this. I have tested it locally. And I think it is unnecessary to check the IR was generated since this patch didn't add new statements except a null statement as marker.

BTW, it shows another big issue for coroutines. We lack test-suite and benchmarks now. Now I tested the correctness of the compiler by test it in our internal projects. But we need an open source test suites absolutely. This is true for benchmarks. I thought to make the two things... but I don't find time for it now...

clang/include/clang/Basic/DiagnosticSemaKinds.td

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 %0">;
 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<

clang/lib/Sema/SemaCoroutine.cpp

bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
 
   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.
+  // [dcl.fct.def.coroutine]/p6
+  // If searches for the names return_­void and return_­value in the scope of
+  // the promise type each find any declarations, the program is ill-formed.
+  // [Note 1: If 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 ([stmt.return.coroutine]). —
+  // end note]
   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_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;
+    // We need to set 'Fallthrough'. Otherwise the other analysis part might
+    // think the coroutine has defined a return_value method. So it might emit
+    // **false** positive warning. e.g.,
+    //
+    //    promise_without_return_func foo() {
+    //        co_await something();
+    //    }
+    //
+    // Then AnalysisBasedWarning would emit a warning about `foo()` lacking a
+    // co_return statements, which isn't correct.
+    Fallthrough = S.ActOnNullStmt(PromiseRecordDecl->getLocation());
+    if (Fallthrough.isInvalid())
       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);
     Fallthrough = S.ActOnFinishFullStmt(Fallthrough.get());
     if (Fallthrough.isInvalid())
       return false;
   }
 
   this->OnFallthrough = Fallthrough.get();
   return true;
 }

clang/test/SemaCXX/coroutines-exp-namespace.cpp

struct std::experimental::coroutine_traits<int, mismatch_gro_type_tag4> {
   };
 };
 
 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 { // 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() noexcept;
-  void unhandled_exception();
-};
-// 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;
-}
-
 struct bad_await_suspend_return {
   bool await_ready();
   // expected-error@+1 {{return type of 'await_suspend' is required to be 'void' or 'bool' (have 'char')}}
   char await_suspend(std::experimental::coroutine_handle<>);
   void await_resume();
 };
 struct bad_await_ready_return {
   // expected-note@+1 {{return type of 'await_ready' is required to be contextually convertible to 'bool'}}

clang/test/SemaCXX/coroutines.cpp

struct std::coroutine_traits<int, mismatch_gro_type_tag4> {
   };
 };
 
 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 { // expected-note {{'bad_promise_no_return_func' defined here}}
-  coro<bad_promise_no_return_func> get_return_object();
+struct promise_no_return_func {
+  coro<promise_no_return_func> get_return_object();
   suspend_always initial_suspend();
   suspend_always final_suspend() noexcept;
   void unhandled_exception();
 };
-// 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'}}
+// [dcl.fct.def.coroutine]/p6
+// If searches for the names return_­void and return_­value in the scope of
+// the promise type each find any declarations, the program is ill-formed.
+// [Note 1: If 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 ([stmt.return.coroutine]). —
+// end note]
+//
+// So it isn't ill-formed if the promise doesn't define return_value and return_void.
+// It is just a potential UB.
+coro<promise_no_return_func> no_return_value_or_return_void() {
   co_await a;
 }
 
+// The following two tests that it would emit correct diagnostic message
+// if we co_return in `promise_no_return_func`.
+coro<promise_no_return_func> no_return_value_or_return_void_2() {
+  co_return; // expected-error {{no member named 'return_void'}}
+}
+
+coro<promise_no_return_func> no_return_value_or_return_void_3() {
+  co_return 43; // expected-error {{no member named 'return_value'}}
+}
+
 struct bad_await_suspend_return {
   bool await_ready();
   // expected-error@+1 {{return type of 'await_suspend' is required to be 'void' or 'bool' (have 'char')}}
   char await_suspend(std::coroutine_handle<>);
   void await_resume();
 };
 struct bad_await_ready_return {
   // expected-note@+1 {{return type of 'await_ready' is required to be contextually convertible to 'bool'}}