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

Please check our new types! #2775

Closed
A5rocks opened this issue Aug 26, 2023 · 22 comments · Fixed by #2853
Closed

Please check our new types! #2775

A5rocks opened this issue Aug 26, 2023 · 22 comments · Fixed by #2853
Labels
typing Adding static types to trio's interface

Comments

@A5rocks
Copy link
Contributor

A5rocks commented Aug 26, 2023

trio now has some level of typed-ness! Unfortunately, we did not just copy over trio-typing meaning the type hints are slightly different.

If your project:

  1. uses trio-typing or some custom type stubs (!?!)
  2. is typechecked against a recent trio version

then please help us check our new typed interface by checking your code against git+https://github.com/python-trio/trio and telling us your results here! I expect a couple minor-ish typing changes before releasing 0.23.0 (TypeVarTuples, #2761, and fixes for any type issues) but the current state of things should be final-ish.

@A5rocks A5rocks added the typing Adding static types to trio's interface label Aug 26, 2023
@A5rocks A5rocks pinned this issue Aug 26, 2023
@agronholm
Copy link
Contributor

I ran this against AnyIO and got 7 errors about unused type: ignore comments 👍

@A5rocks
Copy link
Contributor Author

A5rocks commented Aug 29, 2023

Checking against tricycle, it just looks like over-adaptation to trio-typing AND that _NurseryStartFunc isn't typed correctly:

(venv) PS C:\Users\A5rocks\Documents\Temp\tricycle> mypy --strict --implicit-reexport -p tricycle
tricycle\_service_nursery.py:4: error: Cannot find implementation or library stub for module named "trio_typing"  [import]
tricycle\_service_nursery.py:4: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
tricycle\_service_nursery.py:121: error: Unused "type: ignore" comment
tricycle\_service_nursery.py:128: error: Unused "type: ignore" comment
tricycle\_service_nursery.py:132: error: Argument 2 to "start" of "Nursery" has incompatible type "Callable[[NamedArg(Any, 'task_status')], Coroutine[Any, Any, None]]"; expected "_NurseryStartFunc[Any]"  [arg-type]
tricycle\_tests\test_tree_var.py:5: error: Cannot find implementation or library stub for module named "trio_typing"  [import]
tricycle\_tests\test_tree_var.py:96: error: Need type annotation for "target_nursery"  [var-annotated]
tricycle\_tests\test_tree_var.py:96: error: Argument 1 to "start" of "Nursery" has incompatible type "Callable[[Any], Coroutine[Any, Any, None]]"; expected "_NurseryStartFunc[<nothing>]"  [arg-type]
tricycle\_tests\test_tree_var.py:100: error: Argument 1 to "start" of "Nursery" has incompatible type "Callable[[str, DefaultNamedArg(Any, 'task_status')], Coroutine[Any, Any, None]]"; expected "_NurseryStartFunc[<nothing>]"  [arg-type]

(this is just the start of the output)

@jakkdl
Copy link
Member

jakkdl commented Aug 29, 2023

Checking against tricycle, it just looks like over-adaptation to trio-typing AND that _NurseryStartFunc isn't typed correctly:

(venv) PS C:\Users\A5rocks\Documents\Temp\tricycle> mypy --strict --implicit-reexport -p tricycle
tricycle\_service_nursery.py:4: error: Cannot find implementation or library stub for module named "trio_typing"  [import]
tricycle\_service_nursery.py:4: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
tricycle\_service_nursery.py:121: error: Unused "type: ignore" comment
tricycle\_service_nursery.py:128: error: Unused "type: ignore" comment
tricycle\_service_nursery.py:132: error: Argument 2 to "start" of "Nursery" has incompatible type "Callable[[NamedArg(Any, 'task_status')], Coroutine[Any, Any, None]]"; expected "_NurseryStartFunc[Any]"  [arg-type]
tricycle\_tests\test_tree_var.py:5: error: Cannot find implementation or library stub for module named "trio_typing"  [import]
tricycle\_tests\test_tree_var.py:96: error: Need type annotation for "target_nursery"  [var-annotated]
tricycle\_tests\test_tree_var.py:96: error: Argument 1 to "start" of "Nursery" has incompatible type "Callable[[Any], Coroutine[Any, Any, None]]"; expected "_NurseryStartFunc[<nothing>]"  [arg-type]
tricycle\_tests\test_tree_var.py:100: error: Argument 1 to "start" of "Nursery" has incompatible type "Callable[[str, DefaultNamedArg(Any, 'task_status')], Coroutine[Any, Any, None]]"; expected "_NurseryStartFunc[<nothing>]"  [arg-type]

(this is just the start of the output)

Ah no, that's an incorrect type hint that's getting fixed in #2773 (comment)

@apollo13
Copy link
Contributor

apollo13 commented Aug 31, 2023

I am running with pyright and it shows this error:

error: Object of type "Lock" cannot be used with "with" because it does not implement __aenter__ (reportGeneralTypeIssues)
error: Object of type "Lock" cannot be used with "with" because it does not implement __aexit__ (reportGeneralTypeIssues)

for this code (trio from master as of now):

import trio


async def test():
    async with trio.Lock():
        print("HI")


trio.run(test)

@A5rocks
Copy link
Contributor Author

A5rocks commented Aug 31, 2023

My first guess is that pyright doesn't support the self-type pattern we use in trio/_sync for async with, added in e97bcb6

If someone could confirm this with a small reproduction and make an issue on pyright's repository, I believe that will be fixed quickly.

EDIT: actually, no longer sure cause I couldn't make a simple reproduction of what I thought the bug was. I'll check this out later.

@apollo13
Copy link
Contributor

This is going way over my head. I played around a bit with it and replaced AsyncContextManagerMixin with the following:

class AsyncContextManagerMixin:
    #@enable_ki_protection
    async def __aenter__(self: _HasAcquireRelease) -> None:
        await self.acquire()
    __aenter__ = enable_ki_protection(__aenter__)

    #@enable_ki_protection
    async def __aexit__(
        self: _HasAcquireRelease,
        exc_type: type[BaseException] | None,
        exc_value: BaseException | None,
        traceback: TracebackType | None,
    ) -> None:
        self.release()
    __aexit__ = enable_ki_protection(__aexit__)        

so essentially I replaced @enable_ki_protection with __aexit__ = enable_ki_protection(__aexit__). Which should be equivalent?

Now pyright complains:

Could not bind method "__aenter__" because "Lock" is not assignable to parameter "self"
  "Lock" is incompatible with protocol "_HasAcquireRelease"
    "acquire" is an incompatible type
      Type "(**ArgsT) -> RetT" cannot be assigned to type "() -> Coroutine[Any, Any, object]"
        Function return type "RetT" is incompatible with type "Coroutine[Any, Any, object]"
          "object*" is incompatible with "Coroutine[Any, Any, object]"
Could not bind method "__aexit__" because "Lock" is not assignable to parameter "self"
  "Lock" is incompatible with protocol "_HasAcquireRelease"
    "acquire" is an incompatible type
      Type "(**ArgsT) -> RetT" cannot be assigned to type "() -> Coroutine[Any, Any, object]"
        Function return type "RetT" is incompatible with type "Coroutine[Any, Any, object]"
          "object*" is incompatible with "Coroutine[Any, Any, object]"

@agronholm
Copy link
Contributor

Why is self even annotated here?

@apollo13
Copy link
Contributor

According to the doc-string of _HasAcquireRelease to prevent misusing it (though I guess that goes a bit over the top):

class _HasAcquireRelease(Protocol):
    """Only classes with acquire() and release() can use the mixin's implementations."""

    async def acquire(self) -> object:
        ...

    def release(self) -> object:
        ...

@agronholm
Copy link
Contributor

I'm very uncomfortable with the amount of "defensive" code in Trio.

@GalaxySnail
Copy link
Contributor

The py.typed file is missing in wheels, I have created a pull request to fix it: #2780.

@A5rocks
Copy link
Contributor Author

A5rocks commented Aug 31, 2023

Why is self even annotated here?

Either we could annotate it like so or we could add type ignores (what we had before); might as well be type safe as this should be guaranteed to work (and if it doesn't that's indicative of a type checker bug somewhere).

I'm not sure why pyright is erroring though -- it looks like it's wrong about the Lock.acquire's value.

@TeamSpen210
Copy link
Contributor

It's not so much protection against misuse, but it's because the implementation there doesn't type check without it - since acquire() doesn't exist in that mixin class.

@apollo13
Copy link
Contributor

apollo13 commented Sep 1, 2023

And another weird one (the code is completely non-sensical, I just reduced it to the minimum):

import array
import socket

import trio


async def echo_server(server_stream: trio.SocketStream):
    fdarray = array.array("i", [server_stream.socket.fileno()])
    await server_stream.socket.sendmsg(
        [b"\x00"], [(socket.SOL_SOCKET, socket.SCM_RIGHTS, fdarray)]
    )

results in:

/tmp/test.py
  /tmp/test.py:10:9 - error: Argument of type "list[bytes]" cannot be assigned to parameter "self" of type "_SocketType" in function "__call__"
    "list[bytes]" is incompatible with "_SocketType" (reportGeneralTypeIssues)
  /tmp/test.py:10:21 - error: Argument of type "list[tuple[int, int, array[int]]]" cannot be assigned to parameter "__buffers" of type "Iterable[Buffer]" in function "__call__"
    "tuple[int, int, array[int]]" is incompatible with protocol "Buffer"
      "__buffer__" is not present (reportGeneralTypeIssues)
2 errors, 0 warnings, 0 informations 

it seems as if it thinks the first argument would be self; ie detects function vs method wrongly?

@jakkdl
Copy link
Member

jakkdl commented Sep 1, 2023

@apollo13 I'm having trouble reproducing that one, what platform/python version/flags/etc? Also, does it still happen in #2774?

@apollo13
Copy link
Contributor

apollo13 commented Sep 1, 2023

Platform is fedora F8, Python 3.11.4, Pyright 1.1.325. Pyright executed with no flags on the code above (that is the whole code. Will test #2774 later today.

@apollo13
Copy link
Contributor

apollo13 commented Sep 1, 2023

#2774 does not fix the reported issue from above.

@jakkdl
Copy link
Member

jakkdl commented Sep 1, 2023

Platform is fedora F8, Python 3.11.4, Pyright 1.1.325. Pyright executed with no flags on the code above (that is the whole code. Will test #2774 later today.

Ah, I was only testing with mypy - can reproduce with pyright. Will look into it and/or open an issue!

@jakkdl
Copy link
Member

jakkdl commented Sep 1, 2023

The error is due to functools.wraps usage in trio/_socket.py:

trio/trio/_socket.py

Lines 1002 to 1009 in d3255a0

@_wraps(_stdlib_socket.socket.sendmsg, assigned=(), updated=())
async def sendmsg(
self,
__buffers: Iterable[Buffer],
__ancdata: Iterable[tuple[int, int, Buffer]] = (),
__flags: int = 0,
__address: Address | None = None,
) -> int:

Type of "server_stream.socket.sendmsg" is "_Wrapped[Unknown, Unknown]"

it's maaaybe microsoft/pyright#4843 but it's not the same error, so I'm not sure.
(I also checked sendto and it's got the same error)

I'm not entirely sure what the wrapping is for in SocketType/_SocketType (copy docstrings?), or if we can get rid of it / work around it when type checking / achieve it in some other way / report upstream bugs in typeshed/pyright

@apollo13
Copy link
Contributor

apollo13 commented Sep 1, 2023

Replacing wraps with update_wrapper does indeed fix it.

I'm not entirely sure what the wrapping is for in SocketType/_SocketType (copy docstrings?), or if we can get rid of it / work around it when type checking / achieve it in some other way / report upstream bugs in typeshed/pyright

I'd also love to hear the usecase for @wraps here. With assigned=(), updated=() it literally updates nothing aside from setting __wrapped__ -- what does that give us?

@jakkdl
Copy link
Member

jakkdl commented Sep 2, 2023

Replacing wraps with update_wrapper does indeed fix it.

I'm not entirely sure what the wrapping is for in SocketType/_SocketType (copy docstrings?), or if we can get rid of it / work around it when type checking / achieve it in some other way / report upstream bugs in typeshed/pyright

I'd also love to hear the usecase for @wraps here. With assigned=(), updated=() it literally updates nothing aside from setting __wrapped__ -- what does that give us?

git blame says @njsmith added them, would you care to elucidate?

@A5rocks
Copy link
Contributor Author

A5rocks commented Sep 6, 2023

We could probably use update_wrapper instead then... (I assume that works identically?)

Setting __wrapped__ is useful for like making inspect things work as they should, so maybe that's why.

@A5rocks
Copy link
Contributor Author

A5rocks commented Oct 3, 2023

All reports in the issue should be fixed now, as far as I know.

Any more testing of types would be appreciated!

@A5rocks A5rocks unpinned this issue Nov 3, 2023
@A5rocks A5rocks linked a pull request Nov 3, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Adding static types to trio's interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants