Merge #2947

2947: change PyModule::add_class to return an error if class creation fails r=adamreichold a=davidhewitt

Related to #2942 

At the moment there are panics deep in the `#[pyclass]` machinery when initialising the type fails. This PR adjusts a number of these functions to return `PyResult` instead, so that we can handle the error more appropriately further down the pipeline.

For example, take the following snippet:

```rust
#[pyclass(extends = PyBool)]
struct ExtendsBool;

#[pymodule]
fn pyo3_scratch(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add_class::<ExtendsBool>()?;
    Ok(())
}
```

Currently, importing this module will fail with a panic:

```
TypeError: type 'bool' is not an acceptable base type
thread '<unnamed>' panicked at 'An error occurred while initializing class ExtendsBool', /Users/david/Dev/pyo3/src/pyclass.rs:412:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module>
    from .pyo3_scratch import *
pyo3_runtime.PanicException: An error occurred while initializing class ExtendsBool
```

After this PR, this import still fails, but with a slightly cleaner, more Pythonic error:

```
TypeError: type 'bool' is not an acceptable base type

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module>
    from .pyo3_scratch import *
RuntimeError: An error occurred while initializing class ExtendsBool
```

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

benches/bench_pyclass.rs

@@ -1,5 +1,5 @@
 use criterion::{criterion_group, criterion_main, Criterion};
-use pyo3::{impl_::pyclass::LazyStaticType, prelude::*};
+use pyo3::{impl_::pyclass::LazyTypeObject, prelude::*};
 
 /// This is a feature-rich class instance used to benchmark various parts of the pyclass lifecycle.
 #[pyclass]
@@ -31,8 +31,8 @@ pub fn first_time_init(b: &mut criterion::Bencher<'_>) {
         b.iter(|| {
             // This is using an undocumented internal PyO3 API to measure pyclass performance; please
             // don't use this in your own code!
-            let ty = LazyStaticType::new();
-            ty.get_or_init::<MyClass>(py);
+            let ty = LazyTypeObject::<MyClass>::new();
+            ty.get_or_init(py);
         });
     });
 }

guide/src/class.md

@@ -979,9 +979,9 @@ unsafe impl pyo3::type_object::PyTypeInfo for MyClass {
     const MODULE: ::std::option::Option<&'static str> = ::std::option::Option::None;
     #[inline]
     fn type_object_raw(py: pyo3::Python<'_>) -> *mut pyo3::ffi::PyTypeObject {
-        use pyo3::impl_::pyclass::LazyStaticType;
-        static TYPE_OBJECT: LazyStaticType = LazyStaticType::new();
-        TYPE_OBJECT.get_or_init::<Self>(py)
+        <Self as pyo3::impl_::pyclass::PyClassImpl>::lazy_type_object()
+            .get_or_init(py)
+            .as_type_ptr()
     }
 }
 
@@ -1033,6 +1033,12 @@ impl pyo3::impl_::pyclass::PyClassImpl for MyClass {
         static INTRINSIC_ITEMS: PyClassItems = PyClassItems { slots: &[], methods: &[] };
         PyClassItemsIter::new(&INTRINSIC_ITEMS, collector.py_methods())
     }
+
+    fn lazy_type_object() -> &'static pyo3::impl_::pyclass::LazyTypeObject<MyClass> {
+        use pyo3::impl_::pyclass::LazyTypeObject;
+        static TYPE_OBJECT: LazyTypeObject<MyClass> = LazyTypeObject::new();
+        &TYPE_OBJECT
+    }
 }
 
 # Python::with_gil(|py| {

newsfragments/2947.changed.md

@@ -0,0 +1 @@
+Improve exception raised when creating `#[pyclass]` type object fails during module import.

pyo3-macros-backend/src/pyclass.rs

@@ -756,9 +756,9 @@ fn impl_pytypeinfo(
             fn type_object_raw(py: _pyo3::Python<'_>) -> *mut _pyo3::ffi::PyTypeObject {
                 #deprecations
 
-                use _pyo3::impl_::pyclass::LazyStaticType;
-                static TYPE_OBJECT: LazyStaticType = LazyStaticType::new();
-                TYPE_OBJECT.get_or_init::<Self>(py)
+                <#cls as _pyo3::impl_::pyclass::PyClassImpl>::lazy_type_object()
+                    .get_or_init(py)
+                    .as_type_ptr()
             }
         }
     }
@@ -1038,6 +1038,12 @@ impl<'a> PyClassImplsBuilder<'a> {
                 #dict_offset
 
                 #weaklist_offset
+
+                fn lazy_type_object() -> &'static _pyo3::impl_::pyclass::LazyTypeObject<Self> {
+                    use _pyo3::impl_::pyclass::LazyTypeObject;
+                    static TYPE_OBJECT: LazyTypeObject<#cls> = LazyTypeObject::new();
+                    &TYPE_OBJECT
+                }
             }
 
             #[doc(hidden)]

src/impl_/pyclass.rs

@@ -15,8 +15,8 @@ use std::{
     thread,
 };
 
-mod lazy_static_type;
-pub use lazy_static_type::LazyStaticType;
+mod lazy_type_object;
+pub use lazy_type_object::LazyTypeObject;
 
 /// Gets the offset of the dictionary from the start of the object in bytes.
 #[inline]
@@ -137,7 +137,7 @@ unsafe impl Sync for PyClassItems {}
 ///
 /// Users are discouraged from implementing this trait manually; it is a PyO3 implementation detail
 /// and may be changed at any time.
-pub trait PyClassImpl: Sized {
+pub trait PyClassImpl: Sized + 'static {
     /// Class doc string
     const DOC: &'static str = "\0";
 
@@ -194,6 +194,8 @@ pub trait PyClassImpl: Sized {
     fn weaklist_offset() -> Option<ffi::Py_ssize_t> {
         None
     }
+
+    fn lazy_type_object() -> &'static LazyTypeObject<Self>;
 }
 
 /// Iterator used to process all class items during type instantiation.

src/impl_/pyclass/lazy_static_type.rs → src/impl_/pyclass/lazy_type_object.rs

@@ -1,63 +1,96 @@
 use std::{
     borrow::Cow,
     ffi::CStr,
+    marker::PhantomData,
     thread::{self, ThreadId},
 };
 
 use parking_lot::{const_mutex, Mutex};
 
 use crate::{
-    ffi, once_cell::GILOnceCell, pyclass::create_type_object, IntoPyPointer, PyClass,
-    PyMethodDefType, PyObject, PyResult, Python,
+    exceptions::PyRuntimeError, ffi, once_cell::GILOnceCell, pyclass::create_type_object,
+    types::PyType, AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject,
+    PyResult, Python,
 };
 
 use super::PyClassItemsIter;
 
 /// Lazy type object for PyClass.
 #[doc(hidden)]
-pub struct LazyStaticType {
-    // Boxed because Python expects the type object to have a stable address.
-    value: GILOnceCell<*mut ffi::PyTypeObject>,
+pub struct LazyTypeObject<T>(LazyTypeObjectInner, PhantomData<T>);
+
+// Non-generic inner of LazyTypeObject to keep code size down
+struct LazyTypeObjectInner {
+    value: GILOnceCell<Py<PyType>>,
     // Threads which have begun initialization of the `tp_dict`. Used for
     // reentrant initialization detection.
     initializing_threads: Mutex<Vec<ThreadId>>,
-    tp_dict_filled: GILOnceCell<PyResult<()>>,
+    tp_dict_filled: GILOnceCell<()>,
 }
 
-impl LazyStaticType {
-    /// Creates an uninitialized `LazyStaticType`.
+impl<T> LazyTypeObject<T> {
+    /// Creates an uninitialized `LazyTypeObject`.
     pub const fn new() -> Self {
-        LazyStaticType {
-            value: GILOnceCell::new(),
-            initializing_threads: const_mutex(Vec::new()),
-            tp_dict_filled: GILOnceCell::new(),
-        }
+        LazyTypeObject(
+            LazyTypeObjectInner {
+                value: GILOnceCell::new(),
+                initializing_threads: const_mutex(Vec::new()),
+                tp_dict_filled: GILOnceCell::new(),
+            },
+            PhantomData,
+        )
     }
+}
 
-    /// Gets the type object contained by this `LazyStaticType`, initializing it if needed.
-    pub fn get_or_init<T: PyClass>(&self, py: Python<'_>) -> *mut ffi::PyTypeObject {
-        fn inner<T: PyClass>() -> *mut ffi::PyTypeObject {
-            // Safety: `py` is held by the caller of `get_or_init`.
-            let py = unsafe { Python::assume_gil_acquired() };
-            create_type_object::<T>(py)
-        }
+impl<T: PyClass> LazyTypeObject<T> {
+    /// Gets the type object contained by this `LazyTypeObject`, initializing it if needed.
+    pub fn get_or_init<'py>(&'py self, py: Python<'py>) -> &'py PyType {
+        self.get_or_try_init(py).unwrap_or_else(|err| {
+            err.print(py);
+            panic!("failed to create type object for {}", T::NAME)
+        })
+    }
+
+    /// Fallible version of the above.
+    pub(crate) fn get_or_try_init<'py>(&'py self, py: Python<'py>) -> PyResult<&'py PyType> {
+        self.0
+            .get_or_try_init(py, create_type_object::<T>, T::NAME, T::items_iter())
+    }
+}
 
-        // Uses explicit GILOnceCell::get_or_init::<fn() -> *mut ffi::PyTypeObject> monomorphization
-        // so that only this one monomorphization is instantiated (instead of one closure monormization for each T).
-        let type_object = *self
-            .value
-            .get_or_init::<fn() -> *mut ffi::PyTypeObject>(py, inner::<T>);
-        self.ensure_init(py, type_object, T::NAME, T::items_iter());
-        type_object
+impl LazyTypeObjectInner {
+    // Uses dynamically dispatched fn(Python<'py>) -> PyResult<Py<PyType>
+    // so that this code is only instantiated once, instead of for every T
+    // like the generic LazyTypeObject<T> methods above.
+    fn get_or_try_init<'py>(
+        &'py self,
+        py: Python<'py>,
+        init: fn(Python<'py>) -> PyResult<Py<PyType>>,
+        name: &str,
+        items_iter: PyClassItemsIter,
+    ) -> PyResult<&'py PyType> {
+        (|| -> PyResult<_> {
+            let type_object = self.value.get_or_try_init(py, || init(py))?.as_ref(py);
+            self.ensure_init(type_object, name, items_iter)?;
+            Ok(type_object)
+        })()
+        .map_err(|err| {
+            wrap_in_runtime_error(
+                py,
+                err,
+                format!("An error occurred while initializing class {}", name),
+            )
+        })
     }
 
     fn ensure_init(
         &self,
-        py: Python<'_>,
-        type_object: *mut ffi::PyTypeObject,
+        type_object: &PyType,
         name: &str,
         items_iter: PyClassItemsIter,
-    ) {
+    ) -> PyResult<()> {
+        let py = type_object.py();
+
         // We might want to fill the `tp_dict` with python instances of `T`
         // itself. In order to do so, we must first initialize the type object
         // with an empty `tp_dict`: now we can create instances of `T`.
@@ -71,7 +104,7 @@ impl LazyStaticType {
 
         if self.tp_dict_filled.get(py).is_some() {
             // `tp_dict` is already filled: ok.
-            return;
+            return Ok(());
         }
 
         let thread_id = thread::current().id();
@@ -80,7 +113,7 @@ impl LazyStaticType {
             if threads.contains(&thread_id) {
                 // Reentrant call: just return the type object, even if the
                 // `tp_dict` is not filled yet.
-                return;
+                return Ok(());
             }
             threads.push(thread_id);
         }
@@ -113,21 +146,26 @@ impl LazyStaticType {
 
                     match (attr.meth.0)(py) {
                         Ok(val) => items.push((key, val)),
-                        Err(e) => panic!(
-                            "An error occurred while initializing `{}.{}`: {}",
-                            name,
-                            attr.name.trim_end_matches('\0'),
-                            e
-                        ),
+                        Err(err) => {
+                            return Err(wrap_in_runtime_error(
+                                py,
+                                err,
+                                format!(
+                                    "An error occurred while initializing `{}.{}`",
+                                    name,
+                                    attr.name.trim_end_matches('\0')
+                                ),
+                            ))
+                        }
                     }
                 }
             }
         }
 
         // Now we hold the GIL and we can assume it won't be released until we
         // return from the function.
-        let result = self.tp_dict_filled.get_or_init(py, move || {
-            let result = initialize_tp_dict(py, type_object as *mut ffi::PyObject, items);
+        let result = self.tp_dict_filled.get_or_try_init(py, move || {
+            let result = initialize_tp_dict(py, type_object.as_ptr(), items);
 
             // Initialization successfully complete, can clear the thread list.
             // (No further calls to get_or_init() will try to init, on any thread.)
@@ -137,9 +175,14 @@ impl LazyStaticType {
         });
 
         if let Err(err) = result {
-            err.clone_ref(py).print(py);
-            panic!("An error occurred while initializing `{}.__dict__`", name);
+            return Err(wrap_in_runtime_error(
+                py,
+                err.clone_ref(py),
+                format!("An error occurred while initializing `{}.__dict__`", name),
+            ));
         }
+
+        Ok(())
     }
 }
 
@@ -157,5 +200,12 @@ fn initialize_tp_dict(
     Ok(())
 }
 
-// This is necessary for making static `LazyStaticType`s
-unsafe impl Sync for LazyStaticType {}
+// This is necessary for making static `LazyTypeObject`s
+unsafe impl<T> Sync for LazyTypeObject<T> {}
+
+#[cold]
+fn wrap_in_runtime_error(py: Python<'_>, err: PyErr, message: String) -> PyErr {
+    let runtime_err = PyRuntimeError::new_err(message);
+    runtime_err.set_cause(py, Some(err));
+    runtime_err
+}

src/pyclass/create_type_object.rs

@@ -5,7 +5,8 @@ use crate::{
         assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc,
         PyClassItemsIter,
     },
-    PyClass, PyErr, PyMethodDefType, PyResult, PyTypeInfo, Python,
+    types::PyType,
+    Py, PyClass, PyMethodDefType, PyResult, PyTypeInfo, Python,
 };
 use std::{
     collections::HashMap,
@@ -15,11 +16,11 @@ use std::{
     ptr,
 };
 
-pub(crate) fn create_type_object<T>(py: Python<'_>) -> *mut ffi::PyTypeObject
+pub(crate) fn create_type_object<T>(py: Python<'_>) -> PyResult<Py<PyType>>
 where
     T: PyClass,
 {
-    match unsafe {
+    unsafe {
         PyTypeBuilder::default()
             .type_doc(T::DOC)
             .offsets(T::dict_offset(), T::weaklist_offset())
@@ -30,9 +31,6 @@ where
             .set_is_sequence(T::IS_SEQUENCE)
             .class_items(T::items_iter())
             .build(py, T::NAME, T::MODULE, std::mem::size_of::<T::Layout>())
-    } {
-        Ok(type_object) => type_object,
-        Err(e) => type_object_creation_failed(py, e, T::NAME),
     }
 }
 
@@ -325,7 +323,7 @@ impl PyTypeBuilder {
         name: &'static str,
         module_name: Option<&'static str>,
         basicsize: usize,
-    ) -> PyResult<*mut ffi::PyTypeObject> {
+    ) -> PyResult<Py<PyType>> {
         // `c_ulong` and `c_uint` have the same size
         // on some platforms (like windows)
         #![allow(clippy::useless_conversion)]
@@ -373,23 +371,15 @@ impl PyTypeBuilder {
         };
 
         // Safety: We've correctly setup the PyType_Spec at this point
-        let type_object = unsafe { ffi::PyType_FromSpec(&mut spec) };
-        if type_object.is_null() {
-            Err(PyErr::fetch(py))
-        } else {
-            for cleanup in std::mem::take(&mut self.cleanup) {
-                cleanup(&self, type_object as _);
-            }
+        let type_object: Py<PyType> =
+            unsafe { Py::from_owned_ptr_or_err(py, ffi::PyType_FromSpec(&mut spec))? };
 
-            Ok(type_object as _)
+        for cleanup in std::mem::take(&mut self.cleanup) {
+            cleanup(&self, type_object.as_ref(py).as_type_ptr());
         }
-    }
-}
 
-#[cold]
-fn type_object_creation_failed(py: Python<'_>, e: PyErr, name: &str) -> ! {
-    e.print(py);
-    panic!("An error occurred while initializing class {}", name)
+        Ok(type_object)
+    }
 }
 
 fn py_class_doc(class_doc: &str) -> Option<*mut c_char> {

src/types/module.rs

@@ -294,7 +294,8 @@ impl PyModule {
     where
         T: PyClass,
     {
-        self.add(T::NAME, T::type_object(self.py()))
+        let py = self.py();
+        self.add(T::NAME, T::lazy_type_object().get_or_try_init(py)?)
     }
 
     /// Adds a function or a (sub)module to a module, using the functions name as name.