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 std::error::Error for PyErr #1115

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Aug 25, 2020

This is a rework of PyErr to implement std::error::Error (and std::fmt:Display).

The rework was necessary because functions like PyErr::normalize() taking &mut self and PyErr::instance() taking self were incompatible with std::error::Error.

A couple of notes about the implementation:

  • Display for PyError automatically uses Python::with_gil internally. I think as this is the error path, and also it's rare that the GL will not already be acquired, this overhead is fine.
  • PyErr now contains an UnsafeCell. The safety is assured because, once normalized, the UnsafeCell value never changes. Almost every nontrivial access to the PyErr immediately normalizes it.

Closes #682
Closes #1034

TODO:

  • Lots of documentation / guide updates!
  • Add more test coverage.
  • Rename PyConcreteException::py_err (... to PyConcreteException::new_err ?)
  • Add std::error::Error::cause() ?

@davidhewitt davidhewitt requested a review from kngwyu August 25, 2020 20:07
@kngwyu
Copy link
Member

kngwyu commented Aug 26, 2020

Thank you for this hard work and refactoring!

It's well designed overall, but I want to list up some design questions before reviewing in detail:

  • PyErrValue. Now it's only used in Lazy variant, so maybe another name is suitable. In addition, maybe it's better to have Lazy { ptype: Py<PyType>, value: Option<PyErrValue> } instead of PyErrValue::None.
  • Normalizing variant of PyErrState. I think some other ways (Option or boolean flag) also work and should be considered.
  • Visibility. Though I haven't checked it precisely, are PyErrState and PyErrValue need to be pub?

@davidhewitt
Copy link
Member Author

Thanks, good points:

  • I had kept None / Normalizing in their respective enums because I thought it was more layout-efficient. But from a quick test I see that rustc is clever enough to optimize the size of Option<SomeEnum>:

    std::mem::size_of::<PyErrState>() = 40
    std::mem::size_of::<Option<PyErrState>>() = 40
    std::mem::size_of::<PyErrValue>() = 24
    std::mem::size_of::<Option<PyErrValue>>() = 24
    

    So I think in both cases let's use Option.

  • Visibility: You're correct, I think both PyErrValue and PyErrState can be pub(crate).

I'll push a commit tweaking as suggested tonight.

@davidhewitt
Copy link
Member Author

(still planning to push fix later, sorry didn't get round to it yesterday.)

@davidhewitt davidhewitt force-pushed the std-py-err branch 2 times, most recently from 412209c to 80ea2e4 Compare August 29, 2020 15:35
@davidhewitt
Copy link
Member Author

This is ready for a first round of review. Once we've made any design changes necessary, I also need to write more tests and the migration guide entry before we can merge this.

Copy link
Member

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

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

While overhauling PyErr, can we fix the pretty confusing and duplicate APIs around creating new errors?

I mean mostly PyConcreteError::py_err and PyConcreteError::into:

  • for me, the first doesn't really make the association with PyErr. I'd much rather this be called new: AFAICT, that method is not defined currently.

  • the second reads really strange, since I'm hardwired to think of into() as a self-consuming method.

The two are also named completely differently, although the do very similar things (AFAICS, into() is just Err(py_err())). IMO into() can be removed, I can't think of any other library that provides this shortcut.

I also think the PyErr: Into<PyResult> impl is unidiomatic and should be removed.

src/exceptions.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

👍 I'm completely happy to rework PyException::py_err and PyException::into. I was wondering along similar lines.

I'm definitely in favour of removing .into and agree on the impls being unidiomatic, so I'll remove those immediately.

Regarding py_err vs new, the only thing that makes me unsure about this is that PyConcreteException::new() could also return &PyConcreteException similar to all the rest of the PyType::new(). Perhaps PyConcreteException::new_err() -> PyErr is a less ambiguous choice?

@birkenfeld
Copy link
Member

Yes, I was thinking about that too. new_err is certainly nicer than py_err, but since the concrete exception structs are just void structs, I don't think you can have a useful other definition of new there?

@davidhewitt
Copy link
Member Author

Since #1024 it's possible to have a reference to all the exception objects in just the same way as for the rest of the native types (i.e. they're no longer void structs).

@davidhewitt
Copy link
Member Author

The last piece of the puzzle, which I haven't finished yet but plan to do soon, is have a #[pyexception] macro which is pretty much just #[pyclass] with extra goodness to make exception types.

@birkenfeld
Copy link
Member

Oops, I missed that. Yes, then it makes sense not to call that new.

@kngwyu
Copy link
Member

kngwyu commented Aug 30, 2020

new_err sounds good to me, though I mostly use PyErr::new and don't have any strong opinion about how the exception constructor would be.

@birkenfeld
Copy link
Member

BTW, could PyErr::new be made nicer to use with turbofish by making V an impl ... parameter?

@davidhewitt
Copy link
Member Author

davidhewitt commented Aug 30, 2020

Sadly it's not possible to combine turbofish with impl Trait parameters: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=81e2e2f20891dbb7456b3480b2fede79

PyErr::new::<T, _>(some_arg) is not too bad...

@davidhewitt
Copy link
Member Author

One last point of discussion: At the moment there's no implementation for std::error::Error::cause(), because it needs to return Option<&dyn std::error::Error>.

At the moment we only offer this implementation for &PyBaseException, because we can attach the cause object to the GIL lifetime.

I think it should be possible to implement for PyErr. We'd need to have a kind of "mini" object storage which the .cause() could be stored in, which could be cleaned up in impl Drop for PyErr. It feels complicated to get right, which is why I wanted to separate it from the rest of this PR (if we choose to implement it at all).

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

💯

src/err/err_value.rs Outdated Show resolved Hide resolved
src/err/mod.rs Show resolved Hide resolved
src/err/mod.rs Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Aug 30, 2020

I think it should be possible to implement for PyErr. We'd need to have a kind of "mini" object storage which the .cause() could be stored in, which could be cleaned up in impl Drop for PyErr.

But what is the source? PyErrValue? Or Python's backtrace?

src/err/mod.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

But what is the source? PyErrValue? Or Python's backtrace?

It would be the normalized python exceptions's cause, as seen at https://docs.python.org/3/c-api/exceptions.html#c.PyException_GetCause

davidhewitt added a commit to davidhewitt/pyo3 that referenced this pull request Aug 30, 2020
@davidhewitt
Copy link
Member Author

Thanks for the comments, updated as suggested.

I'll start writing docs and tests later 👍

davidhewitt added a commit to davidhewitt/pyo3 that referenced this pull request Aug 30, 2020
@davidhewitt davidhewitt force-pushed the std-py-err branch 3 times, most recently from 96905d9 to 79dd892 Compare September 2, 2020 06:54
@davidhewitt
Copy link
Member Author

Rebased, added some further test coverage, and also simplified PyErrState further.

I was able to take out the PyErrValue enum completely, and changed PyErrArguments to use IntoPy rather than ToPyObject (which means less copying!).

#[doc(hidden)]
pub fn ensure_gil() -> EnsureGIL {
/// Ensure the GIL is held, used in the implementation of Python::with_gil
pub(crate) fn ensure_gil() -> EnsureGIL {
Copy link
Member Author

Choose a reason for hiding this comment

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

rust-numpy will need to migrate to Python::with_gil

@davidhewitt
Copy link
Member Author

Right, I've just pushed a load of documentation for this. I now think this is ready to merge, so would appreciate some final reviews from anyone who's got a few minutes!

@davidhewitt davidhewitt mentioned this pull request Sep 7, 2020
6 tasks
Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be better if we have another review for the document.
@birkenfeld
Could you take a glance on it?

guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Thanks for the great feedback on the docs; I've pushed some quick changes based on your review.

@davidhewitt
Copy link
Member Author

I'd like to merge this tonight; maybe @sebpuetz is interested in reviewing?

@sebpuetz
Copy link
Contributor

sebpuetz commented Sep 9, 2020

Sure, I should find some time within the next hour

Copy link
Contributor

@sebpuetz sebpuetz left a comment

Choose a reason for hiding this comment

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

I didn't have time to do a very thorough review but this looks pretty good, just a handful of questions from my side.

src/err/err_state.rs Outdated Show resolved Hide resolved
src/err/mod.rs Outdated Show resolved Hide resolved
src/err/mod.rs Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Thanks for the review! In the end I didn't have time to merge yesterday evening. I've just pushed tweaks to address your comments and will merge shortly...

@davidhewitt davidhewitt merged commit 151af7a into PyO3:master Sep 10, 2020
mtreinish added a commit to mtreinish/rust-numpy that referenced this pull request Sep 13, 2020
In PyO3/pyo3#1115 the internal_utils module was removed from pyo3. There
is a note in the review that rust-numpy will need to migrate to using
with_gil() instead. This commit makes this change to enable running with
pyo3 0.12.0.
@davidhewitt davidhewitt deleted the std-py-err branch August 10, 2021 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants