1573740 - Add static type checking for Python

Status: NEW

(Developer Infrastructure :: Lint and Formatting, task, P3)

Description

[Kelsey Gilbert [:jgilbert]]

There's a flake8 plugin for integrating mypy, so it may be easiest to just do that.

Comment 1

[Marco Castelluccio [:marco]]

I'd love this, static type annotations and mypy have helped me countless times avoiding stupid mistakes.

Comment 2

[Andrew Halberstadt [:ahal]]

I think we should consider using pyright over mypy. I've brought this up few times when type checking comes up, but haven't ever expanded on why I think this, so will share my reasoning here:

1. Pyright is faster. I've seen a claim that it is 5x faster than mypy according to a benchmark (though I'm a bit skeptical there). I'm not convinced perf is a huge factor but it might come into play when running on the entire tree (e.g in CI).

2. Pyright has a very nice VS Code plugin, which is our recommended editor on Windows. It does stuff like automatically add missing imports, display annotations on hover and much more.

3. Pyright has type inference. Even when running on code that has no annotations at all, it'll often find real bugs. This means we could stand it up immediately on all of m-c (maybe at the warning level) and get value out of it. Marco mentioned that he prefers when annotations are required, which brings us to #4...

4. Pyright is very configurable. Configuration can be set at the root of the repo or any subdirectory. But more importantly, you can specify the list of paths to include/exclude as well as a list of paths to enable strict mode on. This addresses the concern about inferring vs requiring annotations. It means we can have a broad set of files in the "include/exclude" section which will rely on type inference, then as we work to add annotations module by module, we can add them to the strict list (alternatively we could use multiple pyrightconfig.json files in subdirectories for more fine-grained control).

5. Pyright has a built-in file watcher which can provide rapid feedback while editing (I assume my editor plugin uses it too).

I'll admit I haven't done as deep of a dive into mypy, so it might have some of these features as well.

The main knock I've heard against pyright is that it's implemented in Typescript and requires node. But given that we already have an in-tree package.json, getting it bootstrapped for everyone is literally as easy as ./mach npm install pyright then committing the changes.

Comment 3

[Andrew Halberstadt [:ahal]]

Also requiring annotations could easily be handled independently of the type checker via something like flake8-annotations. Doing this might be cleaner overall anyway (whether we use mypy or pyright or something else).

Comment 4

[Marco Castelluccio [:marco]]

First of all, let me summarize what we've discussed so far.

The plan is not to introduce types everywhere from the start (it would be an impossible feat, and moreover most of the risk lies in new code, not existing code), nor to enforce adding types (which would sometimes cause people to spend too much time when there are complex data structures), but to suggest developers to use types whenever touching Python code in m-c.

In order to remind people about the suggestion, we will introduce a linter (e.g. https://pypi.org/project/flake8-annotations/, or even just modify mozlint to run mypy/pyright/whatever on certain directories at ERROR level and other directories as WARNING level) that warns about missing type annotations in functions touched by patches. Note: it will be a warning and not an error, so CI will be happy anyway if the patch lands.

In the future, we might want to try some of those tools that automatically add type annotations, but for now this will be left to each module owner's best judgement (i.e. they will do it themselves if they want to).

Regarding the choice between mypy, pytype, pyright and pyre-check, I don't have a strong opinion between them. I've only used mypy so far and it served me well, but they were small projects compared to m-c. The story might be totally different there!
I guess whoever ends up implementing this can do a deep-dive into the alternatives and estimate pros and cons, and then we settle on one of them.

Comment 5

[Andrew Halberstadt [:ahal]]

One thing I discovered is that many type checkers don't support the attrs package. Not a big deal since we can hopefully use dataclasses soon (or at least the backport of it to 3.6), but something to consider.