Add ParamSpec typing for AsyncToSync and SyncToAsync

[LucidDan]

This uses typing-extensions to add ParamSpec typing to the decorators and their related classes sync_to_async and async_to_sync.

The typing-extensions dependency needed to be extended to python 3.9, to provide backported support for ParamSpec.

Also found through some manual testing that the minimum mypy version that works with ParamSpec is 0.991. Maybe this means this PR needs to wait for at least an asgiref minor release, as it will affect the version of mypy people need to be using.

[LucidDan]

After doing the work, I realised there was at least one existing draft PR in #298 , albeit a couple of years old now. The approach we took seems pretty similar; I've included at least one change to my PR from the feedback in that PR's comments.

[LucidDan]

Worth noting; this probably won't pass all tests until #372 is fixed, so I've left it in Draft for now.
All good now. 👍

[LucidDan]

Feedback welcomed, I haven't done ParamSpec generics on a publicly used project before...I am not 100% sure if this will play well with users' deployments, but I've tested it with my own projects and seems okay so far.

Also, to link it into the original issue - this is to fix #270

[markedwards]

One comment that I think applies to both of these implementations — Coroutine[Any, Any, R] is better than Awaitable[R] for sync_to_async, because it will raise an error if the wrapped function is not awaited. This is not an error for an Awaitable, and that can lead to ugly bugs.

I’ve been using this to work around the lack of typing and it works well:

class SyncToAsyncCallable(Protocol):
    def __call__(
        self,
        func: Callable[P, R],
        thread_sensitive: bool = True,
        executor: Optional[ThreadPoolExecutor] = None,
    ) -> Callable[P, Coroutine[Any, Any, R]]: ...


sync_to_async: Final[SyncToAsyncCallable] = asgiref.sync.sync_to_async

[LucidDan]

One comment that I think applies to both of these implementations — Coroutine[Any, Any, R] is better than Awaitable[R] for sync_to_async, because it will raise an error if the wrapped function is not awaited. This is not an error for an Awaitable, and that can lead to ugly bugs.

Thanks for the suggestion Mark!
I've only used Callable[P, Awaitable[T]] in AsyncToSync, where I believe it makes sense, because you can absolutely pass a callable to it that returns some other type of awaitable object (for example, a Future, instead of a Coroutine object). For that case, I don't think it makes sense to narrow the type more than necessary, but I'm happy to get further input on that if I've missed something.

For sync_to_async/SyncToAsync, there's no Awaitable or Coroutine typing in my PR; there's no need, because the __call__ is an "async def", which makes it implicitly a Coroutine return type, so that's already covered.

Again, let me know if I'm missing something or misunderstood you here. Feel free to annotate the PR changes with specific comments if you want, as well.

[markedwards]

For sync_to_async/SyncToAsync, there's no Awaitable or Coroutine typing in my PR; there's no need, because the __call__ is an "async def", which makes it implicitly a Coroutine return type, so that's already covered.

Got it, that makes sense. Obviously I didn’t look closely enough at your diff yet. I’ll comment directly on it if I see anything.

[andrewgodwin]

Agreed with the idea that this will need to be a minor release due to the mypy dep change.

I've looked over it and it all seems to make sense to me; I've not dabbled with ParamSpec more than a few times, but the arguments being passed through without modification makes this simpler than the times I have.

My one question - is there a way to write a test for this? Obviously a standard unit test is not going to work, but something that fails the typechecker if it's not correct might be possible.

[LucidDan]

My one question - is there a way to write a test for this? Obviously a standard unit test is not going to work, but something that fails the typechecker if it's not correct might be possible.

Good question; I'd thought about this in passing but now you've prompted me to go looking- upon a little research found that Sobolevn wrote about it a while back and mentions complex generics specifically - https://sobolevn.me/2019/08/testing-mypy-types
Short tldr version - there is a pytest plugin that allows testing for expected error cases with mypy.

I've started experimenting with it, and already found some issues with the return types! I'll add some tests to the PR.

[LucidDan]

To sum up my findings so far:

• pytest-mypy-plugins is useful; I wish it worked in a python module style rather than YML definitions, but either way, it's a good fit for testing types on a python package. I think it could be quite useful in general, not just for this PR - there's a general example of this in tests/test_typing.yml, with a couple of tests for ASGI3Application.
• I've added a test spec (tests/test_sync_types.yml) to the PR that currently fails. I found that mypy fails to correctly pass through the paramspec and typevar through the SyncToAsync and AsyncToSync classes; I get errors indicating that the return type is 'Any'.
• When I tried to reproduce with a minimal test case (eg https://gist.github.com/mypy-play/f7005753d50af85ba6a9c7242291c448), it doesn't fail. It (asgiref) also doesn't fail in pylance, for what that's worth; VSCode and pylance correctly detect the return type and arguments for both sync_to_async and async_to_sync.

I'll take a further look in the next couple of days as time allows and try to figure out what the issue is.

And sorry, I might've made a mess of the review history due to a force-push or two while I was trying to wrangle the commit history.

[Kludex]

We do this on Starlette: https://github.com/encode/starlette/blob/master/tests/test_config.py#L12-L30

[LucidDan]

We do this on Starlette: https://github.com/encode/starlette/blob/master/tests/test_config.py#L12-L30

Thanks, that does avoid the extra dependency at least, I'll explore that as an alternative too.