typecheck trio/_abc.py

[jakkdl]

No description provided.

[codecov]

Codecov Report

Merging #2703 (37149b3) into master (315dbed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2703   +/-   ##
=======================================
  Coverage   98.88%   98.88%           
=======================================
  Files         113      113           
  Lines       16482    16483    +1     
  Branches     3000     3000           
=======================================
+ Hits        16299    16300    +1     
  Misses        126      126           
  Partials       57       57           

Impacted Files	Coverage Δ
trio/_abc.py	100.00% <100.00%> (ø)

[TeamSpen210]

LGTM, though I'm not very experienced with sockets so someone else might want to check that bit.

[A5rocks]

This seems good! Discussion about what to do with _SocketType goes into #2705 so I think that's fine.

I'm also not too familiar with sockets so... not completely sure.

[A5rocks]

I went ahead and checked against typeshed just to make sure the socket types are correct!

My only concern is related to going from =None in the def socket(...) classmethod. Why was it done this way to begin with @njsmith?

[A5rocks]

Meh, I don't really feel comfortable changing away from the =None without knowing why exactly they're here in the first place. Additonally, in typeshed this is how a similar function is defined:

if sys.platform == "win32":
    def socketpair(family: int = ..., type: int = ..., proto: int = 0) -> tuple[socket, socket]: ...

else:
    def socketpair(
        family: int | AddressFamily | None = None, type: SocketType | int = ..., proto: int = 0
    ) -> tuple[socket, socket]: ...

[jakkdl]

Actually figuring out the value of the defaults required a bit of digging, and it took me a bit to be certain enough that they were static and not dynamic or something, given that pretty much all default values in typeshed is = .... I wouldn't have set the default values to anything but ... if not for typeright --verifytypes complaining when default values aren't explicit.

trio/trio/_socket.py

Lines 270 to 289 in 315dbed

	def socket(
	family=_stdlib_socket.AF_INET,
	type=_stdlib_socket.SOCK_STREAM,
	proto=0,
	fileno=None,
	):
	"""Create a new Trio socket, like :class:`socket.socket`.
	This function's behavior can be customized using
	:func:`set_custom_socket_factory`.
	"""
	if fileno is None:
	sf = _socket_factory.get(None)
	if sf is not None:
	return sf.socket(family, type, proto)
	else:
	family, type, proto = _sniff_sockopts_for_fileno(family, type, proto, fileno)
	stdlib_socket = _stdlib_socket.socket(family, type, proto, fileno)
	return from_stdlib_socket(stdlib_socket)

does have explicit logic for None though, so reverting the default value to None seems eminently plausible.

[jakkdl]

I'm also not too familiar with sockets so... not completely sure.

Given that we're (to the extent possible) only changing typing, and not any behaviour, I don't think we need to be 100% certain that all the types are correct and I expect some incorrect signatures to slip through, and/or existing weird behavior that surfaces because of previously untyped code now being typechecked, that will require followup PRs/releases.

[jakkdl]

The failing RTD build is due to _SocketType, which will be addressed in #2705

[A5rocks]

Seems good to me.