Forward deletion of __module__ and __doc__ on a proxy to the wrapped object

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
 -------------
 

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;
 }
 
 /* ------------------------------------------------------------------------- */

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.
 

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()