Issue 24298: inspect.signature includes bound argument for wrappers around bound methods

When obtaining the signature of a bound method, inspect.signature, by default, omits the "self" argument to the method, since it is already specified in the bound method.  However, if you create a wrapper around a bound method with functools.update_wrapper() or @functools.wraps, calling inspect.signature on the wrapper will return a signature which includes the "self" argument.

Reproducer:

    import inspect
    import functools

    class Foo(object):
        def bar(self, testarg):
            pass

    f = Foo()

    @functools.wraps(f.bar)
    def baz(*args):
        f.bar(*args)


    assert inspect.signature(baz) == inspect.signature(f.bar)

The program will fail with an assertion error. Examining inspect.signature(baz) shows:

    >>> print(inspect.signature(baz))
    (self, testarg)
    >>> print(inspect.signature(f.bar))
    (testarg)

Looking at the code in inspect.py:

The handling of bound methods appears at the top of inspect._signature_internal().  Since baz is not itself a bound method, it doesn't trigger this case.  Instead inspect.unwrap is called, returning f.bar.

inspect._signature_is_functionlike(f.bar) returns True, causing Signature.from_function to be called.  Unlike the direct bound method case, this includes the bound method's "self" argument.

Reported by David Gibson here: https://bugzilla.redhat.com/show_bug.cgi?id=1201990

Thanks for reporting this, Petr!

Nick, could you please take a look at the patch?

It occurs to me we're also bypassing the check that the unwrapped obj is a callable, so it probably makes sense to just recurse unconditionally after unwrapping the object, rather than only recursing for methods.

That's also a little more future-proof, in case any further checks happen to be inserted ahead of the check for __wrapped__.

> That's also a little more future-proof, in case any further checks happen to be inserted ahead of the check for __wrapped__.

Well, we unwrap until we see a "__signature__" attribute (or we can't unwrap anymore).  And right after unwrapping we try to return the __signature__; so inserting more checks before unwrapping with unconditional recursion after it won't be so safe.

I'm OK with the patch as is, but I'm definitely concerned about the maintainability of inspect.signature in general.

I'm trying to decide if a block comment covering the order of calling protocols that we check, and where we potentially recurse, would be a help (by providing a map of the function for the benefit of future maintainers) or a hindrance (by providing the opportunity for that map to get out of sync)

> I'm OK with the patch as is, but I'm definitely concerned about the maintainability of inspect.signature in general.

I agree, _signature_from_callable is getting quite complex.  The good news is that we have a very good test coverage for signatures.

As for a big block comment -- it might be useful.  OTOH the code of _signature_from_callable is mostly if..else statements with calls to helpers and recursion.

New changeset 42819b176d63 by Yury Selivanov in branch '3.4':
Issue 24298: Fix signature() to properly unwrap wrappers around bound methods
https://hg.python.org/cpython/rev/42819b176d63

New changeset d7e392c5c53a by Yury Selivanov in branch '3.5':
Issue 24298: Fix signature() to properly unwrap wrappers around bound methods
https://hg.python.org/cpython/rev/d7e392c5c53a

New changeset ab46801ca359 by Yury Selivanov in branch 'default':
Issue 24298: Fix signature() to properly unwrap wrappers around bound methods
https://hg.python.org/cpython/rev/ab46801ca359

assignee: yselivanov
keywords: + patch
stage: patch review