[coroutines] Fix co_await for range statement

[EricWF]

Summary

Currently we build the co_await expressions on the wrong implicit statements of the implicit ranged for; Specifically we build the co_await expression wrapping the range declaration, but it should wrap the begin expression.

This patch fixes co_await on range for.

[EricWF]

• Add FIXME comments for incorrect use of getCurScope() after initial parse.

[EricWF]

• Fix clang-format nonsense in tests.

[EricWF]

• More test cleanup. Sorry for the noise.

[EricWF]

• Fix value category of co_await/co_yield expression. ie use the return type of the resume expression to determine the value category.

@rsmith Should the CoroutineSuspendExpr constructor be using getCallReturnType() to determine the correct value category, or is the current formulation correct?

[EricWF]

test/SemaCXX/coawait_range_for.cpp

133

This test is incorrect WRT ADL lookup.

[GorNishanov]

LGTM

[EricWF]

include/clang/AST/ExprCXX.h

4136

@rsmith Does this make sense, since the resume expression is a function call, or am I way off base in determining the expressions value category?

[EricWF]

• Remove changes to how CoroutineSuspendExprs ExprValueType is calculated. They were incorrect. However this means that Clang still fails to compile co_await and co_yield expressions where await_resume returns an lvalue reference. @GorNishanov Can you take a look at this? Reproducer:

struct AwaitResumeReturnsLValue {
  bool await_ready();
  void await_suspend(std::experimental::coroutine_handle<>);
  int& await_resume();
};

void AwaitReturnsLValue() {
  AwaitResumeReturnsLValue a;
  int& x = co_await a;
}

include/clang/AST/ExprCXX.h

   Stmt *SubExprs[SubExpr::Count];
   OpaqueValueExpr *OpaqueValue = nullptr;
 
   friend class ASTStmtReader;
 public:
   CoroutineSuspendExpr(StmtClass SC, SourceLocation KeywordLoc, Expr *Common,
                        Expr *Ready, Expr *Suspend, Expr *Resume,
                        OpaqueValueExpr *OpaqueValue)
-      : Expr(SC, Resume->getType(), Resume->getValueKind(),
+      : Expr(SC, Resume->getType(),
+             (Resume->getType()->isLValueReferenceType() ? VK_LValue :
+              Resume->getType()->isRValueReferenceType() ? VK_XValue :
+             VK_RValue),
              Resume->getObjectKind(), Resume->isTypeDependent(),
              Resume->isValueDependent(), Common->isInstantiationDependent(),
              Common->containsUnexpandedParameterPack()),
         KeywordLoc(KeywordLoc), OpaqueValue(OpaqueValue) {
     SubExprs[SubExpr::Common] = Common;
     SubExprs[SubExpr::Ready] = Ready;
     SubExprs[SubExpr::Suspend] = Suspend;
     SubExprs[SubExpr::Resume] = Resume;

include/clang/Sema/Sema.h

 
   void AddNSConsumedAttr(SourceRange AttrRange, Decl *D,
                          unsigned SpellingListIndex, bool isNSConsumed,
                          bool isTemplateInstantiation);
 
   //===--------------------------------------------------------------------===//
   // C++ Coroutines TS
   //
+  bool ActOnCoroutineBodyStart(Scope *S, SourceLocation KwLoc,
+                               StringRef Keyword);
   ExprResult ActOnCoawaitExpr(Scope *S, SourceLocation KwLoc, Expr *E);
   ExprResult ActOnCoyieldExpr(Scope *S, SourceLocation KwLoc, Expr *E);
   StmtResult ActOnCoreturnStmt(Scope *S, SourceLocation KwLoc, Expr *E);
 
   ExprResult BuildResolvedCoawaitExpr(SourceLocation KwLoc, Expr *E,
                                       bool IsImplicit = false);
   ExprResult BuildUnresolvedCoawaitExpr(SourceLocation KwLoc, Expr *E,
                                         UnresolvedLookupExpr* Lookup);

lib/Sema/SemaCoroutine.cpp

static FunctionScopeInfo *checkCoroutineContext(Sema &S, SourceLocation Loc,
 
   ScopeInfo->CoroutinePromise = S.buildCoroutinePromise(Loc);
   if (!ScopeInfo->CoroutinePromise)
     return nullptr;
 
   return ScopeInfo;
 }
 
-static bool actOnCoroutineBodyStart(Sema &S, Scope *SC, SourceLocation KWLoc,
-                                    StringRef Keyword) {
-  if (!checkCoroutineContext(S, KWLoc, Keyword))
+bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
+  if (!checkCoroutineContext(*this, KWLoc, Keyword))
     return false;
-  auto *ScopeInfo = S.getCurFunction();
+  auto *ScopeInfo = getCurFunction();
   assert(ScopeInfo->CoroutinePromise);
 
   // If we have existing coroutine statements then we have already built
   // the initial and final suspend points.
   if (!ScopeInfo->NeedsCoroutineSuspends)
     return true;
 
   ScopeInfo->setNeedsCoroutineSuspends(false);
 
-  auto *Fn = cast<FunctionDecl>(S.CurContext);
+  auto *Fn = cast<FunctionDecl>(CurContext);
   SourceLocation Loc = Fn->getLocation();
   // Build the initial suspend point
   auto buildSuspends = [&](StringRef Name) mutable -> StmtResult {
     ExprResult Suspend =
-        buildPromiseCall(S, ScopeInfo->CoroutinePromise, Loc, Name, None);
+        buildPromiseCall(*this, ScopeInfo->CoroutinePromise, Loc, Name, None);
     if (Suspend.isInvalid())
       return StmtError();
-    Suspend = buildOperatorCoawaitCall(S, SC, Loc, Suspend.get());
+    Suspend = buildOperatorCoawaitCall(*this, SC, Loc, Suspend.get());
     if (Suspend.isInvalid())
       return StmtError();
-    Suspend = S.BuildResolvedCoawaitExpr(Loc, Suspend.get(),
-                                         /*IsImplicit*/ true);
-    Suspend = S.ActOnFinishFullExpr(Suspend.get());
+    Suspend = BuildResolvedCoawaitExpr(Loc, Suspend.get(),
+    Suspend = ActOnFinishFullExpr(Suspend.get());
     if (Suspend.isInvalid()) {
-      S.Diag(Loc, diag::note_coroutine_promise_suspend_implicitly_required)
+      Diag(Loc, diag::note_coroutine_promise_suspend_implicitly_required)
           << ((Name == "initial_suspend") ? 0 : 1);
-      S.Diag(KWLoc, diag::note_declared_coroutine_here) << Keyword;
+      Diag(KWLoc, diag::note_declared_coroutine_here) << Keyword;
       return StmtError();
     }
     return cast<Stmt>(Suspend.get());
   };
 
   StmtResult InitSuspend = buildSuspends("initial_suspend");
   if (InitSuspend.isInvalid())
     return true;
 
   StmtResult FinalSuspend = buildSuspends("final_suspend");
   if (FinalSuspend.isInvalid())
     return true;
 
   ScopeInfo->setCoroutineSuspends(InitSuspend.get(), FinalSuspend.get());
 
   return true;
 }
 
 ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!actOnCoroutineBodyStart(*this, S, Loc, "co_await")) {
+  if (!ActOnCoroutineBodyStart(S, Loc, "co_await")) {
     CorrectDelayedTyposInExpr(E);
     return ExprError();
   }
 
   if (E->getType()->isPlaceholderType()) {
     ExprResult R = CheckPlaceholderExpr(E);
     if (R.isInvalid()) return ExprError();
     E = R.get();
ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *E,
   Expr *Res =
       new (Context) CoawaitExpr(Loc, E, RSS.Results[0], RSS.Results[1],
                                 RSS.Results[2], RSS.OpaqueValue, IsImplicit);
 
   return Res;
 }
 
 ExprResult Sema::ActOnCoyieldExpr(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!actOnCoroutineBodyStart(*this, S, Loc, "co_yield")) {
+  if (!ActOnCoroutineBodyStart(S, Loc, "co_yield")) {
     CorrectDelayedTyposInExpr(E);
     return ExprError();
   }
 
   // Build yield_value call.
   ExprResult Awaitable = buildPromiseCall(
       *this, getCurFunction()->CoroutinePromise, Loc, "yield_value", E);
   if (Awaitable.isInvalid())
ExprResult Sema::BuildCoyieldExpr(SourceLocation Loc, Expr *E) {
 
   Expr *Res = new (Context) CoyieldExpr(Loc, E, RSS.Results[0], RSS.Results[1],
                                         RSS.Results[2], RSS.OpaqueValue);
 
   return Res;
 }
 
 StmtResult Sema::ActOnCoreturnStmt(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!actOnCoroutineBodyStart(*this, S, Loc, "co_return")) {
+  if (!ActOnCoroutineBodyStart(S, Loc, "co_return")) {
     CorrectDelayedTyposInExpr(E);
     return StmtError();
   }
   return BuildCoreturnStmt(Loc, E);
 }
 
 StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
                                    bool IsImplicit) {

lib/Sema/SemaStmt.cpp

 
   Decl *LoopVar = DS->getSingleDecl();
   if (LoopVar->isInvalidDecl() || !Range ||
       DiagnoseUnexpandedParameterPack(Range, UPPC_Expression)) {
     LoopVar->setInvalidDecl();
     return StmtError();
   }
 
-  // Coroutines: 'for co_await' implicitly co_awaits its range.
-  if (CoawaitLoc.isValid()) {
-    ExprResult Coawait = ActOnCoawaitExpr(S, CoawaitLoc, Range);
-    if (Coawait.isInvalid()) return StmtError();
-    Range = Coawait.get();
+  // Build the coroutine state immediately and not later during template
+  // instantiation
+  if (!CoawaitLoc.isInvalid()) {
+    if (!ActOnCoroutineBodyStart(S, CoawaitLoc, "co_await"))
+      return StmtError();
   }
 
   // Build  auto && __range = range-init
   SourceLocation RangeLoc = Range->getLocStart();
   VarDecl *RangeVar = BuildForRangeVarDecl(*this, RangeLoc,
                                            Context.getAutoRRefDeductType(),
                                            "__range");
   if (FinishForRangeVarDecl(*this, RangeVar, Range, RangeLoc,

 /// the range-based for loop expression.
 /// This function does not handle array-based for loops,
 /// which are created in Sema::BuildCXXForRangeStmt.
 ///
 /// \returns a ForRangeStatus indicating success or what kind of error occurred.
 /// BeginExpr and EndExpr are set and FRS_Success is returned on success;
 /// CandidateSet and BEF are set and some non-success value is returned on
 /// failure.
-static Sema::ForRangeStatus BuildNonArrayForRange(Sema &SemaRef,
-                                            Expr *BeginRange, Expr *EndRange,
-                                            QualType RangeType,
-                                            VarDecl *BeginVar,
-                                            VarDecl *EndVar,
-                                            SourceLocation ColonLoc,
-                                            OverloadCandidateSet *CandidateSet,
-                                            ExprResult *BeginExpr,
-                                            ExprResult *EndExpr,
-                                            BeginEndFunction *BEF) {
+static Sema::ForRangeStatus
+BuildNonArrayForRange(Sema &SemaRef, Expr *BeginRange, Expr *EndRange,
+                      QualType RangeType, VarDecl *BeginVar, VarDecl *EndVar,
+                      SourceLocation ColonLoc, SourceLocation CoawaitLoc,
+                      OverloadCandidateSet *CandidateSet, ExprResult *BeginExpr,
+                      ExprResult *EndExpr, BeginEndFunction *BEF) {
   DeclarationNameInfo BeginNameInfo(
       &SemaRef.PP.getIdentifierTable().get("begin"), ColonLoc);
   DeclarationNameInfo EndNameInfo(&SemaRef.PP.getIdentifierTable().get("end"),
                                   ColonLoc);
 
   LookupResult BeginMemberLookup(SemaRef, BeginNameInfo,
                                  Sema::LookupMemberName);
   LookupResult EndMemberLookup(SemaRef, EndNameInfo, Sema::LookupMemberName);

                                         BeginRange, BeginExpr);
 
   if (RangeStatus != Sema::FRS_Success) {
     if (RangeStatus == Sema::FRS_DiagnosticIssued)
       SemaRef.Diag(BeginRange->getLocStart(), diag::note_in_for_range)
           << ColonLoc << BEF_begin << BeginRange->getType();
     return RangeStatus;
   }
+  if (!CoawaitLoc.isInvalid()) {
+    // FIXME: getCurScope() should not be used during template instantiation.
+    // We should pick up the set of unqualified lookup results for operator
+    // co_await during the initial parse.
+    *BeginExpr = SemaRef.ActOnCoawaitExpr(SemaRef.getCurScope(), ColonLoc,
+                                          BeginExpr->get());
+    if (BeginExpr->isInvalid())
+      return Sema::FRS_DiagnosticIssued;
+  }
   if (FinishForRangeVarDecl(SemaRef, BeginVar, BeginExpr->get(), ColonLoc,
                             diag::err_for_range_iter_deduction_failure)) {
     NoteForRangeBeginEndFunction(SemaRef, BeginExpr->get(), *BEF);
     return Sema::FRS_DiagnosticIssued;
   }
 
   *BEF = BEF_end;
   RangeStatus =

     if (const ArrayType *UnqAT = RangeType->getAsArrayTypeUnsafe()) {
       // - if _RangeT is an array type, begin-expr and end-expr are __range and
       //   __range + __bound, respectively, where __bound is the array bound. If
       //   _RangeT is an array of unknown size or an array of incomplete type,
       //   the program is ill-formed;
 
       // begin-expr is __range.
       BeginExpr = BeginRangeRef;
+      if (!CoawaitLoc.isInvalid()) {
+        BeginExpr = ActOnCoawaitExpr(S, ColonLoc, BeginExpr.get());
+        if (BeginExpr.isInvalid())
+          return StmtError();
+      }
       if (FinishForRangeVarDecl(*this, BeginVar, BeginRangeRef.get(), ColonLoc,
                                 diag::err_for_range_iter_deduction_failure)) {
         NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
         return StmtError();
       }
 
       // Find the array bound.
       ExprResult BoundExpr;

                                 diag::err_for_range_iter_deduction_failure)) {
         NoteForRangeBeginEndFunction(*this, EndExpr.get(), BEF_end);
         return StmtError();
       }
     } else {
       OverloadCandidateSet CandidateSet(RangeLoc,
                                         OverloadCandidateSet::CSK_Normal);
       BeginEndFunction BEFFailure;
-      ForRangeStatus RangeStatus =
-          BuildNonArrayForRange(*this, BeginRangeRef.get(),
-                                EndRangeRef.get(), RangeType,
-                                BeginVar, EndVar, ColonLoc, &CandidateSet,
-                                &BeginExpr, &EndExpr, &BEFFailure);
+      ForRangeStatus RangeStatus = BuildNonArrayForRange(
+          *this, BeginRangeRef.get(), EndRangeRef.get(), RangeType, BeginVar,
+          EndVar, ColonLoc, CoawaitLoc, &CandidateSet, &BeginExpr, &EndExpr,
+          &BEFFailure);
 
       if (Kind == BFRK_Build && RangeStatus == FRS_NoViableFunction &&
           BEFFailure == BEF_begin) {
         // If the range is being built from an array parameter, emit a
         // a diagnostic that it is being treated as a pointer.
         if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Range)) {
           if (ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(DRE->getDecl())) {
             QualType ArrayTy = PVD->getOriginalType();

     // Build and check ++__begin expression.
     BeginRef = BuildDeclRefExpr(BeginVar, BeginRefNonRefType,
                                 VK_LValue, ColonLoc);
     if (BeginRef.isInvalid())
       return StmtError();
 
     IncrExpr = ActOnUnaryOp(S, ColonLoc, tok::plusplus, BeginRef.get());
     if (!IncrExpr.isInvalid() && CoawaitLoc.isValid())
+      // FIXME: getCurScope() should not be used during template instantiation.
+      // We should pick up the set of unqualified lookup results for operator
+      // co_await during the initial parse.
       IncrExpr = ActOnCoawaitExpr(S, CoawaitLoc, IncrExpr.get());
     if (!IncrExpr.isInvalid())
       IncrExpr = ActOnFinishFullExpr(IncrExpr.get());
     if (IncrExpr.isInvalid()) {
       Diag(RangeLoc, diag::note_for_range_invalid_iterator)
         << RangeLoc << 2 << BeginRangeRef.get()->getType() ;
       NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
       return StmtError();

test/SemaCXX/coawait_range_for.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++14 -fcoroutines-ts \
+// RUN:    -fsyntax-only -Wignored-qualifiers -Wno-error=return-type -verify \
+// RUN:    -fblocks
+#include "Inputs/std-coroutine.h"
+
+using namespace std::experimental;
+
+
+template <class Begin>
+struct Awaiter {
+  bool await_ready();
+  void await_suspend(coroutine_handle<>);
+  Begin await_resume();
+};
+
+template <class Iter> struct BeginTag { BeginTag() = delete; };
+template <class Iter> struct IncTag { IncTag() = delete; };
+
+template <class Iter, bool Delete = false>
+struct CoawaitTag { CoawaitTag() = delete; };
+
+template <class T>
+struct Iter {
+  using value_type = T;
+  using reference = T &;
+  using pointer = T *;
+
+  IncTag<Iter> operator++();
+  reference operator*();
+  pointer operator->();
+};
+template <class T> bool operator==(Iter<T>, Iter<T>);
+template <class T> bool operator!=(Iter<T>, Iter<T>);
+
+template <class T>
+struct Range {
+  BeginTag<Iter<T>> begin();
+  Iter<T> end();
+};
+
+struct MyForLoopArrayAwaiter {
+  struct promise_type {
+    MyForLoopArrayAwaiter get_return_object() { return {}; }
+    void return_void();
+    void unhandled_exception();
+    suspend_never initial_suspend();
+    suspend_never final_suspend();
+    template <class T>
+    Awaiter<T *> await_transform(T *) = delete; // expected-note {{explicitly deleted}}
+  };
+};
+MyForLoopArrayAwaiter g() {
+  int arr[10] = {0};
+  for co_await(auto i : arr) {}
+  // expected-error@-1 {{call to deleted member function 'await_transform'}}
+  // expected-note@-2 {{'await_transform' implicitly required by 'co_await' here}}
+}
+
+struct ForLoopAwaiterBadBeginTransform {
+  struct promise_type {
+    ForLoopAwaiterBadBeginTransform get_return_object();
+    void return_void();
+    void unhandled_exception();
+    suspend_never initial_suspend();
+    suspend_never final_suspend();
+
+    template <class T>
+    Awaiter<T> await_transform(BeginTag<T>) = delete; // expected-note 1+ {{explicitly deleted}}
+
+    template <class T>
+    CoawaitTag<T> await_transform(IncTag<T>); // expected-note 1+ {{candidate}}
+  };
+};
+ForLoopAwaiterBadBeginTransform bad_begin() {
+  Range<int> R;
+  for co_await(auto i : R) {}
+  // expected-error@-1 {{call to deleted member function 'await_transform'}}
+  // expected-note@-2 {{'await_transform' implicitly required by 'co_await' here}}
+}
+template <class Dummy>
+ForLoopAwaiterBadBeginTransform bad_begin_template(Dummy) {
+  Range<Dummy> R;
+  for co_await(auto i : R) {}
+  // expected-error@-1 {{call to deleted member function 'await_transform'}}
+  // expected-note@-2 {{'await_transform' implicitly required by 'co_await' here}}
+}
+template ForLoopAwaiterBadBeginTransform bad_begin_template(int); // expected-note {{requested here}}
+
+template <class Iter>
+Awaiter<Iter> operator co_await(CoawaitTag<Iter, true>) = delete;
+// expected-note@-1 1+ {{explicitly deleted}}
+
+struct ForLoopAwaiterBadIncTransform {
+  struct promise_type {
+    ForLoopAwaiterBadIncTransform get_return_object();
+    void return_void();
+    void unhandled_exception();
+    suspend_never initial_suspend();
+    suspend_never final_suspend();
+
+    template <class T>
+    Awaiter<T> await_transform(BeginTag<T> e);
+
+    template <class T>
+    CoawaitTag<T, true> await_transform(IncTag<T>);
+  };
+};
+ForLoopAwaiterBadIncTransform bad_inc_transform() {
+  Range<float> R;
+  for co_await(auto i : R) {}
+  // expected-error@-1 {{overload resolution selected deleted operator 'co_await'}}
+  // expected-note@-2 {{in implicit call to 'operator++' for iterator of type 'Range<float>'}}
+}
+
+template <class Dummy>
+ForLoopAwaiterBadIncTransform bad_inc_transform_template(Dummy) {
+  Range<Dummy> R;
+  for co_await(auto i : R) {}
+  // expected-error@-1 {{overload resolution selected deleted operator 'co_await'}}
+  // expected-note@-2 {{in implicit call to 'operator++' for iterator of type 'Range<long>'}}
+}
+template ForLoopAwaiterBadIncTransform bad_inc_transform_template(long); // expected-note {{requested here}}
+
+// Ensure we mark and check the function as a coroutine even if it's
+// never instantiated.
+template <class T>
+constexpr void never_instant(T) {
+  static_assert(sizeof(T) != sizeof(T), "function should not be instantiated");
+  for co_await(auto i : foo(T{})) {}
+  // expected-error@-1 {{'co_await' cannot be used in a constexpr function}}
+}
+
+struct ForLoopAwaiterCoawaitLookup {
+  struct promise_type {
+    ForLoopAwaiterCoawaitLookup get_return_object();
+    void return_void();
+    void unhandled_exception();
+    suspend_never initial_suspend();
+    suspend_never final_suspend();
+    template <class T>
+    CoawaitTag<T, false> await_transform(BeginTag<T> e);
+    template <class T>
+    Awaiter<T> await_transform(IncTag<T>);
+  };
+};
+template <class T>
+ForLoopAwaiterCoawaitLookup test_coawait_lookup(T) {
+  Range<T> R;
+  for co_await(auto i : R) {}
+  // expected-error@-1 {{no member named 'await_ready' in 'CoawaitTag<Iter<int>, false>'}}
+}
+template ForLoopAwaiterCoawaitLookup test_coawait_lookup(int); // expected-note {{requested here}}
+
+// FIXME: This test should fail as well since the newly declared operator co_await
+// should not be found by lookup.
+template <class Iter>
+Awaiter<Iter> operator co_await(CoawaitTag<Iter, false>);
+template ForLoopAwaiterCoawaitLookup test_coawait_lookup(long);