Tracking for Static Typing Related Bugs

[InvincibleRMC]

From testing rclpy with the new changes two bugs/issue have appeared.

1. typing.NoReturn is buggy.
   Service and Actions are treated as Callable[[], NoReturn] which just creates all sorts of badness.
   See NoReturn on __new__ and __init__ disables accessing @classmethods and ClassVars directly from the class python/mypy#14044 for more details.

Simple enough fix just replace the more correct typing.NoReturn with None.
#229

2. Assuming only list for containers
   Containers are defined as a Union[list[T], Sequence[T], AbstractSet[T]]. However, in rclpy operations like append and remove are called on these fields when they might not exist.

Unlike 1. this is not a bug since mypy is now correctly exposing incorrect behavior, but I imagine that users might find it cumbersome to have to wrap the result in a more finite type to use any methods at all on them.

[InvincibleRMC]

cc: @christophebedard

Made this to track some of the things I noticed testing out the messages in rclpy since they were released.

[InvincibleRMC]

2. will be simpler with the merge of Deprecate using set for container based input #243 Since the types will at least be compatible with collections.abc.Sequence since it won't be a union with the set type anymore.

[mini-1235]

Hello, we are experiencing new ament_mypy failures after recent syncs,

We are seeing the following errors:

nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py: note: In member "footprintCost" of class "FootprintCollisionChecker":
nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py:69:43: error: Value of type "Sequence[Point32] | AbstractSet[Point32]" is not indexable  [index]
nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py:79:17: error: Value of type "Sequence[Point32] | AbstractSet[Point32]" is not indexable  [index]
nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py: note: In member "footprintCostAtPose" of class "FootprintCollisionChecker":
nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py:216:17: error: Value of type "Sequence[Point32] | AbstractSet[Point32]" is not indexable  [index]
nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py:219:17: error: Value of type "Sequence[Point32] | AbstractSet[Point32]" is not indexable  [index]
nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py:221:13: error: Item "Sequence[Point32]" of "Sequence[Point32] | AbstractSet[Point32]" has no attribute "append"  [union-attr]
nav2_simple_commander/nav2_simple_commander/footprint_collision_checker.py:221:13: error: Item "AbstractSet[Point32]" of "Sequence[Point32] | AbstractSet[Point32]" has no attribute "append"  [union-attr]

These errors correspond to the following code:

oriented_footprint = Polygon()

        for i in range(len(footprint.points)):
            new_pt = Point32()
            new_pt.x = x + (
                footprint.points[i].x * cos_th - footprint.points[i].y * sin_th
            )
            new_pt.y = y + (
                footprint.points[i].x * sin_th + footprint.points[i].y * cos_th
            )
            oriented_footprint.points.append(new_pt)

This was not happening before, and it looks like we would need to add casting or assertions to resolve the mypy errors. Do we expect this to be fixed in the next sync?

cc: @SteveMacenski

[InvincibleRMC]

@mini-1235

1. The index error shouldn't be a problem after the next sync since the AbstractSet[T] input was deprecated and removed from the type hint here.
2. Neither AbstractSet[T] or Sequence[T] implement the append method so it is not safe to use. See more info about Abstract Base Classes here.

[SteveMacenski]

Neither AbstractSet[T] or Sequence[T] implement the append method so it is not safe to use. See more info about Abstract Base Classes here.

I think we understand that, but we feel its not realistic to ask ROS users to need to cast/assert every vector message into a list just to use it. This feels like a regression in the practical use of mypy. It becomes less helpful and more a hurtle if the underlying typing isn't accurate to what's stored or how its commonly used. Many ROS messages have many array types (sensor msgs, geometry msgs, etc) in many pieces of code. It is an appendable data structure (unless I'm vastly misunderstanding these changes and you're modifying not just the type hint stubs but also the actual underlying data structure of the ROS message arrays).

In a similar vein, at the same time we started having issues like:

- def getPaths(results: list[ComputePathToPose.Result]) -> list[Path]:
+ def getPaths(results: Any) -> list[Path]:

That we couldnt use use the result types from actions in the type hints which is a huge issue for a system based on alot of actions interacting with each other. I believe we had a few other types of errors that were introduced, but the indexing, Action Result (and Feedback?), and handling of vector messages are the ones off of the top of my head.

I'd ask for whatever behavior that made these failures appear all of the sudden be reverted or reconsidered. It makes us have to move so many things to Any to a point that mypy no longer serves the purpose. We'd probably have to remove type hinting from Nav2 in this case since it acts as a hurdle rather than a protection. Any doesn't help Any-one 😆

[InvincibleRMC]

In a similar vein, at the same time we started having issues like:

- def getPaths(results: list[ComputePathToPose.Result]) -> list[Path]:
+ def getPaths(results: Any) -> list[Path]:

That we couldnt use use the result types from actions in the type hints which is a huge issue for a system based on alot of actions interacting with each other. I believe we had a few other types of errors that were introduced, but the indexing, Action Result (and Feedback?), and handling of vector messages are the ones off of the top of my head.

The Action Result stuff should be resolved by this.
Note, before August 2025 and this pr. Mypy was treating every message, service, and action as Any so several of things that used to "work" where silently getting turned into Any causing them to pass, but not actually work.

Array types are only guaranteed to be append-able after it goes through a rmw layer. Before that the only guarantees currently are that it is a Sequence.

The type hints are the way they are because they match the current runtime behavior. Some user could make their own SummableSequence class and then pass it into a ROS message for example.

from collections.abc import Sequence
from typing import TypeVar, overload


from test_msgs.msg import Arrays

T = TypeVar("T", int, float)


class SummableSequence(Sequence[T]):
    _data: list[T]

    def __init__(self, *args: T):
        self._data = list(args)

    def sum(self) -> T:
        return sum(self._data)

    def __len__(self) -> int:
        return len(self._data)

    @overload
    def __getitem__(self, index: int) -> T: ...
    @overload
    def __getitem__(self, index: slice) -> Sequence[T]: ...
    def __getitem__(self, index: int | slice) -> T | Sequence[T]:
        return self._data[index]


a = SummableSequence(3)
b = Arrays()
b.int8_values = a

Now the real question is whether the runtime check should be a probably more precise MutableSequence. But, this would be a breaking change and would need to add a deprecation notice and could affect a lot of users and would likely need a vote from the PMC. In my opinion the type hints should match the runtime behavior and if the runtime behavior needs to change then we should change it.

[SteveMacenski]

The Action Result stuff should be resolved by #241.

🥳

Mypy was treating every message, service, and action as Any so several of things that used to "work" where silently getting turned into Any causing them to pass, but not actually work.

🤦

---

So I think the last concern is then the append item. It seems like of wild that some standard code like to append to an array type in a message is no longer expected to be compliant. That doesn't seem like a realistic use IMO. I can't imagine how much code would need to change / unexpectedly doesn't work when enabling mypy onto a repo of major python use.

      oriented_footprint = Polygon()
      for i in range(10):
            new_pt = Point32()
            oriented_footprint.points.append(new_pt)

[InvincibleRMC]

Experiments for allowing append can be found at #244

[InvincibleRMC]

Ok so the PMC agreed to my proposal to the solution will be to make message fields will keep taking any Sequence but, will cast to list if not, so then the typing API can then be updated. But Note: due to this old issue that was recently fixed this we can't use differing return types on setters and getters. And unfortunately the upcoming rhel 10 has a mypy version to old to flip the switch yet. So actually type checking sequence and array fields in message is blocked until rhel bumps mypy>=1.16.0. But we set it to Any to avoid leak false positive. so the errors will go away but, mypy will then allow you to do unsafe functions on them.

An example of how this is already being used for array fields for numpy supported fields.
typing.Annotated[typing.Any, numpy.typing.NDArray[numpy.uint8]]: # typing.Annotated can be remove after mypy 1.16+ see mypy#3004

[InvincibleRMC]

Ok #249 has been merged. The append problems should be gone in the next rolling sync.

[christophebedard]

I created a new Rolling release: ros/rosdistro#50085