Repository: django/asgiref

Issue:
`AsyncToSync` incorrectly passes arguments when called function arguments to internal wrapper
If the function being wrapped uses a kwarg that has the same name as the `AsyncToSync.main_wrap` function, it causes the execution to fail with `AsyncToSync.main_wrap() got multiple values for argument '<kwarg>'`.

How to reproduce - run the following example:

```python
from asgiref.sync import async_to_sync

@async_to_sync
async def example(context):
    pass

example(context=1)
```

Stacktrace:
```
Traceback (most recent call last):
  File "example.py", line 9, in <module>
    example(context=1)
  File "<venv>/asgiref/sync.py", line 215, in __call__
    awaitable = self.main_wrap(
                ^^^^^^^^^^^^^^^
TypeError: AsyncToSync.main_wrap() got multiple values for argument 'context'
```


I have tested this on latest `main`, on 3.8.1, 3.7.2, 3.7.1, and 3.7.0 versions of asgiref.
From my investigation, the culprit is #390. I tested on the commit before that PR (fd7a63c591653146379b55e68a29883b9bf7e706) and the bug does not reproduce. So the last working version which does not exhibit the bug is 3.6.0.

Hints from issue comments:
@andrewgodwin: Right, yes - the movement of the kwargs to actual `**kwargs` in order to make things type correctly would have indeed introduced this conflict.

I'd rather not undo the typing work so I suspect the only real approach here is to rename the kwargs in the `main_wrap` function to be less common - e.g. preceded by underscores.@andrewgodwin: Hmm, actually, on closer inspection I don't know why the kwargs were unpacked like that - I don't think it's necessary. Should be possible to revert it to the old way, I think.

@bellini666 Do you remember enough about this change to be able to comment either way?@Krismix1: I'll share some extra findings I had here, perhaps they will help.
I tried to change the signature of `main_wrap` from `*args: _P.args, **kwargs: _P.kwargs` to `args: _P.args, kwargs: _P.kwargs`, but my LSP (pyright) raises an error saying that `ParamSpec` must be used with the `*args` and `**kwargs` syntax. I am not familiar enough with the specification of ParamSpec, so I don't know if pyright is right or wrong. So it made me cautious about this approach.

We can also make use of `functools.partial` to generate a callable that takes no arguments and pass it to `main_wrap`. This removes the need to pass-through the arguments. As long as `main_wrap` does not need to modify the arguments (like for example injecting `self` or something), it may work.
```diff
diff --git a/asgiref/sync.py b/asgiref/sync.py
index 4427fc2..179bf48 100644
--- a/asgiref/sync.py
+++ b/asgiref/sync.py
@@ -212,13 +212,13 @@ class AsyncToSync(Generic[_P, _R]):
         # main event loop's thread if it's there, otherwise make a new loop
         # in this thread.
         try:
+            partial_fn = functools.partial(self.awaitable, *args, **kwargs)
             awaitable = self.main_wrap(
                 call_result,
                 sys.exc_info(),
                 task_context,
                 context,
-                *args,
-                **kwargs,
+                partial_fn,
             )
 
             if not (self.main_event_loop and self.main_event_loop.is_running()):
@@ -302,8 +302,7 @@ class AsyncToSync(Generic[_P, _R]):
         exc_info: "OptExcInfo",
         task_context: "Optional[List[asyncio.Task[Any]]]",
         context: List[contextvars.Context],
-        *args: _P.args,
-        **kwargs: _P.kwargs,
+        awaitable: Callable[[], Awaitable[_R]],
     ) -> None:
         """
         Wraps the awaitable with something that puts the result into the
@@ -326,9 +325,9 @@ class AsyncToSync(Generic[_P, _R]):
                 try:
                     raise exc_info[1]
                 except BaseException:
-                    result = await self.awaitable(*args, **kwargs)
+                    result = await awaitable()
             else:
-                result = await self.awaitable(*args, **kwargs)
+                result = await awaitable()
         except BaseException as e:
             call_result.set_exception(e)
         else:
```@andrewgodwin: OK, that confirms my suspicion of why it was changed, then - ParamSpec is slightly weird.

I don't see the reason not to wrap everything in a further partial except for potential performance concerns - we used to treat args and kwargs as opaque objects anyway, so I can't think of anything it would disrupt.

Do all the tests pass with your proposed change, and is the typechecker happy?@bellini666: Yes, as @Krismix1 discovered, the reason was the `ParamSpec`. It unfortunately requires `*args`/`**kwargs`.

I think the partial idea is the way to go, indeed, and I don't think it would have any performance implications as partials add close to nothing in terms of overhead (AFAIK they are even faster than lambdas)

![Screenshot 2024-09-26 at 11 04 11](https://github.com/user-attachments/assets/8a4e1935-7349-4486-9c72-e7cce3da4cfd)


Btw, my bad here for not noticing this at the time 🥲@Krismix1: @andrewgodwin yes, it passes the type checker and all tests, including a new test for this bug.@andrewgodwin: Let's get it in a PR and merge it then!@Krismix1: I'll prep a PR for it@Krismix1: PR #471 is up. I realized that we don't necessarily need to use `functools.partial`, as we could also just create the inner awaitable earlier. Tests pass, and I don't see any immediate downside to this approach.
But let me know if I missed something@andrewgodwin: I think that approach makes sense - merged it in! I'll give it a week for anyone who wants to early-test it then cut a release; if I forget and you remember, do remind me to!

Solution patch:
diff --git a/asgiref/sync.py b/asgiref/sync.py
index 4427fc2a..87ee4064 100644
--- a/asgiref/sync.py
+++ b/asgiref/sync.py
@@ -217,8 +217,12 @@ def __call__(self, *args: _P.args, **kwargs: _P.kwargs) -> _R:
                 sys.exc_info(),
                 task_context,
                 context,
-                *args,
-                **kwargs,
+                # prepare an awaitable which can be passed as is to self.main_wrap,
+                # so that `args` and `kwargs` don't need to be
+                # destructured when passed to self.main_wrap
+                # (which is required by `ParamSpec`)
+                # as that may cause overlapping arguments
+                self.awaitable(*args, **kwargs),
             )
 
             if not (self.main_event_loop and self.main_event_loop.is_running()):
@@ -302,8 +306,7 @@ async def main_wrap(
         exc_info: "OptExcInfo",
         task_context: "Optional[List[asyncio.Task[Any]]]",
         context: List[contextvars.Context],
-        *args: _P.args,
-        **kwargs: _P.kwargs,
+        awaitable: Union[Coroutine[Any, Any, _R], Awaitable[_R]],
     ) -> None:
         """
         Wraps the awaitable with something that puts the result into the
@@ -326,9 +329,9 @@ async def main_wrap(
                 try:
                     raise exc_info[1]
                 except BaseException:
-                    result = await self.awaitable(*args, **kwargs)
+                    result = await awaitable
             else:
-                result = await self.awaitable(*args, **kwargs)
+                result = await awaitable
         except BaseException as e:
             call_result.set_exception(e)
         else:


Test patch:
diff --git a/tests/test_sync.py b/tests/test_sync.py
index a4d2413b..0c67308c 100644
--- a/tests/test_sync.py
+++ b/tests/test_sync.py
@@ -7,6 +7,7 @@
 import warnings
 from concurrent.futures import ThreadPoolExecutor
 from functools import wraps
+from typing import Any
 from unittest import TestCase
 
 import pytest
@@ -1174,3 +1175,36 @@ async def async_task():
         assert task_complete
 
     assert task_executed
+
+
+def test_async_to_sync_overlapping_kwargs() -> None:
+    """
+    Tests that AsyncToSync correctly passes through kwargs to the wrapped function,
+    particularly in the case where the wrapped function uses same names for the parameters
+    as the wrapper.
+    """
+
+    @async_to_sync
+    async def test_function(**kwargs: Any) -> None:
+        assert kwargs
+
+    # AsyncToSync.main_wrap has a param named `context`.
+    # So we pass the same argument here to test for the error
+    # "AsyncToSync.main_wrap() got multiple values for argument '<kwarg>'"
+    test_function(context=1)
+
+
+@pytest.mark.asyncio
+async def test_sync_to_async_overlapping_kwargs() -> None:
+    """
+    Tests that SyncToAsync correctly passes through kwargs to the wrapped function,
+    particularly in the case where the wrapped function uses same names for the parameters
+    as the wrapper.
+    """
+
+    @sync_to_async
+    def test_function(**kwargs: Any) -> None:
+        assert kwargs
+
+    # SyncToAsync.__call__.loop.run_in_executor has a param named `task_context`.
+    await test_function(task_context=1)