Don't use intocallback in method macros (#2664)

* Don't use intocallback in method macros

Co-authored-by: mejrs <>

Cargo.toml

@@ -44,7 +44,8 @@ serde = { version = "1.0", optional = true }
 assert_approx_eq = "1.1.0"
 chrono = { version = "0.4" }
 criterion = "0.3.5"
-trybuild = "1.0.49"
+# Required for "and $N others" normalization
+trybuild = ">=1.0.70"
 rustversion = "1.0"
 # 1.0.0 requires Rust 1.50
 proptest = { version = "0.10.1", default-features = false, features = ["std"] }

newsfragments/2664.changed.md

@@ -0,0 +1 @@
+PyO3's macros now emit a much nicer error message if function return values don't implement the required trait(s).

pyo3-macros-backend/src/method.rs

@@ -468,17 +468,12 @@ impl<'a> FnSpec<'a> {
             quote!(#func_name)
         };
 
-        // The method call is necessary to generate a decent error message.
         let rust_call = |args: Vec<TokenStream>| {
             quote! {
                 let mut ret = #rust_name(#self_arg #(#args),*);
-
-                if false {
-                    use _pyo3::impl_::ghost::IntoPyResult;
-                    ret.assert_into_py_result();
-                }
-
-                _pyo3::callback::convert(#py, ret)
+                let owned = _pyo3::impl_::pymethods::OkWrap::wrap(ret, #py);
+                owned.map(|obj| _pyo3::conversion::IntoPyPointer::into_ptr(obj))
+                    .map_err(::core::convert::Into::into)
             }
         };
 

pyo3-macros-backend/src/pymethod.rs

@@ -389,11 +389,8 @@ fn impl_py_class_attribute(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Result<Me
         fn #wrapper_ident(py: _pyo3::Python<'_>) -> _pyo3::PyResult<_pyo3::PyObject> {
             #deprecations
             let mut ret = #fncall;
-            if false {
-                use _pyo3::impl_::ghost::IntoPyResult;
-                ret.assert_into_py_result();
-            }
-            _pyo3::callback::convert(py, ret)
+            let owned = _pyo3::impl_::pymethods::OkWrap::wrap(ret, py);
+            owned.map_err(::core::convert::Into::into)
         }
     };
 

src/impl_.rs

@@ -10,7 +10,6 @@ pub mod deprecations;
 pub mod extract_argument;
 pub mod freelist;
 pub mod frompyobject;
-pub mod ghost;
 pub(crate) mod not_send;
 pub mod panic;
 pub mod pycell;

src/impl_/pymethods.rs

@@ -1,5 +1,5 @@
 use crate::internal_tricks::{extract_cstr_or_leak_cstring, NulByteInString};
-use crate::{ffi, PyAny, PyObject, PyResult, PyTraverseError, Python};
+use crate::{ffi, IntoPy, Py, PyAny, PyErr, PyObject, PyResult, PyTraverseError, Python};
 use std::ffi::CStr;
 use std::fmt;
 use std::os::raw::c_int;
@@ -262,3 +262,29 @@ pub fn unwrap_traverse_result(result: Result<(), PyTraverseError>) -> c_int {
         Err(PyTraverseError(value)) => value,
     }
 }
+
+// The macros need to Ok-wrap the output of user defined functions; i.e. if they're not a result, make them into one.
+pub trait OkWrap<T> {
+    type Error;
+    fn wrap(self, py: Python<'_>) -> Result<Py<PyAny>, Self::Error>;
+}
+
+impl<T> OkWrap<T> for T
+where
+    T: IntoPy<PyObject>,
+{
+    type Error = PyErr;
+    fn wrap(self, py: Python<'_>) -> PyResult<Py<PyAny>> {
+        Ok(self.into_py(py))
+    }
+}
+
+impl<T, E> OkWrap<T> for Result<T, E>
+where
+    T: IntoPy<PyObject>,
+{
+    type Error = E;
+    fn wrap(self, py: Python<'_>) -> Result<Py<PyAny>, Self::Error> {
+        self.map(|o| o.into_py(py))
+    }
+}

src/pyclass.rs

@@ -341,6 +341,10 @@ impl PyTypeBuilder {
         module_name: Option<&'static str>,
         basicsize: usize,
     ) -> PyResult<*mut ffi::PyTypeObject> {
+        // `c_ulong` and `c_uint` have the same size
+        // on some platforms (like windows)
+        #![allow(clippy::useless_conversion)]
+
         self.finalize_methods_and_properties();
 
         if !self.has_new {
@@ -376,9 +380,7 @@ impl PyTypeBuilder {
             name: py_class_qualified_name(module_name, name)?,
             basicsize: basicsize as c_int,
             itemsize: 0,
-            // `c_ulong` and `c_uint` have the same size
-            // on some platforms (like windows)
-            #[allow(clippy::useless_conversion)]
+
             flags: (ffi::Py_TPFLAGS_DEFAULT | self.class_flags)
                 .try_into()
                 .unwrap(),

tests/ui/invalid_result_conversion.stderr

@@ -1,30 +1,18 @@
-error[E0599]: no method named `assert_into_py_result` found for enum `Result` in the current scope
+error[E0277]: the trait bound `PyErr: From<MyError>` is not satisfied
   --> tests/ui/invalid_result_conversion.rs:21:1
    |
 21 | #[pyfunction]
-   | ^^^^^^^^^^^^^ method not found in `Result<(), MyError>`
+   | ^^^^^^^^^^^^^ the trait `From<MyError>` is not implemented for `PyErr`
    |
-note: the method `assert_into_py_result` exists on the type `()`
-  --> src/impl_/ghost.rs
-   |
-   |     fn assert_into_py_result(&mut self) {}
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = help: the following other types implement trait `From<T>`:
+             <PyErr as From<&CancelledError>>
+             <PyErr as From<&IncompleteReadError>>
+             <PyErr as From<&InvalidStateError>>
+             <PyErr as From<&LimitOverrunError>>
+             <PyErr as From<&PanicException>>
+             <PyErr as From<&PyArithmeticError>>
+             <PyErr as From<&PyAssertionError>>
+             <PyErr as From<&PyAttributeError>>
+           and $N others
+   = note: required because of the requirements on the impl of `Into<PyErr>` for `MyError`
    = note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)
-help: use the `?` operator to extract the `()` value, propagating a `Result::Err` value to the caller
-   |
-21 | #[pyfunction]?
-   |              +
-
-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 trait `IntoPyCallbackOutput<U>` is implemented for `Result<T, E>`
-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 `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)

tests/ui/missing_intopy.stderr

@@ -1,45 +1,18 @@
-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 struct
-    | 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 {
-    | ^^^^^^^^^^^^^^^^^^^^^^^^^^
-    = 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`
-    |
-    = help: the following other types implement trait `IntoPyCallbackOutput<Target>`:
-              <() as IntoPyCallbackOutput<()>>
-              <() as IntoPyCallbackOutput<i32>>
-              <*mut PyObject as IntoPyCallbackOutput<*mut PyObject>>
-              <HashCallbackOutput as IntoPyCallbackOutput<isize>>
-              <IterANextOutput<Py<PyAny>, Py<PyAny>> as IntoPyCallbackOutput<*mut PyObject>>
-              <IterANextOutput<T, U> as IntoPyCallbackOutput<IterANextOutput<Py<PyAny>, Py<PyAny>>>>
-              <IterNextOutput<Py<PyAny>, Py<PyAny>> as IntoPyCallbackOutput<*mut PyObject>>
-              <IterNextOutput<T, U> as IntoPyCallbackOutput<IterNextOutput<Py<PyAny>, Py<PyAny>>>>
-            and 7 others
-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)
+error[E0277]: the trait bound `Blah: IntoPy<Py<PyAny>>` is not satisfied
+ --> tests/ui/missing_intopy.rs:3:1
+  |
+3 | #[pyo3::pyfunction]
+  | ^^^^^^^^^^^^^^^^^^^ the trait `IntoPy<Py<PyAny>>` is not implemented for `Blah`
+  |
+  = help: the following other types implement trait `IntoPy<T>`:
+            <&'a OsString as IntoPy<Py<PyAny>>>
+            <&'a Path as IntoPy<Py<PyAny>>>
+            <&'a PathBuf as IntoPy<Py<PyAny>>>
+            <&'a PyErr as IntoPy<Py<PyAny>>>
+            <&'a String as IntoPy<Py<PyAny>>>
+            <&'a [u8] as IntoPy<Py<PyAny>>>
+            <&'a str as IntoPy<Py<PyAny>>>
+            <&'a str as IntoPy<Py<PyString>>>
+          and $N others
+  = note: required because of the requirements on the impl of `OkWrap<Blah>` for `Blah`
+  = note: this error originates in the attribute macro `pyo3::pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)