From 1a0c42d367645fcacda5b25a669ae7d0dc19356e Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Fri, 7 Aug 2026 17:12:14 +0200 Subject: [PATCH] Fix several issues related to type hints and signature parameters --- CHANGELOG.rst | 17 +++++ DOCUMENTATION.rst | 8 ++- jsonargparse/_parameter_resolvers.py | 10 ++- jsonargparse/_signatures.py | 25 ++++--- jsonargparse/_typehints.py | 58 ++++++++++++++++- jsonargparse_tests/test_signatures.py | 32 +++++++++ jsonargparse_tests/test_subclasses.py | 22 ++++++- jsonargparse_tests/test_typehints.py | 93 ++++++++++++++++++++++++++- 8 files changed, 244 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 67ab4990..43c83d33 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -43,6 +43,9 @@ Added Previously adding an argument with these types failed with ``TypeError: 'member_descriptor' object is not iterable`` (`#945 `__). +- Support ``Collection``, ``Container`` and ``Reversible``, validated as a list, + and ``AbstractSet``, validated as a set (`#950 + `__). Fixed ^^^^^ @@ -81,6 +84,20 @@ Fixed as the instances given for a subclass type, i.e. as an import path when the value can be imported back, otherwise as a message that says that it was not serializable (`#948 `__). +- ``AssertionError`` without a message when adding an argument typed as a + subscripted user defined generic class, e.g. ``Optional[Strategy[T]]`` (`#950 + `__). +- A ``Callable`` type accepting values that are neither callable nor an import + path, e.g. a list, which in a union such as ``Union[Callable, + list[Callable]]`` prevented the list items from being resolved (`#950 + `__). +- ``type[T]`` never validating. Now the ``TypeVar`` is replaced by its bound, + its constraints, or ``object`` when it has neither, so that the help shows + what is accepted, e.g. ``type[object]`` (`#950 + `__). +- Parameters of a subscripted generic class being dropped when their type is a + PEP 604 union, e.g. ``p: int | None`` in a ``Generic[T]`` class added as + ``MyClass[int]`` (`#950 `__). Changed ^^^^^^^ diff --git a/DOCUMENTATION.rst b/DOCUMENTATION.rst index b64b93a0..17e59231 100644 --- a/DOCUMENTATION.rst +++ b/DOCUMENTATION.rst @@ -520,7 +520,8 @@ Some notes about this support are: - Fully supported types are: ``str``, ``bool`` (more details in :ref:`boolean-arguments`), ``int``, ``float``, ``Decimal``, ``complex``, ``bytes``/``bytearray`` (Base64 encoding), ``range``, ``list`` (more details - in :ref:`list-append`), ``Deque``, ``Iterable``, ``Sequence``, ``Any``, + in :ref:`list-append`), ``Deque``, ``Iterable``, ``Sequence``, + ``MutableSequence``, ``Collection``, ``Container``, ``Reversible``, ``Any``, ``Union``/``Optional`` (more details in :ref:`union-types`), ``Type``, ``Enum``, ``PathLike``, ``UUID``, ``timedelta``, restricted types as explained in sections :ref:`restricted-numbers` and :ref:`restricted-strings` and path @@ -541,8 +542,9 @@ Some notes about this support are: i.e. it has all the keys of the expected ``TypedDict``, with the same types and requiredness. -- ``tuple``, ``set``, ``frozenset`` and ``MutableSet`` are supported even though - they can't be represented in JSON distinguishable from a list. Each ``tuple`` +- ``tuple``, ``set``, ``frozenset``, ``AbstractSet`` and ``MutableSet`` are + supported even though they can't be represented in JSON distinguishable from + a list. Each ``tuple`` element position can have its own type and will be validated as such. ``tuple`` with ellipsis (``tuple[type, ...]``) is also supported. In command line arguments, config files and environment variables, tuples and sets are diff --git a/jsonargparse/_parameter_resolvers.py b/jsonargparse/_parameter_resolvers.py index 46b4bb00..087e60a1 100644 --- a/jsonargparse/_parameter_resolvers.py +++ b/jsonargparse/_parameter_resolvers.py @@ -302,14 +302,18 @@ def get_signature_parameters_and_indexes(component, parent, logger): def replace_generic_type_vars(params: ParamList, parent) -> None: if is_generic_class(parent) and parent.__args__ and getattr(parent.__origin__, "__parameters__", None): + from ._typehints import rebuild_typehint_args + type_vars = dict(zip(parent.__origin__.__parameters__, parent.__args__)) def replace_type_vars(annotation): if annotation in type_vars: return type_vars[annotation] - if getattr(annotation, "__args__", None): - origin = annotation.__origin__ - return origin[tuple(replace_type_vars(a) for a in annotation.__args__)] + args = getattr(annotation, "__args__", None) + # only a tuple, since e.g. types.UnionType has __args__ as a class level slot + # descriptor, which is truthy but not the subtypes of an instance + if isinstance(args, tuple) and args: + return rebuild_typehint_args(annotation, tuple(replace_type_vars(a) for a in args)) return annotation for param in params: diff --git a/jsonargparse/_signatures.py b/jsonargparse/_signatures.py index df1f008b..38346d2c 100644 --- a/jsonargparse/_signatures.py +++ b/jsonargparse/_signatures.py @@ -341,15 +341,25 @@ def _add_signature_parameter( name = param.name kind = param.kind annotation = param.annotation - register_pydantic_types(annotation) # before the check of what can be validated + src = get_parameter_origins(param.component, param.parent) + skip_message = f'Skipping parameter "{name}" from "{src}" because of: ' + # Before anything is done with the annotation, so that a type that jsonargparse + # is unable to handle can be worked around by skipping the parameter. + if skip and name in skip: + self.logger.debug(skip_message + "Parameter requested to be skipped.") + return unvalidated: list = [] - unvalidatable_replaced = replace_unvalidatable_typehints(annotation, unvalidated) + try: + register_pydantic_types(annotation) # before the check of what can be validated + unvalidatable_replaced = replace_unvalidatable_typehints(annotation, unvalidated) + except Exception as ex: + raise ValueError(f'Unable to add parameter "{name}" from "{src}": {ex}') from ex if unvalidated: reasons = " ".join(f"{u.name}: {u.reason}." for u in unvalidated) self.logger.debug( - f'Parameter "{name}" from "{get_parameter_origins(param.component, param.parent)}" has ' - f"a type that can't be fully validated: {annotation}. {reasons} These parts are shown " - "in the help as Unvalidated<...> and accept any value without validation." + f'Parameter "{name}" from "{src}" has a type that can\'t be fully validated: ' + f"{annotation}. {reasons} These parts are shown in the help as Unvalidated<...> " + "and accept any value without validation." ) annotation = unvalidatable_replaced if default == inspect_empty: @@ -380,8 +390,6 @@ def _add_signature_parameter( is_non_positional = False # Can be positional else: raise RuntimeError(f"The code should never reach here: kind={kind}") # pragma: no cover - src = get_parameter_origins(param.component, param.parent) - skip_message = f'Skipping parameter "{name}" from "{src}" because of: ' if annotation != inspect_empty: # Checked before linked_targets and fail_untyped adjust is_required, since the wrappers # are meant to agree with the requiredness that the signature itself defines. @@ -405,9 +413,6 @@ def _add_signature_parameter( is_required_link_target = True if not is_required and name[0] == "_": return - elif skip and name in skip: - self.logger.debug(skip_message + "Parameter requested to be skipped.") - return if is_factory_class(default): default = param.parent.__dataclass_fields__[name].default_factory() if annotation == inspect_empty and not is_required: diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index ff2f2207..f5442089 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -20,8 +20,11 @@ from operator import or_ from types import FunctionType, GenericAlias, MappingProxyType, ModuleType, UnionType from typing import ( + AbstractSet, Any, Callable, + Collection, + Container, Deque, Dict, ForwardRef, @@ -34,6 +37,7 @@ MutableSequence, MutableSet, NoReturn, + Reversible, Sequence, Set, Tuple, @@ -54,6 +58,7 @@ remove_actions, ) from ._common import ( + get_generic_origin, get_parsing_setting, get_unaliased_type, is_generic_class, @@ -134,10 +139,16 @@ def _capture_typing_extension_shadows(name: str, *collections) -> None: FrozenSet, Deque, deque, + Collection, + Container, Iterable, + Reversible, Sequence, MutableSequence, + abc.Collection, + abc.Container, abc.Iterable, + abc.Reversible, abc.Sequence, abc.MutableSequence, Tuple, @@ -146,7 +157,9 @@ def _capture_typing_extension_shadows(name: str, *collections) -> None: FrozenSet, set, frozenset, + AbstractSet, MutableSet, + abc.Set, abc.MutableSet, Dict, dict, @@ -175,16 +188,22 @@ def _capture_typing_extension_shadows(name: str, *collections) -> None: leaf_or_root_types = leaf_types.union(root_types) -tuple_set_origin_types = {Tuple, tuple, Set, set, frozenset, MutableSet, abc.Set, abc.MutableSet} +tuple_set_origin_types = {Tuple, tuple, Set, set, frozenset, AbstractSet, MutableSet, abc.Set, abc.MutableSet} sequence_origin_types = { List, list, Deque, deque, + Collection, + Container, Iterable, + Reversible, Sequence, MutableSequence, + abc.Collection, + abc.Container, abc.Iterable, + abc.Reversible, abc.Sequence, abc.MutableSequence, } @@ -339,7 +358,7 @@ def __init__(self, typehint: type | None = None, enable_path: bool = False, **kw kwargs["logger"].debug(f"Discarding unsupported subtypes {discard} from {typehint}") subtypes = tuple(t for t, s in zip(typehint.__args__, subtype_supported) if s) typehint = Union[subtypes] - self._typehint = sort_unions_in_typehint(typehint) + self._typehint = sort_unions_in_typehint(replace_type_vars_in_type_subtype(typehint)) self._enable_path = False if is_pathlike(typehint) else enable_path elif "_typehint" not in kwargs: raise ValueError("Expected typehint keyword argument.") @@ -1453,6 +1472,10 @@ def adapt_typehints( partial_skip_args=partial_skip_args, prev_val=prev_val, ) + elif not callable(val): + # e.g. a list, which without this would be accepted unchanged, silently + # preventing the resolution by other subtypes when part of a union + raise ImportError(f"Expected an import path or a subclass spec, but got {val_input}") except (ImportError, AttributeError, ArgumentError) as ex: raise_unexpected_value(f"Type {typehint} expects a function or a callable class: {ex}", val, ex) @@ -1838,7 +1861,9 @@ def yield_class_types(typehint, is_single, also_lists=False, callable_return=Fal for subtype in typehint.__args__: yield from yield_class_types(subtype, **kwargs) if is_single(typehint, typehint_origin): - yield typehint + # a subscripted user defined generic, e.g. Strategy[T], is yielded as its origin + # class, since the consumers use the class itself, e.g. to look up its subclasses + yield get_generic_origin(typehint) def get_subclass_types(typehint, also_lists=False, callable_return=False): @@ -2157,6 +2182,33 @@ def union_subtype_sort_key(subtype) -> int: return 0 +def replace_type_vars_in_type_subtype(typehint): + """Returns the type hint with the TypeVar subtype of all its type[...] replaced, including nested ones. + + Done when an argument is added, since a TypeVar can't be used to validate. + What a TypeVar stands for is given by its bound or its constraints, and + ``object`` when it has neither, i.e. any class. The help then shows what is + accepted, e.g. ``type[object]`` instead of ``type[~T]``. + """ + if get_typehint_origin(typehint) in literal_types: + return typehint # the args of a Literal are values, not types + args = getattr(typehint, "__args__", None) + # only a tuple, since e.g. types.UnionType and types.GenericAlias have __args__ as a + # class level slot descriptor, which is truthy but not the subtypes of an instance + if not isinstance(args, tuple) or not args: + return typehint + if get_typehint_origin(typehint) in {Type, type} and isinstance(args[0], TypeVar): + if args[0].__constraints__: + new_args = (Union[args[0].__constraints__],) + else: + new_args = (args[0].__bound__ or object,) + else: + new_args = tuple(replace_type_vars_in_type_subtype(a) for a in args) + if all(new is old for new, old in zip(new_args, args)): + return typehint + return rebuild_typehint_args(typehint, new_args) + + def sort_unions_in_typehint(typehint): """Returns the type hint with the subtypes of all its unions sorted, including nested ones. diff --git a/jsonargparse_tests/test_signatures.py b/jsonargparse_tests/test_signatures.py index edc81146..7a469cc3 100644 --- a/jsonargparse_tests/test_signatures.py +++ b/jsonargparse_tests/test_signatures.py @@ -342,6 +342,23 @@ def test_add_class_skip_parameter_debug_logging(parser, logger): assert "because of: Parameter requested to be skipped" in logs.getvalue() +class UnusableType: + def __init__(self, a1: int = 1, a2: Optional[Namespace] = None): + pass # pragma: no cover + + +def test_add_class_skip_parameter_with_unusable_type(parser): + parser.add_class_arguments(UnusableType, "c", skip={"a2"}) + assert parser.parse_args([]) == Namespace(c=Namespace(a1=1)) + + +def test_add_class_parameter_with_unusable_type_error_context(parser): + with pytest.raises(ValueError) as ctx: + parser.add_class_arguments(UnusableType, "c") + ctx.match('Unable to add parameter "a2" from ".*UnusableType.__init__"') + ctx.match("jsonargparse.Namespace is only intended for parsing results") + + class WithinSubcommand: def __init__(self, a: int = 1): self.a = a @@ -427,6 +444,21 @@ def test_add_class_generics(parser): assert cfg.p == Namespace(a=5, b=6 + 7j) +class WithGenericsPep604Union(Generic[X]): + def __init__(self, a: X | None = None, b: int | None = None): # pragma: no cover + self.a = a + self.b = b + + +def test_add_class_generics_pep604_union(parser): + parser.add_class_arguments(WithGenericsPep604Union[int], "p") + cfg = parser.parse_args(["--p.a=5", "--p.b=6"]) + assert cfg.p == Namespace(a=5, b=6) + help_str = get_parser_help(parser) + assert "--p.a A" in help_str + assert "--p.b B" in help_str + + class UnmatchedDefaultType: def __init__(self, p1: str, p2: bool = "deprecated"): # type: ignore[assignment] self.p2 = p2 diff --git a/jsonargparse_tests/test_subclasses.py b/jsonargparse_tests/test_subclasses.py index 9ea20e19..fd3eb756 100644 --- a/jsonargparse_tests/test_subclasses.py +++ b/jsonargparse_tests/test_subclasses.py @@ -9,7 +9,7 @@ from dataclasses import dataclass from gzip import GzipFile from pathlib import Path -from typing import Any, Dict, Iterable, List, Mapping, Optional, Protocol, Union +from typing import Any, Dict, Generic, Iterable, List, Mapping, Optional, Protocol, Type, TypeVar, Union from unittest.mock import patch from uuid import NAMESPACE_OID @@ -2234,6 +2234,26 @@ def test_subclass_class_name_ambiguous(parser, option): parser.parse_args([f"{option}=LocaleTextCalendar"]) +T = TypeVar("T") + + +class GenericStrategy(Generic[T]): + def __init__(self, schema: Optional[Type[T]] = None): + self.schema = schema + + +def test_subclass_generic_alias_in_union(parser): + parser.add_argument("--op", type=Optional[GenericStrategy[T]]) + help_str = get_parser_help(parser) + assert "Show the help for the given subclass of GenericStrategy" in help_str + help_str = get_parse_args_stdout(parser, [f"--op.help={__name__}.GenericStrategy"]) + assert "--op.schema" in help_str + cfg = parser.parse_args([f"--op={__name__}.GenericStrategy"]) + assert cfg.op.class_path == f"{__name__}.GenericStrategy" + init = parser.instantiate(cfg) + assert isinstance(init.op, GenericStrategy) + + def test_subclass_help_not_subclass(parser): parser.add_argument("--op", type=BaseC) with pytest.raises(ArgumentError) as ctx: diff --git a/jsonargparse_tests/test_typehints.py b/jsonargparse_tests/test_typehints.py index 5f9bc697..672ce7d5 100644 --- a/jsonargparse_tests/test_typehints.py +++ b/jsonargparse_tests/test_typehints.py @@ -16,8 +16,11 @@ from textwrap import dedent from types import GenericAlias, MappingProxyType, ModuleType, UnionType from typing import ( + AbstractSet, Any, Callable, + Collection, + Container, Deque, Dict, FrozenSet, @@ -29,6 +32,7 @@ NoReturn, Optional, Protocol, + Reversible, Sequence, Set, Tuple, @@ -320,6 +324,36 @@ def test_type_typehint_help_known_subclasses(parser): assert f"known subclasses: {__name__}.BaseC," in help_str +UnboundVar = TypeVar("UnboundVar") +BoundVar = TypeVar("BoundVar", bound=BaseC) +ConstrainedVar = TypeVar("ConstrainedVar", int, str) + + +def test_type_typehint_unbound_typevar_arg(parser): + parser.add_argument("--cls", type=type[UnboundVar]) + assert parser.parse_args([f"--cls={__name__}.SubC"]).cls is SubC + assert parser.parse_args(["--cls=uuid.UUID"]).cls is uuid.UUID + pytest.raises(ArgumentError, lambda: parser.parse_args(["--cls=time.time"])) + assert "(type: type[object], default: null)" in get_parser_help(parser) + + +def test_type_typehint_bound_typevar_arg(parser): + parser.add_argument("--cls", type=Optional[type[BoundVar]]) + assert parser.parse_args([f"--cls={__name__}.SubC"]).cls is SubC + pytest.raises(ArgumentError, lambda: parser.parse_args(["--cls=uuid.UUID"])) + help_str = get_parser_help(parser) + assert f"(type: {type_to_str(Optional[type[BaseC]])}, default: null" in help_str + assert f"known subclasses: {__name__}.BaseC, {__name__}.SubC" in help_str + + +def test_type_typehint_constrained_typevar_arg(parser): + parser.add_argument("--cls", type=type[ConstrainedVar]) + assert parser.parse_args(["--cls=builtins.str"]).cls is str + pytest.raises(ArgumentError, lambda: parser.parse_args(["--cls=uuid.UUID"])) + expected = type_to_str(type[Union[int, str]]) + assert f"(type: {expected}, default: null)" in get_parser_help(parser) + + # enum tests @@ -404,6 +438,17 @@ def test_frozenset(parser): ctx.match("Expected a ") +@pytest.mark.parametrize("set_type", [AbstractSet, abc.Set], ids=str) +def test_abstract_set(parser, set_type): + parser.add_argument("--set", type=set_type[int]) + cfg = parser.parse_args(["--set=[1, 2]"]) + assert {1, 2} == cfg.set + assert parser.dump(cfg, format="json") == '{"set":[1,2]}' + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(['--set=["a", "b"]']) + ctx.match("Expected a ") + + # tuple tests @@ -458,11 +503,34 @@ def test_tuple_union(parser, tmp_cwd): @parser_modes -@pytest.mark.parametrize("list_type", [Iterable, List, Sequence], ids=str) +@pytest.mark.parametrize( + "list_type", + [Iterable, List, Sequence, Collection, Container, Reversible, abc.Collection, abc.Container, abc.Reversible], + ids=str, +) def test_list_variants(parser, list_type): parser.add_argument("--list", type=list_type[int]) cfg = parser.parse_args(["--list=[1, 2]"]) assert [1, 2] == cfg.list + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(['--list=["a"]']) + ctx.match("Expected a ") + + +class WithCollection: + def __init__(self, allowed: Optional[Collection[str]] = None): + self.allowed = allowed + + +def test_collection_signature_parameter(parser): + parser.add_class_arguments(WithCollection, "t") + expected = type_to_str(Optional[Collection[str]]) + assert f"(type: {expected}, default: null)" in get_parser_help(parser) + cfg = parser.parse_args(['--t.allowed=["a", "b"]']) + assert cfg.t.allowed == ["a", "b"] + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(["--t.allowed=[1]"]) + ctx.match("Expected a ") def test_deque(parser): @@ -1699,6 +1767,29 @@ def test_callable_list_of_function_paths(parser): ctx.match("Callable expects a function or a callable class") +def test_callable_not_a_function_path(parser): + parser.add_argument("--callable", type=Callable) + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(['--callable=["time.time"]']) + ctx.match("Callable expects a function or a callable class") + ctx.match("Expected an import path or a subclass spec") + + +@pytest.mark.parametrize( + "callable_type", + [ + Optional[List[Callable]], + Union[Callable, List[Callable], None], + Union[List[Callable], Callable, None], + ], + ids=str, +) +def test_callable_union_with_list_of_callables(parser, callable_type): + parser.add_argument("--callables", type=callable_type) + cfg = parser.parse_args(['--callables=["random.randint", "time.time"]']) + assert [random.randint, time.time] == cfg.callables + + class CallableClassPath: def __init__(self, p1: int = 1): self.p1 = p1