iscoroutinecheck check is more restrictive than documented types

[FasterSpeeding]

Steps to reproduce

hikari/hikari/impl/event_manager_base.py

Line 278 in c956f6c

if not inspect.iscoroutinefunction(callback):

This line errors if an object with an async __call__ is passed or if a normal function which returns a coroutine is passed even though the documented types allow both cases.

[davfsa]

Would changing that line to iscoroutine be better? Seems to allow async def __call__ like this:

import inspect

class Foo:
    async def __call__(self):
        print("hi")

inspect.iscoroutine(Foo()()) # True

What use case would you have for this?

[FasterSpeeding]

import inspect

class Foo:
    async def __call__(self):
        print("hi")

inspect.iscoroutine(Foo()()) # True

there's no guarantee that a function matching Callable[[Event], Coroutine[Any, Any, None] won't do stuff before yielding or have any side effects when called but not awaited though like

async def _foo_async(event: Event) -> None:
    # Do async stuff

def foo(event: Event) -> Coroutine[Any, Any, None]
    # do_time_sensitive_stuff ...
    return _foo_async(event)

(plus you'd get warnings about unawaited coroutines)

[davfsa]

Think the best way to go around this would be to just have people pass the coroutine to call (__call__ in the case of the class).

For the other example you said, they could just do this:

async def _foo_async(event: Event) -> None:
    # Time sensitive stuff can be done in another non-async function or just here avoiding awaits
    #
    # Do async stuff

I unfortunately dont see any way of doing this, as the way inspect.iscoroutinefunction work is by checking an internal flag, nothing to do with the public facing API

[davfsa]

Looking back at these old issues, I dont think this is something we can support nor encourage. Documentation change might be needed, but we do already error warning that the subscribed function is not a coroutine function

[FasterSpeeding]

Ngl I still don't really underarms why the check has to be there in the first place tbh, kinda goes against the library's normal approach of don't make the interface more restrictive or worse just for the sake of babying ppl imo

[davfsa]

The issue I see with not checking whether the function we get passed is a coroutine or not is that it leads to nasty errors later. I'll look into better typing or improving the errors later down the line (preferably the first)