diff --git a/CHANGELOG.md b/CHANGELOG.md index bd321860f37..870157acaf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Implement `ToPyObject` for `[T; N]`. [#2313](https://github.com/PyO3/pyo3/pull/2313) +- Added the internal `IntoPyResult` trait to give better error messages when function return types do not implement `IntoPy`. [#2326](https://github.com/PyO3/pyo3/pull/2326) ### Changed @@ -18,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Move `PyTypeObject::type_object` method to `PyTypeInfo` trait, and deprecate `PyTypeObject` trait. [#2287](https://github.com/PyO3/pyo3/pull/2287) - The deprecated `pyproto` feature is now disabled by default. [#2322](https://github.com/PyO3/pyo3/pull/2322) - 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) ### Fixed diff --git a/codecov.yml b/codecov.yml index 84c56f0dd18..cfef461049a 100644 --- a/codecov.yml +++ b/codecov.yml @@ -11,3 +11,4 @@ coverage: ignore: - tests/*.rs - src/test_hygiene/*.rs + - src/impl_/ghost.rs diff --git a/guide/src/migration.md b/guide/src/migration.md index b2b1b2cfca3..8c770f52a12 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -69,6 +69,10 @@ fn get_type_object(py: Python<'_>) -> &PyType { # Python::with_gil(|py| { get_type_object::(py); }); ``` +### `impl IntoPy for [T; N]` now requires `T: IntoPy` rather than `T: ToPyObject` + +If this leads to errors, simply implement `IntoPy`. Because pyclasses already implement `IntoPy`, you probably don't need to worry about this. + ## from 0.15.* to 0.16 ### Drop support for older technologies diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index ddf2244f199..e25575c1f41 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -478,8 +478,19 @@ 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 mut ret = #rust_name(#self_arg #(#arg_names),*); + + if false { + use _pyo3::impl_::ghost::IntoPyResult; + ret.assert_into_py_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/conversions/array.rs b/src/conversions/array.rs index 614b56c46a3..c6e9e70c15c 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 + } } } @@ -218,7 +295,7 @@ fn invalid_sequence_length(expected: usize, actual: usize) -> PyErr { #[cfg(test)] mod tests { - use crate::{types::PyList, PyResult, Python}; + use crate::{types::PyList, IntoPy, PyResult, Python, ToPyObject}; #[test] fn test_extract_small_bytearray_to_array() { @@ -233,7 +310,6 @@ mod tests { } #[test] fn test_topyobject_array_conversion() { - use crate::ToPyObject; Python::with_gil(|py| { let array: [f32; 4] = [0.0, -16.0, 16.0, 42.0]; let pyobject = array.to_object(py); @@ -258,4 +334,31 @@ mod tests { ); }) } + + #[test] + fn test_intopy_array_conversion() { + Python::with_gil(|py| { + let array: [f32; 4] = [0.0, -16.0, 16.0, 42.0]; + let pyobject = array.into_py(py); + let pylist: &PyList = pyobject.extract(py).unwrap(); + assert_eq!(pylist[0].extract::().unwrap(), 0.0); + assert_eq!(pylist[1].extract::().unwrap(), -16.0); + assert_eq!(pylist[2].extract::().unwrap(), 16.0); + assert_eq!(pylist[3].extract::().unwrap(), 42.0); + }); + } + + #[cfg(feature = "macros")] + #[test] + fn test_pyclass_intopy_array_conversion() { + #[crate::pyclass(crate = "crate")] + struct Foo; + + Python::with_gil(|py| { + let array: [Foo; 8] = [Foo, Foo, Foo, Foo, Foo, Foo, Foo, Foo]; + let pyobject = array.into_py(py); + let list: &PyList = pyobject.cast_as(py).unwrap(); + let _cell: &crate::PyCell = list.get_item(4).unwrap().extract().unwrap(); + }); + } } diff --git a/src/impl_.rs b/src/impl_.rs index 5f27a1fc20f..64ca136062c 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -9,6 +9,7 @@ pub mod extract_argument; pub mod freelist; #[doc(hidden)] pub mod frompyobject; +pub mod ghost; pub(crate) mod not_send; #[doc(hidden)] pub mod pyclass; diff --git a/src/impl_/ghost.rs b/src/impl_/ghost.rs new file mode 100644 index 00000000000..667607dd001 --- /dev/null +++ b/src/impl_/ghost.rs @@ -0,0 +1,18 @@ +/// If it does nothing, was it ever really there? 👻 +/// +/// This is code that is just type checked to e.g. create better compile errors, +/// but that never affects anything at runtime, +use crate::{IntoPy, PyErr, PyObject}; + +pub trait IntoPyResult { + fn assert_into_py_result(&mut self) {} +} + +impl IntoPyResult for T where T: IntoPy {} + +impl IntoPyResult for Result +where + T: IntoPy, + E: Into, +{ +} diff --git a/src/types/list.rs b/src/types/list.rs index e104d245341..2538833dec7 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -33,8 +33,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; diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 713c50925c8..989928c2775 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -96,6 +96,7 @@ fn _test_compile_errors() { fn tests_rust_1_60(t: &trybuild::TestCases) { t.compile_fail("tests/ui/invalid_immutable_pyclass_borrow.rs"); t.compile_fail("tests/ui/invalid_pymethod_receiver.rs"); + t.compile_fail("tests/ui/missing_intopy.rs"); } #[rustversion::before(1.60)] diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index f9f1355532e..f2ca0f5cc03 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -1,3 +1,11 @@ +error[E0599]: no method named `assert_into_py_result` found for enum `Result` in the current scope + --> tests/ui/invalid_result_conversion.rs:21:1 + | +21 | #[pyfunction] + | ^^^^^^^^^^^^^ method not found in `Result<(), MyError>` + | + = note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info) + error[E0277]: the trait bound `Result<(), MyError>: IntoPyCallbackOutput<_>` is not satisfied --> tests/ui/invalid_result_conversion.rs:21:1 | diff --git a/tests/ui/missing_intopy.rs b/tests/ui/missing_intopy.rs new file mode 100644 index 00000000000..7a465b14221 --- /dev/null +++ b/tests/ui/missing_intopy.rs @@ -0,0 +1,8 @@ +struct Blah; + +#[pyo3::pyfunction] +fn blah() -> Blah{ + Blah +} + +fn main(){} \ No newline at end of file diff --git a/tests/ui/missing_intopy.stderr b/tests/ui/missing_intopy.stderr new file mode 100644 index 00000000000..b714d798dd2 --- /dev/null +++ b/tests/ui/missing_intopy.stderr @@ -0,0 +1,38 @@ +error[E0599]: the method `assert_into_py_result` exists for struct `Blah`, but its trait bounds were not satisfied + --> tests/ui/missing_intopy.rs:3:1 + | +1 | struct Blah; + | ------------ + | | + | method `assert_into_py_result` not found for this + | doesn't satisfy `Blah: IntoPy>` + | doesn't satisfy `Blah: IntoPyResult` +2 | +3 | #[pyo3::pyfunction] + | ^^^^^^^^^^^^^^^^^^^ method cannot be called on `Blah` due to unsatisfied trait bounds + | + = note: the following trait bounds were not satisfied: + `Blah: IntoPy>` + which is required by `Blah: IntoPyResult` +note: the following trait must be implemented + --> src/conversion.rs + | + | / pub trait IntoPy: Sized { + | | /// Performs the conversion. + | | fn into_py(self, py: Python<'_>) -> T; + | | } + | |_^ + = note: this error originates in the attribute macro `pyo3::pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the trait bound `Blah: IntoPyCallbackOutput<_>` is not satisfied + --> tests/ui/missing_intopy.rs:3:1 + | +3 | #[pyo3::pyfunction] + | ^^^^^^^^^^^^^^^^^^^ the trait `IntoPyCallbackOutput<_>` is not implemented for `Blah` + | +note: required by a bound in `pyo3::callback::convert` + --> src/callback.rs + | + | T: IntoPyCallbackOutput, + | ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `pyo3::callback::convert` + = note: this error originates in the attribute macro `pyo3::pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)