From 645600ae5de7c66ddbc84659bb24fc91c6bcb43c Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Tue, 17 May 2022 19:06:09 +0100 Subject: [PATCH] Allow `#[classattr]` methods to be fallible --- CHANGELOG.md | 1 + guide/src/class.md | 6 +++-- pyo3-macros-backend/src/pyimpl.rs | 4 ++-- pyo3-macros-backend/src/pymethod.rs | 9 ++++++-- src/impl_/pymethods.rs | 2 +- src/type_object.rs | 36 +++++++++++++++++++++++------ tests/test_class_attributes.rs | 24 ++++++++++++++++++- 7 files changed, 67 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a184d25e81..d8a0cc9f703 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Deprecate `ToBorrowedObject` trait (it is only used as a wrapper for `ToPyObject`). [#2333](https://github.com/PyO3/pyo3/pull/2333) - `impl IntoPy for [T; N]` now requires `T: IntoPy` rather than `T: ToPyObject`. [#2326](https://github.com/PyO3/pyo3/pull/2326) - Correct `wrap_pymodule` to match normal namespacing rules: it no longer "sees through" glob imports of `use submodule::*` when `submodule::submodule` is a `#[pymodule]`. [#2363](https://github.com/PyO3/pyo3/pull/2363) +- Allow `#[classattr]` methods to be fallible. [#2385](https://github.com/PyO3/pyo3/pull/2385) ### Fixed diff --git a/guide/src/class.md b/guide/src/class.md index 4ce944911c4..fc284030f93 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -583,8 +583,7 @@ impl MyClass { ## Class attributes To create a class attribute (also called [class variable][classattr]), a method without -any arguments can be annotated with the `#[classattr]` attribute. The return type must be `T` for -some `T` that implements `IntoPy`. +any arguments can be annotated with the `#[classattr]` attribute. ```rust # use pyo3::prelude::*; @@ -604,6 +603,9 @@ Python::with_gil(|py| { }); ``` +> Note: if the method has a `Result` return type and returns an `Err`, PyO3 will panic during +class creation. + If the class attribute is defined with `const` code only, one can also annotate associated constants: diff --git a/pyo3-macros-backend/src/pyimpl.rs b/pyo3-macros-backend/src/pyimpl.rs index b426b39c35f..3338c76cdeb 100644 --- a/pyo3-macros-backend/src/pyimpl.rs +++ b/pyo3-macros-backend/src/pyimpl.rs @@ -171,9 +171,9 @@ pub fn gen_py_const(cls: &syn::Type, spec: &ConstSpec) -> TokenStream { _pyo3::class::PyClassAttributeDef::new( #python_name, _pyo3::impl_::pymethods::PyClassAttributeFactory({ - fn __wrap(py: _pyo3::Python<'_>) -> _pyo3::PyObject { + fn __wrap(py: _pyo3::Python<'_>) -> _pyo3::PyResult<_pyo3::PyObject> { #deprecations - _pyo3::IntoPy::into_py(#cls::#member, py) + ::std::result::Result::Ok(_pyo3::IntoPy::into_py(#cls::#member, py)) } __wrap }) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 9d5a1bbf20f..20bb476c704 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -349,9 +349,14 @@ fn impl_py_class_attribute(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { _pyo3::class::PyClassAttributeDef::new( #python_name, _pyo3::impl_::pymethods::PyClassAttributeFactory({ - fn __wrap(py: _pyo3::Python<'_>) -> _pyo3::PyObject { + fn __wrap(py: _pyo3::Python<'_>) -> _pyo3::PyResult<_pyo3::PyObject> { #deprecations - _pyo3::IntoPy::into_py(#cls::#name(), py) + let mut ret = #cls::#name(); + if false { + use _pyo3::impl_::ghost::IntoPyResult; + ret.assert_into_py_result(); + } + _pyo3::callback::convert(py, ret) } __wrap }) diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 57024ae7cfe..12fbac75a2c 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -76,7 +76,7 @@ pub struct PyGetter(pub ffi::getter); #[derive(Clone, Copy, Debug)] pub struct PySetter(pub ffi::setter); #[derive(Clone, Copy)] -pub struct PyClassAttributeFactory(pub for<'p> fn(Python<'p>) -> PyObject); +pub struct PyClassAttributeFactory(pub for<'p> fn(Python<'p>) -> PyResult); // TODO: it would be nice to use CStr in these types, but then the constructors can't be const fn // until `CStr::from_bytes_with_nul_unchecked` is const fn. diff --git a/src/type_object.rs b/src/type_object.rs index 5f800e8eb06..d0693df90e7 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -133,8 +133,8 @@ impl LazyStaticType { return; } + let thread_id = thread::current().id(); { - let thread_id = thread::current().id(); let mut threads = self.initializing_threads.lock(); if threads.contains(&thread_id) { // Reentrant call: just return the type object, even if the @@ -144,13 +144,29 @@ impl LazyStaticType { threads.push(thread_id); } + struct InitializationGuard<'a> { + initializing_threads: &'a Mutex>, + thread_id: ThreadId, + } + impl Drop for InitializationGuard<'_> { + fn drop(&mut self) { + let mut threads = self.initializing_threads.lock(); + threads.retain(|id| *id != self.thread_id); + } + } + + let guard = InitializationGuard { + initializing_threads: &self.initializing_threads, + thread_id, + }; + // Pre-compute the class attribute objects: this can temporarily // release the GIL since we're calling into arbitrary user code. It // means that another thread can continue the initialization in the // meantime: at worst, we'll just make a useless computation. let mut items = vec![]; for_all_items(&mut |class_items| { - items.extend(class_items.methods.iter().filter_map(|def| { + for def in class_items.methods { if let PyMethodDefType::ClassAttribute(attr) = def { let key = extract_cstr_or_leak_cstring( attr.name, @@ -158,12 +174,17 @@ impl LazyStaticType { ) .unwrap(); - let val = (attr.meth.0)(py); - Some((key, val)) - } else { - None + 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 + ), + } } - })); + } }); // Now we hold the GIL and we can assume it won't be released until we @@ -173,6 +194,7 @@ impl LazyStaticType { // Initialization successfully complete, can clear the thread list. // (No further calls to get_or_init() will try to init, on any thread.) + std::mem::forget(guard); *self.initializing_threads.lock() = Vec::new(); result }); diff --git a/tests/test_class_attributes.rs b/tests/test_class_attributes.rs index 55f0d8828d6..55353d405a1 100644 --- a/tests/test_class_attributes.rs +++ b/tests/test_class_attributes.rs @@ -1,6 +1,6 @@ #![cfg(feature = "macros")] -use pyo3::prelude::*; +use pyo3::{exceptions::PyValueError, prelude::*}; mod common; @@ -89,3 +89,25 @@ fn recursive_class_attributes() { py_assert!(py, foo_obj, "foo_obj.bar.x == 2"); py_assert!(py, bar_obj, "bar_obj.a_foo.x == 3"); } + +#[test] +#[should_panic( + expected = "An error occurred while initializing `BrokenClass.fails_to_init`: \ + ValueError: failed to create class attribute" +)] +fn test_fallible_class_attribute() { + #[pyclass] + struct BrokenClass; + + #[pymethods] + impl BrokenClass { + #[classattr] + fn fails_to_init() -> PyResult { + Err(PyValueError::new_err("failed to create class attribute")) + } + } + + Python::with_gil(|py| { + py.get_type::(); + }) +}