classification
Title: inspect.signature includes bound argument for wrappers around bound methods
Type: Stage: resolved
Components: Versions: Python 3.6, Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: encukou, ncoghlan, python-dev, yselivanov
Priority: normal Keywords: patch

Created on 2015-05-27 15:33 by encukou, last changed 2015-05-28 02:02 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
signature.patch yselivanov, 2015-05-27 19:59 review
signature2.patch yselivanov, 2015-05-27 21:52 review
Messages (8)
msg244178 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-27 15:33
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.
msg244179 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-27 15:33
Reported by David Gibson here: https://bugzilla.redhat.com/show_bug.cgi?id=1201990
msg244216 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-27 19:59
Thanks for reporting this, Petr!

Nick, could you please take a look at the patch?
msg244228 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-28 00:22
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__.
msg244234 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-28 01:04
> 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.
msg244238 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-28 01:25
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)
msg244241 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-28 01:37
> 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.
msg244243 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-28 02:01
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
History
Date User Action Args
2015-05-28 02:02:36yselivanovsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-05-28 02:01:32python-devsetnosy: + python-dev
messages: + msg244243
2015-05-28 01:37:44yselivanovsetmessages: + msg244241
2015-05-28 01:25:07ncoghlansetmessages: + msg244238
2015-05-28 01:04:03yselivanovsetmessages: + msg244234
2015-05-28 00:22:43ncoghlansetmessages: + msg244228
2015-05-27 21:52:14yselivanovsetfiles: + signature2.patch
2015-05-27 19:59:27yselivanovsetfiles: + signature.patch
versions: + Python 3.6
messages: + msg244216

assignee: yselivanov
keywords: + patch
stage: patch review
2015-05-27 16:34:40yselivanovsetnosy: + ncoghlan
2015-05-27 16:34:25yselivanovsetnosy: + yselivanov
2015-05-27 15:33:45encukousetmessages: + msg244179
2015-05-27 15:33:36encukoucreate