Rewrite the set attr / del attr function to avoid specialization

src/class/basic.rs

@@ -4,6 +4,9 @@
 //!
 //! Check [python c-api information](https://docs.python.org/3/reference/datamodel.html#basic-customization)
 //! for more information.
+//!
+//! Parts of the documentation are copied from the respective methods from the
+//! [typeobj docs](https://docs.python.org/3/c-api/typeobj.html)
 
 use std;
 use std::os::raw::c_int;
@@ -185,12 +188,7 @@ where
         type_object.tp_hash = Self::tp_hash();
         type_object.tp_getattro = Self::tp_getattro();
         type_object.tp_richcompare = Self::tp_richcompare();
-
+        type_object.tp_setattro = tp_setattro_impl::tp_setattro::<Self>();
-        type_object.tp_setattro = if let Some(df) = Self::tp_delattro() {
-            Some(df)
-        } else {
-            Self::tp_setattro()
-        };
     }
     fn nb_bool_fn() -> Option<ffi::inquiry> {
         Self::nb_bool()
@@ -202,7 +200,9 @@ trait PyObjectGetAttrProtocolImpl {
         None
     }
 }
+
 impl<'p, T> PyObjectGetAttrProtocolImpl for T where T: PyObjectProtocol<'p> {}
+
 impl<T> PyObjectGetAttrProtocolImpl for T
 where
     T: for<'p> PyObjectGetAttrProtocol<'p>,
@@ -217,41 +217,79 @@ where
     }
 }
 
-trait PyObjectSetAttrProtocolImpl {
+/// An object may support setting attributes (by implementing PyObjectSetAttrProtocol)
-    fn tp_setattro() -> Option<ffi::setattrofunc> {
+/// and may support deleting attributes (by implementing PyObjectDelAttrProtocol)
+/// and we need to generate a single extern c function that supports only setting, only deleting
+/// or both, and return None in case none of the two is supported.
+mod tp_setattro_impl {
+    use super::*;
+
+    /// setattrofunc PyTypeObject.tp_setattro
+    ///
+    /// An optional pointer to the function for setting and deleting attributes.
+    ///
+    /// The signature is the same as for PyObject_SetAttr(), but setting v to NULL to delete an
+    /// attribute must be supported. It is usually convenient to set this field to
+    /// PyObject_GenericSetAttr(), which implements the normal way of setting object attributes.
+    pub(super) fn tp_setattro<'p, T: PyObjectProtocol<'p>>() -> Option<ffi::setattrofunc> {
+        if let Some(set_del) = T::set_del_attr() {
+            Some(set_del)
+        } else if let Some(set) = T::set_attr() {
+            Some(set)
+        } else if let Some(del) = T::del_attr() {
+            Some(del)
+        } else {
             None
         }
     }
 
-impl<'p, T> PyObjectSetAttrProtocolImpl for T where T: PyObjectProtocol<'p> {}
+    trait SetAttr {
-impl<T> PyObjectSetAttrProtocolImpl for T
+        fn set_attr() -> Option<ffi::setattrofunc> {
+            None
+        }
+    }
+
+    impl<'p, T: PyObjectProtocol<'p>> SetAttr for T {}
+
+    impl<T> SetAttr for T
     where
         T: for<'p> PyObjectSetAttrProtocol<'p>,
     {
-    fn tp_setattro() -> Option<ffi::setattrofunc> {
+        fn set_attr() -> Option<ffi::setattrofunc> {
-        py_func_set!(PyObjectSetAttrProtocol, T::__setattr__)
+            py_func_set!(PyObjectSetAttrProtocol, T, __setattr__)
         }
     }
 
-trait PyObjectDelAttrProtocolImpl {
+    trait DelAttr {
-    fn tp_delattro() -> Option<ffi::setattrofunc> {
+        fn del_attr() -> Option<ffi::setattrofunc> {
             None
         }
     }
-impl<'p, T> PyObjectDelAttrProtocolImpl for T where T: PyObjectProtocol<'p> {}
+
-impl<T> PyObjectDelAttrProtocolImpl for T
+    impl<'p, T> DelAttr for T where T: PyObjectProtocol<'p> {}
+
+    impl<T> DelAttr for T
     where
         T: for<'p> PyObjectDelAttrProtocol<'p>,
     {
-    default fn tp_delattro() -> Option<ffi::setattrofunc> {
+        fn del_attr() -> Option<ffi::setattrofunc> {
-        py_func_del!(PyObjectDelAttrProtocol, T::__delattr__)
+            py_func_del!(PyObjectDelAttrProtocol, T, __delattr__)
         }
     }
-impl<T> PyObjectDelAttrProtocolImpl for T
+
+    trait SetDelAttr {
+        fn set_del_attr() -> Option<ffi::setattrofunc> {
+            None
+        }
+    }
+
+    impl<'p, T> SetDelAttr for T where T: PyObjectProtocol<'p> {}
+
+    impl<T> SetDelAttr for T
     where
         T: for<'p> PyObjectSetAttrProtocol<'p> + for<'p> PyObjectDelAttrProtocol<'p>,
     {
-    fn tp_delattro() -> Option<ffi::setattrofunc> {
+        fn set_del_attr() -> Option<ffi::setattrofunc> {
             py_func_set_del!(
                 PyObjectSetAttrProtocol,
                 PyObjectDelAttrProtocol,
@@ -261,6 +299,7 @@ where
             )
         }
     }
+}
 
 trait PyObjectStrProtocolImpl {
     fn tp_str() -> Option<ffi::unaryfunc> {

src/class/macros.rs

@@ -310,41 +310,40 @@ macro_rules! py_ternary_self_func {
     }};
 }
 
-#[macro_export]
 #[doc(hidden)]
 macro_rules! py_func_set {
-    ($trait:ident, $class:ident :: $f:ident) => {{
+    ($trait_name:ident, $generic:ident, $fn_set:ident) => {{
-        unsafe extern "C" fn wrap<T>(
+        unsafe extern "C" fn wrap<$generic>(
             slf: *mut $crate::ffi::PyObject,
             name: *mut $crate::ffi::PyObject,
             value: *mut $crate::ffi::PyObject,
         ) -> $crate::libc::c_int
         where
-            T: for<'p> $trait<'p>,
+            T: for<'p> $trait_name<'p>,
         {
             use $crate::ObjectProtocol;
 
             let _pool = $crate::GILPool::new();
             let py = $crate::Python::assume_gil_acquired();
-            let slf = py.mut_from_borrowed_ptr::<T>(slf);
+            let slf = py.mut_from_borrowed_ptr::<$generic>(slf);
 
+            let result;
             if value.is_null() {
-                let e = $crate::PyErr::new::<exc::NotImplementedError, _>(format!(
+                result = Err($crate::PyErr::new::<exc::NotImplementedError, _>(format!(
                     "Subscript deletion not supported by {:?}",
-                    stringify!(T)
+                    stringify!($generic)
-                ));
+                )));
-                e.restore(py);
-                -1
             } else {
                 let name = py.mut_from_borrowed_ptr::<$crate::PyObjectRef>(name);
                 let value = py.from_borrowed_ptr::<$crate::PyObjectRef>(value);
-                let result = match name.extract() {
+                result = match name.extract() {
                     Ok(name) => match value.extract() {
-                        Ok(value) => slf.$f(name, value).into(),
+                        Ok(value) => slf.$fn_set(name, value).into(),
                         Err(e) => Err(e.into()),
                     },
                     Err(e) => Err(e.into()),
                 };
+            }
             match result {
                 Ok(_) => 0,
                 Err(e) => {
@@ -353,38 +352,43 @@ macro_rules! py_func_set {
                 }
             }
         }
-        }
 
-        Some(wrap::<T>)
+        Some(wrap::<$generic>)
     }};
 }
 
-#[macro_export]
 #[doc(hidden)]
 macro_rules! py_func_del {
-    ($trait:ident, $class:ident :: $f:ident) => {{
+    ($trait_name:ident, $generic:ident, $fn_del:ident) => {{
-        #[allow(unused_mut)]
+        unsafe extern "C" fn wrap<$generic>(
-        unsafe extern "C" fn wrap<T>(
             slf: *mut $crate::ffi::PyObject,
             name: *mut $crate::ffi::PyObject,
             value: *mut $crate::ffi::PyObject,
         ) -> $crate::libc::c_int
         where
-            T: for<'p> $trait<'p>,
+            T: for<'p> $trait_name<'p>,
         {
             use $crate::ObjectProtocol;
 
             let _pool = $crate::GILPool::new();
             let py = $crate::Python::assume_gil_acquired();
 
+            let result;
             if value.is_null() {
-                let slf = py.mut_from_borrowed_ptr::<T>(slf);
+                let slf = py.mut_from_borrowed_ptr::<$generic>(slf);
                 let name = py.from_borrowed_ptr::<$crate::PyObjectRef>(name);
 
-                let result = match name.extract() {
+                result = match name.extract() {
-                    Ok(name) => slf.$f(name).into(),
+                    Ok(name) => slf.$fn_del(name).into(),
                     Err(e) => Err(e.into()),
                 };
+            } else {
+                result = Err(PyErr::new::<exc::NotImplementedError, _>(format!(
+                    "Subscript assignment not supported by {:?}",
+                    stringify!($generic)
+                )));
+            }
+
             match result {
                 Ok(_) => 0,
                 Err(e) => {
@@ -392,17 +396,9 @@ macro_rules! py_func_del {
                     -1
                 }
             }
-            } else {
-                let e = PyErr::new::<exc::NotImplementedError, _>(format!(
-                    "Subscript assignment not supported by {:?}",
-                    stringify!(T)
-                ));
-                e.restore(py);
-                -1
-            }
         }
 
-        Some(wrap::<T>)
+        Some(wrap::<$generic>)
     }};
 }
 
@@ -421,7 +417,7 @@ macro_rules! py_func_set_del {
 
             let _pool = $crate::GILPool::new();
             let py = $crate::Python::assume_gil_acquired();
-            let slf = py.mut_from_borrowed_ptr::<T>(slf);
+            let slf = py.mut_from_borrowed_ptr::<$generic>(slf);
             let name = py.from_borrowed_ptr::<$crate::PyObjectRef>(name);
 
             let result;
@@ -448,6 +444,6 @@ macro_rules! py_func_set_del {
                 }
             }
         }
-        Some(wrap::<T>)
+        Some(wrap::<$generic>)
     }};
 }

src/class/mapping.rs

@@ -208,7 +208,7 @@ where
 {
     #[inline]
     fn mp_ass_subscript() -> Option<ffi::objobjargproc> {
-        py_func_set!(PyMappingSetItemProtocol, T::__setitem__)
+        py_func_set!(PyMappingSetItemProtocol, T, __setitem__)
     }
 }
 
@@ -226,7 +226,7 @@ where
 {
     #[inline]
     default fn mp_del_subscript() -> Option<ffi::objobjargproc> {
-        py_func_del!(PyMappingDelItemProtocol, T::__delitem__)
+        py_func_del!(PyMappingDelItemProtocol, T, __delitem__)
     }
 }
 

src/class/sequence.rs

@@ -330,21 +330,16 @@ where
             let py = Python::assume_gil_acquired();
             let slf = py.mut_from_borrowed_ptr::<T>(slf);
 
+            let result;
             if value.is_null() {
-                let result = slf.__delitem__(key as isize).into();
+                result = slf.__delitem__(key as isize).into();
-                match result {
-                    Ok(_) => 0,
-                    Err(e) => {
-                        e.restore(py);
-                        -1
-                    }
-                }
             } else {
                 let value = py.from_borrowed_ptr::<PyObjectRef>(value);
-                let result = match value.extract() {
+                result = match value.extract() {
                     Ok(value) => slf.__setitem__(key as isize, value).into(),
                     Err(e) => Err(e),
                 };
+            }
             match result {
                 Ok(_) => 0,
                 Err(e) => {
@@ -353,7 +348,6 @@ where
                 }
             }
         }
-        }
         Some(wrap::<T>)
     }
 }