Repository: agronholm/exceptiongroup

Issue:
`with exceptiongroup.catch({Exception: async_fn}):` is a tempting and silent footgun
It's surprisingly easy to write a handler which silently discards exceptions that you thought were handled:

```python
import asyncio
from exceptiongroup import ExceptionGroup, catch

async def handler(eg):
    # Log some stuff, then re-raise
    raise eg

async def main():
    with catch({TypeError: handler}):
        raise ExceptionGroup("message", TypeError("uh-oh"))

asyncio.run(main())
```
and in a large program, all too easy to miss your only warning:
```python-traceback
$ python t.py 
exceptiongroup/_catch.py:52: RuntimeWarning: coroutine 'handler' was never awaited
  handler(matched)
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
```

...the problem being that we expect _sync_ handlers, and [so we just call it](https://github.com/agronholm/exceptiongroup/blob/9bfc03f402cca2d671a42d00dafb56f5c1e75847/src/exceptiongroup/_catch.py#L52) - which if it's an async function won't actually run the body of the handler, and thus won't re-raise any exception, which is treated as success.

--------------------

IMO this code is unrecoverably correct, and we should bail out with an error as soon as possible.  Based on [Trio's internal comments](https://github.com/python-trio/trio/blob/314663806cbaf717d70d70b71631e340396a1bb1/trio/_util.py#L137-L140) we should rely on checking the return value rather than inspecting the handler callable - I suspect doing an initial branch on `is not None` would keep things super-fast for the vast majority of happy cases.  Thoughts?

Discussion:
coveralls: ## Pull Request Test Coverage Report for [Build 5531903375](https://coveralls.io/builds/61296078)

* **5** of **5**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**2.5%**) to **98.638%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/61296078/badge)](https://coveralls.io/builds/61296078) |
| :-- | --: |
| Change from base [Build 5513148018](https://coveralls.io/builds/61249941): |  2.5% |
| Covered Lines: | 507 |
| Relevant Lines: | 514 |

---
##### 💛  - [Coveralls](https://coveralls.io)

jakkdl: Hm, I'm utterly failing to suppress the `RuntimeWarning` about handler not being awaited in the test when at the same checking for `pytest.raises`. I've tried pretty much all variants in https://docs.pytest.org/en/7.1.x/how-to/capture-warnings.html and putting the guards at different places in the test case but it seems like `pytest.raises` overrides `warnings.catch_warnings()` / `pytest.warns()` / `recwarn`
jakkdl: sorry for the force-pushing, I pushed ignoring the `RuntimeError` just before I saw your change suggestion which fixed that, so I rebased away my commit.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 040c6f9..eccb5b3 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -3,6 +3,11 @@ Version history
 
 This library adheres to `Semantic Versioning 2.0 <http://semver.org/>`_.
 
+**UNRELEASED**
+- `catch()` now raises a `TypeError` if passed an async exception handler instead of
+  just giving a `RuntimeWarning` about the coroutine never being awaited. (#66, PR by
+  John Litborn)
+
 **1.1.2**
 
 - Changed handling of exceptions in exception group handler callbacks to not wrap a
diff --git a/src/exceptiongroup/_catch.py b/src/exceptiongroup/_catch.py
index 0be39b4..2d82be1 100644
--- a/src/exceptiongroup/_catch.py
+++ b/src/exceptiongroup/_catch.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import inspect
 import sys
 from collections.abc import Callable, Iterable, Mapping
 from contextlib import AbstractContextManager
@@ -49,9 +50,15 @@ class _Catcher:
             matched, excgroup = excgroup.split(exc_types)
             if matched:
                 try:
-                    handler(matched)
+                    result = handler(matched)
                 except BaseException as new_exc:
                     new_exceptions.append(new_exc)
+                else:
+                    if inspect.iscoroutine(result):
+                        raise TypeError(
+                            f"Error trying to handle {matched!r} with {handler!r}. "
+                            "Exception handler must be a sync function."
+                        ) from exc
 
             if not excgroup:
                 break


Test patch:
diff --git a/tests/test_catch.py b/tests/test_catch.py
index 0af2fa0..caf1368 100644
--- a/tests/test_catch.py
+++ b/tests/test_catch.py
@@ -162,3 +162,17 @@ def test_catch_subclass():
     assert isinstance(lookup_errors[0], ExceptionGroup)
     exceptions = lookup_errors[0].exceptions
     assert isinstance(exceptions[0], KeyError)
+
+
+def test_async_handler(request):
+    async def handler(eg):
+        pass
+
+    def delegate(eg):
+        coro = handler(eg)
+        request.addfinalizer(coro.close)
+        return coro
+
+    with pytest.raises(TypeError, match="Exception handler must be a sync function."):
+        with catch({TypeError: delegate}):
+            raise ExceptionGroup("message", TypeError("uh-oh"))