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

Implement eyre feature #1893

Merged
merged 11 commits into from
Oct 13, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ jobs:
# TODO suppress linking using config file rather than extension-module feature
PYO3_BUILD_CONFIG=$(pwd)/config.txt cargo check --all-targets --features "extension-module"
PYO3_BUILD_CONFIG=$(pwd)/config.txt cargo check --all-targets --features "extension-module abi3"
PYO3_BUILD_CONFIG=$(pwd)/config.txt cargo check --all-targets --features "extension-module macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods"
PYO3_BUILD_CONFIG=$(pwd)/config.txt cargo check --all-targets --features "extension-module abi3 macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods"
PYO3_BUILD_CONFIG=$(pwd)/config.txt cargo check --all-targets --features "extension-module macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods eyre"
PYO3_BUILD_CONFIG=$(pwd)/config.txt cargo check --all-targets --features "extension-module abi3 macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods eyre"
done

build:
Expand Down Expand Up @@ -163,7 +163,7 @@ jobs:
id: settings
shell: bash
run: |
echo "::set-output name=all_additive_features::macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods"
echo "::set-output name=all_additive_features::macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods eyre"

- if: matrix.msrv == 'MSRV'
name: Prepare minimal package versions (MSRV only)
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/guide.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ jobs:
# This adds the docs to gh-pages-build/doc
- name: Build the doc
run: |
cargo +nightly rustdoc --lib --no-default-features --features="macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods" -- --cfg docsrs
cargo +nightly rustdoc --lib --no-default-features --features="macros num-bigint num-complex hashbrown indexmap serde multiple-pymethods eyre" -- --cfg docsrs
cp -r target/doc gh-pages-build/doc
echo "<meta http-equiv=refresh content=0;url=pyo3/index.html>" > gh-pages-build/doc/index.html

Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ links = "python"
[dependencies]
cfg-if = { version = "1.0" }
# must stay at 0.3.x for Rust 1.41 compatibility
eyre = {version = ">= 0.4" , optional = true}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think these need to be swapped (new dep has split comment from its target). Also, should we place an upper bound?

Suggested change
# must stay at 0.3.x for Rust 1.41 compatibility
eyre = {version = ">= 0.4" , optional = true}
eyre = {version = ">= 0.4, <0.5" , optional = true}
# must stay at 0.3.x for Rust 1.41 compatibility

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(oops)

Eyre is at 0.6.5 now, so that upper bound is too strict. 0.4 is the first version that has the eyre::Report type (?), so I think any version above that should be fine.

I'll admit that I don't know anything about how cargo resolves dependency versions, but this looks like how you should say "works with any eyre version of 0.4 and up". Is that too liberal and should it be pinned to ">= 0.4, <0.7" instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that by default cargo will pick the highest eyre possible for the pyo3 dependency, not the one which the user already has in their own Cargo.toml. (This is a bit awkward.) The user can adjust it manually using some incantation of cargo update --precise (like we do in our MSRV CI jobs).

Is that too liberal and should it be pinned to ">= 0.4, <0.7" instead?

Generally we've only offered a range for dependencies when the MSRV support requires an older version. I'd be tempted to ask the other question - is just supporting eyre = "0.6" acceptable?

As for the upper bound: if we were to take semver literally, eyre 0.7 could hypothetically be very breaking and completely remove the eyre::Report type. (Although I would be suprised if this happened in practice.) So I'd definitely prefer have an upper bound of < 0.7 if we go for a range. Dependabot can remind us to update when 0.7 releases 😄

indoc = { version = "0.3.6", optional = true }
inventory = { version = "0.1.4", optional = true }
libc = "0.2.62"
Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ fmt:
black . --check

clippy:
cargo clippy --features="num-bigint num-complex hashbrown serde" --tests -- -Dwarnings
cargo clippy --features="abi3 num-bigint num-complex hashbrown serde" --tests -- -Dwarnings
cargo clippy --features="num-bigint num-complex hashbrown serde indexmap eyre " --tests -- -Dwarnings
cargo clippy --features="abi3 num-bigint num-complex hashbrown serde indexmap eyre" --tests -- -Dwarnings
for example in examples/*/; do cargo clippy --manifest-path $$example/Cargo.toml -- -Dwarnings || exit 1; done

lint: fmt clippy
Expand Down
158 changes: 158 additions & 0 deletions src/conversions/eyre.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
#![cfg(feature = "eyre")]
#![cfg_attr(docsrs, doc(cfg(feature = "eyre")))]
//! A conversion from [eyre]’s [`Report`] type to [`PyErr`].
//!
//! Use of an error handling library like [eyre] is common in application code and when you just
//! want error handling to be easy. If you are writing a library or you need more control over your
//! errors you might want to design your own error type instead.
//!
//! This implementation always creates a Python [`RuntimeError`]. You might find that you need to
//! map the error from your Rust code into another Python exception. See [`PyErr::new`] for more
//! information about that.
//!
//! For information about error handling in general, see the [Error handling] chapter of the Rust
//! book.
//!
//! # Setup
//!
//! To use this feature, add this to your **`Cargo.toml`**:
//!
//! ```toml
//! [dependencies]
//! ## change * to the version you want to use, ideally the latest.
//! eyre = "*"
// workaround for `extended_key_value_attributes`: https://github.com/rust-lang/rust/issues/82768#issuecomment-803935643
#![cfg_attr(docsrs, cfg_attr(docsrs, doc = concat!("pyo3 = { version = \"", env!("CARGO_PKG_VERSION"), "\", features = [\"eyre\"] }")))]
#![cfg_attr(not(docsrs), doc = "pyo3 = { version = \"*\", features = [\"eyre\"] }")]
//! ```
//!
//! Note that you must use compatible versions of eyre and PyO3.
//! The required eyre version may vary based on the version of PyO3.
//!
//! # Example: Propagating a `PyErr` into [`eyre::Report`]
//!
//! ```rust
//! use pyo3::prelude::*;
//! use pyo3::wrap_pyfunction;
//! use std::path::PathBuf;
//!
//! // An example function that returns `eyre::Result<...>`.
//! fn rust_open(filename: PathBuf) -> eyre::Result<Vec<u8>> {
//! let data = std::fs::read(filename)?;
//! Ok(data)
//! }
//!
//! // A wrapper around a Rust function.
//! #[pyfunction]
//! fn py_open(filename: PathBuf) -> PyResult<Vec<u8>> {
//! // The `?` ("try") operator performs the conversion
//! // into a `PyErr` - if it is an error.
//! let data = rust_open(filename)?;
//! Ok(data)
//! }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be possible to also just have a #[pyfunction] which returns eyre::Result<Vec<u8>> - which might be useful to have as an example (as a kind of test)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing that but I want to point at the where the conversion happens. A #[pyfunction] returning eyre::Result<Vec<u8>> has that conversion hidden somewhere in the pyfunction macro.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. I wonder if your second example "using eyre in general" could be tweaked slightly so that the function decompress is a pyfunction returning eyre::Result? The conversion from PyErr -> eyre::Result can still be there.

Alternatively maybe it's better to keep away from that here, and instead in the guide's function.md there is probably scope for an improved "error handling" section which comments on how to use Result with error types that convert to PyErr. (And can use Result<_, eyre::Report> as an example?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I do want a pyfunction returning eyre's error. I think it is too easy for readers to assume that a pyfunction must return a PyResult and they have to wrap functions. I'm not sure if that was your reasoning though.

I do want to expand the error handling in the guide, but not with this PR.

//!
//! fn main() {
//! let error = Python::with_gil(|py| -> PyResult<Vec<u8>> {
//! let fun = wrap_pyfunction!(py_open, py)?;
//! let text = fun.call1(("foo.txt",))?.extract::<Vec<u8>>()?;
//! Ok(text)
//! }).unwrap_err();
//!
//! println!("{}", error);
//! }
//! ```
//!
//! # Example: Using `eyre` in general
//!
//! Note that you don't need this feature to convert a [`PyErr`] into an [`eyre::Report`], because
//! it can already convert anything that implements [`Error`](std::error::Error):
//!
//! ```rust
//! use pyo3::prelude::*;
//! use pyo3::types::PyBytes;
//!
//! // An example function that must handle multiple error types.
//! //
//! // To do this you usually need to design your own error type or use
//! // `Box<dyn Error>`. `eyre` is a convenient alternative for this.
//! pub fn decompress(bytes: &[u8]) -> eyre::Result<String> {
//! // An arbitrary example of a Python api you
//! // could call inside an application...
//! // This might return a `PyErr`.
//! let res = Python::with_gil(|py| {
//! let zlib = PyModule::import(py, "zlib")?;
//! let decompress = zlib.getattr("decompress")?;
//! let bytes = PyBytes::new(py, bytes);
//! let value = decompress.call1((bytes,))?;
//! value.extract::<Vec<u8>>()
//! })?;
//!
//! // This might be a `FromUtf8Error`.
//! let text = String::from_utf8(res)?;
//!
//! Ok(text)
//! }
//!
//! fn main() -> eyre::Result<()> {
//! let bytes: &[u8] = b"x\x9c\x8b\xcc/U(\xce\xc8/\xcdIQ((\xcaOJL\xca\xa9T\
//! (-NU(\xc9HU\xc8\xc9LJ\xcbI,IUH.\x02\x91\x99y\xc5%\
//! \xa9\x89)z\x00\xf2\x15\x12\xfe";
//! let text = decompress(bytes)?;
//!
//! println!("The text is \"{}\"", text);
//! # assert_eq!(text, "You should probably use the libflate crate instead.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😂

//! Ok(())
//! }
//! ```
//!
//! [eyre]: https://docs.rs/eyre/ "A library for easy idiomatic error handling and reporting in Rust applications."
//! [`RuntimeError`]: https://docs.python.org/3/library/exceptions.html#RuntimeError "Built-in Exceptions — Python documentation"
//! [Error handling]: https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html "Recoverable Errors with Result - The Rust Programming Language"

use crate::exceptions::PyRuntimeError;
use crate::PyErr;
use eyre::Report;

/// Converts [`eyre::Report`] to a [`PyErr`] containing a [`PyRuntimeError`].
///
/// If you want to raise a different Python exception you will have to do so manually. See
/// [`PyErr::new`] for more information about that.
impl From<eyre::Report> for PyErr {
fn from(error: Report) -> Self {
PyRuntimeError::new_err(format!("{:?}", error))
}
}

#[cfg(test)]
mod tests {
use pyo3::prelude::*;
use pyo3::types::IntoPyDict;

use eyre::{bail, Result, WrapErr};

fn f() -> Result<()> {
use std::io;
bail!(io::Error::new(io::ErrorKind::PermissionDenied, "oh no!"));
}

fn g() -> Result<()> {
f().wrap_err("f failed")
}

fn h() -> Result<()> {
g().wrap_err("g failed")
}

#[test]
fn test_pyo3_exception_contents() {
let err = h().unwrap_err();
let expected_contents = format!("{:?}", err);
let pyerr = PyErr::from(err);

Python::with_gil(|py| {
let locals = [("err", pyerr)].into_py_dict(py);
let pyerr = py.run("raise err", None, Some(locals)).unwrap_err();
assert_eq!(pyerr.pvalue(py).to_string(), expected_contents);
})
}
}
1 change: 1 addition & 0 deletions src/conversions/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! This module contains conversions between various Rust object and their representation in Python.

mod array;
pub mod eyre;
pub mod hashbrown;
pub mod indexmap;
pub mod num_bigint;
Expand Down
5 changes: 4 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
//! crate, which is not supported on all platforms.
//!
//! The following features enable interactions with other crates in the Rust ecosystem:
//
//! - [`eyre`]: Enables a conversion from [eyre]’s [`Report`] type to [`PyErr`].
//! - [`hashbrown`]: Enables conversions between Python objects and [hashbrown]'s [`HashMap`] and
//! [`HashSet`] types.
//! - [`indexmap`]: Enables conversions between Python dictionary and [indexmap]'s [`IndexMap`].
Expand Down Expand Up @@ -254,6 +254,9 @@
//! [`Complex`]: https://docs.rs/num-complex/latest/num_complex/struct.Complex.html
//! [`Deserialize`]: https://docs.rs/serde/latest/serde/trait.Deserialize.html
//! [`Serialize`]: https://docs.rs/serde/latest/serde/trait.Serialize.html
//! [eyre]: https://docs.rs/eyre/ "A library for easy idiomatic error handling and reporting in Rust applications."
//! [`Report`]: https://docs.rs/eyre/latest/eyre/struct.Report.html
//! [`eyre`]: ./eyre/index.html
//! [`hashbrown`]: ./hashbrown/index.html
//! [`indexmap`]: <./indexmap/index.html>
//! [`maturin`]: https://github.com/PyO3/maturin "Build and publish crates with pyo3, rust-cpython and cffi bindings as well as rust binaries as python packages"
Expand Down