Skip to content

Conversation

kiminuo
Copy link
Contributor

@kiminuo kiminuo commented Feb 25, 2020

This PR adds initial support for type hints checking in python scripts.

Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."

Mypy is used in lint-python.sh to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.

Notes:

  • --ignore-missing-imports switch is passed on to mypy checker for now. The effect of this is that one does not need # type: ignore for import zmq. More information about import processing can be found here. This can be changed in a follow-up PR, if it is deemed useful.
  • We are stuck with Python 3.5 until 04/2021 (see https://packages.ubuntu.com/xenial/python3). When Python version is bumped to 3.6+, one can change:
    _opcode_instances = []  # type: List[CScriptOp]
    to
    _opcode_instances:List[CScriptOp] = [] 
    for type hints that are not function parameters and function return types.

Useful resources:

@maflcko
Copy link
Member

maflcko commented Feb 25, 2020

Is there a way to make python crash if the type hint is incorrect? In that case they seem useful, otherwise they might get outdated and cause more confusion.

@DrahtBot DrahtBot added the Tests label Feb 25, 2020
@kiminuo
Copy link
Contributor Author

kiminuo commented Feb 25, 2020

There is Mypy - a static type checker for Python 3 and Python 2.7.

Example

PS C:\temp> cat test.py
#!/usr/bin/env python3

s: str
s = 1
PS C:\temp> python -m mypy test.py
test.py:4: error: Incompatible types in assignment (expression has type "int", variable has type "str")
Found 1 error in 1 file (checked 1 source file)

Links

See also: https://github.com/python/mypy

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 25, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK - assuming the types are actually checked/enforced in some way, documentation is written for contributors etc. As Marco mentioned, shotgunning type annotations all over the code base without anything actually using them is likely to just lead them them being out of date and/or confusing.

I did look at doing this for our test framework at one point, but think I came to the conclusion it wasn't quite worth it while we are still supporting 3.5+, given you are basically limited to function param and return type annotations. However maybe we can require 3.6+ at some point in the near future?

I did end up implementing some types + type checking via mypy for the HWI project in bitcoin-core/HWI#203, so you could check that out for some example usage.

@@ -90,6 +90,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):

This class also contains various public and private helper methods."""

chain: str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use these annotations while we are still supporting Python 3.5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's unfortunately true. However, Python 3.5 EOL is 09/2020. Is that a sufficient reason to bump Python version, or what is the policy for upgrade here?

@fanquake fanquake changed the title Type hints in Python tests test: type hints in Python tests Feb 26, 2020
@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch 5 times, most recently from e70e2e9 to f532426 Compare February 26, 2020 09:13
@kiminuo
Copy link
Contributor Author

kiminuo commented Feb 26, 2020

  1. Python 3.5 EOL is 09/2020. Maybe Python may be bumped to 3.6 in not too distant future. What is the policy for Python upgrade?
  2. Should a specific version of mypy be installed?

@practicalswift
Copy link
Contributor

Concept ACK on checking type hints using mypy and gradually adding type hints to our Python code base :)

@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch from f532426 to c23f7af Compare February 26, 2020 12:20
@maflcko
Copy link
Member

maflcko commented Feb 26, 2020

https://packages.ubuntu.com/xenial/python3 is still supported and not EOL until April next year, so we can't bump to python 3.6 yet.

@kiminuo
Copy link
Contributor Author

kiminuo commented Feb 26, 2020

@MarcoFalke So Ubuntu provides support for Python 3.5 even after Python developers do not support the software? I thought that this is relevant source of information: https://devguide.python.org/#status-of-python-branches (saying EOF is 2020-09-13)

@laanwj
Copy link
Member

laanwj commented Feb 26, 2020

Agree that they need to be checked in the CI, but very much concept ACK, thanks for doing this.

@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch 7 times, most recently from 65fa76b to 150e950 Compare February 27, 2020 22:28
@kiminuo kiminuo requested a review from fanquake February 27, 2020 22:28
@kiminuo kiminuo marked this pull request as ready for review February 27, 2020 22:30
@elichai
Copy link
Contributor

elichai commented Mar 1, 2020

Concept ACK cool to hear about MyPy, tested it locally and it's pretty nice except for the need to ignore it on all imports. maybe adding this flag is better? https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports

Example, changed:

diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py
--- a/test/functional/feature_cltv.py
+++ b/test/functional/feature_cltv.py
-def cltv_validate(node, tx, height):
+def cltv_validate(node, tx: int, height):
 $ mypy test/functional/feature_cltv.py 
test/functional/test_framework/script.py:19: error: Need type annotation for 'OPCODE_NAMES' (hint: "OPCODE_NAMES: Dict[<type>, <type>] = ...")
test/functional/test_framework/script.py:25: error: Need type annotation for '_opcode_instances' (hint: "_opcode_instances: List[<type>] = ...")
test/functional/test_framework/test_framework.py:599: error: No library stub file for module 'zmq'
test/functional/test_framework/test_framework.py:599: note: (Stub files are from https://github.com/python/typeshed)
test/functional/feature_cltv.py:41: error: "int" has no attribute "vin"
test/functional/feature_cltv.py:42: error: "int" has no attribute "nLockTime"
Found 5 errors in 3 files (checked 1 source file)

@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch from e2a07a0 to fe58ea7 Compare March 17, 2020 20:53
@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch 2 times, most recently from e475947 to 024027e Compare May 4, 2020 09:00
@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch from 024027e to b466ecb Compare May 29, 2020 07:04
@troygiorshev
Copy link
Contributor

Concept ACK. The combination of Type Hinting and mypy is the single best improvement to ever occur to the python ecosystem IMO. However we'll need to go about this gradually. This may be a useful guide: https://mypy.readthedocs.io/en/stable/existing_code.html

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. Fine with me.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, just one question.

@@ -9,6 +9,7 @@ export LC_ALL=C
travis_retry pip3 install codespell==1.15.0
travis_retry pip3 install flake8==3.7.8
travis_retry pip3 install yq
travis_retry pip3 install mypy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be set to a specific version or is this assumed to be a stable program? We had trillion of issues with flake8 and codespell being updated from down under, so I'd like to avoid any such mishaps in the future.

Copy link
Contributor Author

@kiminuo kiminuo Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is a new release every month or so: https://github.com/python/mypy/releases (it should be stable enough though as they seem to fix bugs and improve it, they don't redesign it every month)

So it makes sense to set: travis_retry pip3 install mypy==0.770

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch from b466ecb to 600ba91 Compare June 1, 2020 18:29
@maflcko
Copy link
Member

maflcko commented Jun 1, 2020

ACK 600ba91

Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."

Mypy is used in lint-python.sh to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.

Useful resources:

* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
@kiminuo kiminuo force-pushed the feature/type-hint-minimum branch from 600ba91 to bd7e530 Compare June 2, 2020 06:08
@maflcko
Copy link
Member

maflcko commented Jun 2, 2020

ACK bd7e530 fine with me

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bd7e530 - the type checking is not the most robust (there are things it fails to detect), but I think this is worth adopting (in a limited capacity while we maintain 3.5 compat).

Sanity checked that the linter complains when you change things:

diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
index bc1b5b26f..206e3eebc 100644
--- a/test/functional/test_framework/script.py
+++ b/test/functional/test_framework/script.py
@@ -22,7 +22,7 @@ from .messages import (
 )
 
 MAX_SCRIPT_ELEMENT_SIZE = 520
-OPCODE_NAMES = {}  # type: Dict[CScriptOp, str]
+OPCODE_NAMES = {}  # type: Dict[CScriptOp, int]
 
 def hash160(s):
test/functional/test_framework/script.py:251: error: Dict entry 0 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
test/functional/test_framework/script.py:252: error: Dict entry 1 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
test/functional/test_framework/script.py:253: error: Dict entry 2 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
index 6e72db1d9..14dffcee4 100644
--- a/test/functional/data/invalid_txs.py
+++ b/test/functional/data/invalid_txs.py
@@ -57,7 +57,7 @@ class BadTxTemplate:
     __metaclass__ = abc.ABCMeta
 
     # The expected error code given by bitcoind upon submission of the tx.
-    reject_reason = ""  # type: Optional[str]
+    reject_reason = ""  # type: Optional[bool]
test/functional/data/invalid_txs.py:60: error: Incompatible types in assignment (expression has type "str", variable has type "Optional[bool]")
test/functional/data/invalid_txs.py:84: error: Incompatible types in assignment (expression has type "str", base class "BadTxTemplate" defined the type as "Optional[bool]")

@fanquake fanquake merged commit 234faba into bitcoin:master Jun 3, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 3, 2020
bd7e530 This PR adds initial support for type hints checking in python scripts. (Kiminuo)

Pull request description:

  This PR adds initial support for type hints checking in python scripts.

  Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."

  [Mypy](https://mypy.readthedocs.io/en/latest/index.html) is used in `lint-python.sh` to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.

  **Notes:**

  * [--ignore-missing-imports](https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports) switch is passed on to `mypy` checker for now. The effect of this is that one does not need `# type: ignore` for `import zmq`. More information about import processing can be found [here](https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports). This can be changed in a follow-up PR, if it is deemed useful.
  * We are stuck with Python 3.5 until 04/2021 (see https://packages.ubuntu.com/xenial/python3). When Python version is bumped to 3.6+, one can change:
      ```python
      _opcode_instances = []  # type: List[CScriptOp]
      ```
      to
      ```python
      _opcode_instances:List[CScriptOp] = []
      ```
      for type hints that are **not** function parameters and function return types.

  **Useful resources:**

  * https://docs.python.org/3.5/library/typing.html
  * https://www.python.org/dev/peps/pep-0484/

ACKs for top commit:
  fanquake:
    ACK bd7e530 - the type checking is not the most robust (there are things it fails to detect), but I think this is worth adopting (in a limited capacity while we maintain 3.5 compat).
  MarcoFalke:
    ACK bd7e530 fine with me

Tree-SHA512: 21ef213915fb1dec6012f59ef17484e6c9e0abf542a316b63d5f21a7778ad5ebabf8961ef5fc8e5414726c2ee9c6ae07c7353fb4dd337f8fcef5791199c8987a
@hebasto
Copy link
Member

hebasto commented Jun 5, 2020

I have linter warnings on master (4ede05d):

$ python3 --version
Python 3.6.9
$ ./test/lint/lint-python.sh
test/functional/data/invalid_txs.py:24:1: F401 'typing.Optional' imported but unused
test/functional/test_framework/script.py:12:1: F401 'typing.List' imported but unused
test/functional/test_framework/script.py:12:1: F401 'typing.Dict' imported but unused
Success: no issues found in 168 source files

UPDATE: False positive warnings due to the outdated local flake8 version:

$ pip3 show flake8 | grep Version
Version: 3.5.0

fanquake added a commit that referenced this pull request Jun 6, 2020
5d77549 doc: Add mypy to test dependencies (Hennadii Stepanov)
7dda912 test: Do not swallow flake8 exit code (Hennadii Stepanov)

Pull request description:

  After #18210 the `flake8` exit code in `test/lint/lint-python.sh` just not used that makes the linter broken.

  This PR:
  - combines exit codes of `flake8` and `mypy` into the  `test/lint/lint-python.sh` exit code
  - documents `mypy` as the test dependency

ACKs for top commit:
  MarcoFalke:
    Approach ACK 5d77549, fine with me
  practicalswift:
    ACK 5d77549

Tree-SHA512: e948ba04dc4d73393967ebf3c6a26c40d428d33766382a0310fc64746cb7972e027bd62e7ea76898b742a656cf7d0fcda2fdd61560a21bfd7be249cea27f3d41
@@ -9,6 +9,7 @@ export LC_ALL=C
travis_retry pip3 install codespell==1.15.0
travis_retry pip3 install flake8==3.7.8
travis_retry pip3 install yq
travis_retry pip3 install mypy==0.700

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you set 0.700 instead of 0.770 like you mentioned in this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I screwed up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be bumped because version 700 can not be compiled with python3.8-dev

@troygiorshev troygiorshev mentioned this pull request Jun 26, 2020
3 tasks
@kiminuo kiminuo deleted the feature/type-hint-minimum branch December 15, 2020 18:41
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 24, 2021
Summary:
These tests no longer work, and they test trivial stuff that is already
tested in every single existing functional test (starting and stopping
nodes...)

I noticed them when testing `mypy`, a lint tool that will be introduced when backporting [[bitcoin/bitcoin#18210 | core#18210]]

See D237

Test Plan: `ninja check-functional`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D9271
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 25, 2021
Summary:
> Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
>
> Mypy is used  to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
>
> Useful resources:
>
> * https://docs.python.org/3.5/library/typing.html
> * https://www.python.org/dev/peps/pep-0484/

Note: Core still depended on python 3.5 when this was merged, so they had to use old style "type comments". With Python 3.6, it becomes possible to use the typehint syntax for variables and attributes. Unfortunately Postponed Evaluation of Annotations is only available starting from python 3.7, so for now  e have to use quotes when annotating types (classes) that are only defined later in a module.

This is a backport of [[bitcoin/bitcoin#18210 | core#18210]]

Depends on D9270

Test Plan:
`ninja check-functional`

```
sudo arc liberate
arc lint --trace
```

Add an intentional tyephint mistake:
```
diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
index c3d39d8..76179e742 100644
--- a/test/functional/test_framework/script.py
+++ b/test/functional/test_framework/script.py
@@ -23,7 +23,7 @@ from .messages import (
 )

-MAX_SCRIPT_ELEMENT_SIZE = 520
+MAX_SCRIPT_ELEMENT_SIZE: bool = 520
 OPCODE_NAMES: Dict["CScriptOp", str] = {}

```

Run `arc lint` and check that it catches the problem:
```
>>> Lint for test/functional/test_framework/script.py:

   Error  () mypy found an issue:
    Incompatible types in assignment (expression has type "int", variable has
    type "bool")

              23 )
              24
              25
    >>>       26 MAX_SCRIPT_ELEMENT_SIZE: bool = 520
                ^
              27 OPCODE_NAMES: Dict["CScriptOp", str] = {}
              28
              29
```

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9273
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants