Skip to content

fix: fix edge case on forward unresolveable type#1673

Merged
tleonhardt merged 6 commits into
mainfrom
unuse-param-edge-case
Jun 5, 2026
Merged

fix: fix edge case on forward unresolveable type#1673
tleonhardt merged 6 commits into
mainfrom
unuse-param-edge-case

Conversation

@KelvinChung2000
Copy link
Copy Markdown
Contributor

Fix an edge case when doing forward declaration on types:

  if TYPE_CHECKING:
      from myapp.cli import MyCmd

  @with_annotated
  def do_greet(self: "MyCmd", name: str, count: int = 1):
      ...

which happens when trying to declare the command in another file, while still wanting to keep the type hinting for self.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.59%. Comparing base (205e8a6) to head (c55d417).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1673      +/-   ##
==========================================
+ Coverage   99.52%   99.59%   +0.07%     
==========================================
  Files          23       23              
  Lines        5675     5677       +2     
==========================================
+ Hits         5648     5654       +6     
+ Misses         27       23       -4     
Flag Coverage Δ
unittests 99.59% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@tleonhardt
Copy link
Copy Markdown
Member

The one test failure is due to release of Python 3.15.0b2 and is fixed in PR #1674

@KelvinChung2000
Copy link
Copy Markdown
Contributor Author

I will rebase once that is landed. I found another edge case related to the function return type that also needs to be covered

Comment thread cmd2/annotated.py Outdated
hints = get_type_hints(func, include_extras=True)
hints = get_type_hints(
types.SimpleNamespace(__annotations__=relevant_annotations),
globalns=getattr(func, "__globals__", {}),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If func is wrapped by another decorator using @functools.wraps, its __globals__ attribute points to the decorator's module, not the original function's module. Because globalns is explicitly provided here, get_type_hints() won't traverse the __wrapped__ chain to find the correct globals. This will cause forward references (e.g., string annotations) to fail with NameError or resolve to the wrong type.

Suggested change:

ignored = {next(iter(sig.parameters), None), *skip_params}
ignored.discard(None)
relevant_annotations = {name: ann for name, ann in getattr(func, "__annotations__", {}).items() if name not in ignored}
unwrapped = inspect.unwrap(func)
try:
    hints = get_type_hints(
        types.SimpleNamespace(__annotations__=relevant_annotations),
        globalns=getattr(unwrapped, "__globals__", {}),
        include_extras=True,
    )

The key change is created unrwapped = inspect.unwrap(func) before the try and then using unwrapped instead of func in the call to getattr.

@tleonhardt
Copy link
Copy Markdown
Member

@KelvinChung2000 I merged the fix for Python 3.15.0b2. You are free to rebase when you see fit.

@kmvanbrunt
Copy link
Copy Markdown
Member

@KelvinChung2000 I merged main into this branch since I made changes to annotated.py to reflect new naming and metadata classes.

I also noticed that you are missing tests for two areas.

  1. In this function, there is no test for when constants.NS_ATTR_SUBCOMMAND_FUNC is in filtered.
@functools.wraps(func)
def handler(self_arg: Any, ns: Any) -> Any:
    """Unpack Namespace into typed kwargs for the subcommand handler."""
    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:
            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
    )
  1. In build_parser_from_function() you were previously using a possible stale copy of DEFAULT_ARGUMENT_PARSER. I updated the code to always reference argparse_utils.DEFAULT_ARGUMENT_PARSER. Please add a test to ensure the default value for parser_cls is always the current value for argparse_utils.DEFAULT_ARGUMENT_PARSER.

@tleonhardt tleonhardt merged commit 0786a4e into main Jun 5, 2026
29 checks passed
@tleonhardt tleonhardt deleted the unuse-param-edge-case branch June 5, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants