Better trait bounds with PyMethodsProtocol

Author: konstin

src/freelist.rs

@@ -6,7 +6,6 @@ use crate::err::PyResult;
 use crate::ffi;
 use crate::python::Python;
 use crate::typeob::{pytype_drop, PyObjectAlloc, PyTypeInfo};
-use class::methods::PyMethodsProtocol;
 use std::mem;
 use std::os::raw::c_void;
 
@@ -71,7 +70,7 @@ impl<T> FreeList<T> {
 
 impl<T> PyObjectAlloc for T
 where
-    T: PyObjectWithFreeList + PyMethodsProtocol,
+    T: PyObjectWithFreeList,
 {
     unsafe fn alloc(_py: Python) -> PyResult<*mut ffi::PyObject> {
         let obj = if let Some(obj) = <Self as PyObjectWithFreeList>::get_free_list().pop() {

src/typeob.rs

@@ -199,11 +199,8 @@ pub(crate) unsafe fn pytype_drop<T: PyTypeInfo>(py: Python, obj: *mut ffi::PyObj
 ///
 /// All native types and all `#[pyclass]` types use the default functions, while
 /// [PyObjectWithFreeList](crate::freelist::PyObjectWithFreeList) gets a special version.
-pub trait PyObjectAlloc: PyTypeInfo + PyMethodsProtocol + Sized {
+pub trait PyObjectAlloc: PyTypeInfo + Sized {
     unsafe fn alloc(_py: Python) -> PyResult<*mut ffi::PyObject> {
-        // TODO: remove this
-        <Self as PyTypeCreate>::init_type();
-
         let tp_ptr = Self::type_object();
         let alloc = (*tp_ptr).tp_alloc.unwrap_or(ffi::PyType_GenericAlloc);
         let obj = alloc(tp_ptr, 0);
@@ -261,21 +258,8 @@ pub trait PyTypeObject {
 
 /// Python object types that have a corresponding type object and be
 /// instanciated with [Self::create()]
-pub trait PyTypeCreate: PyObjectAlloc + PyTypeInfo + PyMethodsProtocol + Sized {
-    #[inline]
-    fn init_type() {
-        let type_object = unsafe { *<Self as PyTypeInfo>::type_object() };
-
-        if (type_object.tp_flags & ffi::Py_TPFLAGS_READY) == 0 {
-            // automatically initialize the class on-demand
-            let gil = Python::acquire_gil();
-            let py = gil.python();
-
-            initialize_type::<Self>(py, None).unwrap_or_else(|_| {
-                panic!("An error occurred while initializing class {}", Self::NAME)
-            });
-        }
-    }
+pub trait PyTypeCreate: PyObjectAlloc + PyTypeInfo + Sized {
+    fn init_type();
 
     #[inline]
     fn type_object() -> Py<PyType> {
@@ -300,7 +284,25 @@ pub trait PyTypeCreate: PyObjectAlloc + PyTypeInfo + PyMethodsProtocol + Sized {
     }
 }
 
-impl<T> PyTypeCreate for T where T: PyObjectAlloc + PyTypeInfo + PyMethodsProtocol {}
+impl<T> PyTypeCreate for T
+where
+    T: PyObjectAlloc + PyTypeInfo + PyMethodsProtocol,
+{
+    #[inline]
+    fn init_type() {
+        let type_object = unsafe { *<Self as PyTypeInfo>::type_object() };
+
+        if (type_object.tp_flags & ffi::Py_TPFLAGS_READY) == 0 {
+            // automatically initialize the class on-demand
+            let gil = Python::acquire_gil();
+            let py = gil.python();
+
+            initialize_type::<Self>(py, None).unwrap_or_else(|_| {
+                panic!("An error occurred while initializing class {}", Self::NAME)
+            });
+        }
+    }
+}
 
 impl<T> PyTypeObject for T
 where
@@ -316,8 +318,10 @@ where
 }
 
 /// Register new type in python object system.
+///
+/// Currently, module_name is always None, so it defaults to builtins.
 #[cfg(not(Py_LIMITED_API))]
-pub fn initialize_type<T>(py: Python, module_name: Option<&str>) -> PyResult<()>
+pub fn initialize_type<T>(py: Python, module_name: Option<&str>) -> PyResult<*mut ffi::PyTypeObject>
 where
     T: PyObjectAlloc + PyTypeInfo + PyMethodsProtocol,
 {
@@ -434,7 +438,7 @@ where
     // register type object
     unsafe {
         if ffi::PyType_Ready(type_object) == 0 {
-            Ok(())
+            Ok(type_object as *mut ffi::PyTypeObject)
         } else {
             PyErr::fetch(py).into()
         }

src/types/mod.rs

@@ -127,14 +127,6 @@ macro_rules! pyobject_native_type_convert(
             }
         }
 
-        // We currently need to fulfill this trait bound for PyTypeCreate, even though we know
-        // that the function will never actuall be called
-        impl<$($type_param,)*> $crate::class::methods::PyMethodsProtocol for $name {
-            fn py_methods() -> Vec<&'static $crate::class::methods::PyMethodDefType> {
-                unreachable!();
-            }
-        }
-
         impl<$($type_param,)*> $crate::typeob::PyObjectAlloc for $name {}
 
         impl<$($type_param,)*> $crate::typeob::PyTypeCreate for $name {

src/types/module.rs

@@ -9,13 +9,11 @@ use crate::instance::PyObjectWithGIL;
 use crate::object::PyObject;
 use crate::objectprotocol::ObjectProtocol;
 use crate::python::{Python, ToPyPointer};
-use crate::typeob::{initialize_type, PyTypeInfo};
-use crate::types::{exceptions, PyDict, PyObjectRef, PyType};
-use crate::PyObjectAlloc;
-use class::methods::PyMethodsProtocol;
+use crate::types::{exceptions, PyDict, PyObjectRef};
 use std::ffi::{CStr, CString};
 use std::os::raw::c_char;
 use std::str;
+use typeob::PyTypeCreate;
 
 /// Represents a Python `module` object.
 #[repr(transparent)]
@@ -151,23 +149,9 @@ impl PyModule {
     /// and adds the type to this module.
     pub fn add_class<T>(&self) -> PyResult<()>
     where
-        T: PyTypeInfo + PyObjectAlloc + PyMethodsProtocol,
+        T: PyTypeCreate,
     {
-        let ty = unsafe {
-            let ty = <T as PyTypeInfo>::type_object();
-
-            if ((*ty).tp_flags & ffi::Py_TPFLAGS_READY) != 0 {
-                PyType::new::<T>()
-            } else {
-                // automatically initialize the class
-                initialize_type::<T>(self.py(), Some(self.name()?)).unwrap_or_else(|_| {
-                    panic!("An error occurred while initializing class {}", T::NAME)
-                });
-                PyType::new::<T>()
-            }
-        };
-
-        self.setattr(T::NAME, ty)
+        self.setattr(T::NAME, <T as PyTypeCreate>::type_object())
     }
 
     /// Adds a function or a (sub)module to a module, using the functions __name__ as name.

tests/test_class_basics.rs

@@ -68,11 +68,14 @@ fn empty_class_in_module() {
         ty.getattr("__name__").unwrap().extract::<String>().unwrap(),
         "EmptyClassInModule"
     );
+    // Rationale: The class can be added to many modules, but will only be initialized once.
+    // We currently have no way of determining a canonical module, so builtins is better
+    // than using whatever calls init first.
     assert_eq!(
         ty.getattr("__module__")
             .unwrap()
             .extract::<String>()
             .unwrap(),
-        "test_module.nested"
+        "builtins"
     );
 }