Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners] #2774

jakkdl · 2023-08-25T15:44:29Z

Resolves #2720

Quite messy, and looks like there's some signatures in _socket.py that might warrant changing. Will write a summary when I've finished up test_highlevel_open_tcp_listeners

TODO:

test generic-ness raises type errors
document how users might want to approach the generic type

codecov · 2023-08-25T15:47:29Z

Codecov Report

Merging #2774 (a20a909) into master (4ee5413) will increase coverage by 0.12%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2774      +/-   ##
==========================================
+ Coverage   98.97%   99.10%   +0.12%     
==========================================
  Files         115      115              
  Lines       17117    17208      +91     
  Branches     3079     3084       +5     
==========================================
+ Hits        16942    17054     +112     
+ Misses        121      106      -15     
+ Partials       54       48       -6

Files	Coverage Δ
trio/_abc.py	`100.00% <ø> (ø)`
trio/_core/_io_epoll.py	`100.00% <100.00%> (ø)`
trio/_core/_io_kqueue.py	`87.20% <100.00%> (+1.48%)`	⬆️
trio/_dtls.py	`96.60% <100.00%> (ø)`
trio/_highlevel_open_tcp_listeners.py	`100.00% <ø> (ø)`
trio/_highlevel_open_tcp_stream.py	`97.64% <100.00%> (ø)`
trio/_highlevel_socket.py	`100.00% <ø> (ø)`
trio/_socket.py	`100.00% <100.00%> (ø)`
trio/_tests/test_highlevel_open_tcp_listeners.py	`98.46% <100.00%> (+0.13%)`	⬆️
trio/_tests/test_highlevel_open_tcp_stream.py	`100.00% <100.00%> (ø)`
... and 5 more

... and 1 file with indirect coverage changes

…queue to use _HasFileNo

jakkdl · 2023-08-29T14:53:55Z

Hm, looking at https://docs.python.org/3/library/socket.html#socket-families I now understand why methods like recv have Any in their return type when they're returning a socket address. Might warrant changing a couple signatures to use Any and/or object

CoolCat467

Looks good other than a few formatting things and a question about Any vs object usage. I also think it would be good to annotate the __init__ return types while we are at it so we don't have to do it later.

trio/_highlevel_open_tcp_stream.py

trio/_socket.py

runtime

jakkdl · 2023-09-02T11:14:35Z

I also think it would be good to annotate the __init__ return types while we are at it so we don't have to do it later.

I have repeatedly asked you why you think this, while giving you explanations why it doesn't. It does not matter for pyright --verifytypes and it does not matter for mypy. Would you please elaborate, and show it with a reproducible sample + program invocation, so we can settle this?
They don't hurt in any way, but I don't see any reason to add them, and I have yet to find any check that can be turned on in any type-checker that flags either, when you have parameters that are typed.

CoolCat467

Looks good to me, but I would replace the Address -> Any change with Address -> AddressFormat alias. One of the review comments has more details why I think this should be done.

trio/socket.py

trio/_highlevel_open_tcp_stream.py

trio/_dtls.py

trio/_tests/test_highlevel_open_tcp_stream.py

trio/testing/_fake_net.py

jakkdl

uh, turns out I had a very old review-in-progress and it's a mess to find where I commented on what, so I'll submit this now and it might be outdated.

docs/source/reference-io.rst

jakkdl · 2023-08-29T14:19:34Z

trio/_abc.py

    @abstractmethod
    def socket(
        self,
-        family: socket.AddressFamily | int | None = None,
-        type: socket.SocketKind | int | None = None,
-        proto: int | None = None,
-    ) -> _SocketType:
+        family: socket.AddressFamily | int = socket.AF_INET,
+        type: socket.SocketKind | int = socket.SOCK_STREAM,
+        proto: int = 0,
+    ) -> SocketType:
        """Create and return a socket object.

        Your socket object must inherit from :class:`trio.socket.SocketType`,


I didn't want these types to be | None, as that affects the signature of all subclasses of SocketFactory. But this is a change in behaviour, so it might be better to just do a type: ignore

I think having them default to the values they would have became originally is a good thing

I think this is fine. I approach type hints as not bound by semver (i.e. can narrow them or expand them as long as we're not passing something with a new type in) and subclasses can still default to None. Hopefully that approach is sane...

the issue is not the type hint, but the default value being changed from None -> socket.AF_INET etc etc.
although thinking about it a bit more, I'm having trouble coming up with any code that actually changes in behaviour from this. If you're subclassing SocketFactory and defining your own SocketFactory.socket, the default values in SocketFactory.socket shouldn't modify the behaviour of MyClass.socket in any way .. I think?

trio/_socket.py

jakkdl · 2023-09-20T13:56:56Z

I also think it would be good to annotate the __init__ return types while we are at it so we don't have to do it later.

I have repeatedly asked you why you think this, while giving you explanations why it doesn't. It does not matter for pyright --verifytypes and it does not matter for mypy. Would you please elaborate, and show it with a reproducible sample + program invocation, so we can settle this? They don't hurt in any way, but I don't see any reason to add them, and I have yet to find any check that can be turned on in any type-checker that flags either, when you have parameters that are typed.

@CoolCat467

A5rocks · 2023-09-20T14:46:33Z

(It might just be cause I haven't fully woken up yet, but quoted message feels a little bit too aggressive... it's alright and can stay since it's obviously not meant to be such: just for CoolCat, don't take it in an aggressive way ^^)

I'll make sure to actually review this in a while!! Sorry about the delay.

CoolCat467

With the issues with AddressFormat explained, I think this looks good!

CoolCat467 · 2023-09-21T02:09:14Z

I also think it would be good to annotate the __init__ return types while we are at it so we don't have to do it later.

I have repeatedly asked you why you think this, while giving you explanations why it doesn't. It does not matter for pyright --verifytypes and it does not matter for mypy. Would you please elaborate, and show it with a reproducible sample + program invocation, so we can settle this? They don't hurt in any way, but I don't see any reason to add them, and I have yet to find any check that can be turned on in any type-checker that flags either, when you have parameters that are typed.

@CoolCat467

# example.py

class ClassName:
    def __init__(self):
        pass

dmypy run "example.py" -- --disallow-untyped-defs

yields the following:

Daemon started
example.py:4: error: Function is missing a return type annotation  [no-untyped-def]
example.py:4: note: Use "-> None" if function does not return a value
Found 1 error in 1 file (checked 40 source files)

TeamSpen210 · 2023-09-21T02:31:48Z

That's a special case where -> None must be used, since the __init__() method has no parameters defined at all other than self. Try def __init__(self, a: int, b: str): and you'll see there should be no complaints. Alternatively perhaps it's a problem with daemon mode?

CoolCat467 · 2023-09-21T03:01:13Z

That's a special case where -> None must be used, since the __init__() method has no parameters defined at all other than self. Try def __init__(self, a: int, b: str): and you'll see there should be no complaints. Alternatively perhaps it's a problem with daemon mode?

It indeed does not raise errors when it has arguments in dmypy 1.5.1, but I feel like this was not always the case. I always make sure it has the return type annotated mostly because I don't like the idea of special cases. Everything else has the return types annotated, so why shouldn't this? And plus, then any code that looks at the __annotations__ of it has to have special exceptions, and things aren't nearly as clean.

Confirming this is one of the comments in PEP-484:

(Note that the return type of __init__ ought to be annotated with -> None. The reason for this is subtle. If __init__ assumed a return annotation of -> None, would that mean that an argument-less, un-annotated __init__ method should still be type-checked? Rather than leaving this ambiguous or introducing an exception to the exception, we simply say that __init__ ought to have a return annotation; the default behavior is thus the same as for other methods.)

jakkdl · 2023-09-21T11:38:29Z

I am sorry for the overly aggressive message, but I did get a bit frustrated about it having come up several times :) 1 2 3 4

pyright:
https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#type-completeness

Type annotations can be omitted in a few specific cases where the type is obvious from the context:
...
The return type for an __init__ method does not need to be specified, since it is always None.

mypy:

https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods

The __init__ method is somewhat special – it doesn’t return a value. This is best expressed as -> None. However, since many feel this is redundant, it is allowed to omit the return type declaration on __init__ methods if at least one argument is annotated.

The comment in PEP-484 is specifically about "whether a type checker should check the function, or treat it as an untyped function and ignore the content", and not any confusion about what the return type actually is.
This is the norm in the most widely used type checkers, and in all projects I know of (as it isn't even possible to enable a warning for not having it!) so anybody inspecting __annotations__ for whatever reason would have to special-case this behavior anyway.

(returning non-None will also raise a TypeError: https://docs.python.org/3/reference/datamodel.html?highlight=init#object.__init__ )

A5rocks

I didn't review the tests yet but generally I'm just gonna trust you're doing the right thing 😅

A5rocks · 2023-09-22T05:11:28Z

trio/_abc.py

    @abstractmethod
    def socket(
        self,
-        family: socket.AddressFamily | int | None = None,
-        type: socket.SocketKind | int | None = None,
-        proto: int | None = None,
-    ) -> _SocketType:
+        family: socket.AddressFamily | int = socket.AF_INET,
+        type: socket.SocketKind | int = socket.SOCK_STREAM,
+        proto: int = 0,
+    ) -> SocketType:
        """Create and return a socket object.

        Your socket object must inherit from :class:`trio.socket.SocketType`,


I think this is fine. I approach type hints as not bound by semver (i.e. can narrow them or expand them as long as we're not passing something with a new type in) and subclasses can still default to None. Hopefully that approach is sane...

trio/_core/_io_kqueue.py

trio/_socket.py

A5rocks · 2023-09-22T05:18:39Z

trio/_socket.py

+        not TYPE_CHECKING and hasattr(_stdlib_socket.socket, "share")
+    ):
+
+        def share(self, /, process_id: int) -> bytes:


This coverage warning here also doesn't make sense given we're running the tests on Windows...?

Yeah no clue :<

on a rerun there were no coverage warning, so seems like a temporary glitch.

trio/_socket.py

This reverts commit 7335184.

jakkdl · 2023-09-26T11:59:00Z

uh, thought for a minute that _public could be skipped in coverage ... but since the implementation should get called by the generated files that's certainly not the case.

jakkdl added 2 commits August 23, 2023 18:53

wip

fbae13d

wip2

1a38b7e

A5rocks added the typing Adding static types to trio's interface label Aug 26, 2023

jakkdl mentioned this pull request Aug 26, 2023

add types to io_windows #2761

Merged

jakkdl added 4 commits August 28, 2023 17:07

hide methods in SocketType behind a TYPE_CHECKING guard, update _io_k…

201da1a

…queue to use _HasFileNo

Merge remote-tracking branch 'origin/master' into typing_sockettype

6cd3da7

remove unneeded pragma: no cover

c8fc73c

fix default parameters

e3fc6ae

jakkdl marked this pull request as ready for review August 29, 2023 14:17

jakkdl changed the title ~~[WIP] Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners]~~ Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners] Aug 29, 2023

jakkdl requested review from A5rocks, CoolCat467 and TeamSpen210 and removed request for CoolCat467 August 29, 2023 14:56

CoolCat467 requested changes Aug 30, 2023

View reviewed changes

trio/_highlevel_open_tcp_stream.py Outdated Show resolved Hide resolved

trio/_socket.py Outdated Show resolved Hide resolved

jakkdl mentioned this pull request Sep 1, 2023

Please check our new types! #2775

Closed

jakkdl added 2 commits September 2, 2023 12:25

Merge remote-tracking branch 'origin/master' into typing_sockettype

15e8351

raise NotImplementedError instead of methods not being defined at

d60d1a1

runtime

jakkdl added 4 commits September 2, 2023 13:18

remove docs reference

b213602

experimentally changing SocketType to be a generic

7eb099d

fix url

ab66937

fix old imports

304fd32

jakkdl mentioned this pull request Sep 9, 2023

Replace public usages of functools.wraps #2796

Merged

jakkdl added 2 commits September 16, 2023 12:05

Merge remote-tracking branch 'origin/master' into typing_sockettype

c36958c

fix verify_types

920d663

jakkdl requested a review from CoolCat467 September 16, 2023 10:07

jakkdl added 2 commits September 16, 2023 14:26

reverse making SocketType generic, CI fixes

a3169cc

fix verifytypes

b9bbc06

CoolCat467 requested changes Sep 17, 2023

View reviewed changes

TeamSpen210 approved these changes Sep 18, 2023

View reviewed changes

trio/_tests/test_highlevel_open_tcp_stream.py Outdated Show resolved Hide resolved

trio/testing/_fake_net.py Outdated Show resolved Hide resolved

jakkdl commented Sep 20, 2023

View reviewed changes

jakkdl added 2 commits September 20, 2023 15:57

fix suggestions from teamspen

76ab994

Merge remote-tracking branch 'origin/master' into typing_sockettype

ea36fe8

CoolCat467 approved these changes Sep 21, 2023

View reviewed changes

A5rocks reviewed Sep 22, 2023

View reviewed changes

CoolCat467 and others added 6 commits September 22, 2023 19:16

Update verify_types

0c294ea

clarifying comments after a5rocks review

dc09e7e

Merge remote-tracking branch 'origin/master' into typing_sockettype

11cfd4e

fix verify_types ............

d934724

add @_public to exclude_lines in coveragerc

7335184

Revert "add @_public to exclude_lines in coveragerc"

618fdbf

This reverts commit 7335184.

'pragma: no cover' an added type-converting line

a20a909

TeamSpen210 approved these changes Sep 28, 2023

View reviewed changes

jakkdl merged commit 24226c3 into python-trio:master Sep 29, 2023

jakkdl deleted the typing_sockettype branch September 29, 2023 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners] #2774

Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners] #2774

jakkdl commented Aug 25, 2023 •

edited

Loading

codecov bot commented Aug 25, 2023 •

edited

Loading

jakkdl commented Aug 29, 2023

CoolCat467 left a comment

jakkdl commented Sep 2, 2023 •

edited by smurfix

Loading

CoolCat467 left a comment

jakkdl left a comment

jakkdl Aug 29, 2023

CoolCat467 Sep 21, 2023

A5rocks Sep 22, 2023

jakkdl Sep 22, 2023

jakkdl commented Sep 20, 2023

A5rocks commented Sep 20, 2023

CoolCat467 left a comment

CoolCat467 commented Sep 21, 2023

TeamSpen210 commented Sep 21, 2023

CoolCat467 commented Sep 21, 2023

jakkdl commented Sep 21, 2023 •

edited

Loading

A5rocks left a comment

A5rocks Sep 22, 2023

A5rocks Sep 22, 2023

jakkdl Sep 22, 2023

jakkdl Sep 26, 2023

jakkdl commented Sep 26, 2023

Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners] #2774

Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners] #2774

Conversation

jakkdl commented Aug 25, 2023 • edited Loading

codecov bot commented Aug 25, 2023 • edited Loading

Codecov Report

jakkdl commented Aug 29, 2023

CoolCat467 left a comment

Choose a reason for hiding this comment

jakkdl commented Sep 2, 2023 • edited by smurfix Loading

CoolCat467 left a comment

Choose a reason for hiding this comment

jakkdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Sep 20, 2023

A5rocks commented Sep 20, 2023

CoolCat467 left a comment

Choose a reason for hiding this comment

CoolCat467 commented Sep 21, 2023

TeamSpen210 commented Sep 21, 2023

CoolCat467 commented Sep 21, 2023

jakkdl commented Sep 21, 2023 • edited Loading

A5rocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Sep 26, 2023

jakkdl commented Aug 25, 2023 •

edited

Loading

codecov bot commented Aug 25, 2023 •

edited

Loading

jakkdl commented Sep 2, 2023 •

edited by smurfix

Loading

jakkdl commented Sep 21, 2023 •

edited

Loading