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

Add type info to conversion errors. #1050

Merged
merged 2 commits into from
Jul 19, 2020

Conversation

sebpuetz
Copy link
Contributor

@sebpuetz sebpuetz commented Jul 18, 2020

This is a draft to address #652. Initially, I tried to come up with a solution that keeps the downcast error 0-sized by parameterizing PyDowncastError<T1, T2> where T1: PyTypeInfo, T2: PyTypeInfo but I think there are cases where the underlying type is not easily accessible.

I haven't worked much with pyo3 recently, so any pointers if I missed some obvious solution are much appreciated!


Thank you for contributing to pyo3!

Here are some things you should check for submitting your pull request:

  • Run cargo fmt (This is checked by travis ci)
  • Run cargo clippy and check there are no hard errors (There are a bunch of existing warnings; This is also checked by travis)
  • If applicable, add an entry in the changelog.
  • If applicable, add documentation to all new items and extend the guide.
  • If applicable, add tests for all new or fixed functions
  • If you changed any python code, run black .. You can install black with pip install black)

You might want to run tox (pip install tox) locally to check compatibility with all supported python versions. If you're using linux or mac you might find the Makefile helpful for testing.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

🚀 thank you so much for kicking this effort off! I'm very pleased to see this - it's been on my mind for some time and agree it's a critical UX improvement we can make to PyO3!

I have added a few pieces of feedback below.

Also, if you really want to supercharge this, I have two further things I'd love to see:

  • there's a second trait FromPyObject which has implementations for types like Option<T>. It would be really cool if extraction for that Option<T> mentioned Optional[int] etc as the target type.
  • It would be 👌 if you added impl std::error::Error for PyDowncastError to this PR!

src/err.rs Outdated Show resolved Hide resolved
src/err.rs Outdated Show resolved Hide resolved
src/err.rs Outdated Show resolved Hide resolved
src/types/iterator.rs Outdated Show resolved Hide resolved
@sebpuetz
Copy link
Contributor Author

Thanks for the review! I'll amend the PR, probably some time tomorrow!

@davidhewitt
Copy link
Member

👍 I'll try to remember to merge #1051 in the morning so that you can use that to simplify the PyIterator case. Feel free to ping me if I forget!

@sebpuetz
Copy link
Contributor Author

sebpuetz commented Jul 18, 2020

  • there's a second trait FromPyObject which has implementations for types like Option<T>. It would be really cool if extraction for that Option<T> mentioned Optional[int] etc as the target type.

I spent some time trying to implement this, but I don't think it's easily doable:

impl<'a, T> FromPyObject<'a> for Option<T>
where
    T: FromPyObject<'a>,
{
    fn extract(obj: &'a PyAny) -> PyResult<Self> {
        if obj.as_ptr() == unsafe { ffi::Py_None() } {
            Ok(None)
        } else {
            match T::extract(obj) {
                Ok(v) => Ok(Some(v)),
                Err(e) => Err(e),
            }
        }
    }
}

assumes that T is any Rust type that implements FromPyObject, so we don't get any PyTypeInfo about T. T::extract() implementations know the target types of the casts, but these details aren't available on T.

Unless I'm missing something, PyErr only makes the message available through its pvalue, which is already some Python type that in itself requires extracting a String. We'd have to ensure that the error is in fact a TypeError and that it's a message that we generated in order to manipulate the to part. This seems quite messy and error prone.

In other words, I don't think we can get the nice Option messages without changing extract errors to be generic over E: Into<PyErr>...

edit: Anyways, I might open a PR tomorrow to simplify the match, T::extract(obj).map(|v| Some(v)) should be more concise!

@davidhewitt
Copy link
Member

Ah right, thanks for exploring the Option case further. I agree let's not pursue that right now - maybe I'll rework another time though as you say will be more breaking. The mini PR would be very welcome though!

I've merged #1051 so you should be unblocked here now!

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.

Thank you, but I don't think the lifetime 'b matters.

src/err.rs Outdated Show resolved Hide resolved
src/err.rs Outdated Show resolved Hide resolved
src/err.rs Outdated Show resolved Hide resolved
@sebpuetz sebpuetz marked this pull request as ready for review July 19, 2020 08:54
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, this is now looking excellent to me! I have one final suggestion to handle the case when repr() fails.

If I can make one further ask, could you please add a CHANGELOG entry? I suggest:

- Add additional context to `PyDowncastError`. [#1050](https://github.com/PyO3/pyo3/pull/1050)

src/err.rs Outdated
write!(
f,
"Can't convert {} to {}",
self.from.repr().map_err(|_| std::fmt::Error)?,
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 in the error case we can fall back to showing the type as you did before:

Suggested change
self.from.repr().map_err(|_| std::fmt::Error)?,
self.from.repr().unwrap_or_else(|_| format!("object of type `{}`", self.from.get_type().name())),

@sebpuetz
Copy link
Contributor Author

If I can make one further ask, could you please add a CHANGELOG entry? I suggest:

I had already amended the previous commit with:

- Implement type information for conversion failures  [#1050](https://github.com/PyO3/pyo3/pull/1050)

I pushed out the version with the fallback to the from type!

@davidhewitt
Copy link
Member

Ah brilliant, thanks very much, I missed that. Once CI passes let's get this merged! 🚀

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Oh, actually one last little thing: looks like there's still some 'b lifetimes unused which we can remove...

src/conversion.rs Outdated Show resolved Hide resolved
src/conversion.rs Outdated Show resolved Hide resolved
src/conversion.rs Outdated Show resolved Hide resolved
src/conversion.rs Outdated Show resolved Hide resolved
src/conversion.rs Outdated Show resolved Hide resolved
src/conversion.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

I went ahead and applied that change. Sorry if that breaks it! 😄

@davidhewitt
Copy link
Member

CI is happy so let's merge it! Many thanks again

@@ -56,7 +57,20 @@ pub struct PyErr {
pub type PyResult<T> = Result<T, PyErr>;

/// Marker type that indicates an error while downcasting
Copy link
Contributor

Choose a reason for hiding this comment

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

technically we should stop calling it a marker type now.

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

Successfully merging this pull request may close these issues.

4 participants