[clang] Surface error for plain return statement in coroutine earlier (PR #100985)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 1 01:35:25 PDT 2024
https://github.com/ivanaivanovska updated https://github.com/llvm/llvm-project/pull/100985
>From 8ee4d7bcdbc186e86d4ff2374b12a54f05d1fc38 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/2] 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 8d2a30e3e0b32ef4b7db63d5f320eacf56c00610 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/2] 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}}
More information about the cfe-commits
mailing list