-
Notifications
You must be signed in to change notification settings - Fork 768
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
Convert some std error types to PyErr #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is highly unsafe, if you call this conversions outside of GIL, python would crash. In general case ToPyObject trait does py memory allocation, which is require Gil
src/err.rs
Outdated
impl std::convert::From<std::io::Error> for PyErr { | ||
fn from(err: std::io::Error) -> Self { | ||
let py = unsafe { Python::assume_gil_acquired() }; | ||
PyErr::new::<exc::IOError, _>(py, err.description()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be more precise, check utils.rs from async-tokio
Maybe we can change PyResult type and include unconverted errors. Then we can use custom form of From trait in callback handler when we have python object available. Also we can use unstable |
Another option is to modify PyErr and just store errors unconverted. And convert them inside |
I known it's unsafe, but without it you would more likely to just I'd like to look into other options you mentioned to make it safer. |
I added a The ideal way would be fn my_py_fn(py: Python, path: &str) -> PyResult<String> {
let f = File::open(path)?;
...
} But that's a little complicated to implement now. With this PR we can do: use pyo3::ToPyErr;
fn my_py_fn(py: Python, path: &str) -> PyResult<String> {
let f = File::open(path).map_err(|e| e.to_pyerr(py))?;
...
} which should be good enough for most cases. |
did you look into this feature? rust-lang/rust#42275 |
Lgtm. Could you also update async-tokio |
Address #21