diff --git a/docs/changes.rst b/docs/changes.rst index 0ec1150..f22053c 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -9,12 +9,12 @@ Note that version 1.17.0 drops support for Python 3.6 and 3.7. Python version **Bugs Fixed** -* When a normal function which had `wrapt.decorator` applied, was assigned as a - class attribute, and the function attribute called via the class or an - instance of the class, an additional argument was being passed, inserted as - the first argument, which was the class or instance. This was not the correct - behaviour and the class or instance should not have been passed as the first - argument. +* When a normal function or builtin function which had `wrapt.decorator` or a + function wrapper applied, was assigned as a class attribute, and the function + attribute called via the class or an instance of the class, an additional + argument was being passed, inserted as the first argument, which was the class + or instance. This was not the correct behaviour and the class or instance + should not have been passed as the first argument. Version 1.16.0 -------------- diff --git a/src/wrapt/_wrappers.c b/src/wrapt/_wrappers.c index 0a69624..a924d3b 100644 --- a/src/wrapt/_wrappers.c +++ b/src/wrapt/_wrappers.c @@ -2353,16 +2353,19 @@ static PyObject *WraptFunctionWrapperBase_call( static PyObject *function_str = NULL; static PyObject *callable_str = NULL; static PyObject *classmethod_str = NULL; + static PyObject *instancemethod_str = NULL; if (!function_str) { #if PY_MAJOR_VERSION >= 3 function_str = PyUnicode_InternFromString("function"); callable_str = PyUnicode_InternFromString("callable"); classmethod_str = PyUnicode_InternFromString("classmethod"); + instancemethod_str = PyUnicode_InternFromString("instancemethod"); #else function_str = PyString_InternFromString("function"); callable_str = PyString_InternFromString("callable"); classmethod_str = PyString_InternFromString("classmethod"); + instancemethod_str = PyString_InternFromString("instancemethod"); #endif } @@ -2394,6 +2397,8 @@ static PyObject *WraptFunctionWrapperBase_call( if ((self->instance == Py_None) && (self->binding == function_str || PyObject_RichCompareBool(self->binding, function_str, + Py_EQ) == 1 || self->binding == instancemethod_str || + PyObject_RichCompareBool(self->binding, instancemethod_str, Py_EQ) == 1 || self->binding == callable_str || PyObject_RichCompareBool(self->binding, callable_str, Py_EQ) == 1 || self->binding == classmethod_str || @@ -2439,6 +2444,9 @@ static PyObject *WraptFunctionWrapperBase_descr_get( static PyObject *bound_type_str = NULL; static PyObject *function_str = NULL; static PyObject *callable_str = NULL; + static PyObject *builtin_str = NULL; + static PyObject *class_str = NULL; + static PyObject *instancemethod_str = NULL; if (!bound_type_str) { #if PY_MAJOR_VERSION >= 3 @@ -2454,27 +2462,30 @@ static PyObject *WraptFunctionWrapperBase_descr_get( #if PY_MAJOR_VERSION >= 3 function_str = PyUnicode_InternFromString("function"); callable_str = PyUnicode_InternFromString("callable"); + builtin_str = PyUnicode_InternFromString("builtin"); + class_str = PyUnicode_InternFromString("class"); + instancemethod_str = PyUnicode_InternFromString("instancemethod"); #else function_str = PyString_InternFromString("function"); callable_str = PyString_InternFromString("callable"); + builtin_str = PyString_InternFromString("builtin"); + class_str = PyString_InternFromString("class"); + instancemethod_str = PyString_InternFromString("instancemethod"); #endif } if (self->parent == Py_None) { -#if PY_MAJOR_VERSION < 3 - if (PyObject_IsInstance(self->object_proxy.wrapped, - (PyObject *)&PyClass_Type) || PyObject_IsInstance( - self->object_proxy.wrapped, (PyObject *)&PyType_Type)) { + if (self->binding == builtin_str || PyObject_RichCompareBool( + self->binding, builtin_str, Py_EQ) == 1) { Py_INCREF(self); return (PyObject *)self; } -#else - if (PyObject_IsInstance(self->object_proxy.wrapped, - (PyObject *)&PyType_Type)) { + + if (self->binding == class_str || PyObject_RichCompareBool( + self->binding, class_str, Py_EQ) == 1) { Py_INCREF(self); return (PyObject *)self; } -#endif if (Py_TYPE(self->object_proxy.wrapped)->tp_descr_get == NULL) { PyErr_Format(PyExc_AttributeError, @@ -2513,6 +2524,8 @@ static PyObject *WraptFunctionWrapperBase_descr_get( if (self->instance == Py_None && (self->binding == function_str || PyObject_RichCompareBool(self->binding, function_str, + Py_EQ) == 1 || self->binding == instancemethod_str || + PyObject_RichCompareBool(self->binding, instancemethod_str, Py_EQ) == 1 || self->binding == callable_str || PyObject_RichCompareBool(self->binding, callable_str, Py_EQ) == 1)) { @@ -3105,6 +3118,9 @@ static int WraptFunctionWrapper_init(WraptFunctionWrapperObject *self, static PyObject *classmethod_str = NULL; static PyObject *staticmethod_str = NULL; static PyObject *callable_str = NULL; + static PyObject *builtin_str = NULL; + static PyObject *class_str = NULL; + static PyObject *instancemethod_str = NULL; int result = 0; @@ -3147,35 +3163,74 @@ static int WraptFunctionWrapper_init(WraptFunctionWrapperObject *self, #endif } - if (PyObject_IsInstance(wrapped, (PyObject *)&PyFunction_Type)) { - binding = function_str; + if (!builtin_str) { +#if PY_MAJOR_VERSION >= 3 + builtin_str = PyUnicode_InternFromString("builtin"); +#else + builtin_str = PyString_InternFromString("builtin"); +#endif } - else if (PyObject_IsInstance(wrapped, (PyObject *)&PyClassMethod_Type)) { - binding = classmethod_str; + + if (!class_str) { +#if PY_MAJOR_VERSION >= 3 + class_str = PyUnicode_InternFromString("class"); +#else + class_str = PyString_InternFromString("class"); +#endif } - else if (PyObject_IsInstance(wrapped, (PyObject *)&PyStaticMethod_Type)) { - binding = staticmethod_str; + + if (!instancemethod_str) { +#if PY_MAJOR_VERSION >= 3 + instancemethod_str = PyUnicode_InternFromString("instancemethod"); +#else + instancemethod_str = PyString_InternFromString("instancemethod"); +#endif } - else if ((instance = PyObject_GetAttrString(wrapped, "__self__")) != 0) { -#if PY_MAJOR_VERSION < 3 - if (PyObject_IsInstance(instance, (PyObject *)&PyClass_Type) || - PyObject_IsInstance(instance, (PyObject *)&PyType_Type)) { - binding = classmethod_str; + + if (PyObject_IsInstance(wrapped, (PyObject *)&WraptFunctionWrapperBase_Type)) { + binding = PyObject_GetAttrString(wrapped, "_self_binding"); + } + + if (!binding) { + if (PyCFunction_Check(wrapped)) { + binding = builtin_str; } -#else - if (PyObject_IsInstance(instance, (PyObject *)&PyType_Type)) { + else if (PyObject_IsInstance(wrapped, (PyObject *)&PyFunction_Type)) { + binding = function_str; + } + else if (PyObject_IsInstance(wrapped, (PyObject *)&PyClassMethod_Type)) { binding = classmethod_str; } -#endif - else - binding = callable_str; + else if (PyObject_IsInstance(wrapped, (PyObject *)&PyType_Type)) { + binding = class_str; + } + else if (PyObject_IsInstance(wrapped, (PyObject *)&PyStaticMethod_Type)) { + binding = staticmethod_str; + } + else if ((instance = PyObject_GetAttrString(wrapped, "__self__")) != 0) { + #if PY_MAJOR_VERSION < 3 + if (PyObject_IsInstance(instance, (PyObject *)&PyClass_Type) || + PyObject_IsInstance(instance, (PyObject *)&PyType_Type)) { + binding = classmethod_str; + } + #else + if (PyObject_IsInstance(instance, (PyObject *)&PyType_Type)) { + binding = classmethod_str; + } + #endif + else if (PyObject_IsInstance(wrapped, (PyObject *)&PyMethod_Type)) { + binding = instancemethod_str; + } + else + binding = callable_str; - Py_DECREF(instance); - } - else { - PyErr_Clear(); + Py_DECREF(instance); + } + else { + PyErr_Clear(); - binding = callable_str; + binding = callable_str; + } } result = WraptFunctionWrapperBase_raw_init(self, wrapped, Py_None, diff --git a/src/wrapt/wrappers.py b/src/wrapt/wrappers.py index dd290b9..f785e44 100644 --- a/src/wrapt/wrappers.py +++ b/src/wrapt/wrappers.py @@ -505,51 +505,50 @@ def __init__(self, wrapped, instance, wrapper, enabled=None, object.__setattr__(self, '_self_owner', owner) def __get__(self, instance, owner): - # This method is actually doing double duty for both unbound and - # bound derived wrapper classes. It should possibly be broken up - # and the distinct functionality moved into the derived classes. - # Can't do that straight away due to some legacy code which is - # relying on it being here in this base class. + # This method is actually doing double duty for both unbound and bound + # derived wrapper classes. It should possibly be broken up and the + # distinct functionality moved into the derived classes. Can't do that + # straight away due to some legacy code which is relying on it being + # here in this base class. # - # The distinguishing attribute which determines whether we are - # being called in an unbound or bound wrapper is the parent - # attribute. If binding has never occurred, then the parent will - # be None. + # The distinguishing attribute which determines whether we are being + # called in an unbound or bound wrapper is the parent attribute. If + # binding has never occurred, then the parent will be None. # - # First therefore, is if we are called in an unbound wrapper. In - # this case we perform the binding. + # First therefore, is if we are called in an unbound wrapper. In this + # case we perform the binding. # - # We have one special case to worry about here. This is where we - # are decorating a nested class. In this case the wrapped class - # would not have a __get__() method to call. In that case we - # simply return self. + # We have two special cases to worry about here. These are where we are + # decorating a class or builtin function as neither provide a __get__() + # method to call. In this case we simply return self. # - # Note that we otherwise still do binding even if instance is - # None and accessing an unbound instance method from a class. - # This is because we need to be able to later detect that - # specific case as we will need to extract the instance from the - # first argument of those passed in. + # Note that we otherwise still do binding even if instance is None and + # accessing an unbound instance method from a class. This is because we + # need to be able to later detect that specific case as we will need to + # extract the instance from the first argument of those passed in. if self._self_parent is None: - if not inspect.isclass(self.__wrapped__): - descriptor = self.__wrapped__.__get__(instance, owner) + if self._self_binding == 'builtin': + return self + + if self._self_binding == "class": + return self - return self.__bound_function_wrapper__(descriptor, instance, - self._self_wrapper, self._self_enabled, - self._self_binding, self, owner) + descriptor = self.__wrapped__.__get__(instance, owner) - return self + return self.__bound_function_wrapper__(descriptor, instance, + self._self_wrapper, self._self_enabled, + self._self_binding, self, owner) - # Now we have the case of binding occurring a second time on what - # was already a bound function. In this case we would usually - # return ourselves again. This mirrors what Python does. + # Now we have the case of binding occurring a second time on what was + # already a bound function. In this case we would usually return + # ourselves again. This mirrors what Python does. # - # The special case this time is where we were originally bound - # with an instance of None and we were likely an instance - # method. In that case we rebind against the original wrapped - # function from the parent again. + # The special case this time is where we were originally bound with an + # instance of None and we were likely an instance method. In that case + # we rebind against the original wrapped function from the parent again. - if self._self_instance is None and self._self_binding in ('function', 'callable'): + if self._self_instance is None and self._self_binding in ('function', 'instancemethod', 'callable'): descriptor = self._self_parent.__wrapped__.__get__( instance, owner) @@ -583,7 +582,7 @@ def _unpack_self(self, *args): # a function that was already bound to an instance. In that case # we want to extract the instance from the function and use it. - if self._self_binding in ('function', 'callable', 'classmethod'): + if self._self_binding in ('function', 'instancemethod', 'classmethod', 'callable'): if self._self_instance is None: instance = getattr(self.__wrapped__, '__self__', None) if instance is not None: @@ -634,11 +633,11 @@ def _unpack_self(self, *args): self, args = _unpack_self(*args) - # If enabled has been specified, then evaluate it at this point - # and if the wrapper is not to be executed, then simply return - # the bound function rather than a bound wrapper for the bound - # function. When evaluating enabled, if it is callable we call - # it, otherwise we evaluate it as a boolean. + # If enabled has been specified, then evaluate it at this point and if + # the wrapper is not to be executed, then simply return the bound + # function rather than a bound wrapper for the bound function. When + # evaluating enabled, if it is callable we call it, otherwise we + # evaluate it as a boolean. if self._self_enabled is not None: if callable(self._self_enabled): @@ -647,26 +646,10 @@ def _unpack_self(self, *args): elif not self._self_enabled: return self.__wrapped__(*args, **kwargs) - # We need to do things different depending on whether we are - # likely wrapping an instance method vs a static method or class - # method. + # We need to do things different depending on whether we are likely + # wrapping an instance method vs a static method or class method. if self._self_binding == 'function': - # if self._self_instance is None: - # # This situation can occur where someone is calling the - # # instancemethod via the class type and passing the instance - # # as the first argument. We need to shift the args before - # # making the call to the wrapper and effectively bind the - # # instance to the wrapped function using a partial so the - # # wrapper doesn't see anything as being different. - - # if not args: - # raise TypeError('missing 1 required positional argument') - - # instance, args = args[0], args[1:] - # wrapped = PartialCallableObjectProxy(self.__wrapped__, instance) - # return self._self_wrapper(wrapped, instance, args, kwargs) - if self._self_instance is None and args: instance, newargs = args[0], args[1:] if isinstance(instance, self._self_owner): @@ -679,11 +662,11 @@ def _unpack_self(self, *args): elif self._self_binding == 'callable': if self._self_instance is None: # This situation can occur where someone is calling the - # instancemethod via the class type and passing the instance - # as the first argument. We need to shift the args before - # making the call to the wrapper and effectively bind the - # instance to the wrapped function using a partial so the - # wrapper doesn't see anything as being different. + # instancemethod via the class type and passing the instance as + # the first argument. We need to shift the args before making + # the call to the wrapper and effectively bind the instance to + # the wrapped function using a partial so the wrapper doesn't + # see anything as being different. if not args: raise TypeError('missing 1 required positional argument') @@ -787,27 +770,41 @@ def __init__(self, wrapped, wrapper, enabled=None): # So to get the best outcome we can, whenever we aren't sure what # it is, we label it as a 'callable'. If it was already bound and # that is rebound later, we assume that it will be an instance - # method and try an cope with the possibility that the 'self' + # method and try and cope with the possibility that the 'self' # argument it being passed as an explicit argument and shuffle # the arguments around to extract 'self' for use as the instance. - if inspect.isfunction(wrapped): - binding = 'function' + binding = None - elif isinstance(wrapped, classmethod): - binding = 'classmethod' + if isinstance(wrapped, _FunctionWrapperBase): + binding = wrapped._self_binding - elif isinstance(wrapped, staticmethod): - binding = 'staticmethod' + if not binding: + if inspect.isbuiltin(wrapped): + binding = 'builtin' - elif hasattr(wrapped, '__self__'): - if inspect.isclass(wrapped.__self__): + elif inspect.isfunction(wrapped): + binding = 'function' + + elif inspect.isclass(wrapped): + binding = 'class' + + elif isinstance(wrapped, classmethod): binding = 'classmethod' + + elif isinstance(wrapped, staticmethod): + binding = 'staticmethod' + + elif hasattr(wrapped, '__self__'): + if inspect.isclass(wrapped.__self__): + binding = 'classmethod' + elif inspect.ismethod(wrapped): + binding = 'instancemethod' + else: + binding = 'callable' + else: binding = 'callable' - else: - binding = 'callable' - super(FunctionWrapper, self).__init__(wrapped, None, wrapper, enabled, binding) diff --git a/tests/test_decorators.py b/tests/test_decorators.py index 070f089..0af3851 100644 --- a/tests/test_decorators.py +++ b/tests/test_decorators.py @@ -1,5 +1,6 @@ from __future__ import print_function +import operator import unittest import wrapt @@ -119,7 +120,7 @@ def passthrough(wrapped, instance, args, kwargs): return wrapped(*args, **kwargs) @passthrough - def function(): + def function(self): pass class A: @@ -129,42 +130,38 @@ class A: self.assertEqual(A._function._self_binding, "function") self.assertEqual(A._function._self_owner, A) - A._function() - a = A() + A._function(a) + self.assertTrue(a._function._self_parent is function) self.assertEqual(a._function._self_binding, "function") self.assertEqual(a._function._self_owner, A) - with self.assertRaises(TypeError): - a._function() - - # Test example without using the decorator to show same outcome. The - # exception should be "TypeError: function() takes 0 positional - # arguments but 1 was given". + a._function() - def xpassthrough(wrapped): - return wrapped + # Test example without using the decorator to show same outcome. - xfunction = xpassthrough(function) + def xfunction(self): + pass class B: _xfunction = xfunction b = B() - with self.assertRaises(TypeError): - b._xfunction() + B._xfunction(b) + + b._xfunction() def test_decorated_function_as_instance_attribute(self): @wrapt.decorator def passthrough(wrapped, instance, args, kwargs): return wrapped(*args, **kwargs) - + @passthrough - def function(): + def function(self): pass class A: @@ -177,15 +174,19 @@ def __init__(self): self.assertEqual(a._function._self_binding, "function") self.assertTrue(a._function._self_owner is None) - a._function() + a._function(a) + + bound_a = a._function.__get__(a, A) + self.assertTrue(bound_a._self_parent is function) + self.assertEqual(bound_a._self_binding, "function") + self.assertTrue(bound_a._self_owner is A) - # Test example without using the decorator to show same outcome. The - # call should work with no hidden argumemts being passed. + bound_a() - def xpassthrough(wrapped): - return wrapped + # Test example without using the decorator to show same outcome. - xfunction = xpassthrough(function) + def xfunction(self): + pass class B: def __init__(self): @@ -193,7 +194,108 @@ def __init__(self): b = B() - b._xfunction() + b._xfunction(b) + + def test_decorated_builtin_as_class_attribute(self): + + @wrapt.decorator + def passthrough(wrapped, instance, args, kwargs): + return wrapped(*args, **kwargs) + + function = passthrough(operator.add) + + class A: + _function = function + + self.assertTrue(A._function._self_parent is None) + self.assertEqual(A._function._self_binding, "builtin") + self.assertTrue(A._function._self_owner is None) + + A._function(1, 2) + + a = A() + + self.assertTrue(a._function._self_parent is None) + self.assertEqual(a._function._self_binding, "builtin") + self.assertTrue(a._function._self_owner is None) + + a._function(1, 2) + + # Test example without using the decorator to show same outcome. + + class B: + _xfunction = operator.add + + B._xfunction(1, 2) + + b = B() + + b._xfunction(1, 2) + + def test_call_semantics_for_assorted_decorator_use_cases(self): + def g1(): + pass + + def g2(self): + pass + + class C1: + def __init__(self): + self.f3 = g1 + + def f1(self): + print("SELF", self) + + f2 = g2 + + c1 = C1() + + c1.f2() + c1.f3() + + class C2: + f2 = C1.f1 + f3 = C1.f2 + + c2 = C2() + + c2.f2() + c2.f3() + + class C3: + f2 = c1.f1 + + c3 = C3() + + c3.f2() + + @wrapt.decorator + def passthrough(wrapped, instance, args, kwargs): + return wrapped(*args, **kwargs) + + class C11: + @passthrough + def f1(self): + print("SELF", self) + + c11 = C11() + + class C12: + f2 = C11.f1 + + c12 = C12() + + c12.f2() + + class C13: + f2 = c11.f1 + + c13 = C13() + + c13.f2() + + C11.f1(c11) + C12.f2(c12) if __name__ == '__main__': unittest.main() diff --git a/tests/test_function_wrapper.py b/tests/test_function_wrapper.py index 40f7f88..2ecde68 100644 --- a/tests/test_function_wrapper.py +++ b/tests/test_function_wrapper.py @@ -213,12 +213,7 @@ def function1(self, *args, **kwargs): self.assertEqual(function2._self_wrapper, decorator2) - # We can't identify this as being an instance method in - # Python 3 when it is a class so have to disable the check - # for Python 2. This has flow on effect of not working - # in the case of an instance either. - - self.assertEqual(function2._self_binding, 'callable') + self.assertEqual(function2._self_binding, 'instancemethod') def test_classmethod_attributes_external_instance(self): def decorator1(wrapped, instance, args, kwargs): @@ -515,7 +510,7 @@ def wrapper(wrapped, instance, args, kwargs): def test_re_bind_after_none(self): - def function(): + def function(self): pass def wrapper(wrapped, instance, args, kwargs): @@ -529,6 +524,8 @@ def wrapper(wrapped, instance, args, kwargs): _bound_wrapper_1 = _wrapper.__get__(None, type(instance)) + _bound_wrapper_1(instance) + self.assertTrue(_bound_wrapper_1._self_parent is _wrapper) self.assertTrue(isinstance(_bound_wrapper_1, @@ -543,6 +540,8 @@ def wrapper(wrapped, instance, args, kwargs): wrapt.BoundFunctionWrapper)) self.assertEqual(_bound_wrapper_2._self_instance, instance) + _bound_wrapper_2() + self.assertTrue(_bound_wrapper_1 is not _bound_wrapper_2) class TestInvalidWrapper(unittest.TestCase):