with exceptiongroup.catch({Exception: async_fn}): is a tempting and silent footgun

[Zac-HD]

It's surprisingly easy to write a handler which silently discards exceptions that you thought were handled:

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 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 - 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 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?

[agronholm]

We could use inspect.iscoroutinefunction() and raise TypeError or something on a positive. It's an extra step for everyone who's using this correctly, but maybe the performance hit isn't noticeable.

[Zac-HD]

I think inspect.iscoroutinefunction() is unreliable for python-trio/trio#2670 - style reasons, and I think checking return values is faster too since we can skip almost all the work in most cases. That way 'exception not raised' is free, 'returns None' is almost free, and remaining cases are suspicious enough that I don't mind doing a check.

I think that reuse of the context manager is rare enough that we should actually prefer the smaller number of checks done when a handler is actually called, to checking every handler when setting up the context. The downside there is that maybe we could have detected it earlier or more reliably in cases where the buggy handler is only very rarely invoked, which could be sufficient to justify having both checks...

[agronholm]

Ok, so the consensus is to use inspect.iscoroutine() on the return value?

[Zac-HD]

Sounds good to me!