[clang] Surface error for plain return statement in coroutine earlier (PR #100985)

via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 2 04:22:03 PDT 2024


https://github.com/ivanaivanovska updated https://github.com/llvm/llvm-project/pull/100985

>From d35544d971f073f98fba047cfcbe3cfe92dd78c4 Mon Sep 17 00:00:00 2001
From: Ivana Ivanovska <iivanovska at google.com>
Date: Mon, 29 Jul 2024 08:08:00 +0000
Subject: [PATCH 1/4] Surface error for plain return statement in coroutine
 earlier

---
 clang/lib/Sema/SemaCoroutine.cpp  |  2 +-
 clang/lib/Sema/SemaStmt.cpp       | 10 +++++++++
 clang/test/SemaCXX/coroutines.cpp | 35 +++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 81334c817b2af..87d0d44c5af66 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -1120,7 +1120,7 @@ void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
 
   // [stmt.return.coroutine]p1:
   //   A coroutine shall not enclose a return statement ([stmt.return]).
-  if (Fn->FirstReturnLoc.isValid()) {
+  if (Fn->FirstReturnLoc.isValid() && Fn->FirstReturnLoc < Fn->FirstCoroutineStmtLoc) {
     assert(Fn->FirstCoroutineStmtLoc.isValid() &&
                    "first coroutine location not set");
     Diag(Fn->FirstReturnLoc, diag::err_return_in_coroutine);
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 34d2d398f244d..3909892ef0a6f 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3747,6 +3747,16 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
         Diag(ReturnLoc, diag::err_acc_branch_in_out_compute_construct)
         << /*return*/ 1 << /*out of */ 0);
 
+  // using plain return in a coroutine is not allowed.
+  FunctionScopeInfo *FSI = getCurFunction();
+  if (getLangOpts().Coroutines && FSI->isCoroutine()) {
+    assert(FSI->FirstCoroutineStmtLoc.isValid() &&
+                  "first coroutine location not set");
+    Diag(ReturnLoc, diag::err_return_in_coroutine);
+    Diag(FSI->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
+            << FSI->getFirstCoroutineStmtKeyword();
+  }
+
   StmtResult R =
       BuildReturnStmt(ReturnLoc, RetVal.get(), /*AllowRecovery=*/true);
   if (R.isInvalid() || ExprEvalContexts.back().isDiscardedStatementContext())
diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index 2292932583fff..b4f362c621929 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -154,12 +154,15 @@ namespace std {
 template <class PromiseType = void>
 struct coroutine_handle {
   static coroutine_handle from_address(void *) noexcept;
+  static coroutine_handle from_promise(PromiseType &promise);
 };
 template <>
 struct coroutine_handle<void> {
   template <class PromiseType>
   coroutine_handle(coroutine_handle<PromiseType>) noexcept;
   static coroutine_handle from_address(void *) noexcept;
+  template <class PromiseType>
+  static coroutine_handle from_promise(PromiseType &promise);
 };
 } // namespace std
 
@@ -291,6 +294,38 @@ void mixed_coreturn_template2(bool b, T) {
     return; // expected-error {{not allowed in coroutine}}
 }
 
+struct promise_handle;
+
+struct Handle : std::coroutine_handle<promise_handle> { // expected-note 2{{candidate constructor (the implicit copy constructor) not viable}}
+    // expected-note at -1 2{{candidate constructor (the implicit move constructor) not viable}}
+    using promise_type = promise_handle;
+};
+
+struct promise_handle {
+    Handle get_return_object() noexcept {
+      { return Handle(std::coroutine_handle<Handle::promise_type>::from_promise(*this)); }
+    }
+    suspend_never initial_suspend() const noexcept { return {}; }
+    suspend_never final_suspend() const noexcept { return {}; }
+    void return_void() const noexcept {}
+    void unhandled_exception() const noexcept {}
+};
+
+Handle mixed_return_value() {
+  co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
+  return 0; // expected-error {{return statement not allowed in coroutine}}
+  // expected-error at -1 {{no viable conversion from returned value of type}}
+}
+
+Handle mixed_return_value_return_first(bool b) {
+   if (b) {
+        return 0; // expected-error {{no viable conversion from returned value of type}}
+        // expected-error at -1 {{return statement not allowed in coroutine}}
+    }
+    co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
+    co_return 0; // expected-error {{no member named 'return_value' in 'promise_handle'}}
+}
+
 struct CtorDtor {
   CtorDtor() {
     co_yield 0; // expected-error {{'co_yield' cannot be used in a constructor}}

>From f0f6a1a089ec10fae0010dd93456a844df5693af Mon Sep 17 00:00:00 2001
From: Ivana Ivanovska <iivanovska at google.com>
Date: Thu, 1 Aug 2024 08:34:36 +0000
Subject: [PATCH 2/4] Applied fixes related to the comments in the PR.

---
 clang/lib/Sema/SemaCoroutine.cpp  | 27 +++++++++++++--------
 clang/lib/Sema/SemaStmt.cpp       |  2 +-
 clang/test/SemaCXX/coroutines.cpp | 40 +++++++++++++++++++++++++++++--
 3 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 87d0d44c5af66..fc624975b8a4a 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -684,6 +684,18 @@ bool Sema::checkFinalSuspendNoThrow(const Stmt *FinalSuspend) {
   return ThrowingDecls.empty();
 }
 
+// [stmt.return.coroutine]p1:
+//   A coroutine shall not enclose a return statement ([stmt.return]).
+static void checkReturnStmtInCoroutine(Sema &S, FunctionScopeInfo *FSI) {
+  if (FSI && FSI->FirstReturnLoc.isValid()) {
+      assert(FSI->FirstCoroutineStmtLoc.isValid() &&
+                    "first coroutine location not set");
+      S.Diag(FSI->FirstReturnLoc, diag::err_return_in_coroutine);
+      S.Diag(FSI->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
+              << FSI->getFirstCoroutineStmtKeyword();
+  }
+}
+
 bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
                                    StringRef Keyword) {
   // Ignore previous expr evaluation contexts.
@@ -694,6 +706,10 @@ bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
   auto *ScopeInfo = getCurFunction();
   assert(ScopeInfo->CoroutinePromise);
 
+  if (ScopeInfo->FirstCoroutineStmtLoc == KWLoc) {
+    checkReturnStmtInCoroutine(*this, ScopeInfo);
+  }
+
   // If we have existing coroutine statements then we have already built
   // the initial and final suspend points.
   if (!ScopeInfo->NeedsCoroutineSuspends)
@@ -801,6 +817,7 @@ ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
     if (R.isInvalid()) return ExprError();
     E = R.get();
   }
+
   ExprResult Lookup = BuildOperatorCoawaitLookupExpr(S, Loc);
   if (Lookup.isInvalid())
     return ExprError();
@@ -1118,16 +1135,6 @@ void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
   if (Fn->FirstVLALoc.isValid())
     Diag(Fn->FirstVLALoc, diag::err_vla_in_coroutine_unsupported);
 
-  // [stmt.return.coroutine]p1:
-  //   A coroutine shall not enclose a return statement ([stmt.return]).
-  if (Fn->FirstReturnLoc.isValid() && Fn->FirstReturnLoc < Fn->FirstCoroutineStmtLoc) {
-    assert(Fn->FirstCoroutineStmtLoc.isValid() &&
-                   "first coroutine location not set");
-    Diag(Fn->FirstReturnLoc, diag::err_return_in_coroutine);
-    Diag(Fn->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
-            << Fn->getFirstCoroutineStmtKeyword();
-  }
-
   // Coroutines will get splitted into pieces. The GNU address of label
   // extension wouldn't be meaningful in coroutines.
   for (AddrLabelExpr *ALE : Fn->AddrLabels)
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 3909892ef0a6f..7d4fcfbe321f4 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3749,7 +3749,7 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
 
   // using plain return in a coroutine is not allowed.
   FunctionScopeInfo *FSI = getCurFunction();
-  if (getLangOpts().Coroutines && FSI->isCoroutine()) {
+  if (FSI->FirstReturnLoc.isInvalid() && FSI->isCoroutine()) {
     assert(FSI->FirstCoroutineStmtLoc.isValid() &&
                   "first coroutine location not set");
     Diag(ReturnLoc, diag::err_return_in_coroutine);
diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index b4f362c621929..0f8cb49dd5f47 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -3,6 +3,7 @@
 
 // RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify=expected,cxx20_23,cxx23    %s -fcxx-exceptions -fexceptions -Wunused-result
 // RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,cxx14_20,cxx20_23 %s -fcxx-exceptions -fexceptions -Wunused-result
+// RUN: not %clang_cc1 -std=c++20 -fsyntax-only %s -fcxx-exceptions -fexceptions -Wunused-result 2>&1 | FileCheck %s
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include <coroutine>}}
@@ -209,6 +210,22 @@ void mixed_yield_invalid() {
   return; // expected-error {{return statement not allowed in coroutine}}
 }
 
+void mixed_yield_return_first(bool b) {
+  if (b) {
+    return; // expected-error {{return statement not allowed in coroutine}}
+  }
+  co_yield 0; // expected-note {{function is a coroutine due to use of 'co_yield'}}
+}
+
+template<typename T>
+void mixed_return_for_range(bool b, T t) {
+  if (b) {
+    return; // expected-error {{return statement not allowed in coroutine}}
+  }
+  for co_await (auto i : t){}; // expected-warning {{'for co_await' belongs to CoroutineTS instead of C++20, which is deprecated}}
+  // expected-note at -1 {{function is a coroutine due to use of 'co_await'}}
+}
+
 template <class T>
 void mixed_yield_template(T) {
   co_yield blah; // expected-error {{use of undeclared identifier}}
@@ -267,6 +284,13 @@ void mixed_coreturn(void_tag, bool b) {
     return; // expected-error {{not allowed in coroutine}}
 }
 
+void mixed_coreturn_return_first(void_tag, bool b) {
+  if (b)
+    return; // expected-error {{not allowed in coroutine}}
+  else
+    co_return; // expected-note {{use of 'co_return'}}
+}
+
 void mixed_coreturn_invalid(bool b) {
   if (b)
     co_return; // expected-note {{use of 'co_return'}}
@@ -296,8 +320,8 @@ void mixed_coreturn_template2(bool b, T) {
 
 struct promise_handle;
 
-struct Handle : std::coroutine_handle<promise_handle> { // expected-note 2{{candidate constructor (the implicit copy constructor) not viable}}
-    // expected-note at -1 2{{candidate constructor (the implicit move constructor) not viable}}
+struct Handle : std::coroutine_handle<promise_handle> { // expected-note 4{{not viable}}
+    // expected-note at -1 4{{not viable}}
     using promise_type = promise_handle;
 };
 
@@ -315,6 +339,9 @@ Handle mixed_return_value() {
   co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
   return 0; // expected-error {{return statement not allowed in coroutine}}
   // expected-error at -1 {{no viable conversion from returned value of type}}
+  // CHECK-NOT: error: no viable conversion from returned value of type
+  // CHECK: error: return statement not allowed in coroutine
+  // CHECK: error: no viable conversion from returned value of type
 }
 
 Handle mixed_return_value_return_first(bool b) {
@@ -326,6 +353,15 @@ Handle mixed_return_value_return_first(bool b) {
     co_return 0; // expected-error {{no member named 'return_value' in 'promise_handle'}}
 }
 
+Handle mixed_multiple_returns(bool b) {
+  if (b) {
+    return 0; // expected-error {{no viable conversion from returned value of type}}
+    // expected-error at -1 {{return statement not allowed in coroutine}}
+  }
+  co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
+  return 0; // expected-error {{no viable conversion from returned value of type}}
+}
+
 struct CtorDtor {
   CtorDtor() {
     co_yield 0; // expected-error {{'co_yield' cannot be used in a constructor}}

>From 81eeb74c688be76ee61070ba7f237a7117bcdd2c Mon Sep 17 00:00:00 2001
From: Ivana Ivanovska <iivanovska at google.com>
Date: Thu, 1 Aug 2024 12:35:13 +0000
Subject: [PATCH 3/4] Applied nit comments.

---
 clang/lib/Sema/SemaCoroutine.cpp  | 18 +++++++++---------
 clang/test/SemaCXX/coroutines.cpp | 17 +++++++++++------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index fc624975b8a4a..e379987556f07 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -687,13 +687,14 @@ bool Sema::checkFinalSuspendNoThrow(const Stmt *FinalSuspend) {
 // [stmt.return.coroutine]p1:
 //   A coroutine shall not enclose a return statement ([stmt.return]).
 static void checkReturnStmtInCoroutine(Sema &S, FunctionScopeInfo *FSI) {
-  if (FSI && FSI->FirstReturnLoc.isValid()) {
-      assert(FSI->FirstCoroutineStmtLoc.isValid() &&
-                    "first coroutine location not set");
-      S.Diag(FSI->FirstReturnLoc, diag::err_return_in_coroutine);
-      S.Diag(FSI->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
-              << FSI->getFirstCoroutineStmtKeyword();
-  }
+  assert (!FSI && "FunctionScopeInfo is null");
+  assert(FSI->FirstCoroutineStmtLoc.isValid() &&
+                "first coroutine location not set");
+  if (FSI->FirstReturnLoc.isInvalid())
+    return;
+  S.Diag(FSI->FirstReturnLoc, diag::err_return_in_coroutine);
+  S.Diag(FSI->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
+          << FSI->getFirstCoroutineStmtKeyword();
 }
 
 bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
@@ -706,9 +707,8 @@ bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
   auto *ScopeInfo = getCurFunction();
   assert(ScopeInfo->CoroutinePromise);
 
-  if (ScopeInfo->FirstCoroutineStmtLoc == KWLoc) {
+  if (ScopeInfo->FirstCoroutineStmtLoc == KWLoc)
     checkReturnStmtInCoroutine(*this, ScopeInfo);
-  }
 
   // If we have existing coroutine statements then we have already built
   // the initial and final suspend points.
diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index 0f8cb49dd5f47..068fdab4bfe38 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -3,6 +3,8 @@
 
 // RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify=expected,cxx20_23,cxx23    %s -fcxx-exceptions -fexceptions -Wunused-result
 // RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,cxx14_20,cxx20_23 %s -fcxx-exceptions -fexceptions -Wunused-result
+
+// Run without -verify to check the order of errors we show.
 // RUN: not %clang_cc1 -std=c++20 -fsyntax-only %s -fcxx-exceptions -fexceptions -Wunused-result 2>&1 | FileCheck %s
 
 void no_coroutine_traits_bad_arg_await() {
@@ -339,18 +341,20 @@ Handle mixed_return_value() {
   co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
   return 0; // expected-error {{return statement not allowed in coroutine}}
   // expected-error at -1 {{no viable conversion from returned value of type}}
+  // Check that we first show that return is not allowed in coroutine.
+  // The error about bad conversion is most likely spurious so we prefer to have it afterwards.
   // CHECK-NOT: error: no viable conversion from returned value of type
   // CHECK: error: return statement not allowed in coroutine
   // CHECK: error: no viable conversion from returned value of type
 }
 
 Handle mixed_return_value_return_first(bool b) {
-   if (b) {
-        return 0; // expected-error {{no viable conversion from returned value of type}}
-        // expected-error at -1 {{return statement not allowed in coroutine}}
-    }
-    co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
-    co_return 0; // expected-error {{no member named 'return_value' in 'promise_handle'}}
+  if (b) {
+    return 0; // expected-error {{no viable conversion from returned value of type}}
+    // expected-error at -1 {{return statement not allowed in coroutine}}
+  }
+  co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
+  co_return 0; // expected-error {{no member named 'return_value' in 'promise_handle'}}
 }
 
 Handle mixed_multiple_returns(bool b) {
@@ -359,6 +363,7 @@ Handle mixed_multiple_returns(bool b) {
     // expected-error at -1 {{return statement not allowed in coroutine}}
   }
   co_await a; // expected-note {{function is a coroutine due to use of 'co_await' here}}
+  // The error 'return statement not allowed in coroutine' should appear only once.
   return 0; // expected-error {{no viable conversion from returned value of type}}
 }
 

>From 110540307d4487e081a147209e063d6f480eabfb Mon Sep 17 00:00:00 2001
From: Ivana Ivanovska <iivanovska at google.com>
Date: Fri, 2 Aug 2024 11:13:08 +0000
Subject: [PATCH 4/4] Fixed assert, added comment, fixed formatting.

---
 clang/lib/Sema/SemaCoroutine.cpp | 7 ++++---
 clang/lib/Sema/SemaStmt.cpp      | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index e379987556f07..68ad6e3fd6414 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -687,14 +687,14 @@ bool Sema::checkFinalSuspendNoThrow(const Stmt *FinalSuspend) {
 // [stmt.return.coroutine]p1:
 //   A coroutine shall not enclose a return statement ([stmt.return]).
 static void checkReturnStmtInCoroutine(Sema &S, FunctionScopeInfo *FSI) {
-  assert (!FSI && "FunctionScopeInfo is null");
+  assert(FSI && "FunctionScopeInfo is null");
   assert(FSI->FirstCoroutineStmtLoc.isValid() &&
-                "first coroutine location not set");
+         "first coroutine location not set");
   if (FSI->FirstReturnLoc.isInvalid())
     return;
   S.Diag(FSI->FirstReturnLoc, diag::err_return_in_coroutine);
   S.Diag(FSI->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
-          << FSI->getFirstCoroutineStmtKeyword();
+      << FSI->getFirstCoroutineStmtKeyword();
 }
 
 bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
@@ -707,6 +707,7 @@ bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
   auto *ScopeInfo = getCurFunction();
   assert(ScopeInfo->CoroutinePromise);
 
+  // Avoid duplicate errors, report only on first keyword.
   if (ScopeInfo->FirstCoroutineStmtLoc == KWLoc)
     checkReturnStmtInCoroutine(*this, ScopeInfo);
 
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 7d4fcfbe321f4..d283eaa511011 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3751,10 +3751,10 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
   FunctionScopeInfo *FSI = getCurFunction();
   if (FSI->FirstReturnLoc.isInvalid() && FSI->isCoroutine()) {
     assert(FSI->FirstCoroutineStmtLoc.isValid() &&
-                  "first coroutine location not set");
+           "first coroutine location not set");
     Diag(ReturnLoc, diag::err_return_in_coroutine);
     Diag(FSI->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
-            << FSI->getFirstCoroutineStmtKeyword();
+        << FSI->getFirstCoroutineStmtKeyword();
   }
 
   StmtResult R =



More information about the cfe-commits mailing list