Skip to content

inspect.getfullargspec should use __signature__ #61683

Closed

Description

@voidspace
BPO 17481
Nosy @terryjreedy, @ncoghlan, @larryhastings, @voidspace, @PCManticore, @1st1
Dependencies
  • bpo-20189: inspect.Signature doesn't recognize all builtin types
  • Files
  • getargsspec_01.patch
  • getargsspec_02.patch
  • getargsspec_03.patch
  • getargsspec_04.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/1st1'
    closed_at = <Date 2014-01-29.16:46:22.224>
    created_at = <Date 2013-03-19.17:31:32.474>
    labels = ['type-feature', 'library']
    title = 'inspect.getfullargspec should use __signature__'
    updated_at = <Date 2014-01-29.16:54:26.797>
    user = 'https://github.com/voidspace'

    bugs.python.org fields:

    activity = <Date 2014-01-29.16:54:26.797>
    actor = 'python-dev'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2014-01-29.16:46:22.224>
    closer = 'yselivanov'
    components = ['Library (Lib)']
    creation = <Date 2013-03-19.17:31:32.474>
    creator = 'michael.foord'
    dependencies = ['20189']
    files = ['33420', '33565', '33755', '33777']
    hgrepos = []
    issue_num = 17481
    keywords = ['patch', 'needs review']
    message_count = 31.0
    messages = ['184646', '207919', '208369', '208416', '208437', '208440', '208442', '208443', '208470', '208471', '208475', '208484', '208495', '208496', '208505', '208506', '208509', '208517', '208535', '208547', '208549', '208971', '209025', '209077', '209082', '209141', '209469', '209569', '209655', '209656', '209657']
    nosy_count = 8.0
    nosy_names = ['terry.reedy', 'ncoghlan', 'larry', 'michael.foord', 'Claudiu.Popa', 'Yury.Selivanov', 'python-dev', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue17481'
    versions = ['Python 3.4']

    Activity

    1. voidspace commented on Mar 19, 2013

      @voidspace
      ContributorAuthor

      inspect.getfullargspec (and potentially getargspec?) could *use* function.__signature__ if set. This allows functions that lie about their signature with the new introspection tool (Signature) to still work with older code.

    2. added
      stdlibStandard Library Python modules in the Lib/ directory
      type-featureA feature request or enhancement
      on Mar 19, 2013
    3. 1st1 commented on Jan 11, 2014

      @1st1
      Member

      Please consider the attached patch (getargsspec_01.patch).

      It modifies 'getargspec' and 'getfullargspec' to use the 'inspect.signature' API. The entire test suite passes just fine.

      This also will address issue bpo-16490.

      I can also update the docs, if it's necessary.

    4. 1st1 commented on Jan 17, 2014

      @1st1
      Member

      Can somebody review the patch? I'd be cool if this lands in 3.4.

    5. terryjreedy commented on Jan 18, 2014

      @terryjreedy
      Member

      While I plan to change Idle code to use signature directly, instead of getfullargspec, I agree that changing inspect also, and in 3.4, is a good idea. It may, however, affect tests other than Idle's, if there are any other stdlib consumers. I will try to look at the patch after changing Idle.

    6. ncoghlan commented on Jan 19, 2014

      @ncoghlan
      Contributor

      I upgraded the description to a "should". Argument Clinic and other changes in Python 3.4 greatly improve introspection support for various aspects of the runtime and standard library (for example, bpo-20223 will handle the new functools.partialmethod support). By also making those enhancements available via getfullargpsec, we should significantly increase the amount of existing code which benefits automatically for those enhancements, as it will no longer require explicit migration to the new PEP-362 APIs at the application/library level.

      Reviewing the patch now.

    7. changed the title [-]inspect.getfullargspec could use __signature__[/-] [+]inspect.getfullargspec should use __signature__[/+] on Jan 19, 2014
    8. larryhastings commented on Jan 19, 2014

      @larryhastings
      Contributor

      If we modify inspect.getfullargspec, shouldn't we modify inspect.getargspec too? "deprecated" doesn't mean "unsupported", it means "not recommended for new code, please stop using it".

    9. 1st1 commented on Jan 19, 2014

      @1st1
      Member

      Larry,
      getargspec uses getfullargspec, so it's covered.

    10. ncoghlan commented on Jan 19, 2014

      @ncoghlan
      Contributor

      getargspec() is just a thin wrapper around getfullargspec(), so we get that for free.

      Similarly, getcallargs() is also implicitly updated to full PEP-362 support by changing the implementation of getfullargspec().

      The other two standard library consumers appear to be IDLE (as Terry noted above) and xmlrpc.server.ServerHTMLDoc.docroutine (the self-documenting XML-RPC server API, which sadly appears to be lacking test cases).

      Reviewing those cases (especially the XML-RPC one) have convinced me we need a backwards compatibility fallback in the updated getfullargspec implementation. Specifically, if the new API throws ValueError (because it can't find a way to build a coherent signature object), we should fall back to the old approach. Otherwise we run the risk of introducing unexpected exceptions into introspection code.

      That is, the new call to signature in getfullargspec should look something like:

      try:
          sig = signature(func)
      except ValueError:
          if not hasattr(func, "__code__"):
              raise # The legacy fallback doesn't support this input type
          # The callable signature appears to be incoherent, so we fall
          # back to the legacy implementation to preserve compatibility
          args, varargs, kwonlyargs, varkw = _getfullargs(func.__code__)
          return FullArgSpec(args, varargs, varkw, func.__defaults__,
                  kwonlyargs, func.__kwdefaults__, func.__annotations__)
      

      I suspect this may actually be a better solution for IDLE rather than updating it to call inspect.signature directly (since IDLE probably *wants* the more graceful fallback to the old behaviour).

      The way I would document all this in What's New is to update the current argument clinic section to also explain that we've taken advantage of PEP-362 to improve introspection in multiple areas, including for code that is using the introspection APIs that predate PEP-362.

    11. terryjreedy commented on Jan 19, 2014

      @terryjreedy
      Member

      The relevant code in CallTips.py is
      argspec = ""
      if hasattr(ob, '__call__'):
      ...
      if isinstance(fob, (types.FunctionType, types.MethodType)):
      argspec = inspect.formatargspec(*inspect.getfullargspec(fob))

      So I want to broaden the second condition (or remove it), but if it returns something .formatargspec cannot digest, the outer call will have to be conditioned. My impression is that str(signature object) returns pretty much what .formatargspec does, but I have not experimented yet.

      My bpo-20122 patch moves currently commented out tests in CallTips.py that use the above to test_calltips.py. I will commit as soon as I test on 3.4. Until then, getfullargspec is not being used in the Idle test suite.

    12. larryhastings commented on Jan 19, 2014

      @larryhastings
      Contributor

      I just want to mention, while we're all thinking about this stuff: I plan to enhance the Signature object to reflect "optional groups". Right now Signature objects cannot express parameters that are optional but don't have a published default. (e.g. a string parameter for a builtin having a default of NULL). A lot of these are being converted using optional groups, and I want to make it at least possible for user code to introspect those functions and understand that those are optional parameters.

      My original plan was to add a "group" member, containing an arbitrary identifier of the "group" this parameter belongs to. I'm not sure that's sufficient; I may also need a "parent_group" parameter, or something like that, to represent the group that this group is nested in. I'm still thinking about it. But I'm hoping to get to this in the next two or three days.

    13. ncoghlan commented on Jan 19, 2014

      @ncoghlan
      Contributor

      Sounds reasonable - we'll need that to convert range() and slice()
      anyway (I totally failed at finding time to look at the builtins this
      weekend, but I'd still like to handle those before the 3rd beta).

      An alternative would be to actually add "ParameterGroup" as a new
      class that contains parameters (assuming tuple isn't sufficient), so
      the structure of the parameters more directly matches the underlying
      signature.

    14. YurySelivanov commented on Jan 19, 2014

      YurySelivanovmannequin
      Mannequin

      Larry,

      I saw your message on the tracker regarding adding support for parameters groups to the signature object. Would you mind if I join the discussion with my ideas of how this feature might be implemented?

      Yury

      On Sunday, January 19, 2014 at 5:44 AM, Larry Hastings wrote:

      Larry Hastings added the comment:

      I just want to mention, while we're all thinking about this stuff: I plan to enhance the Signature object to reflect "optional groups". Right now Signature objects cannot express parameters that are optional but don't have a published default. (e.g. a string parameter for a builtin having a default of NULL). A lot of these are being converted using optional groups, and I want to make it at least possible for user code to introspect those functions and understand that those are optional parameters.

      My original plan was to add a "group" member, containing an arbitrary identifier of the "group" this parameter belongs to. I'm not sure that's sufficient; I may also need a "parent_group" parameter, or something like that, to represent the group that this group is nested in. I'm still thinking about it. But I'm hoping to get to this in the next two or three days.

      ----------


      Python tracker <report@bugs.python.org (mailto:report@bugs.python.org)>
      <http://bugs.python.org/issue17481\>


    15. 7 remaining items

    16. 1st1 commented on Jan 20, 2014

      @1st1
      Member

      Instead of keeping the check, we could just unconditionally convert exceptions from the signature call to a TypeError in order to maintain compatibility with the old external behaviour.

      Agreed. See the new patch (getargsspec_02.patch)

      Unfortunately, we have to keep the old 'ismethod' check in place for backwards compatibility purposes.

      Larry,

      The attached patch contains one failing unit-test: 'inspect.signature' returns 'None' for '_testapi.docstring_no_signature'. It should instead raise a ValueError.

    17. 1st1 commented on Jan 20, 2014

      @1st1
      Member

      Larry,

      I created a separate issue for that: http://bugs.python.org/issue20313

    18. 1st1 commented on Jan 23, 2014

      @1st1
      Member

      Larry, Nick,

      So what's the resolution on this one? Do I have a green light?

    19. larryhastings commented on Jan 23, 2014

      @larryhastings
      Contributor

      My solution for pydoc was to call isroutine() instead of isfunction(), and yes handle it throwing an exception.

      (I just checked, and I wasn't catching TypeError, only ValueError. I'll fix that in my next patch for bpo-20189.)

    20. ncoghlan commented on Jan 24, 2014

      @ncoghlan
      Contributor

      Another case of "don't land it until Larry has dealt with the builtins".

      The patch itself looks fine, though :)

    21. larryhastings commented on Jan 24, 2014

      @larryhastings
      Contributor

      The patch from bpo-20189 has landed.

    22. larryhastings commented on Jan 25, 2014

      @larryhastings
      Contributor

      There's a major difference between getfullargspec/getargspec and inspect.signature: getfullargspec shows you the "self" parameter for bound methods, and inspect.signature does not.

      >>> class C:
      ...    def foo(self, a):  pass
      ... 
      >>> c = C()
      >>>
      >>> import inspect
      >>> str(inspect.signature(c.foo))
      '(a)'
      >>> inspect.getfullargspec(c.foo)
      FullArgSpec(args=['self', 'a'], varargs=None, varkw=None, defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
      >>> inspect.getargspec(c.foo)
      ArgSpec(args=['self', 'a'], varargs=None, keywords=None, defaults=None)

      This is why help() (currently) shows bound parameters for methods implemented in Python, but doesn't show them for methods implemented in C. pydoc uses inspect.getfullargspec for pure Python functions and inspect.signature for builtins.

    23. 1st1 commented on Jan 27, 2014

      @1st1
      Member

      There's a major difference between getfullargspec/getargspec and inspect.signature: getfullargspec shows you the "self" parameter for bound methods, and inspect.signature does not.

      Larry, yes, that's correct. The attached patch simulates this behaviour, with:

          if ismethod(func):
              func = func.__func__

      I'm attaching 'getargsspec_03.patch', as the previous one (02) was a bit crippled.

    24. 1st1 commented on Jan 28, 2014

      @1st1
      Member

      Larry and Nick,

      Please review the final patch (getargsspec_04.patch). I'd like to commit it tomorrow.

      This one is the last one (hopefully), and it supports builtin methods properly -- i.e. works around 'self' parameter correctly. To do that, I check if the '__text_signature__' starts with "($", etc.

    25. self-assigned this
      on Jan 28, 2014
    26. python-dev commented on Jan 29, 2014

      python-devmannequin
      Mannequin

      New changeset 6d1e8162e855 by Yury Selivanov in branch 'default':
      inspect.getfullargspec: Use inspect.signature API behind the scenes bpo-17481
      http://hg.python.org/cpython/rev/6d1e8162e855

    27. 1st1 commented on Jan 29, 2014

      @1st1
      Member

      Committed.
      Thanks for the reviews!

    28. python-dev commented on Jan 29, 2014

      python-devmannequin
      Mannequin

      New changeset 0fa2750c7241 by Yury Selivanov in branch 'default':
      inspect.test.getfullargspec: Add a unittest to ensure correct annotations
      http://hg.python.org/cpython/rev/0fa2750c7241

    29. transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    stdlibStandard Library Python modules in the Lib/ directorytype-featureA feature request or enhancement

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions