Make provided/implementedBy and adapter registries respect super(). #181
Conversation
There was a problem hiding this comment.
I am not sure about the __self__ dereferencing before the objects are passed to the adapter factory. Personally, I would be more comfortable when the objects were passed unchanged.
I am quite sure that the mro should be determined from ob.__self_class__ not from ob.__thisclass__.
|
Jason Madden wrote at 2020-3-7 01:44 -0800:
jamadden commented on this pull request.
> + mro = PyObject_CallMethodObjArgs(class_that_invoked_super, strmro, NULL);
+ _NSCHECK(mro, NULL);
+
+ index = PySequence_Index(mro, class_that_invoked_super);
I'm afraid you have that backwards. In `super(Class, self)` `__self_class__` is always the same as `type(self)`. It never changes and is the most derived class in the MRO (index 0). It's `__thisclass__` that changes to reflect where in the MRO the new starting search point actually is.
I am aware of this. That's why I am (quite) sure that
what you need is
mro = PyObject_CallMethodObjArgs(__self_class__, strmro, NULL);
_NSCHECK(mro, NULL);
index = PySequence_Index(mro, class_that_invoked_super);
With your code, `index` will always be 0 as for any class
it is the 0's element of its "mro".
|
|
Jason Madden wrote at 2020-3-7 02:05 -0800:
...
It's not particularly useful because the `super` object cannot access instance variables of `self` (only class descriptors/methods), nor (as a consequence) can it discover any interfaces that are `directlyProvidedBy` the `self` object. It has no MRO or type and `super` doesn't work on it, so you'd only be able to try this trick for exactly one level of hierarchy. The receiving factory would need to unwrap `__self__` in short order to do any of those things��and at that point, you're writing an adapter that's customized to know it's working with `super`, which destroys reusability: given `class Base` and `class Derived(Base)` the same adapter can't work for `super(Derived, self)` *and* `Base()`.
I am convinced.
|
|
CPython 3.5 is crashing on Travis/Linux, apparently hanging on Travis/Mac, and crashing locally for me on my Mac. The local crash occurs in where |
|
The crash in 3.5 turned out to be a memory bug in The temporary objects in question are the objects created for Right now, I'm thinking leave it simple and revisit if it becomes a bottleneck. [EDIT: It was simple to cache, so I tried that.] |
There was a problem hiding this comment.
Something is still inconsistent. I set up the following test:
>>> from zope.interface import implementer, Interface, providedBy
>>>
>>> class IB(Interface): pass
...
>>> class IM1(Interface): pass
...
>>> class IM2(Interface): pass
...
>>> class ID(IB): pass
...
>>> @implementer(IB)
... class B: pass
...
>>> @implementer(IM1)
... class M1(B): pass
...
>>> @implementer(IM2)
... class M2(B): pass
...
>>> @implementer(ID)
... class D(M1, M2): pass
...
>>> d = D()
>>> tuple(providedBy(d))
(<InterfaceClass __main__.ID>, <InterfaceClass __main__.IM1>, <InterfaceClass __main__.IB>, <InterfaceClass __main__.IM2>)
>>> tuple(providedBy(super(D, d)))
(<InterfaceClass __main__.IM1>, <InterfaceClass __main__.IB>, <InterfaceClass __main__.IM2>)
>>> tuple(providedBy(super(M1, d)))
(<InterfaceClass __main__.IM2>, <InterfaceClass __main__.IB>)
>>> tuple(providedBy(super(M2, d)))
()
>>> tuple(providedBy(super(B, d)))
()
>>> D.mro()
[<class '__main__.D'>, <class '__main__.M1'>, <class '__main__.M2'>, <class '__main__.B'>, <class 'object'>]
Either tuple(providedBy(super(M1, d))) should not contain IB or tuple(providedBy(super(M2, d))) should contain IB (because M1 and M2 are symmetric with respect to B). Very likely, the latter should be the case, as super(M2, d) is "d viewed as B object" and B implements IB.
I am also worried that the use of tuple(map(func, it)) (over [func(i) for i in it]) could cause a crash. In my view, this indicates that something is deeply wrong and may hit people without the capabilities to find out why and how to work around it. Did the problem only affected Python 3.5 or other Python 3 versions as well (in other works, were the bug in Python 3.5 or somewhere in zope.interface).
Yeah, I was concerned about that too and kept looking into it. The crash only happened in CPython 3.5, and debugging showed that the call to The change to non-lazily iterate before calling into C is a sufficient fix for this code (although I can and will also eliminate the invoking of this side-effect in the first place). But this is a bug waiting to happen and the C code has been incorrect for a long time. The remainder of the fix is to not get the cache until we can be (more) sure there will be no more arbitrary side effects. |
The query functions now start by looking at the next class in the MRO (interfaces directly provided by the underlying object are not found). Adapter registries automatically pick up providedBy change to start finding the correct implementations of adapters, but to make that really useful they needed to change to unpack super() arguments and pass __self__ to the factory. Fixes #11 Unfortunately, this makes PyPy unable to build the C extensions. Additional crash-safety for adapter lookup. Make the C functions get the cache only after resolving the ``required`` into a tuple, in case of side-effects like...clearing the cache. This could lead to the ``cache`` object being deallocated before we used it. Drop the ``tuplefy`` function in favor of a direct call to ``PySequence_Tuple``. It's what the ``tuple`` constructor would do anyway and saves a few steps. Make sure that getting ``providedBy(super())`` and ``implementedBy(super())`` have no side effects.
You're absolutely right. I've added that as a test case and corrected it. |
| # ``tuple`` and ``list`` cooperate so that ``tuple([some list])`` | ||
| # directly allocates and iterates at the C level without using a | ||
| # Python iterator. That's not the case for | ||
| # ``tuple(generator_expression)`` or ``tuple(map(func, it))``. | ||
| ## | ||
| # 3.8 | ||
| # ``tuple([t for t in range(10)])`` -> 610ns | ||
| # ``tuple(t for t in range(10))`` -> 696ns | ||
| # ``tuple(map(lambda t: t, range(10)))`` -> 881ns | ||
| ## | ||
| # 2.7 | ||
| # ``tuple([t fon t in range(10)])`` -> 625ns | ||
| # ``tuple(t for t in range(10))`` -> 665ns | ||
| # ``tuple(map(lambda t: t, range(10)))`` -> 958ns | ||
| # | ||
| # All three have substantial variance. |
There was a problem hiding this comment.
The comment looks somewhat unmotivated at this place (though I know from the conversation in this PR why it is somewhere).
|
Thanks for the detailed reviews and attention to detail!
Both of those things are covered in the tests, but I will look at making them more explicit. |
The DEFINE_STRING macro prevents the linter from seeing them as unused so I temporarily redefined it to find all such variables.
The query functions now start by looking at the next class in the MRO (interfaces directly provided by the underlying object are not found).
Adapter registries automatically pick up providedBy change to start finding the correct implementations of adapters, but to make that really useful they needed to change to unpack super() arguments and pass
__self__to the factory.Fixes #11
Unfortunately, this makes PyPy unable to build the C extensions.