Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The repr of BoundMethod objects sometimes incorrectly identifies the bound function #65588

Closed
BlckKnght mannequin opened this issue Apr 30, 2014 · 6 comments
Closed

The repr of BoundMethod objects sometimes incorrectly identifies the bound function #65588

BlckKnght mannequin opened this issue Apr 30, 2014 · 6 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@BlckKnght
Copy link
Mannequin

BlckKnght mannequin commented Apr 30, 2014

BPO 21389
Nosy @benjaminp, @BlckKnght
Files
  • method_repr.diff
  • method_repr_tests.diff: Tests for the new repr for bound methods, and a fix for test_defaultdict
  • 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 = None
    closed_at = <Date 2014-08-20.23:42:12.206>
    created_at = <Date 2014-04-30.02:26:41.524>
    labels = ['interpreter-core', 'type-bug']
    title = 'The repr of BoundMethod objects sometimes incorrectly identifies the bound function'
    updated_at = <Date 2014-08-20.23:42:12.203>
    user = 'https://github.com/BlckKnght'

    bugs.python.org fields:

    activity = <Date 2014-08-20.23:42:12.203>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-08-20.23:42:12.206>
    closer = 'python-dev'
    components = ['Interpreter Core']
    creation = <Date 2014-04-30.02:26:41.524>
    creator = 'Steven.Barker'
    dependencies = []
    files = ['35202', '36420']
    hgrepos = []
    issue_num = 21389
    keywords = ['patch']
    message_count = 6.0
    messages = ['217566', '218196', '218206', '225538', '225555', '225595']
    nosy_count = 4.0
    nosy_names = ['benjamin.peterson', 'Arfrever', 'python-dev', 'Steven.Barker']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue21389'
    versions = ['Python 3.4', 'Python 3.5']

    @BlckKnght
    Copy link
    Mannequin Author

    BlckKnght mannequin commented Apr 30, 2014

    The "repr" of bound method objects can be misleading in certain situations. The repr is always is of the format:

    <bound method x.y of <object>>
    

    But "x" is often incorrect.

    Here are some examples where the current code gets it wrong:

        # inherited method
        class Base(object):
            def foo(self):
                pass
    
        class Derived(Base):
            pass
    print(Derived().foo)
    # not too bad, prints "<bound method Derived.foo of <__main__.Derived object at 0xXXXX>>"
    # it probably should say "Base.foo" instead of "Derived.foo", but at least they're two names for the same function
    
        # however, an override and super() it gets very bad
        class Derived2(Base):
            def foo(self):
                pass
    print(super(Derived2, Derived2()).foo)
    # totally wrong, prints "<bound method Dervied2.foo of __main__.derived2 object at 0xXXXX>>"
    # but it actually *is* Base.foo bound to a Derived2 instance!
    
        # bound class methods:
        class Test(object):
            @classmethod
            def foo(cls):
                pass
    
        print(Test.foo)
        # wrong, prints <bound method type.foo of <class '__main__.Test'>>

    I suggest that rather than trying to assemble the "x.y" pair by from "__self__.__class__" and "__func__.__name__", the BoundMethod should just use the "__func__.__qualname__". In each of the cases above, the function's location would be correctly located this way.

    I came across this bug while investigating a confusing (to me) issue with metaclasses and inheritance. The misleading "repr" output made it much harder to figure out that my expectations were wrong. Here's a simplified example of how it led me astray:

        class A(object):
            @classmethod
            def foo(cls):
                return "classmethod from A"
    
        class BMeta(type):
            def foo(cls):
                return "instancemethod from BMeta"
    
        class B(A, metaclass=BMeta):
            pass
    
        print(B.foo()) # surprisingly (to me) prints "classmethod from A"
        print(B.foo)   # incorrectly prints "<bound method BMeta.foo of <class __main__.B>>"

    It is presumably not a bug that inherited class methods take precedence over instance methods from a metaclass (though it was surprising to me at the time). The repr of the bound method though, suggested exactly the opposite. Given that it gets many more common situations wrong as well, I think that the repr should be fixed.

    The relevant code appears to be in the method_repr function in Objects/Classobject.c .

    @BlckKnght BlckKnght mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Apr 30, 2014
    @BlckKnght
    Copy link
    Mannequin Author

    BlckKnght mannequin commented May 9, 2014

    Here's a patch that changes the behavior of method_repr in Objects/classobject.c . It first tries to use __func__.__qualname__, then tries __func__.__name__ as a fallback and finally uses "?" if neither of those attributes are available.

    I'm not sure if the __name__ fallback is tested (as it seems that pretty much all callables have __qualname__ these days). The last "?" case actually does get tested by Lib/test/test_descr.py which creates a messed up method with classmethod(1).__get__(1) (which oddly does not raise an error immediately upon creation, but rather only when it is called).

    I've not written C in several years, so please let me know if you see I've done something obviously wrong, or any places the patch could be improved. It is mostly a copy-and-paste of existing code with a few modifications and deletions, so hopefully I can't have messed up anything too badly!

    I'm currently ignoring a comment in the code that says we "shouldn't use repr()/%R" to format __self__. I don't really understand that, so I've stick with the existing behavior on that front. If that is something that should change, I'd be happy to try reworking it in some other way, just let me know what the concern is.

    Here are some examples of the new repr output in a build with the patch:

    >>> class A():
    ...   def foo(self):
    ...     pass
    ...
    >>> class B(A):
    ...   def foo(self):
    ...     pass
    ...
    >>> class C(A):
    ...   pass
    ...
    >>> class D():
    ...   @classmethod
    ...   def bar():
    ...     pass
    ...
    >>> A().foo
    <bound method A.foo of <__main__.A object at 0x02267508>>
    >>> B().foo
    <bound method B.foo of <__main__.B object at 0x02267578>>
    >>> C().foo
    <bound method A.foo of <__main__.C object at 0x02267658>>
    >>> super(B, B()).foo
    <bound method A.foo of <__main__.B object at 0x022676C8>>
    >>> D.bar
    <bound method D.bar of <class '__main__.D'>>

    @BlckKnght
    Copy link
    Mannequin Author

    BlckKnght mannequin commented May 10, 2014

    Ah, I figured out why using %R may be bad. It breaks for the following silly class:

        class C():
            def __repr__(self):
                return repr(self.__repr__) # or use any other bound method

    repr(C()) will recurse until the recursion limit is hit, both with and without my patch. If this seems like a real issue, I could probably replace the %R code with a variation on the base case code in PyObject_Repr:

        PyUnicode_FromFormat("<%s object at %p>",
                             v->ob_type->tp_name, v)

    @benjaminp
    Copy link
    Contributor

    That seems reasonable. Can you please write tests for the new behavior, though, and also fix test_defaultdict?

    @BlckKnght
    Copy link
    Mannequin Author

    BlckKnght mannequin commented Aug 20, 2014

    OK, I've written some tests of the new bound method repr functionality, which I'm attaching as a patch against the current tip.

    I test the basic repr output, all the cases the old code got wrong (inherited methods, overridden methods, methods called via super, and classmethods) and the strange corner cases that probably won't come up in ordinary code (such as methods manually created from callables that don't have __name__ or __qualname__ attributes).

    I've also fixed the defaultdict test that was relying upon the old repr output. I don't believe there are any other places in the standard library or tests where a bound method's repr is examined.

    My patch adds the tests in a new file, Lib/test/test_bound_method_repr.py. I looked to see if there was an existing file that tested similar behavior, but none that I found really seemed appropriate. If I overlooked a better place to put the new tests, please let me know and I'll update the test patch.

    I'm not very experienced at writing unit tests, so comments and/or criticism is welcomed. I copied parts of the file's structure (such as the test_main() function and if __name__ == "__main__" boilerplate) from other test files, so hopefully I've stayed pretty close to the usual Python test style.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 20, 2014

    New changeset 92dcee426014 by Benjamin Peterson in branch 'default':
    use __qualname__ to compute bound method repr (closes bpo-21389)
    http://hg.python.org/cpython/rev/92dcee426014

    @python-dev python-dev mannequin closed this as completed Aug 20, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant