trio icon indicating copy to clipboard operation
trio copied to clipboard

Provide PEP 484-style type hints

Open njsmith opened this issue 6 years ago • 23 comments

Type hints are one of those things that's perpetually on my "it would be nice to learn about these someday" list, so I haven't done anything with them in trio myself, but people are interested in adding them (e.g. @Fuyukai, + some discussion). Seems like a good idea to me.

Like I said, I don't know much about this. Looking at PEP 561, it sounds like the ideal is to use inline type hints where possible, and where it's not, to fall back to .pyi files that we maintain and ship alongside the corresponding .py files? Is that right?

That means that this interacts with #542: until we make our imports less magical, inline type hints probably aren't going to work. In some cases that might be fine and we should just use .pyi files, but it seems like right now there are lot of places where inline type hints would be the right approach, just we need to clean up the imports first, and that's a good idea anyway, so we might want to start there.

This is something we'd want to do incrementally. Maybe something like: (1) figure out how to get mypy to run on trio at all, (2) get that running in travis-ci so we don't regress, (3) start incrementally adding type hints, while making sure mypy can understand them, starting from the simplest/lowest-hanging fruit?

I know there's a growing body of experience with adding type hints to projects, but I'm not super familiar with it... if you know good articles please add links to them here :-). To get us started:

njsmith avatar Jun 06 '18 21:06 njsmith

IMHO .pyi files only make sense when we can't change the .py files, e.g. because they're shipped by somebody else. This doesn't apply to Trio.

See also https://github.com/dropbox/pyannotate for a way to auto-create (some) type annotations.

smurfix avatar Jun 06 '18 22:06 smurfix

pyi files are good for describing the structure of magic objects that are created at runtime, as well.

Fuyukai avatar Jun 06 '18 23:06 Fuyukai

Sure, but we don't actually have any of these AFAIK.

smurfix avatar Jun 06 '18 23:06 smurfix

Well, unless magic re-importing is removed, trio/__init__.py (and the magic methods in _run.py) fall under those criteria.

Fuyukai avatar Jun 06 '18 23:06 Fuyukai

Oh right, something else we should keep in mind: PyPy doesn't yet support inline type annotation syntax. (I'm pretty sure they have it working in their 3.6 branch, so maybe this will become a non-issue soon. Or maybe not.)

njsmith avatar Jun 08 '18 09:06 njsmith

I've recently done a lot of work on this for HypothesisWorks/hypothesis#200, which now ships with PEP 561 type hints for the public API. Happy to at least do design and mentoring on this :smile:

The Zulip post is old but remains an excellent suggestion if you're aiming to annotate the whole codebase, but the inside-out approach will be much more work than needed if we just want to provide hints for downstream users of the Trio public API. Either is a reasonable choice IMO, and incremental adoption is the best way to start regardless so we can decide later.

Inline annotations are the most idiomatic solution, but sometimes have some issues with circular imports or import time; that's fixable with conditional (if False:) imports and forward references for complex types but at the cost of linter issues and being ugly. I really don't like stub files, because they're hard to keep in sync - python/mypy#5028 will fix that but until then IMO they're unsuitable for same-package hints. Type comments have essentially the same problems as the annotations workaround, with worse linter handling and less runtime metadata.

IMO for Trio annotations are the best solution unless Pypy means we have to use comments.

Once it's clear what our end-goal is, I'll set up an initial config and get CI passing, then it can be open season for incremental contributions.

Zac-HD avatar Jun 29 '18 12:06 Zac-HD

IMO for Trio annotations are the best solution unless Pypy means we have to use comments. Once it's clear what our end-goal is, I'll set up an initial config and get CI passing, then it can be open season for incremental contributions.

Looked at this again, and Python 3.5 also ships with Debian stable and Ubuntu 16.04, so our 3.5 support makes it easier for people to try trio out, plus there's the pypy thing. So rather than agonize over this and block making progress on type annotation entirely, it sounds like we should get started adding type hints using the comment syntax for now, and then eventually when we do drop 3.5 support it should be pretty straightforward to convert everything? It sounds like there's a lot of actual work to do right now to figure out what the type hints should even be, and switching the syntax will be pretty simple.

Does that sound right to those of you who have more experience with type hints?

njsmith avatar Jul 03 '18 09:07 njsmith

Yep, sounds good. Getting the hints right - both passing in CI, and useful + documented for downstream users - is definitely harder than changing the syntax later.

Clarifying question though: is our primary aim to provide PEP 561 hints for users of Trio, or to take advantage of static typing as a CI layer for Trio itself? Obviously they're related, but the initial strategy is a bit different if we decide now that we'd like to be fully annotated at some point.

Zac-HD avatar Jul 03 '18 09:07 Zac-HD

How does the initial strategy change? I don't feel like type hints are super crucial for Trio's internals (we also have almost 100% test coverage); it's more the public API that people have been asking for. But if you think it'll provide value for trio's CI then that'd be cool too...

njsmith avatar Jul 03 '18 09:07 njsmith

Cool, I'm happy to aim for hints where public-or-convenient :smile: (same as Hypothesis!)

If the aim is to end up fully static, you generally go for a ratchet effect and lock in rules like "no calls from annotated to unannotated functions". Mypy actually has a pile of fantastic tools for this workflow thanks to Dropbox, but it's a lot less useful for smaller - and well-tested - FOSS projects that don't mind occasional duck typing.

Zac-HD avatar Jul 03 '18 09:07 Zac-HD

...OK I am very silly.

All our supported Python versions do have inline type annotation support. What pypy and cpython 3.5 are missing is the new inline variable annotation support, the syntax like:

def foo():
    x: int = 0

Apparently I saw some comment about this at 3am and got mixed up. For the much more common function signature annotations, we can go wild.

njsmith avatar Jul 05 '18 06:07 njsmith

We'll still have some issues with changes to the typing module though; it was very experimental for the duration of 3.5

The other fun thing has been discovering just how dynamic many of the names in Trio are - conditionally defined functions, mutation of __all__, etc. Much of this will have to be converted to a static form for Mypy to work, and a few tests added to ensure that everything stays in sync. #542 already covers this, but we're going to need to do a fair bit of that work before working on type hints directly makes much sense.

Zac-HD avatar Jul 05 '18 07:07 Zac-HD

I'm still planning to take this on, just waiting for #594 or similar to be merged first.

Zac-HD avatar Sep 06 '18 03:09 Zac-HD

https://github.com/python-trio/trio/compare/master...Zac-HD:type-hints

...still needs less magic :sob:

Zac-HD avatar Sep 09 '18 01:09 Zac-HD

Well, I guess we can at least start doing type hints for the stuff in trio/_*.py...?

Is there a way to set up the CI to ratchet the type hints, so that even if we initially have all kinds of issues, we at least know we're going in the right direction?

njsmith avatar Sep 09 '18 01:09 njsmith

We could, but it's really not worth doing yet.

In short, the first step with Mypy is to get a clean run without adding any annotations. Once we have that, it's trivial to run Mypy in CI - and the rachet effect is completely automatic once we start adding type hints.

However... to get that clean run, we have to make imports and attributes visible to Mypy's static analysis. We could just sprinkle dozens of # type: ignore comments around, but removing them is usually a huge pain as all the hidden problems come to light.

It's also possible to ignore modules or subpackages with mypy.ini, but the ignore file is not shared by downstream users so it's a short-term workaround at best.

TLDR; we want good static analysis for imports etc. anyway, so IMO it makes sense to add type hints after we sort them out.

Zac-HD avatar Sep 09 '18 03:09 Zac-HD

@oremanj wrote up a "rough draft of type hints for trio", here: https://github.com/oremanj/trio-typestubs

I think ultimately we want this to be integrated into trio proper, so it stays in sync, but figuring out what the annotations should be is a good first step :-).

njsmith avatar Dec 02 '18 18:12 njsmith

Update on this: trio-typing is now a real package that people other than me appear to be using.

I'm interested in adding the type hints to Trio proper at some point; that's currently blocked on #542.

oremanj avatar May 01 '19 06:05 oremanj

As mentioned in #778, adding type hints will require us to publicize some important types like Nursery and TaskStatus. This requires guarding them better against instantiation/subclassing (#1021). Alternatively, we could take the trio-typing approach of exposing ABCs instead of the actual concrete types.

oremanj avatar May 01 '19 07:05 oremanj

Alternatively, we could take the trio-typing approach of exposing ABCs instead of the actual concrete types.

We did something similar with trio.socket.SocketType as well, in order to allow isinstance(..., trio.socket.SocketType) even if people are using the socket mocking hooks.

It does add some overhead, e.g. duplication of signatures. I think it probably makes more sense for cases where we actually have multiple implementations.

TaskStatus we probably do want to allow multiple implementations, see #252. So an ABC would make sense to me.

For Nursery the case isn't as clear... I think people probably will want to make objects that quack like nurseries, but I'm not sure they'll want to implement the full Nursery interface, including child_tasks, parent_task, etc. And we may want to reserve the right to add new attributes without breaking ABC subclasses?

njsmith avatar May 01 '19 18:05 njsmith

Sorry for the bump, but in my search for the TaskStatus type, I've landed here at this dead end. Did TaskStatus ever get exposed somewhere and is simply eluding my inquiries?

flyte avatar May 20 '21 10:05 flyte

I'm making no progress on this at this point :[ but I guess it should be linked here. https://github.com/python-trio/trio/pull/1873

altendky avatar May 20 '21 18:05 altendky

Sorry for the bump, but in my search for the TaskStatus type, I've landed here at this dead end. Did TaskStatus ever get exposed somewhere and is simply eluding my inquiries?

@flyte https://github.com/python-trio/trio-typing/blob/f32f17b0f242daf2d42407f383ca581d64b6c299/trio_typing/init.py#L30 ?

uSpike avatar Jun 01 '21 18:06 uSpike

Closing this in favor of other typing-tagged issues, because we're now well into the implementation phase 😃

Zac-HD avatar Sep 25 '23 04:09 Zac-HD