Incrementally expanding the use of mypy #28833

foolip · 2021-05-05T09:39:29Z

Creating a new issue to consolidate discussion.

I asked in #28815:

@gsnedders I'm having some fun learning more about mypy. I'm not sure it's worth annotating all of wptserve, but the reason I started was because I found it very hard to guess what the types of some arguments in request.py were. But starting from the "leaf" files results in less cascading errors.

Is there an approach to incrementally improving type coverage that you took for other bits that I could copy?

@jgraham wrote in #28757 (comment):

I think if we want to start adding type annotations we should also figure out how we're going to check them (and this is perhaps not a small problem for test files because it likely means vendors should be running the exact same version of mypy in their CI; for tools changes it's kind of assumed that people submitting patches to vendor repos are able to deal with the consequences of failing upstream checks, but that is less true for test authors).

https://mypy.readthedocs.io/en/stable/existing_code.html has some useful guidance, but we'd still need to figure out most of the hard problems stemming from downstream changes ourselves.

Let's treat the problem of tools/ and everything else separately. Outside of tools/, my thoughts are:

webdriver/tests/ is fairly self-contained. With about 18.5k lines of code, it's probably worthwhile.
*.py handlers are going to be the hardest since they're often edited in vendor repos, and not of huge value since they're usually short scripts

foolip · 2021-05-05T09:45:34Z

Is it possible to lint for type annotations so that they're only used where we actually run mypy?

I noticed there's at least webtransport/quic/handlers/client-indication.quic.py which came from #22844.

jgraham · 2021-05-05T09:57:22Z

Note that a lot of WebDriver tests originate in vendor patches. So it's exactly the kind of code where being able to run the typecheck as part of vendor CI using the exact same version of mypy that we use elsewhere would be most worthwhile. That seems like a reasonably complex problem and so isn't the first thing I'd tackle. Also (hopefully) tests are faily small and self contained, so figuring out the code directly isn't too bad (I don't think this is always true but it's a nice idea :) )

Practically I think the largest benefit in the short term is going to come from extending the use of mypy to cover more of the tools/ directory. I'm not so worried about the vendor CI problem there since it's a much smaller group of contributors and they mostly know how to interact with GitHub.

Making wptserve and wptrunner a) pass typechecks and b) actually have type annotations seems like the right next step. I'd guess it's going to be a few weeks of work at least; some Python patterns are easy to express in code and hard to express in types, and I've previously found during this kind of conversion that I just rewrote code to be less dynacially typed rather than trying to figure out how to make mypy happy.

jgraham · 2021-05-05T09:58:42Z

Oh and for linting, I'm pretty sure we could write a lint using the python tokenize module (or build a full ast if needed) to find things that look like type annotations outside a specific list of directories. It's not going to be trivial, but it feels possible at least.

foolip · 2021-05-05T10:01:15Z

How big of a step would it be to run mypy on all of tools/ but only have the strict options apply to the parts of the code that it currently runs on (manifest+lint+gitignore)?

jgraham · 2021-05-05T10:12:32Z

You could rename mypy.ini to mypy_strict.ini and add a mypy_lax.ini and make tox run some modules with one and some with the other. How hard it would be to make the code pass anything other than ignore_errors=True I don't know without running the experiment.

Not yet: #28833

foolip · 2021-05-07T13:21:54Z

I gave this a shot today, and with not too much fixing of fatal errors it's possible to run mypy for all of tools/ with a bunch of packages explicitly ignored, and some paths not checked at all. I'll send PRs of minor things that are blocking with reference to this issue.

It's not clear from the code alone which is which, but these import statements work on Python 3.8.2: ```py from html import escape import queue from urllib.request import urlopen ``` Part of #28776. Part of #28833.

The type annotations here aren't very useful, but simple to do and avoids the need to exempt this script from mypy later. It's been tested with mypy locally. Part of #28833.

gsnedders · 2021-05-07T14:06:08Z

Oh and for linting, I'm pretty sure we could write a lint using the python tokenize module (or build a full ast if needed) to find things that look like type annotations outside a specific list of directories. It's not going to be trivial, but it feels possible at least.

I'm not convinced it's overly worthwhile to actually forbid them elsewhere, tbh?

But I do think getting all of tools/* covered would be good. (And we can use the Py3 syntax now! 💜)

Part of #28833.

foolip · 2021-05-07T15:26:26Z

OK, it was hairy to get done but the result looks clean enough: #28900

It depends on some other changes, so I'm leaving it as a draft. Enough mypy for today I think :)

…28894) It's not clear from the code alone which is which, but these import statements work on Python 3.8.2: ```py from html import escape import queue from urllib.request import urlopen ``` Part of #28776. Part of #28833.

The type annotations here aren't very useful, but simple to do and avoids the need to exempt this script from mypy later. It's been tested with mypy locally. Part of #28833.

Part of #28833.

…ved between Python 2 and 3, a=testonly Automatic update from web-platform-tests Simplify try-except imports of things moved between Python 2 and 3 (#28894) It's not clear from the code alone which is which, but these import statements work on Python 3.8.2: ```py from html import escape import queue from urllib.request import urlopen ``` Part of web-platform-tests/wpt#28776. Part of web-platform-tests/wpt#28833. -- wpt-commits: 20e07ca88113ef6132c4756019e4dd4ab6c46f0a wpt-pr: 28894

…notations, a=testonly Automatic update from web-platform-tests Update retry.py to Python 3 with type annotations (#28895) The type annotations here aren't very useful, but simple to do and avoids the need to exempt this script from mypy later. It's been tested with mypy locally. Part of web-platform-tests/wpt#28833. -- wpt-commits: 4bc9779398edf5c66d7630830e384ba774c5969e wpt-pr: 28895

This code was already mostly annotated, but needs updating to pass our mypy.ini rules. A few changes are noteworthy: - QuicTransportProtocol.streams was removed rather than annotated, because it's actually unused. - An assert for self.handler is added after process_client_indication() to it clear that that it can't be None in the following loop. Part of #28833. Depends on #28900.

Changes are as minimal as possible. The annotation for Kwargs in tools/wpt/utils.py was invalid syntax. `class Kwargs(Dict[Any, Any])` would work but doesn't add any type information, so not done. __init__.py files were added to various test directories so that they're treated as packages, similar to tools/lint/tests/__init__.py and other existing test directories in mypy-checked code. Part of #28833.

This code was already mostly annotated, but needs updating to pass our mypy.ini rules. A few changes are noteworthy: - QuicTransportProtocol.streams was removed rather than annotated, because it's actually unused. - An assert for self.handler is added after process_client_indication() to it clear that that it can't be None in the following loop. Part of #28833. Depends on #28900.

Part of #28833. Depends on #28900.

Part of #28833.

Make ConfigDict a Dict[str, Any] since the keys come from parser.options() which are strings in typeshed: https://github.com/python/typeshed/blob/7a9b4f93baa03fff3ef7f319988cea2cbb6e2135/stdlib/configparser.pyi#L105 Also s/SafeConfigParser/ConfigParser/ since SafeConfigParser is now a legacy alias not mentioned in Python 3 documentation. Part of #28833.

Part of #28833.

Automatic update from web-platform-tests Enable mypy for tools/quic/ (#28916) This code was already mostly annotated, but needs updating to pass our mypy.ini rules. A few changes are noteworthy: - QuicTransportProtocol.streams was removed rather than annotated, because it's actually unused. - An assert for self.handler is added after process_client_indication() to it clear that that it can't be None in the following loop. - Similar asserts are added for origin_string and path_string Part of web-platform-tests/wpt#28833. -- wpt-commits: f137686dc4fdc5cab4018c0c450c655d61cf47ef wpt-pr: 28916

…untyped defs), a=testonly Automatic update from web-platform-tests Run mypy for tools/ci/ (but still allow untyped defs) (#28952) Part of web-platform-tests/wpt#28833. -- wpt-commits: 96d6ddeceb9e8a755e9d2fe95197fc45a3486118 wpt-pr: 28952

…w untyped defs), a=testonly Automatic update from web-platform-tests Run mypy for tools/wave/ (but still allow untyped defs) (#28987) Part of web-platform-tests/wpt#28833. -- wpt-commits: 0b4f07772e96d0fc78558fe414571d7243aeaa9a wpt-pr: 28987

Make ConfigDict a Dict[str, Any] since the keys come from parser.options() which are strings in typeshed: https://github.com/python/typeshed/blob/7a9b4f93baa03fff3ef7f319988cea2cbb6e2135/stdlib/configparser.pyi#L105 Also s/SafeConfigParser/ConfigParser/ since SafeConfigParser is now a legacy alias not mentioned in Python 3 documentation. Part of #28833.

Part of #28833.

…ow untyped defs), a=testonly Automatic update from web-platform-tests Run mypy for tools/serve/ (but still allow untyped defs) Part of web-platform-tests/wpt#28833. -- wpt-commits: 9cbcc977a22a6fe04af23be0876823fb5d5c3d8f wpt-pr: 28988

Part of #28833.

foolip · 2022-03-25T09:06:39Z

Closing this, mypy is now running with optional typing in all of tools/. Getting 100% type coverage is a whole other story, and probably not worth pushing for.

…allow untyped defs), a=testonly Automatic update from web-platform-tests Run mypy for tools/wptserve/ (but still allow untyped defs) (#28954) Part of web-platform-tests/wpt#28833. -- wpt-commits: 2b537ce2f51488259779a2f78c702e368030a6dc wpt-pr: 28954

foolip added infra mypy.ini labels May 5, 2021

foolip mentioned this issue May 5, 2021

Clean up more around base64.decodebytes() calls #28757

Merged

foolip added a commit that referenced this issue May 5, 2021

Revert type annotations

d8c7260

Not yet: #28833

foolip mentioned this issue May 7, 2021

Simplify try-except imports of things moved between Python 2 and 3 #28894

Merged

foolip mentioned this issue May 7, 2021

Update retry.py to Python 3 with type annotations #28895

Merged

foolip added a commit that referenced this issue May 7, 2021

Run mypy for all of tools/ with specific modules exempted

8d40112

Part of #28833.

foolip mentioned this issue May 7, 2021

Run mypy for all of tools/ with specific modules exempted #28900

Merged

foolip added a commit that referenced this issue May 7, 2021

Run mypy for all of tools/ with specific modules exempted

868f28f

Part of #28833.

foolip added a commit that referenced this issue May 7, 2021

Run mypy for all of tools/ with specific modules exempted

c31ea98

Part of #28833.

foolip mentioned this issue May 8, 2021

Run mypy for tools/webdriver/ (but still allow untyped defs) #28913

Closed

foolip mentioned this issue May 9, 2021

Enable mypy for tools/quic/ #28916

Merged

foolip added a commit that referenced this issue May 10, 2021

Run mypy for tools/webdriver/ (but still allow untyped defs)

4094b04

Part of #28833. Depends on #28900.

foolip added a commit that referenced this issue May 17, 2021

Run mypy for tools/wave/ (but still allow untyped defs)

29420be

Part of #28833.

foolip added a commit that referenced this issue May 17, 2021

Run mypy for tools/wave/ (but still allow untyped defs) (#28987)

0b4f077

Part of #28833.

foolip added a commit that referenced this issue May 17, 2021

Run mypy for tools/wptserve/ (but still allow untyped defs)

2dfd716

Part of #28833.

foolip added a commit that referenced this issue May 17, 2021

Run mypy for tools/wptserve/ (but still allow untyped defs)

120e8e2

Part of #28833.

foolip added a commit that referenced this issue May 18, 2021

Run mypy for tools/serve/ (but still allow untyped defs)

92b8453

Part of #28833.

foolip added a commit that referenced this issue May 18, 2021

Run mypy for tools/wptserve/ (but still allow untyped defs)

14eaeb0

Part of #28833.

foolip added a commit that referenced this issue May 18, 2021

Run mypy for tools/wptserve/ (but still allow untyped defs)

ab17061

Part of #28833.

foolip added a commit that referenced this issue May 18, 2021

Run mypy for tools/serve/ (but still allow untyped defs)

607d4da

Part of #28833.

foolip added a commit that referenced this issue May 18, 2021

Run mypy for tools/wptserve/ (but still allow untyped defs)

0e291c0

Part of #28833.

foolip added a commit that referenced this issue May 18, 2021

Run mypy for tools/serve/ (but still allow untyped defs)

349eb96

Part of #28833.

foolip added the priority:backlog label Jun 7, 2021

foolip added a commit that referenced this issue Jun 11, 2021

Run mypy for tools/serve/ (but still allow untyped defs)

85b070c

Part of #28833.

foolip added a commit that referenced this issue Jun 11, 2021

Run mypy for tools/wptserve/ (but still allow untyped defs)

0741fab

Part of #28833.

jgraham pushed a commit that referenced this issue Jun 13, 2021

Run mypy for tools/serve/ (but still allow untyped defs)

9cbcc97

Part of #28833.

foolip added a commit that referenced this issue Mar 10, 2022

Run mypy for tools/wptserve/ (but still allow untyped defs)

799e4cb

Part of #28833.

foolip added a commit that referenced this issue Mar 23, 2022

Run mypy for tools/wptserve/ (but still allow untyped defs)

ebf4fcb

Part of #28833.

foolip added a commit that referenced this issue Mar 23, 2022

Run mypy for tools/wptserve/ (but still allow untyped defs)

3e2e8a4

Part of #28833.

foolip added a commit that referenced this issue Mar 24, 2022

Run mypy for tools/wptserve/ (but still allow untyped defs) (#28954)

2b537ce

Part of #28833.

foolip closed this as completed Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incrementally expanding the use of mypy #28833

Incrementally expanding the use of mypy #28833

foolip commented May 5, 2021

foolip commented May 5, 2021

jgraham commented May 5, 2021

jgraham commented May 5, 2021

foolip commented May 5, 2021

jgraham commented May 5, 2021

foolip commented May 7, 2021

gsnedders commented May 7, 2021

foolip commented May 7, 2021

foolip commented Mar 25, 2022

Incrementally expanding the use of mypy #28833

Incrementally expanding the use of mypy #28833

Comments

foolip commented May 5, 2021

foolip commented May 5, 2021

jgraham commented May 5, 2021

jgraham commented May 5, 2021

foolip commented May 5, 2021

jgraham commented May 5, 2021

foolip commented May 7, 2021

gsnedders commented May 7, 2021

foolip commented May 7, 2021

foolip commented Mar 25, 2022