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

[Deniz Evrenci via Phabricator via cfe-commits]

denizevrenci added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp:75-79
+  // 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;
----------------
ChuanqiXu wrote:
> denizevrenci wrote:
> > ChuanqiXu wrote:
> > > 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.
> > 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.
> 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.
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;
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147417/new/

https://reviews.llvm.org/D147417