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

More verbose conversion/downcasting errors. #652

Closed
sebpuetz opened this issue Oct 26, 2019 · 5 comments
Closed

More verbose conversion/downcasting errors. #652

sebpuetz opened this issue Oct 26, 2019 · 5 comments
Milestone

Comments

@sebpuetz
Copy link
Contributor

pyo3 tries to convert arguments to the specified type in the signature, if that fails it returns TypeError without any message through:

/// Converts `PyDowncastError` to Python `TypeError`.
impl std::convert::From<PyDowncastError> for PyErr {
    fn from(_err: PyDowncastError) -> PyErr {
        exceptions::TypeError.into()
    }
}

Since conversion happens before entering the user-defined method body, there's no way to be verbose about what happened unless some new-type is defined for which extract() is implemented manually or the method is changed to take &PyAny or PyObject to begin with - which also entails manual extraction.

Are there obvious downsides to changing PyDowncastError to wrap a String holding an error message that gets propagated into Python?

In that case, the failed conversions could return something along these lines:

PyDowncastError(format!(Can't extract some_type from {}", ob.get_type().name())
@sebpuetz sebpuetz changed the title More verbose downcasting errors. More verbose conversion/downcasting errors. Oct 26, 2019
@kngwyu
Copy link
Member

kngwyu commented Oct 27, 2019

Looks nice 👍
Thanks.

@sebpuetz
Copy link
Contributor Author

I was looking at the code generation yesterday and it would be possible to be verbose there, but not in a clean way: Either the (possibly in extract() user-defined) error message is lost or the generic error message would need to be extended by the one coming out of extract().

Another option could be changing the conversion impls in types to something like:

/// Allows extracting strings from Python objects.
/// Accepts Python `str` and `unicode` objects.
impl<'a> crate::FromPyObject<'a> for &'a str {
    fn extract(ob: &'a PyAny) -> PyResult<Self> {
        let s: Cow<'a, str> = crate::FromPyObject::extract(ob).map_err(|_| {
            TypeError::py_err(format!(
                "Failed to extract string from {}",
                ob.get_type().name()
            ))
        })?;
        match s {
            Cow::Borrowed(r) => Ok(r),
            Cow::Owned(r) => {
                let r = ob.py().register_any(r);
                Ok(r.as_str())
            }
        }
    }
}

As a workaround within my own project, this little helper method comes in handy:

pub(crate) fn t_from_any<'a, T>(any: &'a PyAny, to: Option<&str>) -> PyResult<T>
where
    T: FromPyObject<'a>,
{
    let any_type = any.get_type().name();
    let msg = to
        .map(|to| format!("Expected '{}' not '{}'", to, any_type))
        .unwrap_or_else(|| format!("Invalid argument: '{}'", any_type));
    any.extract::<T>().map_err(|_| type_err(msg))
}

@cswinter
Copy link

cswinter commented Feb 3, 2020

This would be a really useful feature. It would would also be good to include the name of the argument that has the wrong type.

E.g. right now you get this:

>>> connect(url=b'localhost:3000', timeout=2.0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError

What I would like to happen is something more like this:

>>> connect(url=b'localhost:3000', timeout=2.0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError("Parameter `url` has type `str` but received value of type `bytes`")

@davidhewitt davidhewitt added this to the 0.12 milestone Jul 12, 2020
@davidhewitt
Copy link
Member

I'd like to take a look at some form of this for 0.12.

@davidhewitt
Copy link
Member

Closed via #1050

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

4 participants