github-actions[bot] commented on code in PR #40625:
URL: https://github.com/apache/beam/pull/40625#discussion_r4290185961
##########
sdks/python/apache_beam/typehints/decorators.py:
##########
@@ -334,6 +335,25 @@ def from_callable(cls, fn: Callable) ->
Optional['IOTypeHints']:
for param in signature.parameters.values()) and
signature.return_annotation == signature.empty):
return None
+ if (any(isinstance(param.annotation, str)
Review Comment:
**🟡 Medium**: Checking only `isinstance(param.annotation, str)` misses
forward references nested inside generic types, such as `def fn(x:
typing.List['int']) -> typing.Tuple['str', int]:`, where the top-level
annotation is a generic alias containing `typing.ForwardRef` (or `str` args in
`list['int']`) rather than a `str`. And even if another parameter triggers
`typing.get_type_hints`, line 351 skips replacing non-`str` annotations with
their resolved counterparts.
##########
sdks/python/apache_beam/typehints/decorators.py:
##########
@@ -334,6 +335,25 @@ def from_callable(cls, fn: Callable) ->
Optional['IOTypeHints']:
for param in signature.parameters.values()) and
signature.return_annotation == signature.empty):
return None
+ if (any(isinstance(param.annotation, str)
+ for param in signature.parameters.values()) or
+ isinstance(signature.return_annotation, str)):
+ try:
+ resolved = typing.get_type_hints(
+ fn.__init__ if inspect.isclass(fn) else fn)
Review Comment:
**🟠High**: `typing.get_type_hints` evaluates all annotations on the
callable in a single pass and raises `NameError` on the first unresolved name.
If a function has one unresolvable annotation (e.g., a type only imported under
`if TYPE_CHECKING:`), catching `NameError` here discards resolution for **all**
other string annotations on that callable—which is especially problematic when
`from __future__ import annotations` is enabled and every annotation is a
string.
Additionally:
1. `inspect.signature(fn)` supports `functools.partial`, `@functools.wraps`,
callable instances (`__call__`), and classes defining `__new__`, whereas
`typing.get_type_hints(fn.__init__ if inspect.isclass(fn) else fn)` raises
`TypeError` on `partial` objects, callable instances, or classes inheriting
`object.__init__`.
2. `typing.get_type_hints` defaults to `include_extras=False`, which strips
`typing.Annotated` metadata.
Consider unwrapping the target callable (e.g., handling `functools.partial`
and callable instances via `fn.__call__`) and falling back to per-annotation
evaluation against the callable's `__globals__` when
`typing.get_type_hints(..., include_extras=True)` fails so valid annotations on
other parameters or the return value are still preserved.
##########
sdks/python/apache_beam/typehints/decorators_test.py:
##########
@@ -256,6 +256,51 @@ def fn(
}))
self.assertEqual(th.output_types, ((Tuple[Any, ...], ), {}))
+ def test_from_callable_resolves_string_annotations(self):
+ def fn(value: 'int') -> 'typehints.List[str]':
+ return [str(value)]
+
+ th = decorators.IOTypeHints.from_callable(fn)
+ self.assertEqual(th.input_types, ((int, ), {}))
+ self.assertEqual(th.output_types, ((List[str], ), {}))
+
+ def test_from_callable_resolves_future_annotations(self):
+ namespace = {'typehints': typehints}
+ exec(
+ 'from __future__ import annotations\n'
+ 'def fn(value: int) -> typehints.List[str]:\n'
+ ' return [str(value)]\n', namespace)
+
+ th = decorators.IOTypeHints.from_callable(namespace['fn'])
+ self.assertEqual(th.input_types, ((int, ), {}))
+ self.assertEqual(th.output_types, ((List[str], ), {}))
+
+ def test_from_callable_resolves_class_constructor_string_annotations(self):
+ class Class(object):
+ def __init__(self, value: 'int'):
+ self.value = value
+
+ th = decorators.IOTypeHints.from_callable(Class)
+ self.assertEqual(th.input_types, ((int, ), {}))
+ self.assertEqual(th.output_types, ((Class, ), {}))
+
+ def test_from_callable_resolves_method_string_annotations(self):
+ class Class(object):
+ def process(self, value: 'int') -> 'typehints.List[str]':
+ return [str(value)]
+
+ th = decorators.IOTypeHints.from_callable(Class().process)
+ self.assertEqual(th.input_types, ((int, ), {}))
+ self.assertEqual(th.output_types, ((List[str], ), {}))
+
+ def test_from_callable_unresolved_string_annotations_fall_back_to_any(self):
+ def fn(value: 'MissingType') -> 'int':
+ return 1
+
+ th = decorators.IOTypeHints.from_callable(fn)
+ self.assertEqual(th.input_types, ((Any, ), {}))
+ self.assertEqual(th.output_types, ((Any, ), {}))
Review Comment:
**🟠High**: In `def fn(value: 'MissingType') -> 'int':`, the return
annotation `'int'` is a valid builtin type and should resolve to `int` rather
than falling back to `Any`. It currently becomes `Any` only because
`'MissingType'` caused `typing.get_type_hints(fn)` to raise `NameError` before
`'return'` could be resolved.
```suggestion
self.assertEqual(th.output_types, ((int, ), {}))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
