MyPy, Next Steps #19389

1. 
   troygiorshev commented at 8:09 pm on June 26, 2020: contributor

   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

   • https://github.com/python/mypy/issues/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.

2. troygiorshev added the label good first issue on Jun 26, 2020
3. MarcoFalke added the label Tests on Jun 27, 2020
4. 
   fanquake commented at 2:23 am on June 27, 2020: member

   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.
5. 
   ysangkok commented at 4:51 pm on June 27, 2020: none

   Why would you use stub files for in-tree modules? Why not add the type annotations in the implementation file?
6. 
   troygiorshev commented at 5:40 pm on June 27, 2020: contributor

   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!

7. 
   scorbajio commented at 2:39 pm on July 6, 2020: none

   I am open to take over this issue if there is no one actively working on it.
8. 
   JeremyRubin commented at 7:21 pm on July 6, 2020: contributor

   I have some of the library parts mypy typed (I think messages.py and script.py), if anyone wants them I can share.
9. 
   scorbajio commented at 10:49 pm on July 15, 2020: none

   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?
10. 
    troygiorshev commented at 2:00 pm on July 16, 2020: contributor

    @Ghorbanian Thanks for giving this a look! Yup, that’s exactly what I mean by “scripted diff”. I’ll go check out #19532.
11. 
    emc99 commented at 4:48 pm on March 9, 2021: none

    I am keen on helping out with this issue. Where do I start? Should I go away and learn C++ and Python first?
12. 
    NelsonFrancisco commented at 2:42 pm on March 29, 2021: none

    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.

13. 
    fanquake commented at 3:55 am on May 28, 2021: member

    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.

14. 
    fspv commented at 9:16 am on November 3, 2021: contributor

    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:

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

15. 
    MarcoFalke commented at 10:05 am on November 3, 2021: member

    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).
16. 
    fspv commented at 10:42 am on November 3, 2021: contributor

    These 2 Windows-specific lines are easy to fix

    0./test/functional/test_runner.py:if os.name != "nt" or sys.getwindowsversion() >= (10, 0, 14393):  # type:ignore
    1./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.

17. 
    MarcoFalke commented at 11:03 am on November 3, 2021: member

    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?

18. 
    fspv commented at 11:33 am on November 3, 2021: contributor

    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.

19. 
    MarcoFalke commented at 11:42 am on November 3, 2021: member

    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.
20. MarcoFalke closed this on Nov 3, 2021

    ---

21. fanquake referenced this in commit d217ee25a3 on Nov 11, 2021
22. sidhujag referenced this in commit e6b0f71dc7 on Nov 11, 2021
23. DrahtBot locked this on Nov 3, 2022

Contributors
troygiorshev fanquake ysangkok scorbajio JeremyRubin emc99 NelsonFrancisco fspv MarcoFalke

Labels
Tests good first issue