Wrapping with diamond dependency can break method resolution

[majorgreys]

wrapt (1.21.1) presents a problem when wrapping functions for classes in a diamond dependency. I am using Python 3.8.3 below.

Assume the following use case:

class A(object):
    def run(self):
        return "A"

class B(A):
    pass

class C(A):
    def run(self):
        return ["C", super(C, self).run()]

class D(B, C):
    pass

def double(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs) * 2

Without any patching, we see the following method resolution:

>>> D.run == C.run
True
>>> D().run()
['C', 'A']

If we wrap the intermediate B.run, we end up adding the run attribute to B and thus breaking the MRO since B has precedence over C in the MRO for D:

>>> inspect.getmro(D)
(<class '__main__.D'>, <class '__main__.B'>, <class '__main__.C'>, <class '__main__.A'>, <class 'object'>)
>>> "run" in vars(B)
False
>>> wrap_function_wrapper(B, "run", double)
<FunctionWrapper at 0x10d272a00 for function at 0x10d13d280>
>>> "run" in vars(B)
True
>>> D.run == C.run
False
>>> D.run == B.run
True
>>> D().run()
'AA'

Can others verify that this use case is a valid issue?

The reason for this lies in how wrap_object patches the module with the wrapped method, resulting in an attribute set where there was none before:

https://github.com/GrahamDumpleton/wrapt/blob/264c06fd3850bd0cda6917ca3e87417b573e023f/src/wrapt/wrappers.py#L773-L780

One resolution to this issue could be to change how a class's method is patched. Since resolve_path walks the class hierarchy to find the original attribute being wrapped, we could also patch the original class where the attribute was found.

If that seems like a reasonable solution and can be incorporated into the current api, I can open a PR with such a change.

[GrahamDumpleton]

Looking back at old issues and I thought there had been a bigger discussion about this issue including suggested changes for wrapt code, but now I can't find it. I know it is an old issue, but are you able to explain the nature of the changes to wrapt code you were thinking of while I try and digest the issue?

[GrahamDumpleton]

Thinking back about why it works as it does I don't believe the current behaviour can change as how it works was very deliberate and the best that could be done.

The underlying problem here is that you applied the wrapper in the wrong class. That is, you applied the wrapper to B when there is no run method in that class. It doesn't in this case apply it to the actual owner of the method found via mro search as that causes other problems.

Consider the code example as follows:

class A(object):
    def run(self):
        return "A"

class B(A):
    def run(self):
        return "B"

Imagine now the original wrapper was applied as:

def override(wrapped, instance, args, kwargs):
    return "B:override"

wrap_function_wrapper(B, "run", override)

If one had:

a = A()
print(a.run())

the result should be "A".

If one had:

b = B()
print(b.run())

the result should be "B:override".

Now image that original class definitions were changed to:

class A(object):
    def run(self):
        return "A"

class B(A):
    pass

and the wrapper patch remained because those applying the patch didn't know the original classes had changed.

With how wrapt works at present the result would be the same.

If however you made changes to apply the wrapper to the owner of the method when it wasn't in the target class supplied, then calling a.run() would start returning B:override which would be a complete surprise to any other code using the base class and could break something quite badly.

So the current behaviour was designed to avoid unexpected behaviour in this case where you start patching a base class unintentionally.

So yes this means in a case of an inheritance diamond you get strange results because of method resolution order, but it boils down to applying the wrapper in the wrong class to start with. Am not sure there is anything more sensible one could do in this case.

[GrahamDumpleton]

At this point going to close this issue with the note that is expected behaviour and although changes things for the implementation being patched, not much that can be done except avoid the problem by applying the patch to the correct class type.