Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide PEP 484-style type hints #543

Closed
njsmith opened this issue Jun 6, 2018 · 24 comments
Closed

Provide PEP 484-style type hints #543

njsmith opened this issue Jun 6, 2018 · 24 comments
Labels
missing piece todo soon typing Adding static types to trio's interface

Comments

@njsmith
Copy link
Member

njsmith commented Jun 6, 2018

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:

@smurfix
Copy link
Contributor

smurfix commented Jun 6, 2018

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.

@Fuyukai
Copy link
Member

Fuyukai commented Jun 6, 2018

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

@smurfix
Copy link
Contributor

smurfix commented Jun 6, 2018

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

@Fuyukai
Copy link
Member

Fuyukai commented Jun 6, 2018

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

@njsmith
Copy link
Member Author

njsmith commented Jun 8, 2018

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.)

@Zac-HD
Copy link
Member

Zac-HD commented Jun 29, 2018

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 😄

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.

@njsmith
Copy link
Member Author

njsmith commented Jul 3, 2018

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?

@Zac-HD
Copy link
Member

Zac-HD commented Jul 3, 2018

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.

@njsmith
Copy link
Member Author

njsmith commented Jul 3, 2018

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...

@Zac-HD
Copy link
Member

Zac-HD commented Jul 3, 2018

Cool, I'm happy to aim for hints where public-or-convenient 😄 (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 Zac-HD self-assigned this Jul 3, 2018
@njsmith
Copy link
Member Author

njsmith commented Jul 5, 2018

...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.

@Zac-HD
Copy link
Member

Zac-HD commented Jul 5, 2018

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
Copy link
Member

Zac-HD commented Sep 6, 2018

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

@Zac-HD
Copy link
Member

Zac-HD commented Sep 9, 2018

master...Zac-HD:type-hints

...still needs less magic 😭

@njsmith
Copy link
Member Author

njsmith commented Sep 9, 2018

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?

@Zac-HD
Copy link
Member

Zac-HD commented Sep 9, 2018

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.

@njsmith
Copy link
Member Author

njsmith commented Dec 2, 2018

@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 :-).

@oremanj
Copy link
Member

oremanj commented May 1, 2019

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
Copy link
Member

oremanj commented May 1, 2019

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.

@njsmith
Copy link
Member Author

njsmith commented May 1, 2019

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?

@Zac-HD Zac-HD removed their assignment Apr 2, 2020
@flyte
Copy link

flyte commented May 20, 2021

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?

@altendky
Copy link
Member

I'm making no progress on this at this point :[ but I guess it should be linked here. #1873

@uSpike
Copy link
Member

uSpike commented Jun 1, 2021

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 ?

@Zac-HD
Copy link
Member

Zac-HD commented Sep 25, 2023

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

@Zac-HD Zac-HD closed this as completed Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing piece todo soon typing Adding static types to trio's interface
Projects
None yet
Development

No branches or pull requests

9 participants