`_p_repr` drops Acquisition wrappers, even when found on the instance, with the C extensions

[jamadden]

As reported in https://github.com/zopefoundation/Zope/pull/392 , among others. This has caused some pain for the Zope2/4 and Plone folks.

It would seem the obvious cause is that we look for _p_repr on the type, not the instance. Changing that makes the pure-Python versions work, but using the C extensions still fails (even when using Persistence.Persistent as a base class).

This test passes in pure-Python, fails in C when we look for _p_repr on the instance (it fails in both when we look on the type):

    def test__p_repr_acquisition(self):
        # This works on both PyPy/PURE_PYTHON and on CPython/c-extensions,
        # but it won't work when Base is defined in C and we use the
        # pure-python persistent class.
        if self._getTargetClass() != persistent.Persistent:
            raise unittest.SkipTest("Mixed C/pure")

        from ExtensionClass import Base
        from Acquisition import Implicit
        from Persistence import Persistent as Persistent
        class Foo(Implicit,
                  Persistent,
                  ):
            __slots__ = ()

            def _p_repr(self):
                print("Hi from", type(self))
                return "Parent is %s" % getattr(self, '__parent__')

        class MyContainer(Base):
            def __str__(self):
                return "Base"

        base = MyContainer()
        base.foo = Foo()
        foo = base.foo

        self.assertEqual(repr(foo), 'Parent is Base')

In pure-python, the print gives us Hi from <class 'ExtensionClass.ImplicitAcquisitionWrapper_Foo'>, while with C we get Hi from <class 'persistent.tests.test_persistence._Persistent_Base.test__p_repr_acquisition.<locals>.Foo'>

Here's what I see that goes wrong.

1. The acquisition wrapper's __repr__ slot asks for the __repr__ object of itself. The intent is to get a callable that's itself wrapped.
2. This gets to the "normal lookup" branch of Wrapper_findattr
3. An attribute is found. But instead of finding an object passing PyMethod_Check, which the wrapper knows how to re-wrap, we actually wind up with a "method-wrapper" (<method-wrapper '__repr__' of Foo object at 0x105273bd8>). This is the way that C slots are exposed as Python callables. Acquisition doesn't know how to deal with that, and so it gets returned as-is.
4. We invoke Persistent.__repr__ with the unwrapped object.

It's just not safe to call a C slot with a wrapped object, so it's not clear to me what, if anything, can be done about this.

[jimfulton]

Sorry for coming in late on this.

What is rational for _p_repr? This seems to add needless complexity. Why not let subclasses override __repr__ if they want to override __repr__?

Is there some compelling use case here?

Would the crazyness around acquisition go away if we didn't do the _p_repr dance?

[jamadden]

_p_repr was originally proposed back in #11 by @jimfulton. I believe it allows subclasses to define a representation for the common case without having to worry about error handling for the uncommon case that the connection has been closed or they otherwise cannot access their persistent state (that's a real problem; my group had defined a workaround).

Overriding __repr__ is a viable option, just as it always has been. It works fine in the case of acquisition, as the Zope/Plone projects have had to demonstrate. Really, _p_repr doesn't enter the conversation there, except that it would be really nice if the benefits of _p_repr also happened to work in the case of acquisition. It currently doesn't. I'm not sure there's a way to make it do so (perhaps something in Persistence? I haven't thought that much about it).

This only comes up because the improved __repr__ in persistent.Persistent happened to change the MRO of __repr__ in some complex multiple-inheritance classes in Plone; introducing a _p_repr method (which didn't suffer from multiple inheritance) in a base class could have easily solved the doctest problem in a simple way, but because it doesn't handle acquisition, that didn't work.

[jimfulton]

Ha ha, Ah, right, dealing with closed connections... I vaguely remember this being a problem. :)

[jimfulton]

I'm very confused. Is something actually defining _p_repr and is the problem that this isn't woring as expected?

[jimfulton]

Was this working and broke with some recent PR?

[jamadden]

Acquisition was working for some Zope objects' __repr__. Because of the complicated way that extension classes are added to the MRO, adding __repr__ to persistent.Persistent unexpectedly overrode __repr__ for those objects, and the nice workaround of adding a _p_repr to the base class that defined __repr__ didn't work with acquisition. The only solution we found was to introduce a mixin class that implements __repr__ and explicitly put it at the front of the MRO for the affected classes.

[jimfulton]

I'll play with this a bit today.

Much thanks for your work on this BTW.