[coroutines] Include "this" type when looking up coroutine_traits

[toby-allsopp]

Summary

In N4663, section [dcl.fct.def.coroutine] states that, in a coroutine
that is a non-static member function, the type of the implicit object
parameter is included immediately before the types of the function
parameters.

[EricWF]

I think the test could be improved. First could you add the test within test/SemaCXX/coroutines.cpp? Second could you add some negative tests that check the diagnostics generated when you don't provide a specialization of coroutine traits (ie that the old behaviour of not including the class type now produces diagnostics). Could you also add tests for const/volatile and lvalue/rvalue qualified member functions?

lib/Sema/SemaCoroutine.cpp

85

This seems wrong to me.

getThisType returns the type of the this parameter as specified under [class.this] but according to the coroutines spec the type of the parameter should be the type of the implicit object parameter, which is specified under (over.match.funcs) p4.

441

Huh, I've never seen lambdas used like this before but I really like it.

[toby-allsopp]

In D35046#802838, @EricWF wrote:

I think the test could be improved. First could you add the test within test/SemaCXX/coroutines.cpp? Second could you add some negative tests that check the diagnostics generated when you don't provide a specialization of coroutine traits (ie that the old behaviour of not including the class type now produces diagnostics). Could you also add tests for const/volatile and lvalue/rvalue qualified member functions?

Will do, thanks for the suggestions.

lib/Sema/SemaCoroutine.cpp

85

Oh wow, that's a howler. I will check my tests against MSVC here. Thanks.

441

It's a sometimes controversial style called immediately-invoked lambda/function expression (IILE or IIFE). I saw other instances of it in this file (or possibly some other file in clang) so I'm assuming it's acceptable style.

[GorNishanov]

@toby-allsopp: You mentioned that @EricWF already got this in. Can you close ("abandon") this patch if it is no longer needed.

[toby-allsopp]

Yes, @EricWF made a much more comprehensive change that has been in for a while now. Abandoning.

lib/Sema/SemaCoroutine.cpp

 //===--- SemaCoroutines.cpp - Semantic Analysis for Coroutines ------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements semantic analysis for C++ Coroutines.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CoroutineStmtBuilder.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 
 using namespace clang;
static bool lookupMember(Sema &S, const char *Name, CXXRecordDecl *RD,
                          SourceLocation Loc) {
   bool Res;
   lookupMember(S, Name, RD, Loc, Res);
   return Res;
 }
 
 /// Look up the std::coroutine_traits<...>::promise_type for the given
 /// function type.
-static QualType lookupPromiseType(Sema &S, const FunctionProtoType *FnType,
+static QualType lookupPromiseType(Sema &S, const FunctionDecl *FD,
                                   SourceLocation KwLoc,
                                   SourceLocation FuncLoc) {
+  const FunctionProtoType *FnType = FD->getType()->castAs<FunctionProtoType>();
   // FIXME: Cache std::coroutine_traits once we've found it.
   NamespaceDecl *StdExp = S.lookupStdExperimentalNamespace();
   if (!StdExp) {
     S.Diag(KwLoc, diag::err_implied_coroutine_type_not_found)
         << "std::experimental::coroutine_traits";
     return QualType();
   }
 
if (!CoroTraits) {
     return QualType();
   }
 
   // Form template argument list for coroutine_traits<R, P1, P2, ...>.
   TemplateArgumentListInfo Args(KwLoc, KwLoc);
   Args.addArgument(TemplateArgumentLoc(
       TemplateArgument(FnType->getReturnType()),
       S.Context.getTrivialTypeSourceInfo(FnType->getReturnType(), KwLoc)));
-  // FIXME: If the function is a non-static member function, add the type
+  // If the function is a non-static member function, add the type
   // of the implicit object parameter before the formal parameters.
+  if (auto *MD = dyn_cast_or_null<CXXMethodDecl>(FD)) {
+    if (MD->isInstance()) {
+      QualType T = MD->getThisType(S.Context);
+      Args.addArgument(TemplateArgumentLoc(
+          TemplateArgument(T), S.Context.getTrivialTypeSourceInfo(T, KwLoc)));
+    }
+  }
   for (QualType T : FnType->getParamTypes())
     Args.addArgument(TemplateArgumentLoc(
         TemplateArgument(T), S.Context.getTrivialTypeSourceInfo(T, KwLoc)));
 
   // Build the template-id.
   QualType CoroTrait =
       S.CheckTemplateIdType(TemplateName(CoroTraits), KwLoc, Args);
   if (CoroTrait.isNull())
if (PromiseRef.isInvalid())
     return ExprError();
 
   return buildMemberCall(S, PromiseRef.get(), Loc, Name, Args);
 }
 
 VarDecl *Sema::buildCoroutinePromise(SourceLocation Loc) {
   assert(isa<FunctionDecl>(CurContext) && "not in a function scope");
   auto *FD = cast<FunctionDecl>(CurContext);
+  bool IsThisDependentType = [&] {
+    if (auto *MD = dyn_cast_or_null<CXXMethodDecl>(FD))
+      return MD->isInstance() && MD->getThisType(Context)->isDependentType();
+    else
+      return false;
+  }();
 
   QualType T =
-      FD->getType()->isDependentType()
+      FD->getType()->isDependentType() || IsThisDependentType
           ? Context.DependentTy
-          : lookupPromiseType(*this, FD->getType()->castAs<FunctionProtoType>(),
-                              Loc, FD->getLocation());
+          : lookupPromiseType(*this, FD, Loc, FD->getLocation());
   if (T.isNull())
     return nullptr;
 
   auto *VD = VarDecl::Create(Context, FD, FD->getLocation(), FD->getLocation(),
                              &PP.getIdentifierTable().get("__promise"), T,
                              Context.getTrivialTypeSourceInfo(T, Loc), SC_None);
   CheckVariableDeclarationType(VD);
   if (VD->isInvalidDecl())

test/SemaCXX/coroutine-nonstatic-member-function.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++1z -fcoroutines-ts -fsyntax-only -Wall -Wextra -Wuninitialized  -fblocks
+#include "Inputs/std-coroutine.h"
+
+using namespace std::experimental;
+
+struct coro_t {};
+
+struct coro_promise_type {
+  coro_t get_return_object() { return {}; }
+  suspend_never initial_suspend() { return {}; }
+  suspend_never final_suspend() { return {}; }
+  void return_void() {}
+  static void unhandled_exception() {}
+};
+
+struct C {
+  coro_t f();
+};
+
+namespace std::experimental {
+  template<>
+  struct coroutine_traits<coro_t, C*> {
+    using promise_type = coro_promise_type;
+  };
+}
+
+coro_t C::f() {
+  co_return;
+}
+
+int main() {
+  C c;
+  c.f();
+}