ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
126 stars 144 forks source link

Type checking with mypy #676

Open haudren-woven opened 2 years ago

haudren-woven commented 2 years ago

Bug report

I am sorry to open an issue in such short order, but as part of my work at Woven Planet, I'm trying to use the ROS2 launch system. Unfortunately, I have found that it type information is currently unusable from an external package, thus I'd like to improve it.

Required Info:

Steps to reproduce issue

  1. Create a snippet with this content:
    
    import launch

context = launch.LaunchContext() reveal_type(context)

2. Install mypy
3. Run the following:

MYPYPATH=$PATH_TO_WS/install/launch/lib/python3.8/site-packages/ mypy -m launch

(mypy does not use PYTHONPATH by default)

#### Expected behavior

test.py:4: note: Revealed type is 'launch.launch_context.LaunchContext'


#### Actual behavior

test.py:1: error: Cannot find implementation or library stub for module named 'launch' test.py:1: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports test.py:4: note: Revealed type is 'Any'



## Feature request

#### Feature description
I'd like to make the type checking in launch (and potentially other ROS2 core libraries) a little bit stricter. This would allow to enforce a quality standard, and root out bugs at the base. It would also make the code easier to use from the outside, since type checking would prevent some common bugs in launch files, such as using a substitution where a condition is expected. Since type-checking is strictly opt-in, there would be no impact on the broader community.

#### Implementation considerations
- I have already implemented adding the `py.typed` file to mark the `launch` package as typed to the broader system, and can submit a PR promptly
- However, there are many type-checking issues in the current codebase, including bugs. For example, mypy reported that [this function](https://github.com/ros2/launch/blob/6783ca8988b3ee1910e3eb6de7388407c8c0ce0d/launch/launch/event_handler.py#L92) does not have a return statement, contradicting its signature. Similarly, [this statement](https://github.com/ros2/launch/blob/6783ca8988b3ee1910e3eb6de7388407c8c0ce0d/launch/launch/frontend/parser.py#L213) has invalid type syntax. Probably someone meant `Tuple`? In total mypy reports 224 errors in 47 files.
- Thus, I think we should also add a CI test to check the type information using `mypy`. This is I think very straightforward using [ament_mypy](https://index.ros.org/p/ament_mypy/) in the same way that you are currently using `ament_pep257` and others.

I can take care of the above points, but I'd like to make sure that the maintainers agree with this approach before making a PR full of tiny changes.

Thank you very much!
ros-discourse commented 2 years ago

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/python-type-checking-in-ros2/28507/1

haudren-woven commented 1 year ago

@adityapande-1995 @wjwwood : Any opinions on this? Would you be opposed to some PRs that would improve the situation?

clalancette commented 1 year ago

In short, I think we would welcome additions to the typing (there are already some open PRs for this).

I'm also interested in adding ament_mypy by default, but before we do that I'd want to get a good idea on how much extra time it adds to CI. Our CI already takes many hours, so I don't want to increase that too much (especially on Windows).

haudren-woven commented 1 year ago

I can add that :+1: In my personal testing, running mypy itself only takes a few seconds.

Found 221 errors in 47 files (checked 121 source files)
mypy --follow-imports=silent src/launch/launch  1.07s user 0.07s system 98% cpu 1.163 total