diff --git a/cmd2/annotated.py b/cmd2/annotated.py index 50899b6b4..f0cf46f15 100644 --- a/cmd2/annotated.py +++ b/cmd2/annotated.py @@ -226,7 +226,7 @@ def do_paint( UnboundCompleter, ) -if TYPE_CHECKING: +if TYPE_CHECKING: # pragma: no cover from .argparse_completer import ArgparseCompleter #: ``nargs`` values accepted by cmd2's patched ``add_argument`` (incl. ranged tuples). @@ -1745,8 +1745,19 @@ def _resolve_parameters( f"with_annotated(base_command=True) requires a '{constants.NS_ATTR_SUBCOMMAND_FUNC}' " f"parameter in {func.__qualname__}" ) + # Resolve hints only for the parameters that become arguments: the bound first parameter + # (self/cls), the injected skip_params, and the "return" annotation never become arguments + ignored = {next(iter(sig.parameters), None), "return", *skip_params} + ignored.discard(None) + relevant_annotations = {name: ann for name, ann in getattr(func, "__annotations__", {}).items() if name not in ignored} + # Forward references resolve against the *original* function's module during functools.wraps wrapper. + unwrapped = inspect.unwrap(func) try: - hints = get_type_hints(func, include_extras=True) + hints = get_type_hints( + types.SimpleNamespace(__annotations__=relevant_annotations), + globalns=getattr(unwrapped, "__globals__", {}), + include_extras=True, + ) except (NameError, AttributeError, TypeError) as exc: raise TypeError( f"Failed to resolve type hints for {func.__qualname__}. Ensure all annotations use valid, importable types." @@ -1986,25 +1997,16 @@ def build_parser_from_function( mutually_exclusive_groups=mutually_exclusive_groups, ) - # ``argument_default=argparse.SUPPRESS`` removes an absent argument from the parsed namespace. - # That is safe only for arguments that are always supplied (required) or carry their own default; - # an *omittable* argument with no default (e.g. a ``T | None`` positional -> nargs='?') would be - # dropped when absent, leaving the function without a keyword argument it expects. ``*args`` is - # exempt: the invocation path substitutes an empty tuple for it. Reject the combination here, - # mirroring the per-argument ``default=argparse.SUPPRESS`` rejection. + # ``argument_default=argparse.SUPPRESS`` drops an absent argument from the parsed namespace. + # @with_annotated builds the call from the function signature, so every declared parameter is + # expected at invocation -- an argument vanishing from the namespace can never be valid here. + # Reject it outright, mirroring the per-argument ``default=argparse.SUPPRESS`` rejection. if parser_kwargs.get("argument_default") is argparse.SUPPRESS: - dropped = [ - arg.name - for arg in resolved - if arg.default is _UNSET and arg.omittable and not arg.required and not arg.is_variadic - ] - if dropped: - raise TypeError( - f"argument_default=argparse.SUPPRESS is not supported by @with_annotated for {func.__qualname__}: " - f"it would drop {dropped!r} from the parsed namespace when absent, but the function expects " - f"{'them' if len(dropped) > 1 else 'it'} as a keyword argument. Give each an explicit default or " - f"make it required, or drop argument_default=argparse.SUPPRESS." - ) + raise TypeError( + f"argument_default=argparse.SUPPRESS is not supported by @with_annotated for {func.__qualname__}: " + f"it drops absent arguments from the parsed namespace, but every parameter built from the " + f"signature is expected at invocation. Drop argument_default=argparse.SUPPRESS." + ) # Build the group lookup (member references already validated by _resolve_parameters). target_for, argument_group_for = _build_argument_group_targets(parser, groups=groups) @@ -2124,7 +2126,7 @@ def handler(self_arg: Any, ns: Any) -> Any: filtered = _filtered_namespace_kwargs(ns, accepted=_accepted) if constants.NS_ATTR_SUBCOMMAND_FUNC in filtered: cmd2_h = filtered[constants.NS_ATTR_SUBCOMMAND_FUNC] - if isinstance(cmd2_h, functools.partial) and cmd2_h.func is handler: + if isinstance(cmd2_h, functools.partial) and getattr(cmd2_h.func, "__func__", cmd2_h.func) is handler: filtered[constants.NS_ATTR_SUBCOMMAND_FUNC] = None return _invoke_command_func( func, self_arg, filtered, leading_names=_leading_names, var_positional_name=_var_positional_name diff --git a/docs/features/annotated.md b/docs/features/annotated.md index 04cb4d3c2..33876c595 100644 --- a/docs/features/annotated.md +++ b/docs/features/annotated.md @@ -363,14 +363,11 @@ def do_run(self, verbose: bool = False, quiet: bool = False): ... ``` `parents=` mirrors argparse's standard parents mechanism for sharing argument definitions across -parsers. `argument_default=argparse.SUPPRESS` is accepted only when no argument could be stranded by -it: it removes an absent argument from the parsed namespace, which is safe for an argument that is -always supplied (a required option, a mandatory positional) or that carries its own default, but not -for an _omittable_ argument with no default (for example a `T | None` positional, which becomes -`nargs='?'`). If any such argument is present, `@with_annotated` raises `TypeError` rather than let -the function be called missing a keyword argument it expects (mirroring the per-argument -`default=argparse.SUPPRESS` rejection). `*args` is exempt, since the invocation path substitutes an -empty tuple for it. +parsers. `argument_default=argparse.SUPPRESS` is not supported and raises `TypeError`. It removes an +absent argument from the parsed namespace, but `@with_annotated` builds the call from the function +signature, so every declared parameter is expected at invocation; an argument vanishing from the +namespace can never be valid here (mirroring the per-argument `default=argparse.SUPPRESS` +rejection). Any other `argument_default` value is forwarded to the parser unchanged. The remaining argparse kwargs cover less-common needs but are wired through unchanged: diff --git a/tests/test_annotated.py b/tests/test_annotated.py index 72de56741..a13cf6b3f 100644 --- a/tests/test_annotated.py +++ b/tests/test_annotated.py @@ -9,6 +9,7 @@ import datetime import decimal import enum +import functools import inspect import types import uuid @@ -144,6 +145,54 @@ def _func_positional_only_xy(self, x: str, /, y: int) -> None: ... def _func_positional_only_mixed(self, x: str, /, y: int, *, z: int = 0) -> None: ... +# Forward references with no matching name in this module's globals, so they are unresolvable at +# runtime. They exercise type-hint resolution: hints on parameters that never become arguments +# (``self``/``cls`` and the injected, skipped ``cmd2_statement``/``cmd2_subcommand_func``) must be tolerated, +# while hints on real arguments must still raise. ``noqa: F821`` marks the intentionally-undefined names. +def _func_unresolvable_self(self: "UnimportableCmd", name: str, count: int = 1) -> None: ... # noqa: F821 +def _func_unresolvable_cmd2_statement(self, cmd2_statement: "UnimportableStatement", name: str, count: int = 1) -> None: ... # noqa: F821 +def _func_unresolvable_cmd2_subcommand_func( + self, + cmd2_subcommand_func: "UnimportableHandler", # noqa: F821 + verbose: bool = False, +) -> None: ... +def _func_unresolvable_return(self, name: str) -> "UnimportableReturn": ... # noqa: F821 +def _func_unresolvable_argument(self, name: "NonExistentType") -> None: ... # noqa: F821 +def _func_unresolvable_argument_base(self, cmd2_subcommand_func, name: "NonExistentType") -> None: ... # noqa: F821 + + +def _arg_names_via_parser(func: Any) -> set[str]: + """Resolve argument names through the public ``build_parser_from_function`` entry point.""" + parser = build_parser_from_function(func) + return {action.dest for action in parser._actions if action.dest != "help"} + + +def _arg_names_via_base_command(func: Any) -> set[str]: + """Resolve a base command's argument names (``base_command`` is not exposed on the public builder).""" + from cmd2.annotated import _resolve_parameters + + return {arg.name for arg in _resolve_parameters(func, base_command=True)} + + +def _wrap_in_foreign_module(func: Any) -> Any: + """Wrap *func* as a ``functools.wraps`` decorator would, but give the wrapper a ``__globals__`` + that lacks this module's names. This mimics a user decorator defined in *another* module and + stacked under ``@with_annotated``: ``functools.wraps`` copies ``__annotations__`` but not + ``__globals__``, so a forward reference can only be resolved via ``__wrapped__`` (the original + function's module), not the wrapper's. Rebinding the wrapper's globals is the single-module way + to recreate that cross-module split. + """ + + def wrapper(*args: Any, **kwargs: Any) -> Any: + return func(*args, **kwargs) + + foreign = types.FunctionType( + wrapper.__code__, {"__builtins__": __builtins__}, func.__name__, wrapper.__defaults__, wrapper.__closure__ + ) + functools.update_wrapper(foreign, func) # copies __annotations__ etc. and sets __wrapped__ = func + return foreign + + def _provider(cmd: cmd2.Cmd): return [] @@ -625,22 +674,59 @@ def test_self_only_method_produces_empty_parser(self) -> None: assert dests == set() assert parser.parse_args([]) == argparse.Namespace() - def test_get_type_hints_failure_raises(self) -> None: - def do_broken(self, name: "NonExistentType"): # noqa: F821 - pass + @pytest.mark.parametrize( + ("func", "resolve_arg_names", "expected_args"), + [ + pytest.param(_func_unresolvable_self, _arg_names_via_parser, {"name", "count"}, id="self"), + pytest.param(_func_unresolvable_cmd2_statement, _arg_names_via_parser, {"name", "count"}, id="cmd2_statement"), + pytest.param( + _func_unresolvable_cmd2_subcommand_func, _arg_names_via_base_command, {"verbose"}, id="cmd2_subcommand_func" + ), + pytest.param(_func_unresolvable_return, _arg_names_via_parser, {"name"}, id="return"), + ], + ) + def test_unresolvable_hint_on_ignored_param_is_tolerated(self, func, resolve_arg_names, expected_args) -> None: + """An unresolvable forward reference on an annotation that never becomes an argument must not + abort parser generation -- the bound ``self``/``cls``, the injected, skipped + ``cmd2_statement``/``cmd2_subcommand_func`` parameters, and the function's ``return`` annotation. This + is the common case of annotating with a type only importable under ``TYPE_CHECKING``. Only + hints for parameters that actually become arguments are resolved; without that filtering each + case raises "Failed to resolve type hints". + """ + assert resolve_arg_names(func) == expected_args + @pytest.mark.parametrize( + ("func", "resolve_arg_names"), + [ + pytest.param(_func_unresolvable_argument, _arg_names_via_parser, id="non_base"), + pytest.param(_func_unresolvable_argument_base, _arg_names_via_base_command, id="base_command"), + ], + ) + def test_unresolvable_hint_on_real_argument_raises(self, func, resolve_arg_names) -> None: + """An unresolvable forward reference on a parameter that *does* become an argument must abort + with a clear, actionable error rather than being silently swallowed -- for both plain commands + and base commands. + """ with pytest.raises(TypeError, match="Failed to resolve type hints"): - build_parser_from_function(do_broken) - - def test_validate_base_command_type_hints_failure_raises(self) -> None: - """Base-command validation should raise, not swallow, type hint failures.""" - from cmd2.annotated import _resolve_parameters + resolve_arg_names(func) + + def test_forward_ref_resolves_through_functools_wraps_wrapper(self) -> None: + """A forward reference must resolve against the *original* function's module even when the + function reaching the parser builder is a ``functools.wraps`` wrapper defined in another + module (e.g. a user decorator stacked under ``@with_annotated``). ``functools.wraps`` copies + ``__annotations__`` but not ``__globals__``, so resolution must follow ``__wrapped__`` -- + mirroring what ``typing.get_type_hints`` does for a bare function. + """ - def do_broken(self, cmd2_subcommand_func, name: "NonExistentType"): # noqa: F821 + def do_path(self, target: "Path", count: int = 1): pass - with pytest.raises(TypeError, match="Failed to resolve type hints"): - _resolve_parameters(do_broken, base_command=True) + # The wrapper carries a foreign global namespace lacking ``Path``; resolution must use the + # unwrapped function's globals (this module) instead, or it raises "Failed to resolve". + wrapped = _wrap_in_foreign_module(do_path) + parser = build_parser_from_function(wrapped) + dests = {action.dest for action in parser._actions if action.dest != "help"} + assert dests == {"target", "count"} def test_dest_param_raises(self) -> None: with pytest.raises(ValueError, match="dest"): @@ -1100,6 +1186,28 @@ class MyParser(cmd2.Cmd2ArgumentParser): parser = build_parser_from_function(_make_func(str), parser_class=MyParser) assert isinstance(parser, MyParser) + def test_default_parser_class(self) -> None: + """With no parser_class, the parser is an instance of the configured default.""" + from cmd2 import argparse_utils + + parser = build_parser_from_function(_make_func(str)) + assert type(parser) is argparse_utils.DEFAULT_ARGUMENT_PARSER + + def test_default_parser_class_follows_current_default(self, monkeypatch) -> None: + """The default is resolved at call time, never a copy captured at import. + + ``set_default_argument_parser`` rebinds ``argparse_utils.DEFAULT_ARGUMENT_PARSER`` at runtime; + a build issued afterwards must honor the new value. + """ + from cmd2 import argparse_utils + + class MyDefaultParser(cmd2.Cmd2ArgumentParser): + pass + + monkeypatch.setattr(argparse_utils, "DEFAULT_ARGUMENT_PARSER", MyDefaultParser) + parser = build_parser_from_function(_make_func(str)) + assert type(parser) is MyDefaultParser + def test_completer_class(self) -> None: from cmd2.argparse_completer import ArgparseCompleter @@ -2223,6 +2331,114 @@ def test_subcommand_help(self, subcmd_app) -> None: assert "member" in help_text +class _OptionalIntermediateApp(cmd2.Cmd): + """An intermediate subcommand that is itself a base command with an *optional* subcommand.""" + + @with_annotated(base_command=True) + def do_opt(self, cmd2_subcommand_func) -> None: + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="opt", base_command=True, subcommand_required=False, help="optional middle") + def opt_mid(self, cmd2_subcommand_func) -> None: + # The guard must blank out the self-referential handler so this runs exactly once. + self.poutput("mid:none" if cmd2_subcommand_func is None else "mid:recurse") + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="opt mid", help="leaf") + def opt_mid_leaf(self, name: str) -> None: + self.poutput(f"leaf:{name}") + + +@pytest.fixture +def opt_app() -> _OptionalIntermediateApp: + return _OptionalIntermediateApp() + + +class TestOptionalIntermediateSubcommand: + def test_intermediate_without_deeper_subcommand_runs_once(self, opt_app) -> None: + """The recursion guard blanks the self-referential handler: the body runs once with None.""" + out, _err = run_cmd(opt_app, "opt mid") + assert out == ["mid:none"] + + def test_deeper_subcommand_still_dispatches(self, opt_app) -> None: + """Blanking the self-reference must not break dispatch to a genuine deeper subcommand.""" + out, _err = run_cmd(opt_app, "opt mid leaf Bob") + assert out == ["leaf:Bob"] + + def test_guard_blanks_only_subcommand_func(self) -> None: + """The guard must null *only* cmd2_subcommand_func, leaving the intermediate's own args intact.""" + + class App(cmd2.Cmd): + @with_annotated(base_command=True) + def do_opt(self, cmd2_subcommand_func) -> None: + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="opt", base_command=True, subcommand_required=False) + def opt_mid(self, cmd2_subcommand_func, verbose: bool = False) -> None: + self.poutput(f"none={cmd2_subcommand_func is None}:verbose={verbose}") + if cmd2_subcommand_func: + cmd2_subcommand_func() + + out, _err = run_cmd(App(), "opt mid --verbose") + assert out == ["none=True:verbose=True"] + + def test_guard_fires_at_deeper_nesting_level(self) -> None: + """The guard must work past two levels: the deepest *selected* optional base runs once.""" + + class App(cmd2.Cmd): + @with_annotated(base_command=True) + def do_a(self, cmd2_subcommand_func) -> None: + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="a", base_command=True, subcommand_required=False) + def a_b(self, cmd2_subcommand_func) -> None: + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="a b", base_command=True, subcommand_required=False) + def a_b_c(self, cmd2_subcommand_func) -> None: + self.poutput(f"c:none={cmd2_subcommand_func is None}") + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="a b c", help="leaf") + def a_b_c_leaf(self, name: str) -> None: + self.poutput(f"leaf:{name}") + + app = App() + assert run_cmd(app, "a b c")[0] == ["c:none=True"] + assert run_cmd(app, "a b c leaf Z")[0] == ["leaf:Z"] + + def test_guard_works_for_commandset_subcommand(self) -> None: + """The handler is a CommandSet-bound method here; unwrapping to __func__ must still match.""" + + class _Grp(cmd2.CommandSet): + @cmd2.with_category("grp") + @with_annotated(base_command=True) + def do_grp(self, cmd2_subcommand_func) -> None: + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="grp", base_command=True, subcommand_required=False) + def grp_mid(self, cmd2_subcommand_func) -> None: + self._cmd.poutput(f"mid:none={cmd2_subcommand_func is None}") + if cmd2_subcommand_func: + cmd2_subcommand_func() + + @with_annotated(subcommand_to="grp mid", help="leaf") + def grp_mid_leaf(self, name: str) -> None: + self._cmd.poutput(f"leaf:{name}") + + app = cmd2.Cmd(auto_load_commands=False) + app.register_command_set(_Grp()) + assert run_cmd(app, "grp mid")[0] == ["mid:none=True"] + assert run_cmd(app, "grp mid leaf Q")[0] == ["leaf:Q"] + + class TestSubcommandValidation: def test_subcommand_aliases_none_raises(self) -> None: """aliases=None is off-spec (it must be a Sequence[str]); reject it with a clear message.""" @@ -3196,21 +3412,6 @@ def test_argument_default_passthrough(self) -> None: parser = build_parser_from_function(_make_func(str), argument_default=sentinel) assert parser.argument_default == sentinel - def test_argument_default_suppress_works_with_explicit_defaults(self) -> None: - """``argument_default=SUPPRESS`` is safe when every argument sets its own default. - - Every ``@with_annotated`` argument either is positional (always supplied) or - has an explicit default, so SUPPRESS at the parser level can't drop a kwarg - the function expects. - """ - - def func(self, name: str, count: int = 1) -> None: ... - - parser = build_parser_from_function(func, argument_default=argparse.SUPPRESS) - ns = parser.parse_args(["alice"]) - assert ns.name == "alice" - assert ns.count == 1 - def test_decorator_passes_parser_kwargs(self) -> None: @with_annotated(prog="myprog", usage="usage line") def do_run(self, name: str) -> None: ... @@ -3395,27 +3596,34 @@ def test_const_on_untyped_param_not_validated(self) -> None: class TestArgumentDefaultSuppressGuard: - """``argument_default=argparse.SUPPRESS`` is rejected when it would drop an omittable argument.""" + """``argument_default=argparse.SUPPRESS`` is rejected outright by @with_annotated. - def test_suppress_with_optional_positional_rejected(self) -> None: - def do_t(self, x: int | None): ... + SUPPRESS drops an absent argument from the parsed namespace, but @with_annotated builds the call + from the signature, so every declared parameter is expected at invocation -- a vanished argument + can never be valid. The rejection is unconditional (it never inspects the signature), so one + direct-build case and one subcommand-registration case cover it. + """ + + def test_suppress_rejected(self) -> None: + def do_t(self, a: int, b: str = "x"): ... with pytest.raises(TypeError, match="SUPPRESS"): build_parser_from_function(do_t, argument_default=argparse.SUPPRESS) - def test_suppress_safe_when_all_args_required_or_defaulted(self) -> None: - def do_t(self, a: int, b: str = "x"): ... + def test_suppress_rejected_in_subcommand(self) -> None: + """The subcommand path shares the same builder, so the rejection fires at registration too.""" - # ``a`` is always supplied (required positional); ``b`` carries its own default -> safe. - parser = build_parser_from_function(do_t, argument_default=argparse.SUPPRESS) - assert parser is not None + class App(cmd2.Cmd): + @with_annotated(base_command=True) + def do_calc(self, cmd2_subcommand_func) -> None: + if cmd2_subcommand_func: + cmd2_subcommand_func() - def test_suppress_safe_with_var_positional(self) -> None: - def do_t(self, *vals: int): ... + @with_annotated(subcommand_to="calc", argument_default=argparse.SUPPRESS, help="sum values") + def calc_sum(self, value: str = "x") -> None: ... - # *args is substituted with () by the invocation path, so SUPPRESS cannot strand it. - parser = build_parser_from_function(do_t, argument_default=argparse.SUPPRESS) - assert parser is not None + with pytest.raises(TypeError, match="SUPPRESS"): + App() class TestHelpKwargReserved: