diff --git a/docs/changes.rst b/docs/changes.rst index d407794d..ec1acd61 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -6,6 +6,25 @@ Version 2.4.2 **Bugs Fixed** +* Deleting the ``__module__`` or ``__doc__`` attribute of a proxy object, + using ``del proxy.__doc__`` for example, crashed the Python interpreter + when the C extension was in use. The attribute setters, which CPython + also invokes for a deletion with no value, correctly forwarded the + deletion to the wrapped object but then attempted to store the missing + value in the proxy's own dictionary. The pure Python implementation did + not crash, but instead failed with an ``AttributeError`` as the + properties used for these attributes had no deleter, so the deletion was + never forwarded to the wrapped object at all. + + Both implementations now forward the deletion to the wrapped object, so + the outcome is the same as deleting the attribute on the wrapped object + directly. For a function this leaves the attribute with a value of + ``None``, while for a class the ``TypeError`` raised by Python is + propagated. The C extension also refreshes the copy of the attribute it + holds in the proxy's own dictionary, so that the state of the proxy after + the deletion is the same as for a proxy newly created over the wrapped + object. + Version 2.4.1 ------------- diff --git a/src/wrapt/_wrappers.c b/src/wrapt/_wrappers.c index 0cd0b81f..890c8f45 100644 --- a/src/wrapt/_wrappers.c +++ b/src/wrapt/_wrappers.c @@ -3033,9 +3033,54 @@ static PyObject *WraptObjectProxy_get_module(WraptObjectProxyObject *self) /* ------------------------------------------------------------------------- */ +/* Refresh the copy of __module__ or __doc__ held in the proxy's own dict + * after the attribute has been deleted from the wrapped object. Deleting + * __doc__ from a function, for example, leaves the attribute present with + * a value of None, whereas on other objects it may be gone entirely. Record + * what __init__ would have for a fresh proxy over the wrapped object in its + * current state: the value if the attribute still exists, otherwise no + * entry at all. */ + +static int WraptObjectProxy_refresh_cached_attr(WraptObjectProxyObject *self, + PyObject *wrapped, + PyObject *name) +{ + PyObject *object = PyObject_GetAttr(wrapped, name); + + if (object) + { + int result = PyDict_SetItem(self->dict, name, object); + Py_DECREF(object); + return result; + } + + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) + return -1; + + PyErr_Clear(); + + if (PyDict_DelItem(self->dict, name) == -1) + { + if (!PyErr_ExceptionMatches(PyExc_KeyError)) + return -1; + PyErr_Clear(); + } + + return 0; +} + +/* ------------------------------------------------------------------------- */ + +/* The setters for __module__ and __doc__ are called with a NULL value when + * the attribute is being deleted. PyObject_SetAttr() forwards a deletion to + * the wrapped object, but the copy held in the proxy's own dict must then be + * refreshed rather than stored, as the dict API rejects a NULL value. */ + static int WraptObjectProxy_set_module(WraptObjectProxyObject *self, PyObject *value) { + int result; + if (!self->wrapped) { if (raise_uninitialized_wrapper_error(self) == -1) @@ -3049,14 +3094,16 @@ static int WraptObjectProxy_set_module(WraptObjectProxyObject *self, PyObject *wrapped = wrapt_acquire_wrapped(self); if (PyObject_SetAttr(wrapped, state->str_module, value) == -1) - { - Py_DECREF(wrapped); - return -1; - } + result = -1; + else if (value) + result = PyDict_SetItem(self->dict, state->str_module, value); + else + result = WraptObjectProxy_refresh_cached_attr(self, wrapped, + state->str_module); Py_DECREF(wrapped); - return PyDict_SetItemString(self->dict, "__module__", value); + return result; } /* ------------------------------------------------------------------------- */ @@ -3087,6 +3134,8 @@ static PyObject *WraptObjectProxy_get_doc(WraptObjectProxyObject *self) static int WraptObjectProxy_set_doc(WraptObjectProxyObject *self, PyObject *value) { + int result; + if (!self->wrapped) { if (raise_uninitialized_wrapper_error(self) == -1) @@ -3100,14 +3149,16 @@ static int WraptObjectProxy_set_doc(WraptObjectProxyObject *self, PyObject *wrapped = wrapt_acquire_wrapped(self); if (PyObject_SetAttr(wrapped, state->str_doc, value) == -1) - { - Py_DECREF(wrapped); - return -1; - } + result = -1; + else if (value) + result = PyDict_SetItem(self->dict, state->str_doc, value); + else + result = WraptObjectProxy_refresh_cached_attr(self, wrapped, + state->str_doc); Py_DECREF(wrapped); - return PyDict_SetItemString(self->dict, "__doc__", value); + return result; } /* ------------------------------------------------------------------------- */ diff --git a/src/wrapt/wrappers.py b/src/wrapt/wrappers.py index e489a22b..10498576 100644 --- a/src/wrapt/wrappers.py +++ b/src/wrapt/wrappers.py @@ -36,6 +36,10 @@ def __module__(self): def __module__(self, value): self.__wrapped__.__module__ = value + @__module__.deleter + def __module__(self): + del self.__wrapped__.__module__ + @property def __doc__(self): return self.__wrapped__.__doc__ @@ -44,6 +48,10 @@ def __doc__(self): def __doc__(self, value): self.__wrapped__.__doc__ = value + @__doc__.deleter + def __doc__(self): + del self.__wrapped__.__doc__ + # We similar use a property for __dict__. We need __dict__ to be # explicit to ensure that vars() works as expected. diff --git a/tests/core/test_type_module_attribute.py b/tests/core/test_type_module_attribute.py index fde9ee6a..06cadb74 100644 --- a/tests/core/test_type_module_attribute.py +++ b/tests/core/test_type_module_attribute.py @@ -272,6 +272,129 @@ class MyProxy(wrapt.ObjectProxy): self.assertEqual(target.__module__, "override_module") +# -- Deleting __module__ and __doc__ tests -- + + +class TestDeleteModuleAndDoc(unittest.TestCase): + """Verify deleting __module__ and __doc__ on instances is forwarded to + the wrapped object, in the same way as setting them. Targets are + created per test rather than shared, as deletion changes them.""" + + @staticmethod + def make_function(): + def target(): + "target documentation" + pass + return target + + @staticmethod + def make_class(): + class Target: + "target documentation" + return Target + + @staticmethod + def delete_outcome(obj, name): + # Return None if deletion succeeds, else the exception type and + # message, so the outcome via a proxy can be compared with the + # outcome of the same deletion made directly on the target. + try: + delattr(obj, name) + except Exception as e: + return (type(e), str(e)) + return None + + def test_delete_module_on_object_proxy(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_on_object_proxy(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_after_set(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + wrapper.__module__ = "override_module" + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_after_set(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + wrapper.__doc__ = "override doc" + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_on_function_wrapper(self): + def my_wrapper(wrapped, instance, args, kwargs): + return wrapped(*args, **kwargs) + target = self.make_function() + wrapper = wrapt.FunctionWrapper(target, my_wrapper) + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_on_function_wrapper(self): + def my_wrapper(wrapped, instance, args, kwargs): + return wrapped(*args, **kwargs) + target = self.make_function() + wrapper = wrapt.FunctionWrapper(target, my_wrapper) + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_on_user_subclass(self): + class MyProxy(wrapt.ObjectProxy): + pass + target = self.make_function() + wrapper = MyProxy(target) + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_on_user_subclass(self): + class MyProxy(wrapt.ObjectProxy): + pass + target = self.make_function() + wrapper = MyProxy(target) + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_on_class_target(self): + # A class does not permit deletion of __module__. The outcome via + # the proxy must match a direct deletion on an equivalent class. + expected = self.delete_outcome(self.make_class(), "__module__") + wrapper = wrapt.ObjectProxy(self.make_class()) + self.assertEqual(self.delete_outcome(wrapper, "__module__"), expected) + + def test_delete_doc_on_class_target(self): + expected = self.delete_outcome(self.make_class(), "__doc__") + wrapper = wrapt.ObjectProxy(self.make_class()) + self.assertEqual(self.delete_outcome(wrapper, "__doc__"), expected) + + def test_proxy_state_matches_fresh_proxy_after_delete(self): + # After deletion the proxy's own instance dictionary must be the + # same as for a proxy newly created over the wrapped object in + # its current state, so the C extension's cached copies of the + # attributes do not go stale or linger. + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + del wrapper.__module__ + del wrapper.__doc__ + fresh = wrapt.ObjectProxy(target) + self.assertEqual(dict(wrapper.__self_dict__), dict(fresh.__self_dict__)) + + # -- Wrapped replacement tests -- @@ -365,6 +488,20 @@ def function(): self.assertEqual(proxy.__module__, "set.module") self.assertEqual(proxy.__doc__, "set doc") + def test_delete_module_and_doc_via_non_interned_name(self): + def function(): + """original doc""" + + proxy = BaseObjectProxy(function) + + delattr(proxy, self.dynamic("__module__")) + delattr(proxy, self.dynamic("__doc__")) + + self.assertIsNone(function.__module__) + self.assertIsNone(function.__doc__) + self.assertIsNone(proxy.__module__) + self.assertIsNone(proxy.__doc__) + if __name__ == "__main__": unittest.main()