GrahamDumpleton / wrapt

A Python module for decorators, wrappers and monkey patching.
BSD 2-Clause "Simplified" License
2.04k stars 230 forks source link

Function wrapper on a class staticmethod #153

Closed majorgreys closed 4 years ago

majorgreys commented 4 years ago

I am investigating an issue with how wrapt behaves when wrapping a staticmethod.

To illustrate, let's start with the simple case and wrap the function to see the error:

import wrapt

class Class(object):
    @staticmethod
    def function(arg):
        return arg

def wrapper(wrapped, instance, args, kwargs):
    print("inside wrapper")
    return wrapped(*args, **kwargs)

wrapt.wrap_function_wrapper(Class, "function", wrapper)

Calling the function on an instance fails after wrapping:

>>> c = Class()
>>> Class.function("class")
inside wrapper
'class'
>>> c.function("instance")
inside wrapper
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in wrapper
TypeError: function() takes 1 positional argument but 2 were given

If I instead wrap the function on the instance, the wrapper works as expected on the instance.

Looking at wrappers.py, the helper function resolve_path uses getattr to access the class attribute.

If instead I use inspect.getattr_static to access the class attribute and then apply the wrapper as a patch, calling function on the instance does not raise an exception:

from inspect import getattr_static

original = getattr_static(Class, "function")
wrapped = wrapt.FunctionWrapper(original, wrapper)
setattr(Class, "function", wrapped)

Is this a correct workaround for wrapping staticmethods?

I can open a PR with changes if in fact the actual behavior I am finding is not what is expected.

GrahamDumpleton commented 4 years ago

What versions of Python is this occurring with?

GrahamDumpleton commented 4 years ago

The intent of the code in wrapt was doing the correct thing, the problem was that it was not applying it to the first lookup when doing recursive lookups on the path in resolve_path(). It did the correct thing for nested path elements. So just tweaked the code to do it on the first lookup as well, rather than wholesale replacing what was done by the use of inspect.getattr_static(). I'll review at another time whether inspect.getattr_static() should be used if a reason comes up for it. Fixed in 1.12.1 of wrapt.