[Coroutines] Return false on error of buildSuspends

[lxfind]

Summary

I believe we need to return false when buildSuspends failed, to indicate that ActOnCoroutineBodyStart failed.

[lxfind]

Adjust tests

[modocache]

I don't have a preference as to whether Sema::ActOnCoroutineBodyStart returns true or false to indicate failure, although I think returning true for failures is a bit of a pre-existing convention, such as here, where true is returned for an invalid declaration: https://github.com/llvm/llvm-project/blob/84167a8d58e8af79625abcffdf2c860d556955e6/clang/lib/Sema/SemaDecl.cpp#L1572

However I don't think this change is a good one because it removes test cases for what should be a an "NFC" change -- that is, changing the invalid return value from true to false shouldn't impact the behavior of the compiler in a way that necessitates test cases to be removed.

clang/test/SemaCXX/coroutines.cpp

106

This test ought not change -- we do expect an error here. co_yield can only be used if the promise type defines yield_value.

479

Same as my comment above: this is a valuable diagnostic, why remove the test for it? Same goes for the rest of the test changes in this patch.

[lxfind]

Makes sense. I will abandon this change.

clang/lib/Sema/SemaCoroutine.cpp

if (Suspend.isInvalid()) {
       Diag(KWLoc, diag::note_declared_coroutine_here) << Keyword;
       return StmtError();
     }
     return cast<Stmt>(Suspend.get());
   };
 
   StmtResult InitSuspend = buildSuspends("initial_suspend");
   if (InitSuspend.isInvalid())
-    return true;
+    return false;
 
   StmtResult FinalSuspend = buildSuspends("final_suspend");
   if (FinalSuspend.isInvalid())
-    return true;
+    return false;
 
   ScopeInfo->setCoroutineSuspends(InitSuspend.get(), FinalSuspend.get());
 
   return true;
 }
 
 // Recursively walks up the scope hierarchy until either a 'catch' or a function
 // scope is found, whichever comes first.

clang/test/SemaCXX/coroutines.cpp

double bad_promise_type(double) { // expected-error {{this function cannot be a coroutine: 'experimental::coroutine_traits<double, double>::promise_type' (aka 'int') is not a class}}
   co_await a;
 }
 
 template <>
 struct std::experimental::coroutine_traits<double, int> {
   struct promise_type {};
 };
 double bad_promise_type_2(int) { // expected-error {{no member named 'initial_suspend'}}
-  co_yield 0; // expected-error {{no member named 'yield_value' in 'std::experimental::coroutine_traits<double, int>::promise_type'}}
+  co_yield 0;
 }
 
 struct promise; // expected-note {{forward declaration}}
 struct promise_void;
 struct void_tag {};
 template <typename... T>
 struct std::experimental::coroutine_traits<void, T...> { using promise_type = promise; };
 template <typename... T>
namespace dependent_operator_co_await_lookup {
 
   template <>
   struct dependent_member<long> {
     // FIXME this diagnostic is terrible
     coro<transform_promise> mem_fn() const { // expected-error {{no member named 'await_ready' in 'dependent_operator_co_await_lookup::transformed'}}
       // expected-note@-1 {{call to 'initial_suspend' implicitly required by the initial suspend point}}
       // expected-note@+1 {{function is a coroutine due to use of 'co_await' here}}
       co_await transform_awaitable{};
-      // expected-error@-1 {{no member named 'await_ready'}}
     }
     template <class R, class U>
     coro<R> dep_mem_fn(U u) { co_await u; }
   };
 
   awaitable operator co_await(await_arg_2); // expected-note {{'operator co_await' should be declared prior to the call site}}
 
   template struct dependent_member<basic_promise<await_arg_1>, 0>;
   template struct dependent_member<basic_promise<await_arg_2>, 0>; // expected-note {{in instantiation}}
 
   template <>
   coro<transform_promise>
       // FIXME this diagnostic is terrible
       dependent_member<long>::dep_mem_fn<transform_promise>(int) { // expected-error {{no member named 'await_ready' in 'dependent_operator_co_await_lookup::transformed'}}
     //expected-note@-1 {{call to 'initial_suspend' implicitly required by the initial suspend point}}
     //expected-note@+1 {{function is a coroutine due to use of 'co_await' here}}
     co_await transform_awaitable{};
-    // expected-error@-1 {{no member named 'await_ready'}}
   }
 
   void operator co_await(transform_awaitable) = delete;
   awaitable operator co_await(transformed);
 
   template coro<transform_promise>
       dependent_member<long>::dep_mem_fn<transform_promise>(transform_awaitable);