From 376cf43729c8025eecbd2969522c5687f2a3919f Mon Sep 17 00:00:00 2001
From: Brian Gesiak <modocache@gmail.com>
Date: Mon, 16 Dec 2019 17:19:16 -0500
Subject: [PATCH] [coroutines][PR41909] Generalize fix from D62550

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.

Reviewers: GorNishanov, rsmith, lewissbaker

Reviewed By: rsmith

Subscribers: junparser, EricWF, rsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D70579
---
 clang/lib/Sema/TreeTransform.h    | 29 +++++++++++++++--------------
 clang/test/SemaCXX/coroutines.cpp |  6 ++++++
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index ade0e6a..2e4e912 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7190,8 +7190,12 @@ TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
   // 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());
@@ -7200,8 +7204,9 @@ TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
   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();
@@ -7228,17 +7233,13 @@ TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
     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");
diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index 677c6e6..9e94fe8 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -731,6 +731,12 @@ template<typename T> void ok_generic_lambda_coawait_PR41909() {
   [](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}}
 
-- 
2.7.4