MyPy, Next Steps

[troygiorshev]

MyPy was added in #18210, allowing for static type checking of type hints in our testing framework. However, there is still some work to be done before we can take advantage of these new capabilities. 🚀

Right now, lint-python.sh runs MyPy with the --ignore-imports flag. This is done primarily to ignore the lack of type hinting for zmq, but it hides many errors in our own code too.

To begin to see what's needed, first run mypy test/functional. You'll see two sets of errors. One complains about Skipping analyzing 'zmq'. This one can be ignored for now by changing the identified import zmq lines to import zmq # type: ignore.

The other states that it Cannot find implementation or library stub for module named 'data'. Remember that for a second.

Now go into test/functional/test_framework and run mypy mininode.py. Observe two more Cannot find implementation or library stub for module errors, now for test_framework.messages and test_framework.util.

What's happening here is that MyPy is failing to handle importing our test_framework package. We'll need to do a few things to fix this.

• MYPYPATH and/or mypy_path
  • Using either the environment variable MYPYPATH or the config file option mypy_path we need to tell MyPy where to find our test_framework package.
  • https://mypy.readthedocs.io/en/stable/running_mypy.html#finding-imports
• Stub Files
  • We need to create stub files (ending in .pyi) for the modules in test_framework
  • https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-type-hints-for-third-party-library
  • https://mypy.readthedocs.io/en/stable/stubs.html#stub-files
  • This can be made much easier with stubgen, and possibly MonkeyType or PyAnnotate
• Remove --ignore-missing-imports and add # type: ignore to import zmq
  • --ignore-missing-imports simply hides too many errors.

Resources

• Import handling is confusing and misleading python/mypy#4542
• https://mypy.readthedocs.io/en/stable/existing_code.html

Important Notes

Our minimum supported python version is 3.5, so any changes must be compatible. In particular this means that we must use type comments, as opposed to type hinting, in variables that aren't function parameters. (i.e. foo # type: int as opposed to foo: int). This will complicate the use of MonkeyType, as it doesn't support type comments. It may be useful to make a scripted diff between type comments and type hints.

Useful skills:

MyPy experience

Want to work on this issue?

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

[fanquake]

Just a note for anyone looking into this. Make sure that whatever changes you plan on making are compatible with Python 3.5, as that is our minimum supported version of Python.

[ysangkok]

Why would you use stub files for in-tree modules? Why not add the type annotations in the implementation file?

[troygiorshev]

Just a note for anyone looking into this. Make sure that whatever changes you plan on making are compatible with Python 3.5, as that is our minimum supported version of Python.

This is a very important point, I've updated the issue to reflect it. In particular this means that we must use type comments, as opposed to type hinting, in variables that aren't function parameters. (i.e. foo # type: int as opposed to foo: int). This will complicate the use of MonkeyType, as it doesn't support type comments. It may be easy to make a scripted diff between type comments and type hints.

Why would you use stub files for in-tree modules? Why not add the type annotations in the implementation file?

afaik we will need to do both, and this is simply a limitation of MyPy's design. I'm more than happy to be proven wrong!

[scorbajio]

I am open to take over this issue if there is no one actively working on it.

[JeremyRubin]

I have some of the library parts mypy typed (I think messages.py and script.py), if anyone wants them I can share.

[scorbajio]

I generated the stub files using stubgen, but they do not seem to be python 3.5 compatible. I cannot think of a better solution than to write a script to change non-function-parameter type hints (i.e. foo: int) into type comments (i.e. foo # type: int). I think this is what @troygiorshev means by "scripted diff", right?

[troygiorshev]

@ghorbanian Thanks for giving this a look! Yup, that's exactly what I mean by "scripted diff". I'll go check out #19532.

[emc99]

I am keen on helping out with this issue. Where do I start? Should I go away and learn C++ and Python first?

[NelsonFrancisco]

I just want to point out two things because of my experience:

1. I don't see any mention in this repo about a Python virtualenv to facilitate development. Is it too far fetched or a script/tutorial would be relevant?

2. Maybe it's because it is outdated, so an update would be relevant:

   Now go into test/functional/test_framework and run mypy mininode.py. Observe two more Cannot find implementation or library stub for module errors, now for test_framework.messages and test_framework.util.

   mininode.py does not exist in that folder.

[fanquake]

Since the release of PyZMQ 21.0, the library now contains mypy stubs:

mypy type stubs, which should improve static analysis of pyzmq, especially for dynamically defined attributes such as zmq constants. These are new! Let us know if you find any issues.

This should negate the need to ignore zmq imports.

[fspv]

I created a PR to remove the most of remaining type: ignore comments #23420

Merging it will leave us with just a few suppressed places:

spv@laptop bitcoin $ grep -EIR 'type:.*ignore' . 2>/dev/null | grep -v venv
./contrib/devtools/security-check.py:import lief #type:ignore
./contrib/devtools/symbol-check.py:import lief #type:ignore
./test/functional/test_runner.py:if os.name != "nt" or sys.getwindowsversion() >= (10, 0, 14393):  # type:ignore
./test/functional/test_runner.py:        kernel32 = ctypes.windll.kernel32  # type: ignore
./test/functional/combine_logs.py:        import jinja2  # type:ignore

[maflcko]

Anything left to do here that is a good first issue? If yes, the issue text should be updated or a new issue be filed (and this one closed).

[fspv]

These 2 Windows-specific lines are easy to fix

./test/functional/test_runner.py:if os.name != "nt" or sys.getwindowsversion() >= (10, 0, 14393):  # type:ignore
./test/functional/test_runner.py:        kernel32 = ctypes.windll.kernel32  # type: ignore

https://mypy.readthedocs.io/en/stable/common_issues.html#python-version-and-system-platform-checks

After that it can be closed, I believe, as only untyped libs (lief and jinja2) are left.

UPD: for the ignored imports the description suggests generating stub files, so the issue can't be closed. But needs to be updated anyway.

[maflcko]

These 2 Windows-specific lines are easy to fix

If someone wants to fix those, seems fine to just create a pull. Though, I think there is no risk if they are left unfixed. It is not like color formatting in the test framework output is any critical feature.

UPD: for the ignored imports the description suggests generating stub files

I am not sure if we want to maintain stub files for third party python packages that we use. Is the effort going to be worth the additional checks? jinja2 is only used to print logs, a feature that is know to be working and unlikely to break. lief I am not sure. An alternative would be to just wait for the upstream project to add the annotations and do nothing here?

[fspv]

Though, I think there is no risk if they are left unfixed.

Agree, no risks at all. Just a low-hanging fruit for those looking for the "good first issue".

I am not sure if we want to maintain stub files

Agree with this as well. The description should be updated to reflect this though.

[maflcko]

Ok, I am just going ahead and closing this. Maybe a new issue can be started if people want to discuss whether we want to maintain stub files for 3rd party packages.