ament_mypy pytest is not working as intended and pass the CI when it shouldn't

[tomkimsour]

I've tried to make colcon test fail for ament_mypy to confirm my doubt. In order to do so, I removed every typing annotation in the ament_mypy/ament_mypy/main.py After, I on the cli ament_mypy and this error prompts :

8 files checked
2 errors
ament_mypy/main.py: note: In function "_generate_mypy_report":
ament_mypy/main.py:141:5: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
Success: no issues found in 8 source files

Checked files:

* setup.py
* ament_mypy/__init__.py
* ament_mypy/main.py
* ament_mypy/pytest_marker.py
* test/test_ament_mypy.py
* test/test_flake8.py
* test/test_mypy.py
* test/test_xmllint.py

There, we expect colcon tests to fail with the same behavior and raising 2 errors but if you run colcon test --event-handlers console_direct+ --packages-select ament_mypy the following output (please ignore flake8 errors) :

platform linux -- Python 3.10.12, pytest-8.0.2, pluggy-1.4.0
cachedir: /home/user/exchange/mono_repo/build/ament_mypy/.pytest_cache
rootdir: /home/user/exchange/mono_repo/src/ament_lint/ament_mypy
configfile: pytest.ini
plugins: ament-mypy-0.18.1, ament-xmllint-0.18.1, ament-copyright-0.18.1, ament-pep257-0.18.1, ament-flake8-0.18.1, ament-lint-0.18.1, launch-testing-1.0.6, launch-testing-ros-0.19.7, ament-black-0.2.4, anyio-4.4.0, repeat-0.9.3, rerunfailures-14.0, mock-3.6.1, cov-3.0.0, colcon-core-0.17.0
collecting ...
collected 12 items

test/test_ament_mypy.py .........                                        [ 75%]
test/test_flake8.py F                                                    [ 83%]
test/test_mypy.py .                                                      [ 91%]
test/test_xmllint.py .                                                   [100%]

[tomkimsour]

After a quick glance at main.py of ament_mypy I noticed the return value of the function is some kind of error code that is unrelated to the amount of error raised by the program. If we want a similar behavior to ament_pep257 or ament_flake8. The return number of the main.py should be the amount of error found by the linter.

[christophebedard]

I don't think this means that there were errors:

8 files checked
2 errors
ament_mypy/main.py: note: In function "_generate_mypy_report":
ament_mypy/main.py:141:5: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
Success: no issues found in 8 source files

It says "2 errors" because the output has 2 lines: https://github.com/ament/ament_lint/blob/f701809d982b837a76a5e8823994e6893ae0d5fe/ament_mypy/ament_mypy/main.py#L108 It still says "Success: no issues found in 8 source files." And the main output is just a note, not an error.

In short, this is working as intended. It probably shouldn't say "N errors" if there were no actual errors found, though. PRs would be welcome!

[tomkimsour]

Maybe this was a bad example then but I have been running the file https://github.com/ament/ament_lint/blob/rolling/ament_mypy/test/test_mypy.py as a test in my package and it has been passing without issue while the cli command was not. I am still investigating it. Thanks for your quick response tho. If you have example of working example, i'd be happy to look at it.

[christophebedard]

it has been passing without issue while the cli command was not

Can you show an example of a file that ament_mypy complains about when run through the CLI but not when run through colcon test?

[tomkimsour]

I will try to provide you a minimal file yes.

[tomkimsour]

As you mentionned before, the "errors" I was having were misleading and they were more of warnings than proper errors. Hence why the linter would succeed.

However I have a hard time seeing the purpose of mypy as an ament_package if it's not to enforce typing. As is, it is checking anything. I don't feel like it is coming with good default like other linters or formatters could have here. Would it make sense to update the default config file of mypy ?

[christophebedard]

By default, mypy doesn't enforce type annotations, it just validates them. I get your point, but it's not very practical to make that the default for existing codebases that don't yet have type annotations. For example, it would just not work for the ROS 2 code right now, although it will get there one day: https://github.com/ros2/rclpy/issues/1257.

That being said, mypy has these options:

$ python3 -m mypy --help

...

Untyped definitions and calls:
  Configure how untyped definitions and calls are handled. Note: by default, mypy ignores any untyped function definitions and assumes any calls to such functions have a return type of 'Any'.

  --disallow-untyped-calls  Disallow calling functions without type annotations from functions with type annotations (inverse: --allow-untyped-calls)
  --disallow-untyped-defs   Disallow defining functions without type annotations or with incomplete type annotations (inverse: --allow-untyped-defs)
  --disallow-incomplete-defs
                            Disallow defining functions with incomplete type annotations (inverse: --allow-incomplete-defs)
  --check-untyped-defs      Type check the interior of functions without type annotations (inverse: --no-check-untyped-defs)
  --disallow-untyped-decorators
                            Disallow decorating typed functions with untyped decorators (inverse: --allow-untyped-decorators)

...

See also the --strict combo option.

You could enable these options for your own project using a config file. For example: https://github.com/ament/ament_lint/blob/rolling/ament_mypy/ament_mypy/configuration/ament_mypy.ini. You can provide your config file through the ament_mypy main().

[tomkimsour]

I get your point, but it's not very practical to make that the default for existing codebases that don't yet have type annotations.

I agree it would not be best to enforce it at once however this linter is not mandatory and not part of ament_lint_auto either there is no template provided for testing unlike ament_flake8 or ament_pep257. Hence, wouldn't it make sense to set particular defaults for ament_mypy that could enforce typing over ros packages ? If users wants to have mypy default linting behavior then they may as well use their own mypy test.

[christophebedard]

Hence, wouldn't it make sense to set particular defaults for ament_mypy that could enforce typing over ros packages ? If users wants to have mypy default linting behavior then they may as well use their own mypy test.

Or you could flip it and say that users wanting to force typing annotations should just set the necessary config options. Let's see what others think. Since ament_mypy is working as intended, I'd recommend closing this issue and re-opening another one with your request (changing the default config to force typing annotations rather than just checking existing typing annotations).