[Clang] Warn about [[noreturn]] on coroutines

[Nerixyz]

Declaring a coroutine [[noreturn]] doesn't make sense, because it will always return its handle. Clang previously crashed when trying to warn about this (diagnostic ID was 0).

This PR adds a warning specific to coroutines. As explained in #127327 (comment), Clang will show two warnings:

main.cpp(41,24): warning: function 'f' declared 'noreturn' should not return [-Winvalid-noreturn]
   41 | [[noreturn]] coroutine f() { co_await awaitable{}; }
      |                        ^
main.cpp(41,52): warning: coroutines cannot be declared 'noreturn' as they always return a coroutine handle [-Winvalid-noreturn]
   41 | [[noreturn]] coroutine f() { co_await awaitable{}; }
      |                                                    ^
2 warnings generated.

I'm not sure if that's reasonable, or if the first warning should be omitted.

Fixes #127327.

[llvmbot]

@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-clang

Author: nerix (Nerixyz)

Changes

Declaring a coroutine [[noreturn]] doesn't make sense, because it will always return its handle. Clang previously crashed when trying to warn about this (diagnostic ID was 0).

This PR adds a warning specific to coroutines. As explained in #127327 (comment), Clang will show two warnings:

main.cpp(41,24): warning: function 'f' declared 'noreturn' should not return [-Winvalid-noreturn]
   41 | [[noreturn]] coroutine f() { co_await awaitable{}; }
      |                        ^
main.cpp(41,52): warning: coroutines cannot be declared 'noreturn' as they always return a coroutine handle [-Winvalid-noreturn]
   41 | [[noreturn]] coroutine f() { co_await awaitable{}; }
      |                                                    ^
2 warnings generated.

I'm not sure if that's reasonable, or if the first warning should be omitted.

Fixes #127327.

---

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

3 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
• (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+2-2)
• (added) clang/test/SemaCXX/coroutine-noreturn.cpp (+27)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f10af8f5bd6b2..9136fb83ae0b5 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10606,6 +10606,9 @@ def warn_noreturn_function_has_return_expr : Warning<
 def warn_falloff_noreturn_function : Warning<
   "function declared 'noreturn' should not return">,
   InGroup<InvalidNoreturn>;
+def warn_noreturn_coroutine : Warning<
+  "coroutines cannot be declared 'noreturn' as they always return a coroutine handle">,
+  InGroup<InvalidNoreturn>;
 def err_noreturn_block_has_return_expr : Error<
   "block declared 'noreturn' should not return">;
 def err_carries_dependency_missing_on_first_decl : Error<
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 589869d018657..dc344b60384e3 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -588,10 +588,10 @@ struct CheckFallThroughDiagnostics {
   static CheckFallThroughDiagnostics MakeForCoroutine(const Decl *Func) {
     CheckFallThroughDiagnostics D;
     D.FuncLoc = Func->getLocation();
-    D.diag_MaybeFallThrough_HasNoReturn = 0;
+    D.diag_MaybeFallThrough_HasNoReturn = diag::warn_noreturn_coroutine;
     D.diag_MaybeFallThrough_ReturnsNonVoid =
         diag::warn_maybe_falloff_nonvoid_coroutine;
-    D.diag_AlwaysFallThrough_HasNoReturn = 0;
+    D.diag_AlwaysFallThrough_HasNoReturn = diag::warn_noreturn_coroutine;
     D.diag_AlwaysFallThrough_ReturnsNonVoid =
         diag::warn_falloff_nonvoid_coroutine;
     D.diag_NeverFallThroughOrReturn = 0;
diff --git a/clang/test/SemaCXX/coroutine-noreturn.cpp b/clang/test/SemaCXX/coroutine-noreturn.cpp
new file mode 100644
index 0000000000000..30454ef0f37cf
--- /dev/null
+++ b/clang/test/SemaCXX/coroutine-noreturn.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -Winvalid-noreturn -verify
+
+#include "Inputs/std-coroutine.h"
+
+struct Promise;
+
+struct Awaitable {
+  bool await_ready();
+  void await_suspend(std::coroutine_handle<>);
+  void await_resume();
+};
+
+struct Coro : std::coroutine_handle<> {
+  using promise_type = Promise;
+};
+
+struct Promise {
+  Coro get_return_object();
+  std::suspend_always initial_suspend() noexcept;
+  std::suspend_always final_suspend() noexcept;
+  void return_void();
+  void unhandled_exception();
+};
+
+[[noreturn]] Coro test() { // expected-warning {{function 'test' declared 'noreturn' should not return}}
+  co_await Awaitable{};
+} // expected-warning {{coroutines cannot be declared 'noreturn' as they always return a coroutine handle}}

[Sirraide]

Yeah, I agree we should be warning on this.

However, AnalysisBasedWarnings isn’t really the right place for this, because that’s all about emitting warnings based on control-flow analysis, but whether a function is a coroutine or declared noreturn is known even without control-flow analysis, so this should probably go somewhere else (additionally, the analysis-based warnings pass doesn’t run if there are any errors anywhere else in the TU—another reason why handling this elsewhere would be preferrable). Probably where we create the FunctionDecl?

[Sirraide]

I'm not sure if that's reasonable, or if the first warning should be omitted.

Hmm, I suspect that not doing this in analysis-based warnings might help with that. It would definitely be preferable if the first warning wasn’t there.

[Nerixyz]

Probably where we create the FunctionDecl?

The first warning is emitted when creating the return value. So I'm checking for a coroutine there. To fix the crash in the AnalysisBasedWarnings, I'm checking if a diagnostic with ID 0 would've been emitted.

[Sirraide]

I still think it makes more sense to diagnose this when the FunctionDecl is created rather than when we try and return from it.

[Sirraide]

I still think it makes more sense to diagnose this when the FunctionDecl is created rather than when we try and return from it.

Huh, weird, github went ahead and posted that comment anyway even though it was complaining about the review message being empty. This was supposed to be in reference to the SemaStmt changes.

[Sirraide]

I think the changes are fine now, but we should add one more test case:

// NO warning here. This could be a regular function returning a `Coro` object.
[[noreturn]] Coro test2();

Putting the check in CheckCompletedCoroutineBody() means that this should already work properly, but let’s add a test just to be sure.

[Sirraide]

I think the changes are fine now, but we should add one more test case:

// NO warning here. This could be a regular function returning a `Coro` object.
[[noreturn]] Coro test2();

Putting the check in CheckCompletedCoroutineBody() means that this should already work properly, but let’s add a test just to be sure.

Admittedly, declaring any non-void function noreturn is semantically questionable imo, but that’s not really relevant for this pr.

[Sirraide]

@Nerixyz Just so you know, changes like these should come with a release note (which I just added #127815); I should have pointed that out during review but ended up forgetting about it this time...