[coroutines][PR41909] Generalize fix from D62550

[modocache]

Summary

In https://reviews.llvm.org/D62550 @rsmith pointed out that there are
many situations in which a coroutine body statement may be
transformed/rebuilt as part of a template instantiation, and my naive
check whether the coroutine was a generic lambda was insufficient.

This is indeed true, as I've learned by reading more of the
TreeTransform code. Most transformations are written in a way that
doesn't assume the resulting types are not dependent types. So the
assertion in 'TransformCoroutineBodyStmt', that the promise type must no
longer be dependent, is out of place.

This patch removes the assertion, spruces up some code comments, and
adds a test that would have failed with my naive check from
https://reviews.llvm.org/D62550.

[junparser]

This also fix same ice when build cppcoro with current trunk. FYI

[modocache]

@GorNishanov, @rsmith, friendly ping! @rsmith this patch addresses your requests from https://reviews.llvm.org/D62550.

[modocache]

Thanks for the review! Much appreciated :)

clang/lib/Sema/TreeTransform.h

assert(FD && ScopeInfo && !ScopeInfo->CoroutinePromise &&
          ScopeInfo->CoroutineSuspends.first == nullptr &&
          ScopeInfo->CoroutineSuspends.second == nullptr &&
          "expected clean scope info");
 
   // Set that we have (possibly-invalid) suspend points before we do anything
   // that may fail.
   ScopeInfo->setNeedsCoroutineSuspends(false);
 
-  // The new CoroutinePromise object needs to be built and put into the current
-  // FunctionScopeInfo before any transformations or rebuilding occurs.
+  // We re-build the coroutine promise object (and the coroutine parameters its
+  // type and constructor depend on) based on the types used in our current
+  // function. We must do so, and set it on the current FunctionScopeInfo,
+  // before attempting to transform the other parts of the coroutine body
+  // statement, such as the implicit suspend statements (because those
+  // statements reference the FunctionScopeInfo::CoroutinePromise).
   if (!SemaRef.buildCoroutineParameterMoves(FD->getLocation()))
     return StmtError();
   auto *Promise = SemaRef.buildCoroutinePromise(FD->getLocation());
   if (!Promise)
     return StmtError();
   getDerived().transformedLocalDecl(S->getPromiseDecl(), {Promise});
   ScopeInfo->CoroutinePromise = Promise;
 
-  // Transform the implicit coroutine statements we built during the initial
-  // parse.
+  // Transform the implicit coroutine statements constructed using dependent
+  // types during the previous parse: initial and final suspensions, the return
+  // object, and others. We also transform the coroutine function's body.
   StmtResult InitSuspend = getDerived().TransformStmt(S->getInitSuspendStmt());
   if (InitSuspend.isInvalid())
     return StmtError();
   StmtResult FinalSuspend =
       getDerived().TransformStmt(S->getFinalSuspendStmt());
   if (FinalSuspend.isInvalid())
     return StmtError();
   ScopeInfo->setCoroutineSuspends(InitSuspend.get(), FinalSuspend.get());
TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
   Expr *ReturnObject = S->getReturnValueInit();
   assert(ReturnObject && "the return object is expected to be valid");
   ExprResult Res = getDerived().TransformInitializer(ReturnObject,
                                                      /*NoCopyInit*/ false);
   if (Res.isInvalid())
     return StmtError();
   Builder.ReturnValue = Res.get();
 
+  // If during the previous parse the coroutine still had a dependent promise
+  // statement, we may need to build some implicit coroutine statements
+  // (such as exception and fallthrough handlers) for the first time.
   if (S->hasDependentPromiseType()) {
-    // PR41909: We may find a generic coroutine lambda definition within a
-    // template function that is being instantiated. In this case, the lambda
-    // will have a dependent promise type, until it is used in an expression
-    // that creates an instantiation with a non-dependent promise type. We
-    // should not assert or build coroutine dependent statements for such a
-    // generic lambda.
-    auto *MD = dyn_cast_or_null<CXXMethodDecl>(FD);
-    if (!MD || !MD->getParent()->isGenericLambda()) {
-      assert(!Promise->getType()->isDependentType() &&
-             "the promise type must no longer be dependent");
+    // We can only build these statements, however, if the current promise type
+    // is not dependent.
+    if (!Promise->getType()->isDependentType()) {
       assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
              !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
              "these nodes should not have been built yet");
       if (!Builder.buildDependentStatements())
         return StmtError();
     }
   } else {
     if (auto *OnFallthrough = S->getFallthroughHandler()) {

clang/test/SemaCXX/coroutines.cpp

 
 template<typename T> void ok_generic_lambda_coawait_PR41909() {
   [](auto& arg) -> coro<good_promise_1> { // expected-warning {{expression result unused}}
     co_await 12;
   };
   [](auto &arg) -> coro<good_promise_1> {
     co_await 24;
   }("argument");
+  [](auto &arg) ->coro<good_promise_1> { // expected-warning {{expression result unused}}
+    []() -> coro<good_promise_1> {
+      co_await 36;
+    };
+    co_await 48;
+  };
 }
 template void ok_generic_lambda_coawait_PR41909<int>(); // expected-note {{in instantiation of function template specialization 'ok_generic_lambda_coawait_PR41909<int>' requested here}}
 
 template<> struct std::experimental::coroutine_traits<int, int, const char**>
 { using promise_type = promise; };
 
 int main(int, const char**) {
   co_await a; // expected-error {{'co_await' cannot be used in the 'main' function}}