From 74778422c36aced41f05b51ee5564ab6214d4b9c Mon Sep 17 00:00:00 2001 From: mejrs Date: Sat, 23 Apr 2022 19:59:40 +0200 Subject: [PATCH 1/2] Make arrays of pyclasses easier to sue --- src/conversions/array.rs | 89 +++++++++++++++++++++++++++++++++++++--- src/types/list.rs | 5 ++- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/conversions/array.rs b/src/conversions/array.rs index 614b56c46a3..cf385f365d7 100644 --- a/src/conversions/array.rs +++ b/src/conversions/array.rs @@ -3,14 +3,39 @@ use crate::{exceptions, PyErr}; #[cfg(min_const_generics)] mod min_const_generics { use super::invalid_sequence_length; - use crate::{FromPyObject, IntoPy, PyAny, PyObject, PyResult, PyTryFrom, Python, ToPyObject}; + use crate::conversion::IntoPyPointer; + use crate::{ + ffi, FromPyObject, IntoPy, Py, PyAny, PyObject, PyResult, PyTryFrom, Python, ToPyObject, + }; impl IntoPy for [T; N] where - T: ToPyObject, + T: IntoPy, { fn into_py(self, py: Python<'_>) -> PyObject { - self.as_ref().to_object(py) + unsafe { + #[allow(deprecated)] // we're not on edition 2021 yet + let elements = std::array::IntoIter::new(self); + let len = N as ffi::Py_ssize_t; + + let ptr = ffi::PyList_New(len); + + // We create the `Py` pointer here for two reasons: + // - panics if the ptr is null + // - its Drop cleans up the list if user code panics. + let list: Py = Py::from_owned_ptr(py, ptr); + + for (i, obj) in (0..len).zip(elements) { + let obj = obj.into_py(py).into_ptr(); + + #[cfg(not(Py_LIMITED_API))] + ffi::PyList_SET_ITEM(ptr, i, obj); + #[cfg(Py_LIMITED_API)] + ffi::PyList_SetItem(ptr, i, obj); + } + + list + } } } @@ -149,17 +174,69 @@ mod min_const_generics { #[cfg(not(min_const_generics))] mod array_impls { use super::invalid_sequence_length; - use crate::{FromPyObject, IntoPy, PyAny, PyObject, PyResult, PyTryFrom, Python, ToPyObject}; + use crate::conversion::IntoPyPointer; + use crate::{ + ffi, FromPyObject, IntoPy, Py, PyAny, PyObject, PyResult, PyTryFrom, Python, ToPyObject, + }; + use std::mem::{transmute_copy, ManuallyDrop}; macro_rules! array_impls { ($($N:expr),+) => { $( impl IntoPy for [T; $N] where - T: ToPyObject + T: IntoPy { fn into_py(self, py: Python<'_>) -> PyObject { - self.as_ref().to_object(py) + + struct ArrayGuard { + elements: [ManuallyDrop; $N], + start: usize, + } + + impl Drop for ArrayGuard { + fn drop(&mut self) { + unsafe { + let needs_drop = self.elements.get_mut(self.start..).unwrap(); + for item in needs_drop{ + ManuallyDrop::drop(item); + } + } + } + } + + unsafe { + let ptr = ffi::PyList_New($N as ffi::Py_ssize_t); + + // We create the `Py` pointer here for two reasons: + // - panics if the ptr is null + // - its Drop cleans up the list if user code panics. + let list: Py = Py::from_owned_ptr(py, ptr); + + let slf = ManuallyDrop::new(self); + + let mut guard = ArrayGuard{ + // the transmute size check is _very_ dumb around generics + elements: transmute_copy(&slf), + start: 0 + }; + + for i in 0..$N { + let obj: T = ManuallyDrop::take(&mut guard.elements[i]); + guard.start += 1; + + let obj = obj.into_py(py).into_ptr(); + + #[cfg(not(Py_LIMITED_API))] + ffi::PyList_SET_ITEM(ptr, i as ffi::Py_ssize_t, obj); + #[cfg(Py_LIMITED_API)] + ffi::PyList_SetItem(ptr, i as ffi::Py_ssize_t, obj); + } + + std::mem::forget(guard); + + list + } } } diff --git a/src/types/list.rs b/src/types/list.rs index ab69859e9fd..f8da1119f8b 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -34,8 +34,9 @@ fn new_from_iter( let ptr = ffi::PyList_New(len); - // - Panics if the ptr is null - // - Cleans up the list if `convert` or the asserts panic + // We create the `Py` pointer here for two reasons: + // - panics if the ptr is null + // - its Drop cleans up the list if user code or the asserts panic. let list: Py = Py::from_owned_ptr(py, ptr); let mut counter: Py_ssize_t = 0; From c69d6ca92d7dd944e996ab80ec72d6714e256ee4 Mon Sep 17 00:00:00 2001 From: mejrs Date: Sun, 1 May 2022 11:18:00 +0200 Subject: [PATCH 2/2] Improve `IntoPyCallbackOutput` compile error --- pyo3-macros-backend/src/method.rs | 12 +++++++++-- pyo3-macros-backend/src/pymethod.rs | 19 ++++++++++++++++- src/callback.rs | 24 +++++++++++++++++++++ tests/ui/invalid_result_conversion.stderr | 26 +++++++++++------------ 4 files changed, 64 insertions(+), 17 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index ddf2244f199..d0209c67f5e 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -478,8 +478,16 @@ impl<'a> FnSpec<'a> { } else { quote!(#func_name) }; - let rust_call = - quote! { _pyo3::callback::convert(#py, #rust_name(#self_arg #(#arg_names),*)) }; + + // The method call is necessary to generate a decent error message. + let rust_call = quote! { + let ret = #rust_name(#self_arg #(#arg_names),*); + + use _pyo3::callback::IntoResult; + let ret: _pyo3::PyResult<_> = ret.into_result(); + + _pyo3::callback::convert(#py, ret) + }; let krate = &self.krate; Ok(match self.convention { CallingConvention::Noargs => { diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 9ed9b4300b8..9d5a1bbf20f 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -495,6 +495,22 @@ pub fn impl_py_getter_def(cls: &syn::Type, property_type: PropertyType<'_>) -> R self_type.receiver(cls, ExtractErrorMode::Raise) } }; + + let conversion = match property_type { + PropertyType::Descriptor { .. } => { + quote! { + let item: _pyo3::Py<_pyo3::PyAny> = _pyo3::IntoPy::into_py(item, _py); + ::std::result::Result::Ok(_pyo3::conversion::IntoPyPointer::into_ptr(item)) + } + } + // Forward to `IntoPyCallbackOutput`, to handle `#[getter]`s returning results. + PropertyType::Function { .. } => { + quote! { + _pyo3::callback::convert(_py, item) + } + } + }; + Ok(quote! { _pyo3::class::PyMethodDefType::Getter({ #deprecations @@ -509,7 +525,8 @@ pub fn impl_py_getter_def(cls: &syn::Type, property_type: PropertyType<'_>) -> R let _py = gil.python(); _pyo3::callback::panic_result_into_callback_output(_py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #slf - _pyo3::callback::convert(_py, #getter_impl) + let item = #getter_impl; + #conversion })) } __wrap diff --git a/src/callback.rs b/src/callback.rs index 3794b9e5906..81fb4d1d5b1 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -35,6 +35,30 @@ impl PyCallbackOutput for () { const ERR_VALUE: Self = (); } +#[doc(hidden)] +pub trait IntoResult { + fn into_result(self) -> R; +} + +impl IntoResult> for T +where + T: IntoPy, +{ + fn into_result(self) -> PyResult { + Ok(self) + } +} + +impl IntoResult> for Result +where + T: IntoPy, + E: Into, +{ + fn into_result(self) -> PyResult { + self.map_err(Into::into) + } +} + /// Convert the result of callback function into the appropriate return value. pub trait IntoPyCallbackOutput { fn convert(self, py: Python<'_>) -> PyResult; diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index a606f605fcf..2e3e2be3c74 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -1,14 +1,12 @@ -error[E0277]: the trait bound `Result<(), MyError>: IntoPyCallbackOutput<_>` is not satisfied - --> tests/ui/invalid_result_conversion.rs:21:1 - | -21 | #[pyfunction] - | ^^^^^^^^^^^^^ the trait `IntoPyCallbackOutput<_>` is not implemented for `Result<(), MyError>` - | - = help: the following implementations were found: - as IntoPyCallbackOutput> -note: required by a bound in `pyo3::callback::convert` - --> src/callback.rs:182:8 - | -182 | T: IntoPyCallbackOutput, - | ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `pyo3::callback::convert` - = note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info) +error[E0599]: the method `into_result` exists for enum `Result<(), MyError>`, but its trait bounds were not satisfied + --> tests/ui/invalid_result_conversion.rs:21:1 + | +21 | #[pyfunction] + | ^^^^^^^^^^^^^ method cannot be called on `Result<(), MyError>` due to unsatisfied trait bounds + | + = note: the following trait bounds were not satisfied: + `&Result<(), MyError>: IntoPy>` + which is required by `&Result<(), MyError>: IntoResult, PyErr>>` + `&mut Result<(), MyError>: IntoPy>` + which is required by `&mut Result<(), MyError>: IntoResult, PyErr>>` + = note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)