Page MenuHomePhabricator

[Sema] Explain coroutine_traits template in diag
Needs ReviewPublic

Authored by modocache on Jul 2 2018, 8:24 PM.

Details

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

Diff Detail

Event Timeline

modocache created this revision.Jul 2 2018, 8:24 PM
modocache updated this revision to Diff 153956.Jul 3 2018, 12:29 PM

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

GorNishanov added inline comments.Jul 3 2018, 3:42 PM
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 added inline comments.Jul 3 2018, 4:12 PM
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 added a subscriber: rsmith.Jul 3 2018, 4:41 PM
rsmith added inline comments.
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 added inline comments.Jul 3 2018, 4:44 PM
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"