Surface error for plain return statement in coroutine earlier #100985

ivanaivanovska · 2024-07-29T08:26:12Z

When a plain return statement was used in a coroutine, the error "return statement not allowed in coroutine" was surfaced too late (e.g. after other errors in the return statement). Surfacing it earlier now, to make the issue more obvious.

github-actions · 2024-07-29T08:26:33Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-07-29T08:27:01Z

@llvm/pr-subscribers-clang

Author: None (ivanaivanovska)

Changes

When a plain return statement was used in a coroutine, the error "return statement not allowed in coroutine" was surfaced too late (e.g. after other errors in the return statement). Surfacing it earlier now, to make the issue more obvious.

Full diff: https://github.com/llvm/llvm-project/pull/100985.diff

3 Files Affected:

(modified) clang/lib/Sema/SemaCoroutine.cpp (+1-1)
(modified) clang/lib/Sema/SemaStmt.cpp (+10)
(modified) clang/test/SemaCXX/coroutines.cpp (+35)

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}}

llvmbot · 2024-07-29T08:27:02Z

@llvm/pr-subscribers-coroutines

Author: None (ivanaivanovska)

Changes

When a plain return statement was used in a coroutine, the error "return statement not allowed in coroutine" was surfaced too late (e.g. after other errors in the return statement). Surfacing it earlier now, to make the issue more obvious.

Full diff: https://github.com/llvm/llvm-project/pull/100985.diff

3 Files Affected:

(modified) clang/lib/Sema/SemaCoroutine.cpp (+1-1)
(modified) clang/lib/Sema/SemaStmt.cpp (+10)
(modified) clang/test/SemaCXX/coroutines.cpp (+35)

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}}

ilya-biryukov

Thanks for the change!
This looks like an improvement, but I suspect we can do even better. See the comments I've left in the code.

clang/lib/Sema/SemaStmt.cpp

clang/test/SemaCXX/coroutines.cpp

clang/lib/Sema/SemaCoroutine.cpp

clang/lib/Sema/SemaStmt.cpp

clang/test/SemaCXX/coroutines.cpp

ilya-biryukov

I have a lot of nitpicks, but otherwise LG, happy to approve as soon as they're fixed.

clang/lib/Sema/SemaCoroutine.cpp

clang/test/SemaCXX/coroutines.cpp

clang/lib/Sema/SemaCoroutine.cpp

ilya-biryukov

The assert condition seems wrong, otherwise this should be good to go.

I've also left one more suggestion to leave a comment that I missed in the original review since we need another round anyway.

clang/lib/Sema/SemaCoroutine.cpp

github-actions · 2024-08-01T13:24:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ilya-biryukov

Thanks! The changes LG, I'll wait until premerge checks finish and merge it into mainline.

github-actions · 2024-08-02T11:49:28Z

@ivanaivanovska Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" coroutines C++20 coroutines labels Jul 29, 2024

hokein requested review from ilya-biryukov and usx95 July 29, 2024 08:26

ilya-biryukov reviewed Jul 29, 2024

View reviewed changes

ivanaivanovska added 2 commits August 1, 2024 08:38

Surface error for plain return statement in coroutine earlier

d35544d

Applied fixes related to the comments in the PR.

f0f6a1a

ilya-biryukov reviewed Aug 1, 2024

View reviewed changes

Applied nit comments.

81eeb74

ivanaivanovska force-pushed the fixcoroutine branch from 8d2a30e to 81eeb74 Compare August 1, 2024 12:43

ilya-biryukov requested changes Aug 1, 2024

View reviewed changes

clang/lib/Sema/SemaCoroutine.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaCoroutine.cpp Show resolved Hide resolved

Fixed assert, added comment, fixed formatting.

1105403

ilya-biryukov approved these changes Aug 2, 2024

View reviewed changes

Merge branch 'main' into fixcoroutine

6b95f34

ilya-biryukov merged commit db1375f into llvm:main Aug 2, 2024
4 of 6 checks passed

ivanaivanovska mentioned this pull request Jan 29, 2025

Request Commit Access For ivanaivanovska #124918

Closed

Surface error for plain return statement in coroutine earlier #100985

Surface error for plain return statement in coroutine earlier #100985

Uh oh!

Conversation

ivanaivanovska commented Jul 29, 2024

Uh oh!

github-actions bot commented Jul 29, 2024

Uh oh!

llvmbot commented Jul 29, 2024

Uh oh!

llvmbot commented Jul 29, 2024

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 2, 2024

Uh oh!

Uh oh!

github-actions bot commented Aug 1, 2024 •

edited

Loading