Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow other Result types in #[pyfunction] etc #1106

Closed
davidhewitt opened this issue Aug 15, 2020 · 7 comments
Closed

Allow other Result types in #[pyfunction] etc #1106

davidhewitt opened this issue Aug 15, 2020 · 7 comments

Comments

@davidhewitt
Copy link
Member

davidhewitt commented Aug 15, 2020

At the moment the only Result type we allow in #[pyfunction] is PyResult.

After a question on Gitter yesterday I was thinking if we can allow instead any Result which has E: Into<PyErr>.

That would allow code like this (all implementations replaced with ...):

enum MyError { ... }

impl From<MyError> for PyErr { ... }

#[pyfunction]
fn my_err_function() -> Result<(), MyErr> { ... }

Pros:

  • Extra flexibility for users implementing #[pyfunction]
  • Error handling improvement when calling this #[pyfunction] in Rust - can handle MyError typed enum rather than dynamic PyErr.
  • Possible performance improvement when calling this #[pyfunction] in Rust - no need to create PyErr which is relatively higher-cost than MyError.

Cons:

  • Making this accept more types might over-complicate pyo3's API. I'm not sure it's so bad, though.

cc @marioortizmanero who posted on this topic on Gitter. (At least, I think that was what was being asked!)

@davidhewitt
Copy link
Member Author

davidhewitt commented Aug 15, 2020

Broadly speaking, I don't think the implementation is too hard. I think we can just replace these lines in callback.rs:

pyo3/src/callback.rs

Lines 40 to 47 in 4840619

impl<T, U> IntoPyCallbackOutput<U> for PyResult<T>
where
T: IntoPyCallbackOutput<U>,
{
fn convert(self, py: Python) -> PyResult<U> {
self.and_then(|t| t.convert(py))
}
}

with this implementation:

impl<T, E, U> IntoPyCallbackOutput<U> for Result<T, E>
where
    T: IntoPyCallbackOutput<U>,
    E: Into<PyErr>
{
    fn convert(self, py: Python) -> PyResult<U> {
        self.map_err(Into::into).and_then(|t| t.convert(py))
    }
}

However, at the moment this causes pyo3 to fail to compile because type inference for the new parameter E is not possible. I think this type inference failure only applies to some internal implementations of our #[pyproto], and for external code this would not be a problem (because in #[pyfunction] the return type is always specified, so no type inference applies).

So our internal code would need a little refactoring, but otherwise it's reasonably straightforward.

@birkenfeld
Copy link
Member

Note that the question mark operator already calls into() so I don't think the flexibility implementing the pyfunction is really needed. The other pros might be nice though.

@marioortizmanero
Copy link
Contributor

marioortizmanero commented Aug 15, 2020

I'm the one who suggested this feature, and yes, you got it right.

I think that as long as it's well documented, it shouldn't be an issue. This should be mentioned in the type conversions table of the book with a note that the wrapped Error must implement E: Into<PyErr>, and maybe even in the exceptions section of the book as well.

I could implement this myself but I'm very new to pyo3 so I would need a bit of hand holding. Not sure if I should.

@davidhewitt
Copy link
Member Author

@marioortizmanero if you're interested in implementing this then you're very welcome to provide a PR! I'm happy to review or answer any questions you have.

@marioortizmanero
Copy link
Contributor

marioortizmanero commented Aug 26, 2020

Thanks @davidhewitt, I've opened a new PR at #1118 with the proposed changes. Please take a look and let me know.

@marioortizmanero
Copy link
Contributor

This can be closed now as #1118 was merged, @davidhewitt

@davidhewitt
Copy link
Member Author

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants