[clang-tidy] Do not emit bugprone-exception-escape warnings from coroutines

[denizevrenci]

Summary

All exceptions thrown in coroutine bodies are caught and
unhandled_exception member of the coroutine promise type is called.
In accordance with the existing rules of diagnostics related to
exceptions thrown in functions marked noexcept, even if the promise
type's constructor, get_return_object, or unhandled_exception
throws, diagnostics should not be emitted.

Fixes #61905.

[denizevrenci]

Remove line breaks in CHECK-MESSAGES lines

[denizevrenci]

Fix the line number for the warning

[denizevrenci]

I don't have commit access so after the review is complete, please commit this diff in my place.

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

116

add tests with c_yield and co_await

[ChuanqiXu]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

76–80

I don't understand why we shouldn't emit the warning here. Since the function is marked noexcept but it may throw actually in unhandled_exception. I think it is meaningful to warn for this.

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

92

And based on ChuanqiXu comment, same here.

"All exceptions thrown in coroutine bodies are caught and
unhandled_exception member of the coroutine promise type is called."
This isn't expected behaviour, as it may hide exception.
Based on that we should assume, if there is noexcept, then it should not throw, and if its no noexcept, then it could throw. No point to change default behaviour then.

Other option is to add check option to handle this.

[denizevrenci]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

76–80

Right, I now see that this behavior is different between Clang's -Wexceptions and Clang Tidy's bugprone-exception-escape. The former does not warn on this code, the latter does.

int foo() {
  throw 1;
}

int bar() noexcept {
  return foo();
}

We need to treat coroutines differently and check whether task::task, promise::promise, promise::initial_suspend, promise::get_return_object, and promise::unhandled_exception can throw instead of the body of the coroutine.

[ChuanqiXu]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

76–80

I investigated the exception issue in coroutines before: https://reviews.llvm.org/D108277. And it is much more complex than I thought. The short conclusion here is that the coroutine is still may throw even if all the promise's method wouldn't throw. For example:

struct Evil {
    ~Evil() noexcept(false) { throw 32; }
};

task foo() noexcept { // all promise's method of task wouldn't throw
    throw Evil;
}

And in the above example, foo() may throw actually. Although the implicit catch block of foo() will catch Evil, the exception in the destructor of Evil will be thrown again.

So we can't be sure that a coroutine wouldn't throw even if all of its promise's method wouldn't throw.

[denizevrenci]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

76–80

It looks like the function foo can throw until the first suspension point in the coroutine. If promise::initial_suspend is std::suspend_always, then it will not throw. Of course, determining this statically is quite complicated.
But I also think that this is a rather niche example, it looks like clang-tidy already warns with bugprone-exception-escape on the destructor of Evil even when it is marked noexcept(false). I assume this is due to the other
complications brought by throwing from destructors. Would that not be the appropriate place to warn about this anyway?

For example, the code below terminates because the destructor of Evil gets called while there is an active exception.

task foo() { // all promise's method of task wouldn't throw
    Evil e;
    throw 1;
    co_return;
}

[PiotrZSL]

" clang-tidy already warns with bugprone-exception-escape on the destructor of Evil even when it is marked noexcept(false)"
https://reviews.llvm.org/D145865

[ChuanqiXu]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

76–80

If we've handled the case, the strategy makes sense to me.

[denizevrenci]

Address comments. Implement a special case for coroutines in ExceptionAnalyzer

[denizevrenci]

a_shouldNotDiag -> b_shouldNotDiag

[denizevrenci]

Fix commit message

[denizevrenci]

Add tests for co_yield and co_await

[denizevrenci]

@PiotrZSL & @ChuanqiXu may I request a re-review? I addressed all comments.

[PiotrZSL]

LGTM

• Missing release notes entry.
• I'm not sure about change in StmtCXX.h, do we need it ? Coudn't we just use children() + getBody()
• CHECK-MESSAGES-NOT get me a little bit confused. Maybe we dont need those entrys, just make sure that test fail when non listed warning show up.

[denizevrenci]

• Update release notes
• Remove checks for no messages

[denizevrenci]

Addressed 1 and 3. As for the new methods in StmtCXX.h, I think they may be used elsewhere too where we need to make a distinction between the visible body of the coroutine and the desugared version.

[denizevrenci]

• Rebase on trunk
• Remove unused helpers

[denizevrenci]

Hi @PiotrZSL if you are happy with the changes, could you commit the diff for me as I don't have commit access to LLVM repo?

[ChuanqiXu]

LGTM. Thanks.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
   } else if (const auto *Construct = dyn_cast<CXXConstructExpr>(St)) {
     ExceptionInfo Excs =
         throwsException(Construct->getConstructor(), CallStack);
     Results.merge(Excs);
   } else if (const auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(St)) {
     ExceptionInfo Excs =
         throwsException(DefaultInit->getExpr(), Caught, CallStack);
     Results.merge(Excs);
+  } else if (const auto *Coro = dyn_cast<CoroutineBodyStmt>(St)) {
+    for (const Stmt *Child : Coro->childrenExclBody()) {
+      ExceptionInfo Excs = throwsException(Child, Caught, CallStack);
+      Results.merge(Excs);
+    }
+    ExceptionInfo Excs = throwsException(Coro->getBody(), Caught, CallStack);
+    for (const Type *Throwable : Excs.getExceptionTypes()) {
+      if (const auto ThrowableRec = Throwable->getAsCXXRecordDecl()) {
+        ExceptionInfo DestructorExcs =
+            throwsException(ThrowableRec->getDestructor(), CallStack);
+        Results.merge(DestructorExcs);
+      }
+    }
   } else {
     for (const Stmt *Child : St->children()) {
       ExceptionInfo Excs = throwsException(Child, Caught, CallStack);
       Results.merge(Excs);
     }
   }
   return Results;
 }

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

+// RUN: %check_clang_tidy -std=c++20 %s bugprone-exception-escape %t -- \
+// RUN:     -- -fexceptions
+
+namespace std {
+
+template <class Ret, typename... T> struct coroutine_traits {
+  using promise_type = typename Ret::promise_type;
+};
+
+template <class Promise = void> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  static coroutine_handle from_promise(Promise &promise);
+  constexpr void *address() const noexcept;
+};
+
+template <> struct coroutine_handle<void> {
+  template <class PromiseType>
+  coroutine_handle(coroutine_handle<PromiseType>) noexcept;
+  static coroutine_handle from_address(void *);
+  constexpr void *address() const noexcept;
+};
+
+struct suspend_always {
+  bool await_ready() noexcept { return false; }
+  void await_suspend(coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+
+struct suspend_never {
+  bool await_ready() noexcept { return true; }
+  void await_suspend(coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+
+} // namespace std
+
+template <typename Task, bool ThrowInPromiseConstructor,
+          bool ThrowInInitialSuspend, bool ThrowInGetReturnObject,
+          bool ThrowInUnhandledException>
+struct Promise;
+
+template <
+    typename T, bool ThrowInTaskConstructor = false,
+    bool ThrowInPromiseConstructor = false, bool ThrowInInitialSuspend = false,
+    bool ThrowInGetReturnObject = false, bool ThrowInUnhandledException = false>
+struct Task {
+  using promise_type =
+      Promise<Task, ThrowInPromiseConstructor, ThrowInInitialSuspend,
+              ThrowInGetReturnObject, ThrowInUnhandledException>;
+
+  explicit Task(promise_type &p) {
+    if constexpr (ThrowInTaskConstructor) {
+      throw 1;
+    }
+
+    p.return_val = this;
+  }
+
+  T value;
+};
+
+template <typename Task, bool ThrowInPromiseConstructor,
+          bool ThrowInInitialSuspend, bool ThrowInGetReturnObject,
+          bool ThrowInUnhandledException>
+struct Promise {
+  Promise() {
+    if constexpr (ThrowInPromiseConstructor) {
+      throw 1;
+    }
+  }
+
+  Task get_return_object() {
+    if constexpr (ThrowInGetReturnObject) {
+      throw 1;
+    }
+
+    return Task{*this};
+  }
+
+  std::suspend_never initial_suspend() const {
+    if constexpr (ThrowInInitialSuspend) {
+      throw 1;
+    }
+
+    return {};
+  }
+
+  std::suspend_never final_suspend() const noexcept { return {}; }
+
+  template <typename U> void return_value(U &&val) {
+    return_val->value = static_cast<U &&>(val);
+  }
+
+  void unhandled_exception() {
+    if constexpr (ThrowInUnhandledException) {
+      throw 1;
+    }
+  }
+
+  Task *return_val;
+};
+
+struct Evil {
+  ~Evil() noexcept(false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~Evil' which should not throw exceptions
+    throw 42;
+  }
+};
+
+namespace function {
+
+Task<int> a_ShouldNotDiag(const int a, const int b) {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:11: warning: an exception may be thrown in function 'a_ShouldNotDiag' which should not throw exceptions
+  if (b == 0)
+    throw b;
+
+  co_return a / b;
+}
+
+Task<int> b_ShouldNotDiag(const int a, const int b) noexcept {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:11: warning: an exception may be thrown in function 'b_ShouldNotDiag' which should not throw exceptions
+  if (b == 0)
+    throw b;
+
+  co_return a / b;
+}
+
+Task<int> c_ShouldNotDiag(const int a, const int b) {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:11: warning: an exception may be thrown in function 'c_ShouldNotDiag' which should not throw exceptions
+  if (b == 0)
+    throw Evil{};
+
+  co_return a / b;
+}
+
+Task<int> c_ShouldDiag(const int a, const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: an exception may be thrown in function 'c_ShouldDiag' which should not throw exceptions
+  if (b == 0)
+    throw Evil{};
+
+  co_return a / b;
+}
+
+Task<int, true> d_ShouldNotDiag(const int a, const int b) {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:17: warning: an exception may be thrown in function 'd_ShouldNotDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, true> d_ShouldDiag(const int a, const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: an exception may be thrown in function 'd_ShouldDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, true> e_ShouldNotDiag(const int a, const int b) {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:24: warning: an exception may be thrown in function 'e_ShouldNotDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, true> e_ShouldDiag(const int a, const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: an exception may be thrown in function 'e_ShouldDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, false, true> f_ShouldNotDiag(const int a, const int b) {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:31: warning: an exception may be thrown in function 'f_ShouldNotDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, false, true> f_ShouldDiag(const int a, const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: an exception may be thrown in function 'f_ShouldDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, false, false, true> g_ShouldNotDiag(const int a, const int b) {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:38: warning: an exception may be thrown in function 'g_ShouldNotDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, false, false, true> g_ShouldDiag(const int a,
+                                                  const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-2]]:38: warning: an exception may be thrown in function 'g_ShouldDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, false, false, false, true> h_ShouldNotDiag(const int a,
+                                                            const int b) {
+  // CHECK-MESSAGES-NOT: :[[@LINE-2]]:45: warning: an exception may be thrown in function 'h_ShouldNotDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+Task<int, false, false, false, false, true> h_ShouldDiag(const int a,
+                                                         const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-2]]:45: warning: an exception may be thrown in function 'h_ShouldDiag' which should not throw exceptions
+  co_return a / b;
+}
+
+} // namespace function
+
+namespace lambda {
+
+const auto a_ShouldNotDiag = [](const int a, const int b) -> Task<int> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-2]]:30: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  if (b == 0)
+    throw b;
+
+  co_return a / b;
+};
+
+const auto b_ShouldNotDiag = [](const int a,
+                                const int b) noexcept -> Task<int> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-2]]:30: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  if (b == 0)
+    throw b;
+
+  co_return a / b;
+};
+
+const auto c_ShouldNotDiag = [](const int a, const int b) -> Task<int> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:30: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  if (b == 0)
+    throw Evil{};
+
+  co_return a / b;
+};
+
+const auto c_ShouldDiag = [](const int a, const int b) noexcept -> Task<int> {
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  if (b == 0)
+    throw Evil{};
+
+  co_return a / b;
+};
+
+const auto d_ShouldNotDiag = [](const int a, const int b) -> Task<int, true> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:30: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto d_ShouldDiag = [](const int a,
+                             const int b) noexcept -> Task<int, true> {
+  // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto e_ShouldNotDiag = [](const int a,
+                                const int b) -> Task<int, false, true> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-2]]:30: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto e_ShouldDiag = [](const int a,
+                             const int b) noexcept -> Task<int, false, true> {
+  // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto f_ShouldNotDiag = [](const int a,
+                                const int b) -> Task<int, false, false, true> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-2]]:30: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto f_ShouldDiag =
+    [](const int a, const int b) noexcept -> Task<int, false, false, true> {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto g_ShouldNotDiag =
+    [](const int a, const int b) -> Task<int, false, false, false, true> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto g_ShouldDiag =
+    [](const int a,
+       const int b) noexcept -> Task<int, false, false, false, true> {
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto h_ShouldNotDiag =
+    [](const int a,
+       const int b) -> Task<int, false, false, false, false, true> {
+  // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+const auto h_ShouldDiag =
+    [](const int a,
+       const int b) noexcept -> Task<int, false, false, false, false, true> {
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  co_return a / b;
+};
+
+} // namespace lambda

clang/include/clang/AST/StmtCXX.h

public:
   }
 
   const_child_range children() const {
     return const_child_range(getStoredStmts(), getStoredStmts() +
                                                    SubStmt::FirstParamMove +
                                                    NumParams);
   }
 
+  child_range childrenExclBody() {
+    return child_range(getStoredStmts() + SubStmt::Body + 1,
+                       getStoredStmts() + SubStmt::FirstParamMove + NumParams);
+  }
+
+  const_child_range childrenExclBody() const {
+    return const_child_range(getStoredStmts() + SubStmt::Body + 1,
+                             getStoredStmts() + SubStmt::FirstParamMove +
+                                 NumParams);
+  }
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == CoroutineBodyStmtClass;
   }
 };
 
 /// Represents a 'co_return' statement in the C++ Coroutines TS.
 ///
 /// This statament models the initialization of the coroutine promise