Function argument error handling improvements

[davidhewitt]

This is a sketch of ideas for a follow-up of #1050 to enable #[pyfunction] (and #[pymethods] etc.) to report better errors for function inputs.

Imagine that the following #[pyfunction]:

#[pyfunction]
fn foo(tup: (i32, &str)) {
    println!("{:?}", tup)
}

After #1050, the error message when calling this function with invalid arguments is already much improved from PyO3 0.11.1:

>>> foo(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Can't convert None to PyTuple

I think that this error message can yet be improved further. There's two ways in which I'd like to see this improve:

1. Mention the argument which failed to convert.
   Naming the function argument which failed to convert will help users locate exactly which function input is incorrect.

2. Provide mypy-style type annotations for the target type, where possible.
   It's great to see that None can't convert to PyTuple, but as a Python user really what I need to see is that the expected type is Tuple[int, str].

Put together, this might mean that our foo function above would output error messages like the below:

>>> foo(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Can't convert None to `Tuple[int, str]` (when trying to extract argument `tup`)

I'm not super attached to the exact wording of the above if someone else has a better idea / something easier to implement.

Later today I'm going to add some thoughts on how these two pieces could be implented if anyone in the community is interested in championing these improvements.

[sebpuetz]

I was looking into this and I think the biggest issue is that the extraction methods in the proc macros return PyResult which is already the opaque Python error that we can't easily manipulate.

if spec.is_args(&name) {
    return quote! {
        let #arg_name = <#ty as pyo3::FromPyObject>::extract(_args.as_ref())?;
    };
}

generates code using FromPyObject::extract which fails with PyErr that's propagated into Python.

I'm not sure how to proceed from this point without touching error-handling in FromPyObject which has more failure conditions than just downcasting errors.

[birkenfeld]

One way would be to use a custom TypeError subclass that can be recognized in error-handling code and therefore have its message easily amended.

[sebpuetz]

Following a similar approach, it would be possible to change the value of the TypeError created in PyDowncastError to some different Py type than PyString. The below example uses a PyTuple, but that's probably not desirable:

/// Convert `PyDowncastError` to Python `TypeError`.
impl<'a> std::convert::From<PyDowncastError<'a>> for PyErr {
    fn from(err: PyDowncastError) -> PyErr {
        let from = err.from
            .repr()
            .map(|s| s.to_string_lossy())
            .unwrap_or_else(|_| err.from.get_type().name())
            .into_owned();
        let to = err.to.into_owned();
        exceptions::PyTypeError::py_err((from, to))
    }
}

Then it'd be possible to do something like:

        <&str as FromPyObject>::extract(foo).map_err(|e| match &e.pvalue {
            PyErrValue::ToObject(to_obj) => {
                let v = to_obj.to_object(gil.python());
                if let Ok(downcast_tuple) = PyTuple::try_from(v.as_ref(gil.python())) {
                    let from = downcast_tuple.get_item(0).extract::<&str>().unwrap();
                    let to = downcast_tuple.get_item(1).extract::<&str>().unwrap();
                    return exceptions::PyTypeError::py_err(format!(
                        "Failed to convert the parameter {} with value {} to {}",
                        "foo", from, to
                    ));
                }
                e
            }
            _ => e,
        })

resulting in the following:

>>> SomePyo3Type().foo(1.2)

TypeError: Failed to convert the parameter foo with value 1.2 to PyString

Where "foo" would be filled in with the parameter name in the proc-macro.

In a proper version, the PyTuple could be replaced by some ConversionError class for which we implement __repr__ since that seems to be printed when exceptions are raised.

[davidhewitt]

My thoughts I didn't have time to add earlier:

Adding the parameter name

I was looking into this and I think the biggest issue is that the extraction methods in the proc macros return PyResult which is already the opaque Python error that we can't easily manipulate.

I think you've identified the correct part of the code to change to add the argument names. I was thinking we might be able to do something along the lines of:

if spec.is_args(&name) {
    return quote! {
        let #arg_name = <#ty as pyo3::FromPyObject>::extract(_args.as_ref())
            .map_err(|e| match e.instance(py).str() {
                Ok(s) => exceptions::PyTypeError::py_err(format!("{} (while converting argument `{}`, s, param_name),
                Err(e) => e
            }?;
    };
}

it's not perfect, though seems like a way to tack on the argument name without too much trouble.

Disclaimer: the above snippet almost certainly won't compile as-is; I'm typing this out without trying any code!

Using more accurate types:

I'm wondering if we should change the error type for FromPyObject to either be PyDowncastError, or alternatively an enum which allows either a PyDowncastError or a general PyErr. As long as PyErr has a From implementation from this new enum, I think it's not too big a breaking change.

This would allow us to generate the type information like Tuple[int, str] inside FromPyObject implementations.

[Askaholic]

I'm interested in working on this. It's something I have wanted in pyo3 for a while, and now with Hacktoberfest I have the motivation to actually look into it! (BTW you should label some easy issues with a hacktoberfest label).

I think the most logical place to add this is somewhere around impl_arg_param. I've been toying with the code and it seems there are actually 3 possible places inside that function which will generate the object extraction code:

pyo3/pyo3-derive-backend/src/pymethod.rs

Line 481 in 37ce406

let #arg_name = <#ty as pyo3::FromPyObject>::extract(_args.as_ref())?;

pyo3/pyo3-derive-backend/src/pymethod.rs

Line 521 in 37ce406

Some(_obj) => _obj.extract()?,

pyo3/pyo3-derive-backend/src/pymethod.rs

Line 529 in 37ce406

Some(_obj) => _obj.extract()?,

So I'm thinking it might actually be best to add some error transformation to inject the argument name right here:

pyo3/pyo3-derive-backend/src/pymethod.rs

Line 419 in 37ce406

param_conversion.push(impl_arg_param(&arg, &spec, idx, self_, &mut option_pos));

As this appears to be the only place where impl_arg_param is called.

Other options would be to wrap the entire body of impl_arg_param or to create an impl_arg_extract macro and replace the above mentioned extract calls with that.

[davidhewitt]

Awesome, many thanks for taking this up! (For those interested, discussion has continued on #1212.)

[alex]

Is this complete now?

[davidhewitt]

I think this issue should probably be closed for two follow ups I'd like to see:

• our type names printed in error messages are PyTuple, PyStr etc. It would be nice to make these more like Python type names (I think? Others may disagree.)
• I think we only add the argument name to ValueError and TypeError. We can probably use exception chaining or notes to do it more consistently for all errors.