Repository navigation
Analysis of some mypy adoption problems and the introduction of type: ignore comments #652
Description
Analysis of type: ignore Comments and Potential Fixes
This issue explains why multiple type: ignore comments were introduced
during the mypy adoption (PRs #647, #649, #650, #651) and what API changes
or refactorings would be needed to eliminate them.
Root Cause: Dynamically Typed Value Pipeline
The core issue is that ODVariable.decode_raw() returns
Union[int, float, str, bytes, bytearray] and encode_raw() accepts the same
union. This "wide" union type propagates through the entire Variable property
chain (raw, phys, desc, bits), making it impossible for mypy to know
which concrete type is in play at any given point.
In practice, the concrete type is determined by ODVariable.data_type at
runtime — but mypy cannot track this correlation.
Per-File Analysis
canopen/objectdictionary/__init__.py (7 ignores)
1. encode_raw() — string operations on Union type
return value.encode("ascii") # type: ignore[union-attr]
return value.encode("utf_16_le") # type: ignore[union-attr]
return bytes(value) # type: ignore[arg-type]Why: After the isinstance(value, (bytes, bytearray)) guard, value is
narrowed to Union[int, float, str]. The VISIBLE_STRING / UNICODE_STRING
branches assume value is str, but mypy doesn't correlate data_type with
the value type.
Fix: Use @overload to create typed variants of encode_raw():
@overload
def encode_raw(self, value: str) -> bytes: ...
@overload
def encode_raw(self, value: int) -> bytes: ...
@overload
def encode_raw(self, value: bytes) -> bytes: ...Or split into encode_string(), encode_number(), encode_bytes().
This is a significant API change.
2. encode_raw() — min/max comparisons
if self.min is not None and value < self.min: # type: ignore[operator]
if self.max is not None and value > self.max: # type: ignore[operator]Why: At this point value is Union[int, float, str], but the comparison
only makes sense for numbers. The str type doesn't support </> with int.
Fix: Add an explicit isinstance(value, (int, float)) guard before the
comparisons. This would be safe and backward-compatible:
if isinstance(value, (int, float)):
if self.min is not None and value < self.min:
...3. encode_phys() — division and return
value = int(round(value / self.factor)) # type: ignore[operator]
return value # type: ignore[return-value]Why: value is Union[int, bool, float, str, bytes] but division only
works on numbers. The return value may still be str or bytes when
data_type is not in INTEGER_TYPES.
Fix: Change encode_phys() to accept and return Union[int, float] only.
Callers that pass str/bytes would need to go through encode_raw() instead.
This is an API change.
4. File handling in export_od() / import_od()
dest.close() # type: ignore[union-attr]
filename = source.name # type: ignore[union-attr]Why: dest is Union[str, TextIO, None] — after the opened_here guard,
it's always TextIO. source after hasattr(source, "read") is TextIO.
Fix: Introduce a local variable with explicit narrowing:
if opened_here:
assert isinstance(dest, TextIO)
dest.close()Or restructure using a context manager pattern. This is a safe, non-breaking
change but requires some refactoring of the function flow.
canopen/variable.py (6 ignores)
5. raw property flows into type-specific methods
value = self.od.decode_phys(self.raw) # type: ignore[arg-type]
value = self.od.decode_desc(self.raw) # type: ignore[arg-type]
self.raw = self.od.encode_desc(desc) # type: ignore[assignment]Why: self.raw returns Union[int, bool, float, str, bytes], but
decode_phys() expects int and decode_desc() expects int.
Fix: This is a consequence of the root cause. Possible approaches:
- Make
decode_phys()/decode_desc()accept the full Union and do
runtime validation internally. - Introduce a
Variable.raw_intproperty that returnsintand use it
inphysanddescproperties. - Use
Generictypes parameterized bydata_type— very complex.
6. Bits class raw type
return self.variable.od.decode_bits(self.raw, ...) # type: ignore[arg-type]
self.raw = self.variable.od.encode_bits(self.raw, ...) # type: ignore[arg-type]Why: Bits.raw is Union[int, bool, float, str, bytes] (copied from
Variable.raw), but decode_bits() / encode_bits() expect int.
Fix: Bits should store raw as int and cast on read/write:
def read(self):
raw = self.variable.raw
assert isinstance(raw, int)
self.raw: int = raw7. write() method — desc assignment
self.desc = value # type: ignore[assignment]Why: value is Union[int, bool, float, str, bytes] but desc setter
expects str.
Fix: Add type narrowing or runtime validation in write():
elif fmt == "desc":
if not isinstance(value, str):
raise TypeError("desc format requires str value")
self.desc = valuecanopen/network.py (2 ignores)
8. Node construction with Optional OD
node = RemoteNode(node, object_dictionary) # type: ignore[arg-type]
node = LocalNode(node, object_dictionary) # type: ignore[arg-type]Why: object_dictionary is Union[str, ObjectDictionary, None], but
node constructors don't accept None.
Fix: Make RemoteNode / LocalNode constructors accept None for
object_dictionary and create an empty ObjectDictionary() internally.
Or add proper validation in add_node() / create_node():
if object_dictionary is None:
raise ValueError("object_dictionary is required when node is an int")Note: The current code intentionally allows None to flow through — e.g.
when upload_eds fails, the node is still created with None OD. This is
a design decision that would need discussion.
canopen/sdo/base.py (1 ignore)
9. SdoArray.__len__() return type
return self[0].raw # type: ignore[return-value]Why: self[0].raw is Union[int, bool, float, str, bytes] but
__len__ must return int.
Fix: Cast explicitly:
def __len__(self) -> int:
value = self[0].raw
assert isinstance(value, int)
return valueSummary of Required Changes
| Priority | Change | Impact | Breaking? |
|---|---|---|---|
| Low | encode_raw() min/max guards |
2 ignores removed | No |
| Low | Bits.raw as int |
2 ignores removed | No |
| Low | write() desc validation |
1 ignore removed | No |
| Low | SdoArray.__len__ cast |
1 ignore removed | No |
| Low | File handling narrowing | 2 ignores removed | No |
| Medium | decode_phys/decode_desc accept Union |
3 ignores removed | Minor |
| Medium | Node constructor accept None | 2 ignores removed | Minor |
| High | encode_raw overloads or split |
3 ignores removed | Yes |
| High | encode_phys type narrowing |
2 ignores removed | Yes |
The low priority changes (10 ignores) can be done without any API changes.
The medium/high priority changes (8 ignores) require API discussion and
potentially affect downstream users.
Activity