[coroutines] Diagnose invalid result types for `await_resume` and `await_suspend` and add missing conversions.

[EricWF]

Summary

The expression await_ready is required to be contextually convertible to bool and await_suspend must be a prvalue of either void or bool.
This patch adds diagnostics for when those requirements are violated.

It also correctly performs the contextual conversion to bool on the result of await_ready

[EricWF]

• Format changes

[GorNishanov]

LGTM

test/SemaCXX/coroutines.cpp

842

For test completeness, I would have a positive case for something like a std::true_type returning await_ready

[GorNishanov]

lib/Sema/SemaCoroutine.cpp

324

enum class?

[EricWF]

lib/Sema/SemaCoroutine.cpp

324

We want the explicit conversion to integers so we can use them as indexes to Results.

[EricWF]

• Add requested test case.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

8977–8980

I would drop the leading 'the' from both of these diagnostics for consistency with our normal terse sentence fragment style.

lib/Sema/SemaCoroutine.cpp

393

It looks like you're not checking the 'prvalue' part of this.

[EricWF]

lib/Sema/SemaCoroutine.cpp

393

I thought so too. I'll add tests to make sure it's being handled correctly.
The following test should expose a lack of checking for prvalues, right?

template <class BoolTy>
struct TestAwait {
  bool await_ready();
  template <class F> BoolTy await_suspend(F); // expected-error
  void await_resume();
};
void test() {
  co_await TestAwait<bool&&>{};
  co_await TestAwait<bool&>{}; 
}

include/clang/Basic/DiagnosticSemaKinds.td

   "%0: 'get_return_object_on_allocation_failure()' must be a static member function">;
 def err_seh_in_a_coroutine_with_cxx_exceptions : Error<
   "cannot use SEH '__try' in a coroutine when C++ exceptions are enabled">;
 def err_coroutine_promise_new_requires_nothrow : Error<
   "%0 is required to have a non-throwing noexcept specification when the promise "
    "type declares 'get_return_object_on_allocation_failure()'">;
 def note_coroutine_promise_call_implicitly_required : Note<
   "call to %0 implicitly required by coroutine function here">;
+def err_await_suspend_invalid_return_type : Error<
+  "the return type of 'await_suspend' is required to be 'void' or 'bool' (have %0)"
+>;
+def note_await_ready_no_bool_conversion : Note<
+  "the return type of 'await_ready' is required to be contextually convertible to 'bool'"
+>;
 }
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Instrumentation Issue" in {

lib/Sema/SemaCoroutine.cpp

ExprResult FromAddr =
       S.BuildDeclarationNameExpr(SS, Found, /*NeedsADL=*/false);
   if (FromAddr.isInvalid())
     return ExprError();
 
   return S.ActOnCallExpr(nullptr, FromAddr.get(), Loc, FramePtr, Loc);
 }
 
 struct ReadySuspendResumeResult {
+  enum AwaitCallType { ACT_Ready, ACT_Suspend, ACT_Resume };
   Expr *Results[3];
   OpaqueValueExpr *OpaqueValue;
   bool IsInvalid;
 };
 
 static ExprResult buildMemberCall(Sema &S, Expr *Base, SourceLocation Loc,
                                   StringRef Name, MultiExprArg Args) {
   DeclarationNameInfo NameInfo(&S.PP.getIdentifierTable().get(Name), Loc);
static ReadySuspendResumeResult buildCoawaitCalls(Sema &S, VarDecl *CoroPromise,
   MultiExprArg Args[] = {None, CoroHandle, None};
   for (size_t I = 0, N = llvm::array_lengthof(Funcs); I != N; ++I) {
     ExprResult Result = buildMemberCall(S, Operand, Loc, Funcs[I], Args[I]);
     if (Result.isInvalid())
       return Calls;
     Calls.Results[I] = Result.get();
   }
 
+  // Assume the calls are valid; all further checking should make them invalid.
   Calls.IsInvalid = false;
+
+  using ACT = ReadySuspendResumeResult::AwaitCallType;
+  CallExpr *AwaitReady = cast<CallExpr>(Calls.Results[ACT::ACT_Ready]);
+  if (!AwaitReady->getType()->isDependentType()) {
+    // [expr.await]p3 [...]
+    // — await-ready is the expression e.await_ready(), contextually converted
+    // to bool.
+    ExprResult Conv = S.PerformContextuallyConvertToBool(AwaitReady);
+    if (Conv.isInvalid()) {
+      S.Diag(AwaitReady->getDirectCallee()->getLocStart(),
+             diag::note_await_ready_no_bool_conversion);
+      S.Diag(Loc, diag::note_coroutine_promise_call_implicitly_required)
+          << AwaitReady->getDirectCallee() << E->getSourceRange();
+      Calls.IsInvalid = true;
+    }
+    Calls.Results[ACT::ACT_Ready] = Conv.get();
+  }
+  CallExpr *AwaitSuspend = cast<CallExpr>(Calls.Results[ACT::ACT_Suspend]);
+  if (!AwaitSuspend->getType()->isDependentType()) {
+    // [expr.await]p3 [...]
+    //   - await-suspend is the expression e.await_suspend(h), which shall be
+    //     a prvalue of type void or bool.
+    QualType RetType = AwaitSuspend->getType();
+    if (RetType != S.Context.BoolTy && RetType != S.Context.VoidTy) {
+      S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),
+             diag::err_await_suspend_invalid_return_type)
+          << RetType;
+      S.Diag(Loc, diag::note_coroutine_promise_call_implicitly_required)
+          << AwaitSuspend->getDirectCallee();
+      Calls.IsInvalid = true;
+    }
+  }
+
   return Calls;
 }
 
 static ExprResult buildPromiseCall(Sema &S, VarDecl *Promise,
                                    SourceLocation Loc, StringRef Name,
                                    MultiExprArg Args) {
 
   // Form a reference to the promise.

test/SemaCXX/coroutines.cpp

struct bad_promise_no_return_func { // expected-note {{'bad_promise_no_return_func' defined here}}
   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 {{the 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 {{the return type of 'await_ready' is required to be contextually convertible to 'bool'}}
+  void await_ready();
+  bool await_suspend(std::experimental::coroutine_handle<>);
+  void await_resume();
+};
+struct await_ready_explicit_bool {
+  struct BoolT {
+    explicit operator bool() const;
+  };
+  BoolT await_ready();
+  void await_suspend(std::experimental::coroutine_handle<>);
+  void await_resume();
+};
+void test_bad_suspend() {
+  {
+    // FIXME: The actual error emitted here is terrible, and no number of notes can save it.
+    bad_await_ready_return a;
+    // expected-error@+1 {{value of type 'void' is not contextually convertible to 'bool'}}
+    co_await a; // expected-note {{call to 'await_ready' implicitly required by coroutine function here}}
+  }
+  {
+    bad_await_suspend_return b;
+    co_await b; // expected-note {{call to 'await_suspend' implicitly required by coroutine function here}}
+  }
+  {
+    await_ready_explicit_bool c;
+    co_await c; // OK
+  }
+}