[coroutines] Fix diagnostics depending on the first coroutine statement.

Summary:
Some coroutine diagnostics need to point to the location of the first coroutine keyword in the function, like when diagnosing a `return` inside a coroutine. Previously we did this by storing each *valid* coroutine statement in a list and select the first one to use in diagnostics. However if every coroutine statement is invalid we would have no location to point to.

This patch fixes the storage of the first coroutine statement location, ensuring that it gets stored even when the resulting AST node would be invalid.
This patch also removes the `CoroutineStmts` list in `FunctionScopeInfo` because it was unused.

Reviewers: rsmith, GorNishanov, aaron.ballman

Reviewed By: GorNishanov

Subscribers: mehdi_amini, cfe-commits

Differential Revision: https://reviews.llvm.org/D30776

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@297547 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Sema/ScopeInfo.h b/include/clang/Sema/ScopeInfo.h
index e49e4d94576a6c5a2333b5d49c5edb00a75d2538..4487c7c2ccb6c726e0a4b2161659a9205cbd1a1d 100644 (file)
--- a/include/clang/Sema/ScopeInfo.h
+++ b/include/clang/Sema/ScopeInfo.h
@@ -24,6 +24,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringSwitch.h"
 #include <algorithm>
 
 namespace clang {
@@ -139,6 +140,14 @@ public:
   /// to build, the initial and final coroutine suspend points
   bool NeedsCoroutineSuspends : 1;
 
+  /// \brief An enumeration represeting the kind of the first coroutine statement
+  /// in the function. One of co_return, co_await, or co_yield.
+  unsigned char FirstCoroutineStmtKind : 2;
+
+  /// First coroutine statement in the current function.
+  /// (ex co_return, co_await, co_yield)
+  SourceLocation FirstCoroutineStmtLoc;
+
   /// First 'return' statement in the current function.
   SourceLocation FirstReturnLoc;
 
@@ -166,11 +175,6 @@ public:
   /// \brief The initial and final coroutine suspend points.
   std::pair<Stmt *, Stmt *> CoroutineSuspends;
 
-  /// \brief The list of coroutine control flow constructs (co_await, co_yield,
-  /// co_return) that occur within the function or block. Empty if and only if
-  /// this function or block is not (yet known to be) a coroutine.
-  SmallVector<Stmt*, 4> CoroutineStmts;
-
   /// \brief The stack of currently active compound stamement scopes in the
   /// function.
   SmallVector<CompoundScopeInfo, 4> CompoundScopes;
@@ -384,6 +388,28 @@ public:
           (HasBranchProtectedScope && HasBranchIntoScope));
   }
 
+  void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) {
+    assert(FirstCoroutineStmtLoc.isInvalid() &&
+                   "first coroutine statement location already set");
+    FirstCoroutineStmtLoc = Loc;
+    FirstCoroutineStmtKind = llvm::StringSwitch<unsigned char>(Keyword)
+            .Case("co_return", 0)
+            .Case("co_await", 1)
+            .Case("co_yield", 2);
+  }
+
+  StringRef getFirstCoroutineStmtKeyword() const {
+    assert(FirstCoroutineStmtLoc.isValid()
+                   && "no coroutine statement available");
+    switch (FirstCoroutineStmtKind) {
+    case 0: return "co_return";
+    case 1: return "co_await";
+    case 2: return "co_yield";
+    default:
+      llvm_unreachable("FirstCoroutineStmtKind has an invalid value");
+    };
+  }
+
   void setNeedsCoroutineSuspends(bool value = true) {
     assert((!value || CoroutineSuspends.first == nullptr) &&
             "we already have valid suspend points");

diff --git a/lib/Sema/ScopeInfo.cpp b/lib/Sema/ScopeInfo.cpp
index 8050889d71ae3141c68954d70844f7c7f7ff4e47..b309a36a30a3f250770469a01384d343129af0b3 100644 (file)
--- a/lib/Sema/ScopeInfo.cpp
+++ b/lib/Sema/ScopeInfo.cpp
@@ -40,13 +40,15 @@ void FunctionScopeInfo::Clear() {
   FirstCXXTryLoc = SourceLocation();
   FirstSEHTryLoc = SourceLocation();
 
-  SwitchStack.clear();
-  Returns.clear();
+  // Coroutine state
+  FirstCoroutineStmtLoc = SourceLocation();
   CoroutinePromise = nullptr;
   NeedsCoroutineSuspends = true;
   CoroutineSuspends.first = nullptr;
   CoroutineSuspends.second = nullptr;
-  CoroutineStmts.clear();
+
+  SwitchStack.clear();
+  Returns.clear();
   ErrorTrap.reset();
   PossiblyUnreachableDiags.clear();
   WeakObjectUses.clear();

diff --git a/lib/Sema/SemaCoroutine.cpp b/lib/Sema/SemaCoroutine.cpp
index 7bbb2f46e56cc4f51f124ef2667c07bae8bec459..4d0f8a8f7688d0d3739d9aac62aa8ba94e3b050c 100644 (file)
--- a/lib/Sema/SemaCoroutine.cpp
+++ b/lib/Sema/SemaCoroutine.cpp
@@ -400,7 +400,8 @@ VarDecl *Sema::buildCoroutinePromise(SourceLocation Loc) {
 
 /// Check that this is a context in which a coroutine suspension can appear.
 static FunctionScopeInfo *checkCoroutineContext(Sema &S, SourceLocation Loc,
-                                                StringRef Keyword) {
+                                                StringRef Keyword,
+                                                bool IsImplicit = false) {
   if (!isValidCoroutineContext(S, Loc, Keyword))
     return nullptr;
 
@@ -409,6 +410,9 @@ static FunctionScopeInfo *checkCoroutineContext(Sema &S, SourceLocation Loc,
   auto *ScopeInfo = S.getCurFunction();
   assert(ScopeInfo && "missing function scope for function");
 
+  if (ScopeInfo->FirstCoroutineStmtLoc.isInvalid() && !IsImplicit)
+    ScopeInfo->setFirstCoroutineStmt(Loc, Keyword);
+
   if (ScopeInfo->CoroutinePromise)
     return ScopeInfo;
 
@@ -488,7 +492,7 @@ ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
 }
 
 ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *E,
-                                           UnresolvedLookupExpr *Lookup) {
+                                            UnresolvedLookupExpr *Lookup) {
   auto *FSI = checkCoroutineContext(*this, Loc, "co_await");
   if (!FSI)
     return ExprError();
@@ -504,7 +508,6 @@ ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *E,
   if (Promise->getType()->isDependentType()) {
     Expr *Res =
         new (Context) DependentCoawaitExpr(Loc, Context.DependentTy, E, Lookup);
-    FSI->CoroutineStmts.push_back(Res);
     return Res;
   }
 
@@ -528,7 +531,7 @@ ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *E,
 
 ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *E,
                                   bool IsImplicit) {
-  auto *Coroutine = checkCoroutineContext(*this, Loc, "co_await");
+  auto *Coroutine = checkCoroutineContext(*this, Loc, "co_await", IsImplicit);
   if (!Coroutine)
     return ExprError();
 
@@ -541,8 +544,6 @@ ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *E,
   if (E->getType()->isDependentType()) {
     Expr *Res = new (Context)
         CoawaitExpr(Loc, Context.DependentTy, E, IsImplicit);
-    if (!IsImplicit)
-      Coroutine->CoroutineStmts.push_back(Res);
     return Res;
   }
 
@@ -560,8 +561,7 @@ 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);
-  if (!IsImplicit)
-    Coroutine->CoroutineStmts.push_back(Res);
+
   return Res;
 }
 
@@ -597,7 +597,6 @@ ExprResult Sema::BuildCoyieldExpr(SourceLocation Loc, Expr *E) {
 
   if (E->getType()->isDependentType()) {
     Expr *Res = new (Context) CoyieldExpr(Loc, Context.DependentTy, E);
-    Coroutine->CoroutineStmts.push_back(Res);
     return Res;
   }
 
@@ -614,7 +613,7 @@ 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);
-  Coroutine->CoroutineStmts.push_back(Res);
+
   return Res;
 }
 
@@ -628,7 +627,7 @@ StmtResult Sema::ActOnCoreturnStmt(Scope *S, SourceLocation Loc, Expr *E) {
 
 StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
                                    bool IsImplicit) {
-  auto *FSI = checkCoroutineContext(*this, Loc, "co_return");
+  auto *FSI = checkCoroutineContext(*this, Loc, "co_return", IsImplicit);
   if (!FSI)
     return StmtError();
 
@@ -656,8 +655,6 @@ StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
   Expr *PCE = ActOnFinishFullExpr(PC.get()).get();
 
   Stmt *Res = new (Context) CoreturnStmt(Loc, E, PCE, IsImplicit);
-  if (!IsImplicit)
-    FSI->CoroutineStmts.push_back(Res);
   return Res;
 }
 
@@ -774,15 +771,11 @@ void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
   // Coroutines [stmt.return]p1:
   //   A return statement shall not appear in a coroutine.
   if (Fn->FirstReturnLoc.isValid()) {
+    assert(Fn->FirstCoroutineStmtLoc.isValid() &&
+                   "first coroutine location not set");
     Diag(Fn->FirstReturnLoc, diag::err_return_in_coroutine);
-    // FIXME: Every Coroutine statement may be invalid and therefore not added
-    // to CoroutineStmts. Find another way to provide location information.
-    if (!Fn->CoroutineStmts.empty()) {
-      auto *First = Fn->CoroutineStmts[0];
-      Diag(First->getLocStart(), diag::note_declared_coroutine_here)
-          << (isa<CoawaitExpr>(First) ? "co_await" :
-              isa<CoyieldExpr>(First) ? "co_yield" : "co_return");
-    }
+    Diag(Fn->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
+            << Fn->getFirstCoroutineStmtKeyword();
   }
   SubStmtBuilder Builder(*this, *FD, *Fn, Body);
   if (Builder.isInvalid())

diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h
index 4e22762eb19a044dcaf76a69ce8deca509a3d6ef..7a89c9dea13bf92adec45e6dda960cce4ec9e15c 100644 (file)
--- a/lib/Sema/TreeTransform.h
+++ b/lib/Sema/TreeTransform.h
@@ -6857,7 +6857,7 @@ TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
          ScopeInfo->NeedsCoroutineSuspends &&
          ScopeInfo->CoroutineSuspends.first == nullptr &&
          ScopeInfo->CoroutineSuspends.second == nullptr &&
-         ScopeInfo->CoroutineStmts.empty() && "expected clean scope info");
+         "expected clean scope info");
 
   // Set that we have (possibly-invalid) suspend points before we do anything
   // that may fail.

diff --git a/test/SemaCXX/coroutines.cpp b/test/SemaCXX/coroutines.cpp
index 22fdcde1c216315697f8e18eaa1f8d56b09e93d2..8876a146557fcf6e9971748a46b4869e16cf887c 100644 (file)
--- a/test/SemaCXX/coroutines.cpp
+++ b/test/SemaCXX/coroutines.cpp
@@ -162,11 +162,59 @@ void mixed_yield() {
   return; // expected-error {{not allowed in coroutine}}
 }
 
+void mixed_yield_invalid() {
+  co_yield blah; // expected-error {{use of undeclared identifier}}
+  // expected-note@-1 {{function is a coroutine due to use of 'co_yield'}}
+  return; // expected-error {{return statement not allowed in coroutine}}
+}
+
+template <class T>
+void mixed_yield_template(T) {
+  co_yield blah; // expected-error {{use of undeclared identifier}}
+  // expected-note@-1 {{function is a coroutine due to use of 'co_yield'}}
+  return; // expected-error {{return statement not allowed in coroutine}}
+}
+
+template <class T>
+void mixed_yield_template2(T) {
+  co_yield 42;
+  // expected-note@-1 {{function is a coroutine due to use of 'co_yield'}}
+  return; // expected-error {{return statement not allowed in coroutine}}
+}
+
+template <class T>
+void mixed_yield_template3(T v) {
+  co_yield blah(v);
+  // expected-note@-1 {{function is a coroutine due to use of 'co_yield'}}
+  return; // expected-error {{return statement not allowed in coroutine}}
+}
+
 void mixed_await() {
   co_await a; // expected-note {{use of 'co_await'}}
   return; // expected-error {{not allowed in coroutine}}
 }
 
+void mixed_await_invalid() {
+  co_await 42; // expected-error {{'int' is not a structure or union}}
+  // expected-note@-1 {{function is a coroutine due to use of 'co_await'}}
+  return; // expected-error {{not allowed in coroutine}}
+}
+
+template <class T>
+void mixed_await_template(T) {
+  co_await 42;
+  // expected-note@-1 {{function is a coroutine due to use of 'co_await'}}
+  return; // expected-error {{not allowed in coroutine}}
+}
+
+template <class T>
+void mixed_await_template2(T v) {
+  co_await v; // expected-error {{'long' is not a structure or union}}
+  // expected-note@-1 {{function is a coroutine due to use of 'co_await'}}
+  return; // expected-error {{not allowed in coroutine}}
+}
+template void mixed_await_template2(long); // expected-note {{requested here}}
+
 void only_coreturn(void_tag) {
   co_return; // OK
 }
@@ -178,6 +226,33 @@ void mixed_coreturn(void_tag, bool b) {
     return; // expected-error {{not allowed in coroutine}}
 }
 
+void mixed_coreturn_invalid(bool b) {
+  if (b)
+    co_return; // expected-note {{use of 'co_return'}}
+    // expected-error@-1 {{no member named 'return_void' in 'promise'}}
+  else
+    return; // expected-error {{not allowed in coroutine}}
+}
+
+template <class T>
+void mixed_coreturn_template(void_tag, bool b, T v) {
+  if (b)
+    co_return v; // expected-note {{use of 'co_return'}}
+    // expected-error@-1 {{no member named 'return_value' in 'promise_void'}}
+  else
+    return; // expected-error {{not allowed in coroutine}}
+}
+template void mixed_coreturn_template(void_tag, bool, int); // expected-note {{requested here}}
+
+template <class T>
+void mixed_coreturn_template2(bool b, T) {
+  if (b)
+    co_return v; // expected-note {{use of 'co_return'}}
+    // expected-error@-1 {{use of undeclared identifier 'v'}}
+  else
+    return; // expected-error {{not allowed in coroutine}}
+}
+
 struct CtorDtor {
   CtorDtor() {
     co_yield 0; // expected-error {{'co_yield' cannot be used in a constructor}}