https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/100985
>From d35544d971f073f98fba047cfcbe3cfe92dd78c4 Mon Sep 17 00:00:00 2001
From: Ivana Ivanovska <iivanov...@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@-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@-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@-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 <iivanov...@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@-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@-1 2{{candidate constructor (the implicit move
constructor) not viable}}
+struct Handle : std::coroutine_handle<promise_handle> { // expected-note
4{{not viable}}
+ // expected-note@-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@-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@-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 <iivanov...@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@-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@-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@-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@-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 <iivanov...@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 =
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
