Add type hints to codebase #6742

Eric-Arellano · 2018-11-08T17:08:38Z

Hello, as discussed via Slack and google group, I'm hoping to set us up with type hints in our codebase (PEP 484). I led a similar project while at Foursquare, and think the scope is achievable.

This wouldn't start until we finish the Python 3 port (#6062).

Why type hints

Catch bugs at lint time.
Improved documentation.
More confidence while refactoring.

Scope

Allow any new or refactored code to use type hints.
Enforce type safety through CI integration.
Add type hints to core code.

This issue does not entail 100% type coverage.

Incremental type safety

Rather than trying to rewrite our entire codebase at once, we'd take a more incremental approach. Core code would be ported so that any dependee has some level of type safety. Over time, we would expect any new code and refactored code to have type hints, leading to an incremental increase in coverage.

This is the approach taken by most organizations, including Foursquare and Dropbox (author of MyPy).

Specific approach: `type_checked` and `partially_type_checked` tags

We only run MyPy against targets with either of these two tags. (As a consequence, MyPy will check any transitive dependencies).

type_checked signals that the file is completely typed and good to go.

partially_type_checked signals that the file is not completely typed, but we still want to ensure that MyPy runs against the file (and its dependencies). This is useful to allow someone to add 1-2 type hints to a target without feeling the pressure of having to type the whole target.

Tasks

Setup MyPy config and script as a starting point (Setup MyPy config and enforce types for build-support scripts #7704)
Get MyPy to stop complaining when used against Your Typical Pants Target™️. This requires fixing several issues, including replacing objects.py with standard library alternatives that MyPy understands.
Add type hints to core code, meaning code that has many dependees: https://docs.google.com/spreadsheets/d/1MKg82Fs3uIMOZDoWeNUBD-VirVkOzHU8Tte7oY6ypP0/edit?usp=sharing
- Easier to add this to targets that also have few dependencies, such as the util folder
Get as much of the repo running with partially_type_checked as possible
- Add this tag even if we don't add any type hints to the file! So long as it passes w/ MyPy, we should add the tag. This allows us to ensure the correctness of any new hints we add.
Incrementally convert partially_type_checked to type_checked

The text was updated successfully, but these errors were encountered:

cosmicexplorer · 2019-01-21T22:05:18Z

We should absolutely figure out how to represent type hints such as Union[string, bytes] in python docstring syntax as per this comment on #7120. I think we should probably add that to the checklist? The comment mentions a link which describes how python docstring conventions work, but doesn't answer how type hints should.

cosmicexplorer · 2019-03-29T18:42:16Z

One additional thought is that if we wanted to look at type hints before we're fully py3-compatible, we could start creating stub files to get most of the benefits before we can put them into the code itself.

### Problem We would like to use type hints throughout our code base, per #6742. However, we must configure it with sensible defaults to make it easier to use and to provide a consistent interface for developers. We should also enforce as much type safety as we can via our `pre-commit` check. ### Solution * Add new `mypy.ini` file with fairly loose defaults that are good for first time codebases. Eventually, we will want to do things like require all functions to be typed, but for now we loosen our expectations. * Link to the config file in `pants.ini`, along with using the most modern MyPy release which gives us the `strict_equality` check. * Run `mypy.py` over the build-support scripts, which are fully typed. * Acts as a proof of concept for how this will be integrated into CI. ### Result `./pants mypy` is now a bit more useful on our codebase, even though it will still fail because of pre-existing issues. Further, the build-support folder will not have any typing regressions now.

…uild#7704) ### Problem We would like to use type hints throughout our code base, per pantsbuild#6742. However, we must configure it with sensible defaults to make it easier to use and to provide a consistent interface for developers. We should also enforce as much type safety as we can via our `pre-commit` check. ### Solution * Add new `mypy.ini` file with fairly loose defaults that are good for first time codebases. Eventually, we will want to do things like require all functions to be typed, but for now we loosen our expectations. * Link to the config file in `pants.ini`, along with using the most modern MyPy release which gives us the `strict_equality` check. * Run `mypy.py` over the build-support scripts, which are fully typed. * Acts as a proof of concept for how this will be integrated into CI. ### Result `./pants mypy` is now a bit more useful on our codebase, even though it will still fail because of pre-existing issues. Further, the build-support folder will not have any typing regressions now.

### Problem Per #6742, we will be _incrementally_ adding type hints to Pants. We need some way to mark to our CI that the target should be checked against, to make sure that any gains we make in type hints are actually enforced. ### Solution Introduce a new tag `type_checked`, which indicates to our CI that `./pants mypy` should be ran against that target. Also introduce a helper script `build-support/bin/mypy.py` that runs `./pants mypy` over _all_ eligible targets in Pants. To run over a select few targets, users should instead simply run `./pants mypy path/to:target`. #### Alternative solution considered: centralized whitelist file It was decided that a decentralized system will work better in this particular case, for some of these reasons: 1) There's a convention for keeping it decentralized: the `integration` tag. 2) `BUILD` files are more accessible to modify, e.g. being closer to the source. 3) We want to type check the entire codebase eventually. One centralized file would risk becoming too large and difficult to maintain.

…d#7886) ### Problem Per pantsbuild#6742, we will be _incrementally_ adding type hints to Pants. We need some way to mark to our CI that the target should be checked against, to make sure that any gains we make in type hints are actually enforced. ### Solution Introduce a new tag `type_checked`, which indicates to our CI that `./pants mypy` should be ran against that target. Also introduce a helper script `build-support/bin/mypy.py` that runs `./pants mypy` over _all_ eligible targets in Pants. To run over a select few targets, users should instead simply run `./pants mypy path/to:target`. #### Alternative solution considered: centralized whitelist file It was decided that a decentralized system will work better in this particular case, for some of these reasons: 1) There's a convention for keeping it decentralized: the `integration` tag. 2) `BUILD` files are more accessible to modify, e.g. being closer to the source. 3) We want to type check the entire codebase eventually. One centralized file would risk becoming too large and difficult to maintain.

Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking. Targets to be type checked are tagged according to the '--whitelisted-tag-name' option, defaults to `type_checked` as used from pantsbuild#7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change. New mypy logic added to accomodate new opt-in strategy. Mypy will acts according to the following: 1. Filter out non-whitelisted target root 2. Check if all targets now in context are whitelisted, if not we throw an error 3. Continue as normal, collecting python source files and running mypy See pantsbuild#7886 and pantsbuild#6742 for more context

…opt-in type checking (#8099) ### Problem We want to move MyPy into the lint goal and allow for users to opt-in into type_checking for the new whitelisting/opt-in logic, we can make use of the lint goal capabilities instead of rebuilding the wheel See #7886 and #6742 for more context. ### Solution Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking. Targets to be type checked are tagged according to the `--whitelisted-tag-name` option, defaults to `type_checked` as used from #7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change. New mypy logic added to accommodate new opt-in strategy. Mypy will acts according to the following: 1. Filter out non-whitelisted target roots 2. Check if the transitive deps from the filtered roots are whitelisted, if not we throw a warning 3. Continue as normal, collecting python source files and running mypy

…opt-in type checking (pantsbuild#8099) ### Problem We want to move MyPy into the lint goal and allow for users to opt-in into type_checking for the new whitelisting/opt-in logic, we can make use of the lint goal capabilities instead of rebuilding the wheel See pantsbuild#7886 and pantsbuild#6742 for more context. ### Solution Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking. Targets to be type checked are tagged according to the `--whitelisted-tag-name` option, defaults to `type_checked` as used from pantsbuild#7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change. New mypy logic added to accommodate new opt-in strategy. Mypy will acts according to the following: 1. Filter out non-whitelisted target roots 2. Check if the transitive deps from the filtered roots are whitelisted, if not we throw a warning 3. Continue as normal, collecting python source files and running mypy

…opt-in type checking (#8099) ### Problem We want to move MyPy into the lint goal and allow for users to opt-in into type_checking for the new whitelisting/opt-in logic, we can make use of the lint goal capabilities instead of rebuilding the wheel See #7886 and #6742 for more context. ### Solution Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking. Targets to be type checked are tagged according to the `--whitelisted-tag-name` option, defaults to `type_checked` as used from #7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change. New mypy logic added to accommodate new opt-in strategy. Mypy will acts according to the following: 1. Filter out non-whitelisted target roots 2. Check if the transitive deps from the filtered roots are whitelisted, if not we throw a warning 3. Continue as normal, collecting python source files and running mypy

…s repo (#8464) ### Problem Currently, to have MyPy run against a target, you must mark it with `type_checked` or it must be a dependency of a target marked with this tag. Often, Pants devs want to add 1-2 type hints but don't have time to type check the whole target. So, they must decide if they should still add `type_checked` even though the target is not fully typed. If they add it, it now becomes much harder for us to track what work remains. We want some way to: 1. Have MyPy check partially typed targets, even if the target is not entirely typed. 1. Track what is partially vs. fully typed. ### Solution Introduce the `partially_type_checked` tag and have `mypy.py` run against any targets with `partially_type_checked` or `type_checked`. ### Result The barrier to entry becomes much lower to adding some type hints to a target and enforcing that they work. This PR allows us to pursue a new strategy for the type hint migration (#6742): get as many targets as possible to work with `partially_type_checked`. **Even if a target has zero type hints, we should add `partially_type_checked` to it** so that we enforce that we do not introduce any new errors and so that when we add do add type hints, we know the rest of the codebase is using things correctly.

Eric-Arellano · 2020-02-21T02:30:53Z

Closing this out as a substantial portion of our code base now uses type hints.

This was referenced Dec 12, 2018

Generic incremental strategy (white/blacklist) for fmt/lint #6902

Closed

Replace list with tuples #6903

Closed

This was referenced Jan 21, 2019

deprecate implicit usage of binary_mode=True and mode='wb' in dirutil methods #7120

Merged

add testing for the pantsd/pantsd-runner logic path to ensure we can write fatal exceptions to a log in py3 #7080

Closed

Eric-Arellano mentioned this issue May 11, 2019

Setup MyPy config and enforce types for build-support scripts #7704

Merged

Eric-Arellano mentioned this issue Jun 14, 2019

Set up type_checked tag for internal type hint migration #7886

Merged

mabdi3 mentioned this issue Jul 23, 2019

Move MyPy from isolated goal into 'lint' goal and add MyPy whitelist/opt-in type checking #8099

Merged

Eric-Arellano mentioned this issue Oct 12, 2019

Use partially_type_checked tag for incremental type hinting of Pants repo #8464

Merged

Eric-Arellano closed this as completed Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints to codebase #6742

Add type hints to codebase #6742

Eric-Arellano commented Nov 8, 2018 •

edited

Loading

cosmicexplorer commented Jan 21, 2019 •

edited

Loading

cosmicexplorer commented Mar 29, 2019

Eric-Arellano commented Feb 21, 2020

Add type hints to codebase #6742

Add type hints to codebase #6742

Comments

Eric-Arellano commented Nov 8, 2018 • edited Loading

Why type hints

Scope

Incremental type safety

Specific approach: type_checked and partially_type_checked tags

Tasks

cosmicexplorer commented Jan 21, 2019 • edited Loading

cosmicexplorer commented Mar 29, 2019

Eric-Arellano commented Feb 21, 2020

Eric-Arellano commented Nov 8, 2018 •

edited

Loading

Specific approach: `type_checked` and `partially_type_checked` tags

cosmicexplorer commented Jan 21, 2019 •

edited

Loading