[Sema] Explain coroutine_traits template in diag

[modocache]

Summary

If a user defines a coroutine_traits type that takes an incorrect
number of template parameters, or for some reason they include such
a type in their program, they receive a cryptic error message:
"too few template arguments for class template 'coroutine_traits'".
The problem is that they may not understand how many is the right
number of template arguments.

Add a note diagnostic that explains why the coroutine_traits template
is being instantiated, and with what arguments.

Test Plan: check-clang

[modocache]

Oops, apologies, I included a line I shouldn't have in the previous diff.

[GorNishanov]

include/clang/Basic/DiagnosticSemaKinds.td

9053

I am wondering what is the use case here.
Is it to guard against badly designed standard library?

I played around a little and tried to see how I could get to trigger this error with a user-code, but, failed. If I did not specify enough parameters it will default to primary template defined in the <experimental/coroutine>

[modocache]

include/clang/Basic/DiagnosticSemaKinds.td

9053

That's fair. Here's my rationale: SemaCoroutune.cpp contains diagnostics, even before this change, in case of a "badly designed standard library". There's a diagnostic for if coroutine_traits is defined as non-template class, for example.

A user would not encounter that diagnostic under any normal circumstance. If they follow the instructions the compiler gives them, they'll include <experimental/coroutine> and be done with it. But since we have the code to emit the (unlikely to ever be seen) diagnostic anyway, why not make it as helpful as we can?

If a user for some reason defines their own coroutine_traits, and their definition only takes a single template argument, and if they define a coroutine with a single return type and a single parameter, then they'll see the diagnostic "too many template arguments for class template". At this point to learn why the heck a co_return statement is instantiating a template, they'd have to either read the compiler source code or the standard. My rationale is that we might as well save them this step.

All that being said, this change is just a suggestion, I'm not insisting we put it in! I drew some inspiration for this change from your Naked Coroutines talk, in which you used the co_return keyword and then followed the instructions the compiler gave you until you had a working program. Since the Clang diagnostic tells me in that case that "std::experimental::coroutine_traits is not defined, you must include <experimental/coroutine>", I wondered what would happen if a user decided to just define their own std::experimental::coroutine_traits instead. If they do that, and correct the errors that coroutine_traits must be a class and it must be a template, eventually they could find their way to this diagnostic. I agree that it's an unlikely case, so feel free to reject this and I'll close it :)

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

9053

Rather than try to explain after the fact what happened, I think we should do some up-front checking that the coroutine_traits template we found looks reasonable. Specifically: walk its template parameter list, and check that it has a non-pack type parameter followed by a pack type parameter. If not, diagnose that we don't support that declaration for coroutine_traits with some kind of "unsupported standard library implementation" error.

But the purpose of this would be to check that we're being used with a standard library implementation that we understand, not to allow arbitrary end users to declare coroutine_traits themselves. (Maybe one day we should add a diagnostic for users writing code in namespace std...)

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

9051

Maybe we should also remove the "%0 type was not found; " here to avoid the suggestion that declaring that type yourself would be a reasonable way to solve the problem. (We don't say "std::type_info was not found" when a user uses typeid without a #include <typeinfo> and that doesn't seem to confuse anyone.) For symmetry with what we say for typeid, maybe this should be worded

"you need to include <experimental/coroutine> before defining a coroutine"

include/clang/Basic/DiagnosticSemaKinds.td

 def err_coroutine_outside_function : Error<
   "'%0' cannot be used outside a function">;
 def err_coroutine_invalid_func_context : Error<
   "'%1' cannot be used in %select{a constructor|a destructor"
   "|a copy assignment operator|a move assignment operator|the 'main' function"
   "|a constexpr function|a function with a deduced return type"
   "|a varargs function}0">;
 def err_implied_coroutine_type_not_found : Error<
   "%0 type was not found; include <experimental/coroutine> before defining "
   "a coroutine">;
+def note_coroutine_types_for_traits_here : Note<
+  "the coroutine traits class template is being instantiated using the return "
+  "type, followed by the parameter types, of this coroutine">;
 def err_implicit_coroutine_std_nothrow_type_not_found : Error<
   "std::nothrow was not found; include <new> before defining a coroutine which "
   "uses get_return_object_on_allocation_failure()">;
 def err_malformed_std_nothrow : Error<
   "std::nothrow must be a valid variable declaration">;
 def err_malformed_std_coroutine_handle : Error<
   "std::experimental::coroutine_handle must be a class template">;
 def err_coroutine_handle_missing_member : Error<

lib/Sema/SemaCoroutine.cpp

if (MD->isInstance()) {
               ? S.Context.getRValueReferenceType(T)
               : S.Context.getLValueReferenceType(T, /*SpelledAsLValue*/ true);
       AddArg(T);
     }
   }
   for (QualType T : FnType->getParamTypes())
     AddArg(T);
 
-  // Build the template-id.
+  // Try to instantiate the coroutine traits template specialization, referred
+  // to in [temp.names]p1 as a template-id.
   QualType CoroTrait =
       S.CheckTemplateIdType(TemplateName(CoroTraits), KwLoc, Args);
-  if (CoroTrait.isNull())
+  if (CoroTrait.isNull()) {
+    // The instantiation failed; maybe the user defined a coroutine_traits that
+    // did something strange, like one that takes a non-type template parameter.
+    S.Diag(FD->getLocation(), diag::note_coroutine_types_for_traits_here);
     return QualType();
+  }
+
   if (S.RequireCompleteType(KwLoc, CoroTrait,
                             diag::err_coroutine_type_missing_specialization))
+    // The particular specialization is missing. For example, the user may have
+    // forward-declared it.
     return QualType();
 
   auto *RD = CoroTrait->getAsCXXRecordDecl();
   assert(RD && "specialization of class template is not a class?");
 
   // Look up the ::promise_type member.
   LookupResult R(S, &S.PP.getIdentifierTable().get("promise_type"), KwLoc,
                  Sema::LookupOrdinaryName);

test/SemaCXX/coroutine-traits-incorrect-argument-types.cpp

+// This file contains references to sections of the Coroutines TS, which can be
+// found at http://wg21.link/coroutines.
+
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s -fcxx-exceptions -fexceptions -Wunused-result
+
+namespace std {
+namespace experimental {
+
+template<typename T, typename U>
+struct coroutine_traits { // expected-note {{template is declared here}}
+  struct promise_type {}; // expected-note@-1 {{template is declared here}}
+};
+}}  // namespace std::experimental
+
+void too_few_types_for_coroutine_traits() { // expected-note {{the coroutine traits class template is being instantiated using the return type, followed by the parameter types, of this coroutine}}
+  co_return; // expected-error {{too few template arguments for class template 'coroutine_traits'}}
+}
+
+void too_many_types_for_coroutine_traits(int x, float y) { // expected-note {{the coroutine traits class template is being instantiated using the return type, followed by the parameter types, of this coroutine}}
+  co_return; // expected-error {{too many template arguments for class template 'coroutine_traits'}}
+}