ci: add Python static analysis check via mypy

[yedpodtrzitko]

I've noticed that there are now type annotation in various Python scripts across the codebase, but the type annotations by themselves are useless if they are not enforced.
Thus I added mypy into the CI pipeline which should check their validity. There were some errors in there, so:

• the easy errors are fixed
• type: ignore was added in cases where there's no straightforward fix
• some complex cases were downgraded (eg. in make_method_signature()) as there was no way this annotation could ever worked
• added a few new annotations here and there.

[YuriSizov]

• some complex cases were downgraded (eg. in make_method_signature()) as there was no way this annotation could ever worked

Why wouldn't they? What's wrong with unions?

[yedpodtrzitko]

Why wouldn't they? What's wrong with unions?

Generally nothing, but this one produces 5 errors (see below), which makes me think the code wasnt tested in the final phase. But I think I figured it out.

doc/tools/make_rst.py:1058: error: Item "AnnotationDef" of "Union[AnnotationDef, MethodDef, SignalDef]" has no attribute "return_type"
doc/tools/make_rst.py:1058: error: Item "SignalDef" of "Union[AnnotationDef, MethodDef, SignalDef]" has no attribute "return_type"
doc/tools/make_rst.py:1062: error: Item "SignalDef" of "Union[AnnotationDef, MethodDef, SignalDef]" has no attribute "qualifiers"
doc/tools/make_rst.py:1073: error: Item "AnnotationDef" of "Union[AnnotationDef, MethodDef, SignalDef]" has no attribute "return_type"
doc/tools/make_rst.py:1073: error: Item "SignalDef" of "Union[AnnotationDef, MethodDef, SignalDef]" has no attribute "return_type"

[YuriSizov]

Well, it was tested and it fully works, but we don't enforce type checks in any way, as you've noticed 🙃 Don't know much about type hints in Python, but weird errors. Why would it check the members for irrelevant types of the union? Or is isinstance(definition, MethodDef) failing for some reason?

[yedpodtrzitko]

The union was fixed, it looks like having isinstance() extracted into a variable does not work well for mypy.

[Calinou]

Note for a future PR: it might be worth looking into adding Flake8 checks to CI, as Flake8 is complementary to Mypy.

If we go forward with this, I'd use this .flake8 file in the repository root:

[flake8]
# Black is used for formatting, ignore Flake8's line length limits.
extend-ignore = E501

For Mypy, I use the following mypy.ini in my projects:

[mypy]
disallow_any_generics = True
disallow_untyped_defs = True
pretty = True
show_column_numbers = True
warn_redundant_casts = True
warn_return_any = True
warn_unreachable = True

[yedpodtrzitko]

Note for a future PR: it might be worth looking into adding Flake8 checks to CI, as Flake8 is complementary to Mypy.

Regarding flake8 - in another PR maybe. I'd like to keep this one small, so no it could be hopefully merged soon before more Python code with more potential mypy errors will be merged.

I did run flake8 preliminary however, and it was showing a lot of false positives since there's a lot of templating-like work in the Python scripts. I know errors can be selectively ignored, but then we'd left almost with nothing essential to do - formatting is already in hands of Black, and the remaining things feel a bit nitpicky. But if the general consensus is in favour of that I'm not gonna oppose.

For Mypy, I use the following mypy.ini in my projects:

I created mypy.ini initially, but then I removed it again as I didnt want to pollute the root of the repo with non-essential files.

[yedpodtrzitko]

mypy.ini added

[mhilbrunner]

Can you squash the commits into one please? :)

[yedpodtrzitko]

Can you squash the commits into one please? :)

done

[asmaloney]

👍 Shouldn't this be added to the commit hooks as well - pre-commit-mypy?

(Or maybe this should be a separate PR?)

[Calinou]

+1 Shouldn't this be added to the commit hooks as well - pre-commit-mypy?

(Or maybe this should be a separate PR?)

I think this should be done in #46235.

[akien-mga]

I'm not very familiar with Python type checks yet but it seems very convoluted to have to do all this to still only be able to use the type hint as a string below... Is that really the recommend workflow?

[yedpodtrzitko]

a Python file can be parsed in two different cases with two different parsers:

• when executed by Python (Python parser)
• when typechecked by mypy (mypy parser)

TYPE_CHECKING variable is set to True when mypy parser is in use and the condition if TYPE_CHECKING: is really the recommended way to distinguish the parser.
The condition can be used for various reasons, usually to avoid needles imports on runtime, or the imported resources might not be installed/present on runtime. In this case I use the condition to make sure the script will be unaffected by the new import even in case the file is used in some other scenario than via SCons.

Why is the import/annotation needed in first place (to answer your comment "I'm not really convinced yet by the usability of type hints in Python") below. Type annotations are by default opt-in, so in order to check what's going on in the function, it needs to be annotated even if the annotation itself seems useless. When annotated, it can catch various kind of errors (like #66659) without any manual testing required.

Anyway, thanks for merging it.

[akien-mga]

Looks good to me overall.

I'm not really convinced yet by the usability of type hints in Python, but if it does help find issues and maintain the code I'm not against it.

[akien-mga]

Thanks!