[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

115

add tests with c_yield and co_await

[ChuanqiXu]

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

75–79

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

91

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

75–79

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

75–79

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

75–79

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

75–79

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/bugprone/ExceptionEscapeCheck.cpp

 
 void ExceptionEscapeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "FunctionsThatShouldNotThrow",
                 RawFunctionsThatShouldNotThrow);
   Options.store(Opts, "IgnoredExceptions", RawIgnoredExceptions);
 }
 
 void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
+  auto IsCoroMatcher =
+      hasDescendant(expr(anyOf(coyieldExpr(), coreturnStmt(), coawaitExpr())));
   Finder->addMatcher(
       functionDecl(anyOf(isNoThrow(), cxxDestructorDecl(),
                          cxxConstructorDecl(isMoveConstructor()),
                          cxxMethodDecl(isMoveAssignmentOperator()),
                          hasName("main"), hasName("swap"),
-                         isEnabled(FunctionsThatShouldNotThrow)))
+                         isEnabled(FunctionsThatShouldNotThrow)),
+                   unless(IsCoroMatcher))
           .bind("thrower"),
       this);
 }
 
 void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("thrower");
 
   if (!MatchedDecl)

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 T> struct promise;
+
+template <typename T> struct task {
+  using promise_type = promise<T>;
+
+  explicit task(promise_type &p) {
+    throw 1;
+    p.return_val = this;
+  }
+
+  T value;
+};
+
+template <typename T> struct promise {
+  task<T> get_return_object() { return task{*this}; }
+
+  std::suspend_never initial_suspend() const noexcept { 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() { throw 1; }
+
+  task<T> *return_val;
+};
+
+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;
+}
+
+int c_ShouldDiag(const int a, const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'c_ShouldDiag' which should not throw exceptions
+  if (b == 0)
+    throw b;
+
+  return a / b;
+}
+
+const auto d_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 b;
+
+  co_return a / b;
+};
+
+const auto e_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 f_ShouldDiag = [](const int a, const int b) noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: an exception may be thrown in function 'operator()' which should not throw exceptions
+  if (b == 0)
+    throw b;
+
+  return a / b;
+};
+
+// CHECK-MESSAGES