From 21a85b1bf6e4a3c607c2bf79390faa185ea0eacd Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Wed, 26 Aug 2020 10:17:19 +0200 Subject: [PATCH 1/9] Added a couple basic tests --- tests/test_result_conversion.rs | 47 +++++++++++++++++++++++++++ tests/ui/invalid_result_conversion.rs | 36 ++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 tests/test_result_conversion.rs create mode 100644 tests/ui/invalid_result_conversion.rs diff --git a/tests/test_result_conversion.rs b/tests/test_result_conversion.rs new file mode 100644 index 00000000000..78d9567f6ba --- /dev/null +++ b/tests/test_result_conversion.rs @@ -0,0 +1,47 @@ +//! Testing https://github.com/PyO3/pyo3/issues/1106. A result type that +//! implements `From for PyErr` should be automatically converted +//! when using `#[pyfunction]`. +//! +//! This only makes sure that valid `Result` types do work. For an invalid +//! enum type, see `ui/invalid_result_conversion.py`. +use pyo3::prelude::*; +use pyo3::wrap_pyfunction; + +use std::fmt; + +mod common; + +/// A basic error type for the tests. +#[derive(Debug)] +enum MyError { + Custom(String), + Unexpected(String), +} + +impl fmt::Display for MyError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use MyError::*; + match self { + Custom(e) => write!(f, "My error message: {}", e), + Unexpected(e) => write!(f, "Unexpected: {}", e), + } + } +} + + +impl From for PyErr { + fn from(err: MyError) -> pyo3::PyErr { + pyo3::exceptions::PyOSError::py_err(err.to_string()) + } +} + +#[pyfunction] +fn should_work() -> Result<(), MyError> { +} + +#[test] +fn test_result_conversion() { + let gil = Python::acquire_gil(); + let py = gil.python(); + let f = wrap_pyfunction!(should_work)(py); +} diff --git a/tests/ui/invalid_result_conversion.rs b/tests/ui/invalid_result_conversion.rs new file mode 100644 index 00000000000..068fd97b02f --- /dev/null +++ b/tests/ui/invalid_result_conversion.rs @@ -0,0 +1,36 @@ +//! Testing https://github.com/PyO3/pyo3/issues/1106. A result type that +//! *doesn't* implement `From for PyErr` won't be automatically +//! converted when using `#[pyfunction]`. +use pyo3::prelude::*; +use pyo3::wrap_pyfunction; + +use std::fmt; + +mod common; + +#[derive(Debug)] +enum MyError { + Custom(String), + Unexpected(String), +} + +impl fmt::Display for MyError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use MyError::*; + match self { + Custom(e) => write!(f, "My error message: {}", e), + Unexpected(e) => write!(f, "Unexpected: {}", e), + } + } +} + +#[pyfunction] +fn should_not_work() -> Result<(), MyError> { +} + +#[test] +fn test_result_conversion() { + let gil = Python::acquire_gil(); + let py = gil.python(); + let f = wrap_pyfunction!(should_not_work)(py); +} From a41b598437fd8f24848e49fc93fab5e302e3f740 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Wed, 26 Aug 2020 10:18:54 +0200 Subject: [PATCH 2/9] Implemented suggested change --- src/callback.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/callback.rs b/src/callback.rs index 7dbd1aff822..ead0a19fd2d 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -2,7 +2,7 @@ //! Utilities for a Python callable object that invokes a Rust function. -use crate::err::PyResult; +use crate::err::{PyErr, PyResult}; use crate::exceptions::PyOverflowError; use crate::ffi::{self, Py_hash_t}; use crate::IntoPyPointer; @@ -37,12 +37,13 @@ pub trait IntoPyCallbackOutput { fn convert(self, py: Python) -> PyResult; } -impl IntoPyCallbackOutput for PyResult +impl IntoPyCallbackOutput for Result where T: IntoPyCallbackOutput, + E: Into { fn convert(self, py: Python) -> PyResult { - self.and_then(|t| t.convert(py)) + self.map_err(Into::into).and_then(|t| t.convert(py)) } } From 5dced75166003ab24f34decbf6b19f197a74ef8d Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Wed, 26 Aug 2020 14:33:24 +0200 Subject: [PATCH 3/9] Fixed type inference --- src/class/macros.rs | 2 +- src/class/number.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/class/macros.rs b/src/class/macros.rs index b72efdbd3b0..2ad6a974ab0 100644 --- a/src/class/macros.rs +++ b/src/class/macros.rs @@ -152,7 +152,7 @@ macro_rules! py_binary_self_func { let arg = py.from_borrowed_ptr::<$crate::PyAny>(arg); call_operator_mut!(py, slf_, $f, arg).convert(py)?; ffi::Py_INCREF(slf); - Ok(slf) + Ok::<_, $crate::err::PyErr>(slf) }) } Some(wrap::<$class>) diff --git a/src/class/number.rs b/src/class/number.rs index 149a8b9896f..551cd48f560 100644 --- a/src/class/number.rs +++ b/src/class/number.rs @@ -3,6 +3,7 @@ //! Python Number Interface //! Trait and support implementation for implementing number protocol +use crate::err::PyErr; use crate::callback::IntoPyCallbackOutput; use crate::{ffi, FromPyObject, PyClass, PyObject}; @@ -941,7 +942,7 @@ impl ffi::PyNumberMethods { let other = py.from_borrowed_ptr::(other); call_operator_mut!(py, slf_cell, __ipow__, other).convert(py)?; ffi::Py_INCREF(slf); - Ok(slf) + Ok::<_, PyErr>(slf) }) } self.nb_inplace_power = Some(wrap_ipow::); From 85b3db3496e81448d17d9607241783c19aca95b5 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Wed, 26 Aug 2020 14:34:16 +0200 Subject: [PATCH 4/9] cargo fmt --- src/callback.rs | 2 +- src/class/number.rs | 2 +- tests/test_result_conversion.rs | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/callback.rs b/src/callback.rs index ead0a19fd2d..5f83cfaa96a 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -40,7 +40,7 @@ pub trait IntoPyCallbackOutput { impl IntoPyCallbackOutput for Result where T: IntoPyCallbackOutput, - E: Into + E: Into, { fn convert(self, py: Python) -> PyResult { self.map_err(Into::into).and_then(|t| t.convert(py)) diff --git a/src/class/number.rs b/src/class/number.rs index 551cd48f560..568ed38b780 100644 --- a/src/class/number.rs +++ b/src/class/number.rs @@ -3,8 +3,8 @@ //! Python Number Interface //! Trait and support implementation for implementing number protocol -use crate::err::PyErr; use crate::callback::IntoPyCallbackOutput; +use crate::err::PyErr; use crate::{ffi, FromPyObject, PyClass, PyObject}; /// Number interface diff --git a/tests/test_result_conversion.rs b/tests/test_result_conversion.rs index 78d9567f6ba..de397f98df3 100644 --- a/tests/test_result_conversion.rs +++ b/tests/test_result_conversion.rs @@ -28,7 +28,6 @@ impl fmt::Display for MyError { } } - impl From for PyErr { fn from(err: MyError) -> pyo3::PyErr { pyo3::exceptions::PyOSError::py_err(err.to_string()) @@ -36,8 +35,7 @@ impl From for PyErr { } #[pyfunction] -fn should_work() -> Result<(), MyError> { -} +fn should_work() -> Result<(), MyError> {} #[test] fn test_result_conversion() { From 3ea6a79d97515e44d59d77b6e2ed7a82e5abcc47 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Wed, 26 Aug 2020 14:42:51 +0200 Subject: [PATCH 5/9] Finished tests and removed warnings --- tests/test_compile_error.rs | 1 + tests/test_result_conversion.rs | 20 +++++++++--------- tests/ui/invalid_result_conversion.rs | 23 ++++++++------------- tests/ui/invalid_result_conversion.stderr | 25 +++++++++++++++++++++++ 4 files changed, 45 insertions(+), 24 deletions(-) create mode 100644 tests/ui/invalid_result_conversion.stderr diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 5d839fa5c23..142750d8332 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -10,6 +10,7 @@ fn test_compile_errors() { t.compile_fail("tests/ui/missing_clone.rs"); t.compile_fail("tests/ui/reject_generics.rs"); t.compile_fail("tests/ui/wrong_aspyref_lifetimes.rs"); + t.compile_fail("tests/ui/invalid_result_conversion.rs"); skip_min_stable(&t); diff --git a/tests/test_result_conversion.rs b/tests/test_result_conversion.rs index de397f98df3..e184ed6145f 100644 --- a/tests/test_result_conversion.rs +++ b/tests/test_result_conversion.rs @@ -13,21 +13,17 @@ mod common; /// A basic error type for the tests. #[derive(Debug)] -enum MyError { - Custom(String), - Unexpected(String), +struct MyError { + pub descr: String, } impl fmt::Display for MyError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use MyError::*; - match self { - Custom(e) => write!(f, "My error message: {}", e), - Unexpected(e) => write!(f, "Unexpected: {}", e), - } + write!(f, "My error message: {}", self.descr) } } +/// Important for the automatic conversion to `PyErr`. impl From for PyErr { fn from(err: MyError) -> pyo3::PyErr { pyo3::exceptions::PyOSError::py_err(err.to_string()) @@ -35,11 +31,15 @@ impl From for PyErr { } #[pyfunction] -fn should_work() -> Result<(), MyError> {} +fn should_work() -> Result<(), MyError> { + Err(MyError { + descr: "something went wrong".to_string(), + }) +} #[test] fn test_result_conversion() { let gil = Python::acquire_gil(); let py = gil.python(); - let f = wrap_pyfunction!(should_work)(py); + wrap_pyfunction!(should_work)(py); } diff --git a/tests/ui/invalid_result_conversion.rs b/tests/ui/invalid_result_conversion.rs index 068fd97b02f..cbe4562b4ad 100644 --- a/tests/ui/invalid_result_conversion.rs +++ b/tests/ui/invalid_result_conversion.rs @@ -6,31 +6,26 @@ use pyo3::wrap_pyfunction; use std::fmt; -mod common; - +/// A basic error type for the tests. It's missing `From for PyErr`, +/// though, so it shouldn't work. #[derive(Debug)] -enum MyError { - Custom(String), - Unexpected(String), +struct MyError { + pub descr: String, } impl fmt::Display for MyError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use MyError::*; - match self { - Custom(e) => write!(f, "My error message: {}", e), - Unexpected(e) => write!(f, "Unexpected: {}", e), - } + write!(f, "My error message: {}", self.descr) } } #[pyfunction] -fn should_not_work() -> Result<(), MyError> { +fn should_work() -> Result<(), MyError> { + Err(MyError { descr: "something went wrong".to_string() }) } -#[test] -fn test_result_conversion() { +fn main() { let gil = Python::acquire_gil(); let py = gil.python(); - let f = wrap_pyfunction!(should_not_work)(py); + wrap_pyfunction!(should_not_work)(py); } diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr new file mode 100644 index 00000000000..f2703a671cb --- /dev/null +++ b/tests/ui/invalid_result_conversion.stderr @@ -0,0 +1,25 @@ +error[E0425]: cannot find value `__pyo3_get_function_should_not_work` in this scope + --> $DIR/invalid_result_conversion.rs:30:5 + | +22 | #[pyfunction] + | ------------- similarly named function `__pyo3_get_function_should_work` defined here +... +30 | wrap_pyfunction!(should_not_work)(py); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `__pyo3_get_function_should_work` + | + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the trait bound `std::result::Result<(), MyError>: pyo3::callback::IntoPyCallbackOutput<_>` is not satisfied + --> $DIR/invalid_result_conversion.rs:22:1 + | +22 | #[pyfunction] + | ^^^^^^^^^^^^^ the trait `pyo3::callback::IntoPyCallbackOutput<_>` is not implemented for `std::result::Result<(), MyError>` + | + ::: $WORKSPACE/src/callback.rs + | + | T: IntoPyCallbackOutput, + | ----------------------- required by this bound in `pyo3::callback::convert` + | + = help: the following implementations were found: + as pyo3::callback::IntoPyCallbackOutput> + = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info) From 8aebb7173875735c63a2262065f0a306375bbd6a Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Wed, 26 Aug 2020 14:51:43 +0200 Subject: [PATCH 6/9] Include in CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 442a2365605..9033a4b8dba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Implement type information for conversion failures. [#1050](https://github.com/PyO3/pyo3/pull/1050) - Add `PyBytes::new_with` and `PyByteArray::new_with` for initialising Python-allocated bytes and bytearrays using a closure. [#1074](https://github.com/PyO3/pyo3/pull/1074) - Add `Py::as_ref` and `Py::into_ref`. [#1098](https://github.com/PyO3/pyo3/pull/1098) +- Allow other `Result` types when using `#[pyfunction]`. [#1106](https://github.com/PyO3/pyo3/issues/1106). ### Changed - Exception types have been renamed from e.g. `RuntimeError` to `PyRuntimeError`, and are now only accessible by `&T` or `Py` similar to other Python-native types. The old names continue to exist but are deprecated. [#1024](https://github.com/PyO3/pyo3/pull/1024) From e86867fb0ac518a25384d3b114eefcc79e4f923c Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Thu, 27 Aug 2020 01:20:23 +0200 Subject: [PATCH 7/9] Moved test into separate file --- tests/test_result_conversion.rs | 45 --------------------------------- tests/test_various.rs | 40 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 45 deletions(-) delete mode 100644 tests/test_result_conversion.rs diff --git a/tests/test_result_conversion.rs b/tests/test_result_conversion.rs deleted file mode 100644 index e184ed6145f..00000000000 --- a/tests/test_result_conversion.rs +++ /dev/null @@ -1,45 +0,0 @@ -//! Testing https://github.com/PyO3/pyo3/issues/1106. A result type that -//! implements `From for PyErr` should be automatically converted -//! when using `#[pyfunction]`. -//! -//! This only makes sure that valid `Result` types do work. For an invalid -//! enum type, see `ui/invalid_result_conversion.py`. -use pyo3::prelude::*; -use pyo3::wrap_pyfunction; - -use std::fmt; - -mod common; - -/// A basic error type for the tests. -#[derive(Debug)] -struct MyError { - pub descr: String, -} - -impl fmt::Display for MyError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "My error message: {}", self.descr) - } -} - -/// Important for the automatic conversion to `PyErr`. -impl From for PyErr { - fn from(err: MyError) -> pyo3::PyErr { - pyo3::exceptions::PyOSError::py_err(err.to_string()) - } -} - -#[pyfunction] -fn should_work() -> Result<(), MyError> { - Err(MyError { - descr: "something went wrong".to_string(), - }) -} - -#[test] -fn test_result_conversion() { - let gil = Python::acquire_gil(); - let py = gil.python(); - wrap_pyfunction!(should_work)(py); -} diff --git a/tests/test_various.rs b/tests/test_various.rs index a3b82a84017..bc77fdea65e 100644 --- a/tests/test_various.rs +++ b/tests/test_various.rs @@ -3,6 +3,8 @@ use pyo3::types::IntoPyDict; use pyo3::types::{PyDict, PyTuple}; use pyo3::{py_run, wrap_pyfunction, PyCell}; +use std::fmt; + mod common; #[pyclass] @@ -168,3 +170,41 @@ fn test_pickle() { "# ); } + +/// Testing https://github.com/PyO3/pyo3/issues/1106. A result type that +/// implements `From for PyErr` should be automatically converted +/// when using `#[pyfunction]`. +/// +/// This only makes sure that valid `Result` types do work. For an invalid +/// enum type, see `ui/invalid_result_conversion.py`. +#[derive(Debug)] +struct MyError { + pub descr: String, +} + +impl fmt::Display for MyError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "My error message: {}", self.descr) + } +} + +/// Important for the automatic conversion to `PyErr`. +impl From for PyErr { + fn from(err: MyError) -> pyo3::PyErr { + pyo3::exceptions::PyOSError::py_err(err.to_string()) + } +} + +#[pyfunction] +fn result_conversion_function() -> Result<(), MyError> { + Err(MyError { + descr: "something went wrong".to_string(), + }) +} + +#[test] +fn test_result_conversion() { + let gil = Python::acquire_gil(); + let py = gil.python(); + wrap_pyfunction!(result_conversion_function)(py); +} From b6c533ff38662a8675e079b7d252eaf1a77c8dfc Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Thu, 27 Aug 2020 11:00:53 +0200 Subject: [PATCH 8/9] &'static str and function rename --- tests/test_various.rs | 4 ++-- tests/ui/invalid_result_conversion.rs | 6 +++--- tests/ui/invalid_result_conversion.stderr | 11 ----------- 3 files changed, 5 insertions(+), 16 deletions(-) diff --git a/tests/test_various.rs b/tests/test_various.rs index bc77fdea65e..87270c39186 100644 --- a/tests/test_various.rs +++ b/tests/test_various.rs @@ -179,7 +179,7 @@ fn test_pickle() { /// enum type, see `ui/invalid_result_conversion.py`. #[derive(Debug)] struct MyError { - pub descr: String, + pub descr: &'static str, } impl fmt::Display for MyError { @@ -198,7 +198,7 @@ impl From for PyErr { #[pyfunction] fn result_conversion_function() -> Result<(), MyError> { Err(MyError { - descr: "something went wrong".to_string(), + descr: "something went wrong", }) } diff --git a/tests/ui/invalid_result_conversion.rs b/tests/ui/invalid_result_conversion.rs index cbe4562b4ad..3da4d217182 100644 --- a/tests/ui/invalid_result_conversion.rs +++ b/tests/ui/invalid_result_conversion.rs @@ -10,7 +10,7 @@ use std::fmt; /// though, so it shouldn't work. #[derive(Debug)] struct MyError { - pub descr: String, + pub descr: &'static str, } impl fmt::Display for MyError { @@ -20,8 +20,8 @@ impl fmt::Display for MyError { } #[pyfunction] -fn should_work() -> Result<(), MyError> { - Err(MyError { descr: "something went wrong".to_string() }) +fn should_not_work() -> Result<(), MyError> { + Err(MyError { descr: "something went wrong" }) } fn main() { diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index f2703a671cb..fc92ebf6e67 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -1,14 +1,3 @@ -error[E0425]: cannot find value `__pyo3_get_function_should_not_work` in this scope - --> $DIR/invalid_result_conversion.rs:30:5 - | -22 | #[pyfunction] - | ------------- similarly named function `__pyo3_get_function_should_work` defined here -... -30 | wrap_pyfunction!(should_not_work)(py); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `__pyo3_get_function_should_work` - | - = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) - error[E0277]: the trait bound `std::result::Result<(), MyError>: pyo3::callback::IntoPyCallbackOutput<_>` is not satisfied --> $DIR/invalid_result_conversion.rs:22:1 | From c17c7c3c312763dc2264f64a601500208c858a79 Mon Sep 17 00:00:00 2001 From: Mario Ortiz Manero Date: Thu, 27 Aug 2020 11:07:30 +0200 Subject: [PATCH 9/9] Mention in the book --- guide/src/conversions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guide/src/conversions.md b/guide/src/conversions.md index 3c029442aa3..27f88dc1256 100644 --- a/guide/src/conversions.md +++ b/guide/src/conversions.md @@ -72,7 +72,7 @@ When returning values from functions callable from Python, Python-native types ( Because these types are references, in some situations the Rust compiler may ask for lifetime annotations. If this is the case, you should use `Py`, `Py` etc. instead - which are also zero-cost. For all of these Python-native types `T`, `Py` can be created from `T` with an `.into()` conversion. -If your function is fallible, it should return `PyResult`, which will raise a `Python` exception if the `Err` variant is returned. +If your function is fallible, it should return `PyResult` or `Result` where `E` implements `From for PyErr`. This will raise a `Python` exception if the `Err` variant is returned. Finally, the following Rust types are also able to convert to Python as return values: