Make public function type annotations looser and immutable?

[vfazio]

This is something that was partially discussed in:
#2459

When reviewing the v3.14 changelog I noticed:
#2951

To me, it would seem that by typing registers as list[int], two things happen:

1. An IDE/LSP will analyze the type and decide that list has functions that allow mutating that list, like reverse, making this and other functions prone to similar logic issues
2. We're soft-advertising that because the function takes a list, which is mutable, that the function has ownership and thus permission to perform this mutation.

If the object was instead typed as a Sequence (slicing is used internally) we would flip that:

1. Those arguments now no longer advertise an ability to be mutated without first being copied to a mutable type
2. The API is now a bit more flexible in that it can accept any type that meets the interface requirements (iterable, sliceable, etc) while also advertising that the argument being passed in is immutable.

In this way, we leverage the typing system to narrow the safe operations that are allowed on that object as these types of things are very easy to miss in code review and functional tests.

Using the older version of the now fixed function as an example, if I change the type:

(venv) vfazio@vfazio4:~/development/pymodbus$ git diff
diff --git a/pymodbus/client/mixin.py b/pymodbus/client/mixin.py
index f91268cc..ef8528c5 100644
--- a/pymodbus/client/mixin.py
+++ b/pymodbus/client/mixin.py
@@ -4,6 +4,7 @@ from __future__ import annotations
 
 import enum
 import struct
+from collections.abc import Sequence
 from typing import Generic, Literal, TypeVar, cast
 
 from ..constants import ModbusStatus
@@ -905,7 +906,7 @@ class ModbusClientMixin(Generic[T]):  # pylint: disable=too-many-public-methods
     @classmethod
     def convert_from_registers(  # noqa: C901
         cls,
-        registers: list[int],
+        registers: Sequence[int],
         data_type: DATATYPE,
         word_order: Literal["big", "little"] = "big",
         string_encoding: str = "utf-8",

We see the following:

pymodbus/client/mixin.py:931: error: "Sequence[int]" has no attribute "reverse"  [attr-defined]
pymodbus/client/mixin.py:942: error: Argument 1 to "unpack_bitstring" has incompatible type "bytearray"; expected "bytes"  [arg-type]
pymodbus/client/mixin.py:952: error: "Sequence[int]" has no attribute "reverse"  [attr-defined]
pymodbus/client/mixin.py:996: error: Incompatible types in assignment (expression has type "bytearray", variable has type "bytes")  [assignment]
pymodbus/client/mixin.py:998: error: "bytes" has no attribute "extend"  [attr-defined]

(The bytearray stuff is unrelated, but likely due to object promotion that is deprecated in v2 python/mypy#15313)

This could be useful for public methods where we want to enforce this contractual agreement. Maybe there are some public functions where there is an expectation that these items get mutated on purpose, but maybe those should be specialized functions or return values.

I realize API contracts are difficult, especially when they could break compatibility, but widening the types usually eases that burden to some degree and follows the typing guidance from https://typing.python.org/en/latest/reference/best_practices.html#arguments-and-return-types. It does, however have a slight performance cost for any functions that expect to mutate objects in place vs making mutated copies so something that has to be balanced.

Anyway, figured I'd bring this up as an idea in case it seemed useful.

[janiversen]

Feel free to submit one or more PRs.

[janiversen]

And just for reference we use Zuban not mypy, but of course the arguments you give are valid for both.