From 570107d103ac21e254b6cc40c023c3d61e18dec3 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Tue, 17 May 2022 19:48:40 +0100 Subject: [PATCH 1/3] docs: fix nightly build --- src/exceptions.rs | 4 ++-- src/instance.rs | 21 +++++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/exceptions.rs b/src/exceptions.rs index 6f7c8233e19..fb4be1e4356 100644 --- a/src/exceptions.rs +++ b/src/exceptions.rs @@ -5,8 +5,8 @@ //! The structs in this module represent Python's built-in exceptions, while the modules comprise //! structs representing errors defined in Python code. //! -//! The latter are created with the [`import_exception`] macro, which you can use yourself -//! to import Python exceptions. +//! The latter are created with the [`import_exception`](crate::import_exception) macro, which you +//! can use yourself to import Python exceptions. use crate::{ffi, PyResult, Python}; use std::ffi::CStr; diff --git a/src/instance.rs b/src/instance.rs index 16d9d1a9cdb..8850b452ecc 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -541,8 +541,9 @@ impl Py { /// /// This is equivalent to the Python expression `self.attr_name`. /// - /// If calling this method becomes performance-critical, the [`intern!`] macro can be used - /// to intern `attr_name`, thereby avoiding repeated temporary allocations of Python strings. + /// If calling this method becomes performance-critical, the [`intern!`](crate::intern) macro + /// can be used to intern `attr_name`, thereby avoiding repeated temporary allocations of + /// Python strings. /// /// # Example: `intern!`ing the attribute name /// @@ -577,8 +578,8 @@ impl Py { /// /// This is equivalent to the Python expression `self.attr_name = value`. /// - /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used - /// to intern `attr_name`. + /// To avoid repeated temporary allocations of Python strings, the [`intern!`](crate::intern) + /// macro can be used to intern `attr_name`. /// /// # Example: `intern!`ing the attribute name /// @@ -660,8 +661,8 @@ impl Py { /// /// This is equivalent to the Python expression `self.name(*args, **kwargs)`. /// - /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used - /// to intern `name`. + /// To avoid repeated temporary allocations of Python strings, the [`intern!`](crate::intern) + /// macro can be used to intern `name`. pub fn call_method( &self, py: Python<'_>, @@ -691,8 +692,8 @@ impl Py { /// /// This is equivalent to the Python expression `self.name(*args)`. /// - /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used - /// to intern `name`. + /// To avoid repeated temporary allocations of Python strings, the [`intern!`](crate::intern) + /// macro can be used to intern `name`. pub fn call_method1(&self, py: Python<'_>, name: N, args: A) -> PyResult where N: IntoPy>, @@ -705,8 +706,8 @@ impl Py { /// /// This is equivalent to the Python expression `self.name()`. /// - /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used - /// to intern `name`. + /// To avoid repeated temporary allocations of Python strings, the [`intern!`](crate::intern) + /// macro can be used to intern `name`. pub fn call_method0(&self, py: Python<'_>, name: N) -> PyResult where N: IntoPy>, From 0de0e3f8d606161274c8b9865eb94e3ac8efa037 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 2/3] 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::(); + }) +} From 82b26b7cfab01fa40d486416ac9397ed592c84b4 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Tue, 17 May 2022 19:36:25 +0100 Subject: [PATCH 3/3] datetime: remove reference to leap seconds --- src/types/datetime.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/types/datetime.rs b/src/types/datetime.rs index 67cdf22c3ea..2edcc0e780c 100644 --- a/src/types/datetime.rs +++ b/src/types/datetime.rs @@ -153,9 +153,9 @@ pub trait PyTimeAccess { /// Returns whether this date is the later of two moments with the /// same representation, during a repeated interval. /// - /// This typically occurs at the end of daylight savings time, or during - /// leap seconds. Only valid if the represented time is ambiguous. See - /// [PEP 495](https://www.python.org/dev/peps/pep-0495/) for more detail. + /// This typically occurs at the end of daylight savings time. Only valid if the + /// represented time is ambiguous. + /// See [PEP 495](https://www.python.org/dev/peps/pep-0495/) for more detail. #[cfg(not(PyPy))] fn get_fold(&self) -> bool; } @@ -267,7 +267,12 @@ impl PyDateTime { } /// Alternate constructor that takes a `fold` parameter. A `true` value for this parameter - /// signifies a leap second + /// signifies this this datetime is the later of two moments with the same representation, + /// during a repeated interval. + /// + /// This typically occurs at the end of daylight savings time. Only valid if the + /// represented time is ambiguous. + /// See [PEP 495](https://www.python.org/dev/peps/pep-0495/) for more detail. #[cfg(not(PyPy))] #[allow(clippy::too_many_arguments)] pub fn new_with_fold<'p>( @@ -413,7 +418,7 @@ impl PyTime { } #[cfg(not(PyPy))] - /// Alternate constructor that takes a `fold` argument + /// Alternate constructor that takes a `fold` argument. See [`PyDateTime::new_with_fold`]. pub fn new_with_fold<'p>( py: Python<'p>, hour: u8,