Type Hint asyncssh

[cooperlees]

Low Pri

Since this is a totally Python 3 only code base, how about we propose type hinting all of asyncssh?
For more info:
http://mypy.readthedocs.io/en/latest/cheat_sheet_py3.html
https://docs.python.org/3/library/typing.html

It's seeming Mypy is becoming the standard in static type analysis for Python.

Thoughts?

[ronf]

Funny you should mention this, as I've actually had a tab open in my browser to http://blog.zulip.org/2016/10/13/static-types-in-python-oh-mypy/ for some time now and I've been meaning to check out mypy.

In general, I'm in favor of the idea, but there are a number of places in AsyncSSH right now that take a large variety of types, where the functions try to do the "right thing" and figure out what the caller meant and automatically convert the argument into the right form. I know it's possible to do this using Union, or just leave out the types entirely in places where it gets too complicated to express, but I'll need to experiment a bit to try and find the right balance of adding types without cluttering up the code too much.

[cooperlees]

Awesome. Types will help this Python code style moving forward that we all abuse. I've been finding this a LOT with my own code too!

I vote we start with less 'strict' typing (e.g. more loose Unions + Any) and just identify some places that might benefit from a light refactor.

The good thing about mypy is it will identify the problem functions and types you're not catering for 👍 .

[ronf]

Yeah, makes sense.

I'm currently looking into adding support for X.509 certificates, which is one of the last big missing features. That's turning into a bit of a monster, but once I got that checked in mypy is one of the top things remaining, along with adding debug logging.

[paravoid]

Not sure if this is something you're still planning to do, but if you haven't already check out MonkeyType. It can automatically draft type annotations by tracing the execution of a piece of code (e.g. the test suite).

[ronf]

Thanks for the pointer - I'll check it out.

I made a start on this some time ago, and have good type information for a little under half of the modules. However, I wasn't entirely happy with how the resulting code looked. It was fine for simple functions (like the crypto subdirectory), but there were a number of places where I intentionally allowed a wide variety of types to be provided, and it got to be a real chore to build complex union types to handle all the possible variants. Also, to make mypy happy, I ended up having to add something like 1,000 lines of additional stubs for cryptography and other external modules that didn't have good type coverage yet, and I wasn't even half done.

I thought about doing something where I only added type information for the simple cases and left some of the more complex ones untyped, but even that wouldn't really help with the stubs issue. All in all, I ended up thinking it might be better if I wait for mypy to be a bit more mature, and for more inclusion of type information in other modules (or in typeshed, at least).

[sebastian-philipp]

Still getting Cannot find implementation or library stub for module named "asyncssh".

[ronf]

I abandoned my previous attempt at this back in 2018, primarily because of the stubs issue mentioned here. I see that cryptography now seems to have type extensions included as of version 3.4.4, so it may be worth attempting this again. However, I'll need to see what the state is of stubs for the other third party libraries that AsyncSSH depends on, as I really don't want to have to add annotation stubs in AsyncSSH for those other packages.

There are also a number of places where AsyncSSH is intentionally quite flexible in the types it supports, and I don't really want to lose that. I previously attempted to use things like Union to express that, but I ran into limits on that and was unable to fully resolve errors from mypy. It also often made the code much uglier. The challenge here is going to be finding a happy medium on that.

[sebastian-philipp]

I abandoned my previous attempt at this back in 2018, primarily because of the stubs issue mentioned here. I see that cryptography now seems to have type extensions included as of version 3.4.4, so it may be worth attempting this again. However, I'll need to see what the state is of stubs for the other third party libraries that AsyncSSH depends on, as I really don't want to have to add annotation stubs in AsyncSSH for those other packages.

No please don't do that. If another library doesn't have type annotations, you should just skip it like so:

mypy.ini

[mypy-cryptography.*]
ignore_missing_imports = True

There are also a number of places where AsyncSSH is intentionally quite flexible in the types it supports, and I don't really want to lose that. I previously attempted to use things like Union to express that, but I ran into limits on that and was unable to fully resolve errors from mypy. It also often made the code much uglier. The challenge here is going to be finding a happy medium on that.

in my experience, it's often perfectly fine to annotate things with Any, if you can't express things with annotations properly. Having incomplete support for types is much better than nothing. If you need more flexibility, just annotate it with Any or # type: ignore and don't worry too much about it.

[ronf]

I got a chance this weekend to take a closer look at this, and also to try and revive some of the work I did back in 2018. While third-party stubs are still missing or incomplete in some cases, the experimentation I've done so far is looking promising. I got a lot further than in my previous attempts, without the need for a lot of stubs. I did run across a few problems in the PyCA package that I've filed issues on, and I ended up writing my own stubs for libnacl, but that ended up being only 50 or so lines of code.

There's still a long way to go, but I've managed to update about 25-30 of the files so far (out of 63). The real work starts when I tackle the larger files, though - the top 5 files account for about half of the total code.

I did take a look at the auto-generated code in the pull request, but I'm not sure how usable it will be. There were a lot of places where it didn't fill anything in, and other places where it definitely got things wrong. There are also formatting/style issues that would need to be cleaned up. So far, I've found it to be about equally effective to just start from the original code and use the changes in the PR and my previous attempt at doing this on AsyncSSH 1.x as a reference.

I still need to do a bit more cleanup before I'm ready to push my changes out to Github, but I do have a fully working version of the code including the updates mentioned above with no errors reported by mypy or pylint and with all unit tests passing with 100% code coverage.

Once I finish off getting all the low-level crypto and other utility code converted, I'll probably do an initial check-in, before I start in on the higher-level code.

[sebastian-philipp]

I got a chance this weekend to take a closer look at this, and also to try and revive some of the work I did back in 2018. While third-party stubs are still missing or incomplete in some cases, the experimentation I've done so far is looking promising.

In my experience, this is not a requirement. Typically you just annotate your code independent of libraries. If a library doesn't have any annotations, you can simply ignore it an continue.

I did take a look at the auto-generated code in the pull request, but I'm not sure how usable it will be. There were a lot of places where it didn't fill anything in, and other places where it definitely got things wrong. There are also formatting/style issues that would need to be cleaned up. So far, I've found it to be about equally effective to just start from the original code and use the changes in the PR and my previous attempt at doing this on AsyncSSH 1.x as a reference.
ok!

[ronf]

Fair point. I probably won't end up checking in these stubs. However, they were necessary during development as holes in coverage due to the third-party libraries ended up potentially masking holes in coverage in AsyncSSH itself. By providing the stubs, I could eliminate any coverage issues caused by the library and make sure I got 100% coverage with my AsyncSSH changes.

[ronf]

A quick update on this: Over the last few months, I've been working on this in the background, and I've got all but one of the modules (connection.py) pretty much converted over, with better than 95+% type coverage in most of them (mostly limited by lack of type coverage in external dependencies). I'm hoping to finish this off in the next few weeks, at which point I'll have something I can check in after some review and cleanup.

Overall, I'm not completely happy with the changes, as I ended up needing to do a number of manual type casts where the type system just wasn't smart enough to figure things out on it own, and there are a few places where I allow types to vary at runtime in ways the type system simply can't understand, causing me to have to lie to it in a some cases. However, overall I think there's a net benefit here, so I'll keep at it. Once I've got the first cut checked in, I'll see what I can do to further improve the rough edges.

[ronf]

Ok - it took a while, but I've finally checked in my first cut at type annotations as commit 88dcdac in the "develop" branch. There are some places in the code where I wasn't able to come up with a way to express some of the constraints properly (particular around args/kwargs and in some of my decorators), but I've managed to get better than 97% coverage with these changes and much of what's left are things where typeshed's annotations of external modules is incomplete.

I also updated the "examples" directory, so you can find examples of using type annotations in AsyncSSH applications. See https://asyncssh.readthedocs.io/en/develop/#client-examples for more details.

Feedback is extremely welcome, particularly if you find annotations that you think look wrong, or if you have any suggestions on how to make the code cleaner.

Note: I don't have any type annotations in the "tests" directory yet. I'll work on that, but wanted to get this first cut out there to get initial feedback without waiting for that.

[ronf]

These changes are now available in AsyncSSH 2.9.0.