Fix IntoPyCallbackOutput paper cuts (#2326)

* Implement IntoPy for arrays of IntoPy

* Improve `IntoPyCallbackOutput` compile error

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<T, const N: usize> IntoPy<PyObject> for [T; N]` now requires `T: IntoPy` rather than `T: ToPyObject`. [#2326](https://github.com/PyO3/pyo3/pull/2326)
 
 ### Fixed
 

codecov.yml

@@ -11,3 +11,4 @@ coverage:
 ignore:
   - tests/*.rs
   - src/test_hygiene/*.rs
+  - src/impl_/ghost.rs

guide/src/migration.md

@@ -69,6 +69,10 @@ fn get_type_object<T: PyTypeInfo>(py: Python<'_>) -> &PyType {
 # Python::with_gil(|py| { get_type_object::<pyo3::types::PyList>(py); });
 ```
 
+### `impl<T, const N: usize> IntoPy<PyObject> 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

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 => {

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

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<T, const N: usize> IntoPy<PyObject> for [T; N]
     where
-        T: ToPyObject,
+        T: IntoPy<PyObject>,
     {
         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<PyAny> = 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<T> IntoPy<PyObject> for [T; $N]
                 where
-                    T: ToPyObject
+                    T: IntoPy<PyObject>
                 {
                     fn into_py(self, py: Python<'_>) -> PyObject {
-                        self.as_ref().to_object(py)
+
+                        struct ArrayGuard<T> {
+                            elements: [ManuallyDrop<T>; $N],
+                            start: usize,
+                        }
+
+                        impl<T> Drop for ArrayGuard<T> {
+                            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<PyAny> = 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::<f32>().unwrap(), 0.0);
+            assert_eq!(pylist[1].extract::<f32>().unwrap(), -16.0);
+            assert_eq!(pylist[2].extract::<f32>().unwrap(), 16.0);
+            assert_eq!(pylist[3].extract::<f32>().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<Foo> = list.get_item(4).unwrap().extract().unwrap();
+        });
+    }
 }

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;

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<T> {
+    fn assert_into_py_result(&mut self) {}
+}
+
+impl<T> IntoPyResult<T> for T where T: IntoPy<PyObject> {}
+
+impl<T, E> IntoPyResult<T> for Result<T, E>
+where
+    T: IntoPy<PyObject>,
+    E: Into<PyErr>,
+{
+}

src/types/list.rs

@@ -33,8 +33,9 @@ fn new_from_iter(
 
         let ptr = ffi::PyList_New(len);
 
-        // - Panics if the ptr is null
+        // We create the  `Py` pointer here for two reasons:
-        // - Cleans up the list if `convert` or the asserts panic
+        // - panics if the ptr is null
+        // - its Drop cleans up the list if user code or the asserts panic.
         let list: Py<PyList> = Py::from_owned_ptr(py, ptr);
 
         let mut counter: Py_ssize_t = 0;

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)]

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
     |

tests/ui/missing_intopy.rs

@@ -0,0 +1,8 @@
+struct Blah;
+
+#[pyo3::pyfunction]
+fn blah() -> Blah{
+    Blah
+}
+
+fn main(){}

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<Py<PyAny>>`
+    | doesn't satisfy `Blah: IntoPyResult<Blah>`
+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<Py<PyAny>>`
+            which is required by `Blah: IntoPyResult<Blah>`
+note: the following trait must be implemented
+   --> src/conversion.rs
+    |
+    | / pub trait IntoPy<T>: 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<U>,
+    |        ^^^^^^^^^^^^^^^^^^^^^^^ 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)