[Sema] isValidCoroutineContext FIXME and citations

[modocache]

Summary

Add citations to the Coroutines TS to the isValidCoroutineContext
function, as well as a FIXME and test for [expr.await]p2, which states
a co_await expression cannot be used in a default argument.

Test Plan: check-clang

[modocache]

Also @GorNishanov I'm curious about your two cents on whether comments like these are valuable. If you think they are I may add a few more with post-commit review.

[modocache]

lib/Sema/SemaCoroutine.cpp

264–265

@GorNishanov Is there anything in the TS that states copy and move assignment operators shall not include await or yield expressions? These were added D25292 but I'm not sure whether I'm missing something in the TS text, or if maybe this language was in a prior revision of the TS.

[GorNishanov]

LGTM

lib/Sema/SemaCoroutine.cpp

264–265

Yes. N4499/[special] said:

A special member function shall not be a coroutine.

I think @rsmith wanted to relax it, but, I am not sure if he had a use case in mind.

I am thinking putting the restriction from N4499 back.
My approach is if in doubt, be more restrictive initially, then, we can relax if use cases are discovered. It will be a non-breaking change.

[modocache]

Great, thanks for the review! I added a reference to N4499.

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.
 //
+//  This file contains references to sections of the Coroutines TS, which
+//  can be found at http://wg21.link/coroutines.
+//
 //===----------------------------------------------------------------------===//
 
 #include "CoroutineStmtBuilder.h"
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Lex/Preprocessor.h"
if (S.RequireCompleteType(Loc, CoroHandleType,
                             diag::err_coroutine_type_missing_specialization))
     return QualType();
 
   return CoroHandleType;
 }
 
 static bool isValidCoroutineContext(Sema &S, SourceLocation Loc,
                                     StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands.
+  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
+  // such as subexpressions of \c sizeof.
+  //
+  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
+  // a *potentially evaluated* expression within the compound-statement of a
+  // function-body outside of a handler [...] A context within a function where
+  // an await-expression can appear is called a suspension context of the
+  // function." And per [expr.yield]p1: "A yield-expression shall appear only
+  // within a suspension context of a function."
   if (S.isUnevaluatedContext()) {
     S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
     return false;
   }
 
-  // Any other usage must be within a function.
+  // Per [expr.await]p2, any other usage must be within a function.
+  // FIXME: This also covers [expr.await]p2: "An await-expression shall not
+  // appear in a default argument." But the diagnostic QoI here could be
+  // improved to inform the user that default arguments specifically are not
+  // allowed.
   auto *FD = dyn_cast<FunctionDecl>(S.CurContext);
   if (!FD) {
     S.Diag(Loc, isa<ObjCMethodDecl>(S.CurContext)
                     ? diag::err_coroutine_objc_method
                     : diag::err_coroutine_outside_function) << Keyword;
     return false;
   }
 
auto DiagInvalid = [&](InvalidFuncDiag ID) {
     S.Diag(Loc, diag::err_coroutine_invalid_func_context) << ID << Keyword;
     Diagnosed = true;
     return false;
   };
 
   // Diagnose when a constructor, destructor, copy/move assignment operator,
   // or the function 'main' are declared as a coroutine.
   auto *MD = dyn_cast<CXXMethodDecl>(FD);
+  // [class.ctor]p6: "A constructor shall not be a coroutine."
   if (MD && isa<CXXConstructorDecl>(MD))
     return DiagInvalid(DiagCtor);
+  // [class.dtor]p17: "A destructor shall not be a coroutine."
   else if (MD && isa<CXXDestructorDecl>(MD))
     return DiagInvalid(DiagDtor);
+  // N4499 [special]p6: "A special member function shall not be a coroutine."
+  // Per C++ [special]p1, special member functions are the "default constructor,
+  // copy constructor and copy assignment operator, move constructor and move
+  // assignment operator, and destructor."
   else if (MD && MD->isCopyAssignmentOperator())
     return DiagInvalid(DiagCopyAssign);
   else if (MD && MD->isMoveAssignmentOperator())
     return DiagInvalid(DiagMoveAssign);
+  // [basic.start.main]p3: "The function main shall not be a coroutine."
   else if (FD->isMain())
     return DiagInvalid(DiagMain);
 
   // Emit a diagnostics for each of the following conditions which is not met.
+  // [expr.const]p2: "An expression e is a core constant expression unless the
+  // evaluation of e [...] would evaluate one of the following expressions:
+  // [...] an await-expression [...] a yield-expression."
   if (FD->isConstexpr())
     DiagInvalid(DiagConstexpr);
+  // [dcl.spec.auto]p15: "A function declared with a return type that uses a
+  // placeholder type shall not be a coroutine."
   if (FD->getReturnType()->isUndeducedType())
     DiagInvalid(DiagAutoRet);
+  // [dcl.fct.def.coroutine]p1: "The parameter-declaration-clause of the
+  // coroutine shall not terminate with an ellipsis that is not part of a
+  // parameter-declaration."
   if (FD->isVariadic())
     DiagInvalid(DiagVarargs);
 
   return !Diagnosed;
 }
 
 static ExprResult buildOperatorCoawaitLookupExpr(Sema &SemaRef, Scope *S,
                                                  SourceLocation Loc) {

test/SemaCXX/coroutines.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
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include <experimental/coroutine>}}
   // expected-error@-1 {{use of undeclared identifier 'a'}}
 }
 
 void no_coroutine_traits_bad_arg_yield() {
void unevaluated() {
   decltype(co_await a); // expected-error {{cannot be used in an unevaluated context}}
   sizeof(co_await a); // expected-error {{cannot be used in an unevaluated context}}
   typeid(co_await a); // expected-error {{cannot be used in an unevaluated context}}
   decltype(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
   sizeof(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
   typeid(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
 }
 
+// [expr.await]p2: "An await-expression shall not appear in a default argument."
+// FIXME: A better diagnostic would explicitly state that default arguments are
+// not allowed. A user may not understand that this is "outside a function."
+void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used in a constexpr function}}
   // expected-error@-1 {{'co_yield' cannot be used in a function with a deduced return type}}
 }
 
 void varargs_coroutine(const char *, ...) {
   co_await a; // expected-error {{'co_await' cannot be used in a varargs function}}
 }