typecheck _socket and _core._local #2705

jakkdl · 2023-07-14T16:08:58Z

This one's a bit messier, and I failed to resolve what to do with _SocketType. The stubs in typeshed for socket aren't the cleanest either, but I managed to mess around enough to satisfy pyright --verifytypes stringent requirements of unambigious and specify the actual default value - and I think/hope everything's correct, though no big harm if they aren't.

Typing RunVar with no_value = object() seemed not very possible, but I think this change shouldn't affect behaviour.

I'll do another push to figure out if there's any decent way of resolving autodoc, otherwise I'll just spam nitpick_ignore in these three PRs. _SocketType is the main one here that might merit documenting.

codecov · 2023-07-14T16:11:33Z

Codecov Report

Merging #2705 (975da8a) into master (2fd005b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2705   +/-   ##
=======================================
  Coverage   98.89%   98.89%           
=======================================
  Files         113      113           
  Lines       16552    16600   +48     
  Branches     3006     3014    +8     
=======================================
+ Hits        16369    16417   +48     
  Misses        126      126           
  Partials       57       57

Files Changed	Coverage Δ
trio/socket.py	`100.00% <ø> (ø)`
trio/_core/_local.py	`98.24% <100.00%> (+0.13%)`	⬆️
trio/_socket.py	`100.00% <100.00%> (ø)`
trio/_sync.py	`100.00% <100.00%> (ø)`
trio/_tests/test_socket.py	`99.83% <100.00%> (+<0.01%)`	⬆️
trio/_threads.py	`100.00% <100.00%> (ø)`

Fuyukai · 2023-07-14T18:26:17Z

Typing _socket makes it look legitimate and not something that needs to be put on a road and stomped in the head repeatedly.

TeamSpen210

Found a bunch of minor things. _SocketType seems tricky though, we wouldn't want to require users to use that private name for their annotations, but SocketType is empty so you can't do anything with it...

trio/_core/_local.py

trio/_sync.py

trio/_core/_local.py

trio/_threads.py

trio/_socket.py

TeamSpen210 · 2023-07-14T21:09:09Z

trio/_socket.py

        assert len(normed) == 4
+        # typechecking certainly doesn't like this logic, but given just how broad
+        # Address is, it's kind of impossible to write the below without type: ignore


This might be possible to type, if you define TypeGuard functions for the two len checks here, then change each branch to just build a whole tuple instead of converting to list (so we preserve the heterogeneity). Or unpack it into four variables.

Ooh, I didn't try with TypeGuard. Though I don't think it's terribly important to fix these lines, unless those typeguard functions can be reused elsewhere.

trio/_socket.py

oremanj · 2023-07-14T21:22:51Z

I think the issue with SocketType vs _SocketType is an excellent reason to use a stub file _socket.pyi here. These interfaces don't change often (almost at all, really) so I'm not as concerned about shear between the interface and the implementation.

The other option if you don't like that would be to make SocketType be an ABC with abstractmethods for all the public-facing socket methods. _SocketType has the implementations, but we only use the interface name SocketType in public-facing types.

trio/_socket.py

jakkdl · 2023-07-16T17:32:17Z

I think the issue with SocketType vs _SocketType is an excellent reason to use a stub file _socket.pyi here. These interfaces don't change often (almost at all, really) so I'm not as concerned about shear between the interface and the implementation.

The other option if you don't like that would be to make SocketType be an ABC with abstractmethods for all the public-facing socket methods. _SocketType has the implementations, but we only use the interface name SocketType in public-facing types.

I think I'll just leave _SocketType as is for this PR, and open an issue discussing possible solutions and then a PR dedicated for that. Making SocketType abstract with a bunch of abstract methods will probably break downstream code that inherits from it and just happens not to implement one of the methods made abstract.

trio/_socket.py

…_socket

A5rocks

I didn't look too much but this should be most of my comments.

A5rocks · 2023-07-20T22:39:51Z

trio/_core/_local.py

        """Sets the value of this :class:`RunVar` for this current run
        call.

        """
        try:
            old_value = self.get()
        except LookupError:
-            token = _RunVarToken.empty(self)
+            token = RunVarToken[T]._empty(self)


Huh, I feel like mypy should infer T.

I guess it doesn't backtrack from return token to determine token: RunVarToken[T]. Might be better to just add a variable annotation before the try block for it, then it should be able to infer.

Something something inheritance seems to break the typevar (or maybe because one is trio._socket.T and one is trio._util.T), so line 72 needs an explicit T (otherwise I get Incompatible types in assignment (expression has type "RunVarToken[T]", variable has type "RunVarToken[T]")) and a variable annotation before the try doesn't help. But I could remove the one at line 70.

trio/_socket.py

A5rocks · 2023-07-20T22:49:58Z

trio/_socket.py

@@ -178,9 +225,10 @@ def numeric_only_failure(exc):
            # idna.encode will error out if the hostname has Capital Letters
            # in it; with uts46=True it will lowercase them instead.
            host = _idna.encode(host, uts46=True)
-    hr = _resolver.get(None)
+    hr: HostnameResolver | None = _resolver.get(None)


I think these type hints should also be inferred?

yep, removing it has no effect. Though idk if having redundant explicit type hints is a bad thing.

trio/_socket.py

A5rocks · 2023-07-20T22:55:22Z

trio/_socket.py

+        return self._sock.getsockname()
+
+    @overload
+    def getsockopt(self, __level: int, __optname: int) -> int:


We dropped support for 3.7

I'm not sure what you're referring to here, I'm not finding what's changed and this is what's in typeshed.

3.8 introduced / pos arguments, so we can use that instead of the __arg hack.

Changed it in a bunch of places, but I failed to remove them from sendto, and from the ones where _make_simple_sock_method_wrapper is used to verify the signature.

A5rocks · 2023-07-20T22:58:10Z

trio/_socket.py


    ################################################################
    # sendto
    ################################################################

+    @overload
+    async def sendto(
+        self, __data: Buffer, __address: tuple[Any, ...] | str | Buffer


tuple[Any, ...]...?

taken from typeshed, I've been thinking I should improve them upstream.

Go ahead, typeshed loves contributions! (If you haven't submitted a PR there, it's an experience: they merge them so quickly!)

…_socket

TeamSpen210 · 2023-07-26T14:07:54Z

trio/_socket.py

+    ) -> None:
+        if optlen is None:
+            return self._sock.setsockopt(level, optname, cast("int|Buffer", value))
+        return self._sock.setsockopt(level, optname, cast(None, value), optlen)


Looks like we need a test using value = None. Instead of the casts here, perhaps it’d better to add an a check in each side of the branch to verify that value is or is not None, so that can be eliminated from the union. Better to be a raise instead of assert since it’s user code that would be wrong.

Implemented 👍

The first exception message is wrong, optlen isn’t set there?

…_socket

TeamSpen210

Aside from the sockopt message (and failing test on PyPy), everything looks good here.

jakkdl · 2023-07-28T11:44:29Z

I skimmed through socket documentation on windows and failed to find something that seemed interesting - might even be none, so I'll just skip testing that.

It looks like pypy doesn't support optlen at all. I feel like a comment at the call site that will pop up in the traceback should be good enough if it ever pops up that somebody runs pypy (uncommon) and tries to use the optlen argument (very uncommon - it wasn't even tested for in the trio tests previously). But could also wrap the call in a try/except. Typeshed doesn't make a distinction for pypy, nor do type checkers support guards using sys.implementation.name afaik

…_socket

A5rocks

Looks good to me other than these. I don't think I need to rereview again!

trio/_core/_local.py

A5rocks · 2023-07-29T01:28:33Z

trio/_socket.py

+    def listen(self, /, backlog: int = min(_stdlib_socket.SOMAXCONN, 128)) -> None:
+        return self._sock.listen(backlog)
+
+    def get_inheritable(self) -> bool:


I don't understand why some methods (e.g. listen, setsockopt) have pos-only self params, whereas some (e.g. get_inheritable, set_inheritable) don't.

Uh, neither listen nor setsockopt have pos-only self params? The only ones that do are ones that use __self to match typeshed.

I guess there's some inconsistency with whether methods use pos-only at all, but that inconsistency seems to be there in typeshed as well (set_inheritable doesn't have pos-only in typeshed) - and there were some mismatches previously (bind has pos-only address in typeshed, but previous versions of trio didn't, so I don't think I should change that and possibly break existing code).

A5rocks · 2023-07-29T01:30:06Z

trio/_socket.py

+    # args and kwargs must be starred, otherwise pyright complains:
+    # '"args" member of ParamSpec is valid only when used with *args parameter'
+    # '"kwargs" member of ParamSpec is valid only when used with **kwargs parameter'
+    # wait_fn and fn must also be first in the signature
+    # 'Keyword parameter cannot appear in signature after ParamSpec args parameter'
+


FWIW this isn't just a pyright thing as both these requirements come directly from the Parameter Specification PEP.

Huh, weird that mypy didn't complain about it then.

A5rocks · 2023-07-29T01:30:41Z

trio/_socket.py

    if TYPE_CHECKING:

-        async def recv(self, buffersize: int, flags: int = 0) -> bytes:
+        def recv(__self, __buflen: int, __flags: int = 0) -> Awaitable[bytes]:


Don't forget /!

A5rocks · 2023-07-29T01:31:12Z

trio/_socket.py

+    if TYPE_CHECKING:
+
+        def recv_into(
+            __self, buffer: Buffer, nbytes: int = 0, flags: int = 0


Another pos-only parameter than can use /: I won't comment on any others, as you can just ctrl+f "__"

As I mentioned above (but gosh, navigating discussion threads in long PR's are impossible):

Changed it in a bunch of places, but I failed to remove them from sendto, and from the ones where _make_simple_sock_method_wrapper is used to verify the signature.

As it checks if the signature is identical to typeshed it will require updating those. I'll add a comment in the code

Oh, I do have a comment for that already.

trio/trio/_socket.py

Lines 866 to 868 in 8a8b0b7

# _make_simple_sock_method_wrapper is typed, so this checks that the above is correct

# this requires that we refrain from using `/` to specify pos-only

# args, or mypy thinks the signature differs from typeshed.

A5rocks · 2023-07-29T01:32:26Z

trio/_socket.py


    ################################################################
    # sendto
    ################################################################

+    @overload
+    async def sendto(
+        self, __data: Buffer, __address: tuple[Any, ...] | str | Buffer


Go ahead, typeshed loves contributions! (If you haven't submitted a PR there, it's an experience: they merge them so quickly!)

trio/_sync.py

make `_NoValue` `Final` Co-authored-by: EXPLOSION <git@helvetica.moe>

You said you didn't need to re-review, and need to dismiss in order to be able to merge.

jakkdl · 2023-07-31T15:27:10Z

https://typing.readthedocs.io/en/latest/source/stubs.html#unsupported-features

Currently, the following features are not supported by all type checkers and should not be used in stubs:

Positional-only argument syntax (PEP 570 7). Instead, use the syntax described in the section Functions and Methods. 15

This might be why typeshed is doing __param, though idk if that note is up-to-date

jakkdl · 2023-07-31T15:55:47Z

and even more relevant: python/typeshed#10113

adding typehints to _socket and friends

83f22ec

jakkdl requested a review from TeamSpen210 July 14, 2023 16:08

TeamSpen210 requested changes Jul 14, 2023

View reviewed changes

TeamSpen210 added 2 commits July 15, 2023 10:38

Change fromfd() to use the correct type for file descriptors

ba5a6ee

This also needs to be guarded under TYPE_CHECKING

aa32d87

jakkdl commented Jul 16, 2023

View reviewed changes

trio/_socket.py Outdated Show resolved Hide resolved

jakkdl commented Jul 16, 2023

View reviewed changes

trio/_socket.py Show resolved Hide resolved

jakkdl added 4 commits July 17, 2023 10:01

revert changes for python <3.8

30e272b

fixes after review from TeamSpen210

399cce7

Merge remote-tracking branch 'origin/master' into typing_improvements…

32af4fc

…_socket

"fix" readthedocs build

dd828c2

A5rocks added the typing Adding static types to trio's interface label Jul 18, 2023

A5rocks mentioned this pull request Jul 18, 2023

typecheck trio/_abc.py #2703

Merged

jakkdl requested review from TeamSpen210 and A5rocks July 20, 2023 10:32

A5rocks requested changes Jul 20, 2023

View reviewed changes

jakkdl added 3 commits July 21, 2023 12:51

set merge strategy in .gitattributes

87a05be

Merge remote-tracking branch 'origin/master' into typing_improvements…

f7fee29

…_socket

fixes after review by A5rocks

06e004d

jakkdl requested a review from A5rocks July 21, 2023 13:52

python-trio deleted a comment from jakkdl Jul 21, 2023

jakkdl mentioned this pull request Jul 23, 2023

typecheck trio/_dtls.py #2704

Merged

jakkdl and others added 3 commits July 23, 2023 23:56

use / for pos-only args

2142252

Merge branch 'master' into typing_improvements_socket

576cb6d

Merge remote-tracking branch 'origin/master' into typing_improvements…

7f0911e

…_socket

TeamSpen210 reviewed Jul 26, 2023

View reviewed changes

jakkdl added 2 commits July 27, 2023 14:27

add test to setsockopt, update comments

831087f

Merge remote-tracking branch 'origin/master' into typing_improvements…

d8df8cc

…_socket

jakkdl requested a review from TeamSpen210 July 27, 2023 12:29

jakkdl and others added 3 commits July 27, 2023 17:50

fix tests on non-linux

37c7944

Merge remote-tracking branch 'origin/master' into typing_improvements…

c3f3c9d

…_socket

Fix missing sys import in test_socket

3972526

TeamSpen210 approved these changes Jul 28, 2023

View reviewed changes

jakkdl added 2 commits July 28, 2023 13:16

somewhat hackish cross-platform test

a06aee2

skip hacky test

5abc8ce

Merge remote-tracking branch 'origin/master' into typing_improvements…

8a8b0b7

…_socket

A5rocks previously requested changes Jul 29, 2023

View reviewed changes

jakkdl and others added 2 commits July 29, 2023 12:20

remove extraneous lines

22fa8df

Update trio/_core/_local.py

975da8a

make `_NoValue` `Final` Co-authored-by: EXPLOSION <git@helvetica.moe>

jakkdl enabled auto-merge (squash) July 29, 2023 10:29

jakkdl merged commit cf1f3c7 into python-trio:master Jul 29, 2023

jakkdl mentioned this pull request Jul 29, 2023

Handling _SocketType and SocketType #2720

Closed

jakkdl deleted the typing_improvements_socket branch July 29, 2023 13:42

	# _make_simple_sock_method_wrapper is typed, so this checks that the above is correct
	# this requires that we refrain from using `/` to specify pos-only
	# args, or mypy thinks the signature differs from typeshed.

typecheck _socket and _core._local #2705

typecheck _socket and _core._local #2705

Conversation

jakkdl commented Jul 14, 2023

codecov bot commented Jul 14, 2023 • edited Loading

Codecov Report

Fuyukai commented Jul 14, 2023

TeamSpen210 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oremanj commented Jul 14, 2023

jakkdl commented Jul 16, 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

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

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

Choose a reason for hiding this comment

TeamSpen210 left a comment • edited Loading

Choose a reason for hiding this comment

jakkdl commented Jul 28, 2023

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

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 Jul 31, 2023 • edited Loading

jakkdl commented Jul 31, 2023

codecov bot commented Jul 14, 2023 •

edited

Loading

jakkdl commented Jul 16, 2023 •

edited

Loading

TeamSpen210 left a comment •

edited

Loading

jakkdl commented Jul 31, 2023 •

edited

Loading