Add type annotations for `_ssl.py` by CoolCat467 · Pull Request #2745 · python-trio/trio

CoolCat467 · 2023-08-07T03:34:25Z

This PR adds type annotations to _ssl.py

Three interesting thins I noticed when making sure the type annotations were good:

There was a type annotation bug in trio.abc.Listener not using its T_resource generic in accept, now fixed
Mypy says SSLStream.receive_some could have potentially returned None if my typing for SSLStream._retry is correct. I don't think this is entirely true, I think everything is supposed to raise a trio.BrokenResourceError exception, but if it raised something else it could have returned None. There is a type-narrowing assert not None there now, but I am curious if anyone has more information.
SSLStream.unwrap sets self.transport_stream to None, and nothing else checks to make sure it's not None anywhere. I assume this is ok since it also sets the internal state to CLOSED, so I put a type ignore there to silence the error. The reason I hesitate to allow self.transport_stream to be None is because the docstring says it's required to be a Stream object.

I would love feedback and any comments on how this could be improved to be more accurate.

codecov · 2023-08-07T03:36:46Z

Codecov Report

Merging #2745 (3020001) into master (3bdab93) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2745   +/-   ##
=======================================
  Coverage   98.94%   98.94%           
=======================================
  Files         113      113           
  Lines       16837    16843    +6     
  Branches     3036     3036           
=======================================
+ Hits        16659    16665    +6     
  Misses        123      123           
  Partials       55       55

Files Changed	Coverage Δ
trio/_abc.py	`100.00% <100.00%> (ø)`
trio/_ssl.py	`100.00% <100.00%> (ø)`

trio/_ssl.py

A5rocks · 2023-08-08T03:36:47Z

Oops, didn't mean to click single comment. I think that + moving the __slots__ to your specialized PR for that would be neat.

jakkdl

Guess which file needs to be updated...? pyproject.toml!

Does your setup not use it for enabling checks? In my workflow I first need to update pyproject.toml in order to be able to get the errors and see what needs fixing.

Otherwise mostly just nits

jakkdl · 2023-08-08T12:25:32Z

trio/_ssl.py

+        server_hostname: str | None = None,
+        server_side: bool = False,
+        https_compatible: bool = False,
+    ) -> None:


nit: inconsequential -> None. We ought to create a style guide somewhere for typing, and/or add a check to #2744 that removes/adds it.

It is not inconsequential, I am fairly certain that verify_types.json changes for the better when it's there

Are you referring to #2740 (comment) ?
I removed the -> None without any impact on verify_types.json

Just bumping on this!

trio/_ssl.py

trio/_tests/verify_types.json

trio/_ssl.py

jakkdl · 2023-08-08T12:40:45Z

trio/_ssl.py

    }

-    def __getattr__(self, name):
+    def __getattr__(self, name: str) -> Any:


I suppose there's a case for returning object here to tell the user they need to do isinstance. But they can also enable disallow_any_expression I suppose.
I tried finding an authoritative best-practice of what to do in this case, but no dice

CoolCat467 · 2023-08-08T15:50:16Z

How should this issue with readthedocs be addressed?

/home/docs/checkouts/readthedocs.org/user_builds/trio/checkouts/2745/trio/abc.py:docstring of trio.abc.Listener.accept:1: WARNING: py:class reference target not found: trio._abc.T_resource

TeamSpen210 · 2023-08-08T20:42:12Z

How should this issue with readthedocs be addressed?

/home/docs/checkouts/readthedocs.org/user_builds/trio/checkouts/2745/trio/abc.py:docstring of trio.abc.Listener.accept:1: WARNING: py:class reference target not found: trio._abc.T_resource

You've got to add it to the ignore list in docs/conf.py, rather annoying.

jakkdl

oops nvm, the ambiguous type is still not resolved

A5rocks

Seems mostly good, just a couple things that jumped out at me (most aren't even typing-related).

pyproject.toml

trio/_ssl.py

jakkdl

+1 to a5rock on leaving __slots__ to separate PR as they've been controversial, otherwise seems ready to merge.

trio/_ssl.py

A5rocks

Just a couple comments.

trio/_ssl.py

…typing-ssl

This reverts commit 3406eaf.

Apparently already being handled in python-trio#2756

This reverts commit 40e9bf2.

A5rocks

Just some comments!

trio/_ssl.py

A5rocks · 2023-08-20T02:34:30Z

trio/_ssl.py

+        server_hostname: str | None = None,
+        server_side: bool = False,
+        https_compatible: bool = False,
+    ) -> None:


Just bumping on this!

A5rocks · 2023-08-20T02:35:26Z

trio/_ssl.py

            self._outgoing,
            server_side=server_side,
-            server_hostname=server_hostname,
+            server_hostname=server_hostname,  # type: ignore[arg-type]  # Typeshed bug, does accept bytes as well (typeshed#10590)


Looks like that PR was merged, meaning next mypy release should support this! Nice!

A5rocks · 2023-08-20T02:36:17Z

trio/_ssl.py

-        https_compatible=False,
-    ):
+        https_compatible: bool = False,
+    ) -> None:


FWIW this -> None is probably also unnecessary.

Yeah, I haven't seen any reason why it would change anything.

jakkdl · 2023-08-21T11:25:10Z

merging main ...

$ tox -e verifytypes
******************** 
Checking type completeness hasn't gone down...
creating py.typed
completenessScore has gone up from 0.9904458598726115 to 1
withUnknownType has gone down from 6 to 0
withKnownType has gone up from 622 to 628
Traceback (most recent call last):
  File "trio/_tests/check_type_completeness.py", line 190, in <module>
    sys.exit(main(args))
  File "trio/_tests/check_type_completeness.py", line 131, in main
    assert (
AssertionError: Fully type complete! Delete this script and instead directly run `pyright --verifytypes=trio` (consider `--ignoreexternal`) in CI and checking exit code.

🚀 🚀

although it seems to think some symbols that are in fact public aren't, so it's partly a lie

# Conflicts: # pyproject.toml # trio/_tests/verify_types.json

Somehow this did not get in to edbf981

09c5761

Add type annotations to _ssl.py

d9a7e24

CoolCat467 force-pushed the typing-ssl branch from ae00627 to d9a7e24 Compare August 7, 2023 03:38

CoolCat467 marked this pull request as ready for review August 7, 2023 03:38

CoolCat467 mentioned this pull request Aug 6, 2023

Remaining tasks until --verifytypes passes and check_type_completeness.py can be removed #2734

Closed

41 tasks

CoolCat467 added the typing Adding static types to trio's interface label Aug 7, 2023

CoolCat467 added 5 commits August 6, 2023 22:53

Fix for pre-3.10

abcb7cd

Merge branch 'python-trio:master' into typing-ssl

aaaea9b

Fix CI issues

81d8d85

Update verify_types.json

067527a

Remove unused imports

985acca

A5rocks reviewed Aug 8, 2023

View reviewed changes

trio/_ssl.py Outdated Show resolved Hide resolved

jakkdl requested changes Aug 8, 2023

View reviewed changes

CoolCat467 and others added 4 commits August 8, 2023 09:45

Merge branch 'master' into typing-ssl

11842a9

Change lots of Any to object

edbf981

Update verify_types.json

4a92a2d

Add _ssl to stricker check block and sort modules

564bb92

Add trio._abc.T_resource to doc ignore list

ce776ac

CoolCat467 requested a review from jakkdl August 9, 2023 02:36

jakkdl approved these changes Aug 9, 2023

View reviewed changes

jakkdl requested changes Aug 9, 2023

View reviewed changes

CoolCat467 added 2 commits August 9, 2023 22:18

Attempt to fix pyright issues

c896899

Update verify_types.json

93333bc

CoolCat467 requested a review from jakkdl August 10, 2023 03:37

A5rocks requested changes Aug 10, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

trio/_ssl.py Show resolved Hide resolved

trio/_ssl.py Outdated Show resolved Hide resolved

trio/_ssl.py Outdated Show resolved Hide resolved

jakkdl requested changes Aug 14, 2023

View reviewed changes

CoolCat467 added 3 commits August 14, 2023 12:29

Merge branch 'master' into typing-ssl

1be18a3

Remove __slots__

49c2c1b

Update verify_types.json

cbee73e

CoolCat467 requested review from A5rocks and jakkdl August 14, 2023 17:39

TeamSpen210 approved these changes Aug 15, 2023

View reviewed changes

TeamSpen210 reviewed Aug 15, 2023

View reviewed changes

trio/_ssl.py Outdated Show resolved Hide resolved

A5rocks reviewed Aug 16, 2023

View reviewed changes

trio/_ssl.py Outdated Show resolved Hide resolved

trio/_ssl.py Outdated Show resolved Hide resolved

CoolCat467 added 8 commits August 15, 2023 23:30

Merge branch 'typing-ssl' of https://github.com/CoolCat467/trio into …

b8f18ee

…typing-ssl

Change to object (@A5rocks suggestion)

acfcead

Fix server_hostname

31b0ccf

Update verify_types.json

40e9bf2

Add _highlevel_ssl_helpers to stricter checks block

3406eaf

Revert "Add _highlevel_ssl_helpers to stricter checks block"

38c9186

This reverts commit 3406eaf.

Revert changes to _highlevel_ssl_helpers

a583326

Apparently already being handled in python-trio#2756

Revert "Update verify_types.json"

bf88b26

This reverts commit 40e9bf2.

CoolCat467 requested a review from A5rocks August 16, 2023 05:21

jakkdl approved these changes Aug 17, 2023

View reviewed changes

CoolCat467 added 2 commits August 19, 2023 21:09

Merge branch 'master' into typing-ssl

beaf82d

Update verify_types.json and fix line endings

b6e7303

A5rocks reviewed Aug 20, 2023

View reviewed changes

CoolCat467 added 2 commits August 19, 2023 22:05

Remove slots from exception

cf7a777

Remove _ssl from not fully typed block

e791683

Merge remote-tracking branch 'origin/master' into typing-ssl

3020001

# Conflicts: # pyproject.toml # trio/_tests/verify_types.json

jakkdl enabled auto-merge (squash) August 21, 2023 11:28

jakkdl merged commit f9fdeb7 into python-trio:master Aug 21, 2023

CoolCat467 deleted the typing-ssl branch August 22, 2023 21:03

jakkdl mentioned this pull request Sep 21, 2023

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

Merged

Uh oh!

Conversation

CoolCat467 commented Aug 7, 2023

Uh oh!

codecov bot commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

A5rocks commented Aug 8, 2023

Uh oh!

jakkdl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoolCat467 commented Aug 8, 2023

Uh oh!

TeamSpen210 commented Aug 8, 2023

Uh oh!

jakkdl left a comment

Choose a reason for hiding this comment

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakkdl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakkdl commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Aug 7, 2023 •

edited

Loading

jakkdl commented Aug 21, 2023 •

edited

Loading