Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ParamSpec in .asyncio() #143

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Conversation

dkang-quora
Copy link
Contributor

import asyncio

from asynq import asynq


@asynq()
def f(arg1: int, arg2: str):
    pass


async def main():
    await f.asyncio(1, "2")
    await f.asyncio(1, arg2="2")
    await f.asyncio(arg1=1, arg2="2")
    await f.asyncio(arg1="1", arg2=2)


asyncio.run(main())

pyright test.py reports

asynq/test.py
  asynq/test.py:15:26 - error: Argument of type "Literal['1']" cannot be assigned to parameter "arg1" of type "int" in function "asyncio"
    "Literal['1']" is incompatible with "int" (reportArgumentType)
  asynq/test.py:15:36 - error: Argument of type "Literal[2]" cannot be assigned to parameter "arg2" of type "str" in function "asyncio"
    "Literal[2]" is incompatible with "str" (reportArgumentType)
2 errors, 0 warnings, 0 informations

self,
*args: OriginalFunctionParams.args,
**kwargs: OriginalFunctionParams.kwargs,
) -> _Coroutine: ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loses the return type, use Coroutine[Any, Any, _T]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type checkers think _T is Generator because there is yield

ret1: int = await f.asyncio(1, "2")
  asynq/test.py:18:17 - error: Expression of type "Generator[AsyncTask[int], Unknown, Unknown]" is incompatible with declared type "int"
    "Generator[AsyncTask[int], Unknown, Unknown]" is incompatible with "int" (reportAssignmentType)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This is perhaps fixable with overloads: if the decorator is passed something returning a Generator[Any, Any, T], then this returns T, else it returns the return type directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems important to fix because otherwise we lose type checking for the object returned from the asynq function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to think more about edge cases. Can we merge this PR and make the return type a separate discussion?

@asynq()
def f1() -> Generator[Any, Any, int]:
    yield op1
    yield op2
    return 100

@asynq()
def f2() -> Generator[Any, Any, int]:
    return normal_generator_that_returns_int

(await f1.asyncio()) -> int
(await f2.asyncio()) -> Generator[Any, Any, int]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Case: the function body has yield (inspect.isgeneratorfunction = True)
    fn: Callable[..., Generator[?, ?, T]] -> return: T
  • Case: the function body no yield (inspect.isgeneratorfunction = False)
    fn: Callable[..., T] -> return: T

inspect.isgeneratorfunction cannot be expressed in type annotations.
Not sure if this kind of dynamic type checking would work:

return_type = TypeVar('T')

if TYPE_CHECKING:
    if inspect.isgeneratorfunction(fn):
        orig_fn: Callable[..., Generator[Any, Any, T]] = fn
        new_fn: Callable[... T] =...
        return new_fn 
    else:
        orig_fn: Callable[..., T] = fn
        new_fn: Callable[... T] =...
        return new_fn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can leave this for another PR. What you're suggesting won't work in a stub; I think we need to somehow overload __init__.

@JelleZijlstra JelleZijlstra merged commit 1bdfebb into quora:master Jul 16, 2024
9 of 10 checks passed
@dkang-quora dkang-quora deleted the paramspec branch July 16, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants