[coroutines][PR41909] Don't build dependent coroutine statements for generic lambda

[modocache]

Summary

https://bugs.llvm.org/show_bug.cgi?id=41909 describes an issue in which
a generic lambda that takes a dependent argument auto set causes the
template instantiation machinery for coroutine body statements to crash
with an ICE. The issue is two-fold:

1. The paths taken by the template instantiator contain several asserts that the coroutine promise must not have a dependent type.
2. The template instantiator unconditionally builds corotuine statements that depend on the promise type, which cannot be dependent.

To work around the issue, prevent the template instantiator from building
dependent coroutine statements if the coroutine promise type is dependent.
Since we only expect this to occur in the case of a generic lambda, limit
the workaround behavior to just that case.

[GorNishanov]

LGTM! Thank you for the fix

[modocache]

Great, thanks for the review!

[rsmith]

cfe/trunk/lib/Sema/TreeTransform.h

7173 ↗

(On Diff #202627)

This assert doesn't seem correct to me; there's no reason to assume we have a generic lambda here. (A non-generic lambda inside a generic lambda, whose parameter types are dependent on the generic lambda's parameters, would hit exactly the same issue, for instance.)

Generally we should write the template instantiation code so it could be used to substitute into the definition of a function template to produce another function template, even if we happen to never call it that way right now (though deduction guide processing gets pretty close). The right thing to do here is to either substitute into the representation we already formed (building a dependent representation if it's still dependent and a non-dependent representation otherwise) or to rebuild the coroutine body from scratch (again, creating a dependent representation if the result is still dependent).

[modocache]

cfe/trunk/lib/Sema/TreeTransform.h

7173 ↗

(On Diff #202627)

Sorry I missed this! Indeed you're right, the check here is weak, and even with this patch the following coroutines code crashes in the same way: https://godbolt.org/z/ixLpCq

I'll send a follow-up patch that attempts to implement one of the approaches you describe. Thanks for the guidance!

lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
   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 (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");
       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()) {
       StmtResult Res = getDerived().TransformStmt(OnFallthrough);
       if (Res.isInvalid())
         return StmtError();
       Builder.OnFallthrough = Res.get();
     }
 

test/SemaCXX/coroutines.cpp

struct good_promise_1 {
   Fn return_void = ret_void;
 };
 const call_operator good_promise_1::await_transform;
 coro<good_promise_1> ok_static_coawait() {
   // FIXME this diagnostic is terrible
   co_await 42;
 }
 
+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");
+}
+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}}
 }
 
 struct good_promise_2 {