WIP: Enhanced extract type errors (Issue #1640)

[batconjurer]

This PR includes allows the FromPyObject derive macro give more detailed error messages when conversion fails. It constructs a "traceback" of the nested types that failed the conversion to help more accurately pinpoint from where the type error originated.

The following gives an examples for different complex types (REMEMBER TO CHANGE THIS WHEN FORMATTING IS FINALIZED):

Enum

#[derive(Debug, FromPyObject)]
pub enum Bar {
    #[pyo3(annotation = "str")]
    A(String),
    #[pyo3(annotation = "uint")]
    B(usize),
    #[pyo3(annotation = "int", transparent)]
    C(isize),
}

fn main() {
    let gil = Python::acquire_gil();
    let py = gil.python();
    let dict = PyDict::new(py);
    let f = Bar::extract(dict.as_ref()).unwrap();
}

This produces the following error message:

TypeError: Failed to extract type Bar ('Union[str, uint, int]')
- variant A (str): 'dict' object cannot be converted to 'PyString'
- variant B (uint): 'dict' object cannot be interpreted as an integer
- variant C (int): 'dict' object cannot be interpreted as an integer

Note that for Enums, a reason for the failure of each variant is provided.

Struct

#[pyclass]
struct PyBaz {
    #[pyo3(get)]
    tup: (String, String),
    #[pyo3(get)]
    e: PyE,
}

#[pyclass]
#[derive(Clone)]
pub struct PyE {
    #[pyo3(get)]
    test: String,
    #[pyo3(get)]
    test2: usize,
}


#[derive(Debug, FromPyObject)]
pub struct E<T, T2> {
    test: T,
    test2: T2,
}

#[derive(Debug, FromPyObject)]
struct Baz<U, T> {
    e: E<U, T>,
    tup: Tuple,
}

fn main() {
  let gil = Python::acquire_gil();
  let py = gil.python();
  let pybaz = PyBaz {
      tup: ("test".into(), "test".into()),
      e: PyE {
          test: "foo".into(),
          test2: 0,
      },
  }
  .into_py(py);
  let test: Baz<usize, String> = FromPyObject::extract(pybaz.as_ref(py)).unwrap();
}

This produces the following error message (in the Python interpreter):

TypeError                       Traceback (most recent call last)
TypeError: 'str' object cannot be interpreted as an integer

The above exception was the direct cause of the following exception:

TypeError                       Traceback (most recent call last)
TypeError: failed to extract field E.test

The above exception was the direct cause of the following exception:

TypeError                       Traceback (most recent call last)
<python-process> in <module>
----> 1 module_name.module_func(x)

TypeError: failed to extract field Baz.e

Transparent Struct

#[derive(Debug, FromPyObject)]
#[pyo3(transparent)]
pub struct B {
    test: String,
}

fn main() {
    let gil = Python::acquire_gil();
    let py = gil.python();
    let tup: PyObject = 1.into_py(py);
    let tup = B::extract(tup.as_ref(py)).unwrap();
}

This produces the error message (in Python)

TypeError                       Traceback (most recent call last)
TypeError: 'int' object cannot be converted to 'PyString',

The above exception was the direct cause of the following exception:

TypeError                       Traceback (most recent call last)
<python-process> in <module>
----> 1 module_name.module_func(x)

TypeError: failed to extract field B.test

Tuple Struct

#[derive(Debug, FromPyObject)]
pub struct Tuple(String, usize);

fn main() {
    let gil = Python::acquire_gil();
    let py = gil.python();
    let tup: PyObject = (1, "test").into_py(py);
    let tup = Tuple::extract(tup.as_ref(py)).unwrap();
}

This produces the following error message: (in Python)

TypeError                       Traceback (most recent call last)
TypeError: 'int' object cannot be converted to 'PyString'

The above exception was the direct cause of the following exception:

TypeError                       Traceback (most recent call last)
<python-process> in <module>
----> 1 module_name.module_func(x)

TypeError: failed to extract field Tuple.0

Transparent Tuple Struct

#[derive(Debug, FromPyObject)]
#[pyo3(transparent)]
pub struct TransparentTuple(String);

fn main() {
    let gil = Python::acquire_gil();
    let py = gil.python();
    let tup: PyObject = 1.into_py(py);
    let tup = TransparentTuple::extract(tup.as_ref(py)).unwrap();
}

This produces the following error message (in Python):

TypeError                       Traceback (most recent call last)
TypeError: 'int' object cannot be converted to 'PyString'

The above exception was the direct cause of the following exception:

TypeError                       Traceback (most recent call last)
<python-process> in <module>
----> 1 module_name.module_func(x)

TypeError: failed to extract inner field of TransparentTuple

[davidhewitt]

@batconjurer - thank you for working on this and sorry for my delay in reviewing.

Reading the generated error message and thinking about this some more, I'd like to suggest a slightly denser format which I think is easier for the user to see what happened:

Enum case:

TypeError: failed to extract enum Bar (Union[str,uint,int])
- variant A (str): 'dict' object cannot be converted to 'PyString'
- variant B (uint): 'dict' object cannot be interpreted as an integer
- variant C (int): 'dict' object cannot be interpreted as an integer

Struct field:

TypeError: failed to extract field Bar.foo: 'dict' object cannot be converted to 'PyString'

You can get the error message without the error type using py_err.instance(py).str().

I'd prefer to avoid adding the error_message option for now. I think that customizing errors on that level is rare enough that I'm not in favour of extending the PyO3 macro API yet. (Perhaps my view on that will change in the future.)

As for why PyString appears - this is because the TypeError has come from PyDowncastError. In the future I'd like to improve that error to say 'dict' object cannot be converted to 'str'. I don't think PyString is at all useful for a Python user to see.

[batconjurer]

@davidhewitt I have made the suggested changes. A couple of comments

• In order to get the message body of errors without the error type, I have to acquire the gil many times. I'm wondering if that is a problem
• Again, for very complex structs, these error messages are going to be quite long to be in a single line.

[davidhewitt]

Thanks, I will re-review at the weekend.

[davidhewitt]

@batconjurer I'm sorry this took me so long to re-review. If you've lost interest because of the delay, that's understandable, and I'm happy to take up finishing this off.

The acquire_gil uses can definitely be avoided. I've added some comments on what to do.

I'm really happy with the enum error message.

I agree that the struct message is still too long.

New idea - we added PyErr::set_cause in PyO3 0.14. Rather than formatting the old error into the new error, how about setting the original extraction error as the cause of the "failed to extract B.a" error?

That way it should break up across lines naturally thanks to Python.

[batconjurer]

@davidhewitt I am still interested in working on it. I will try to incorporate your comments as soon as I can.

[batconjurer]

@davidhewitt So I changed the structs to use the set_cause method and I'm pretty happy with it. However, I'm pretty sure that weabsolutely need the gil to do that.

[birkenfeld]

@batconjurer what David means is that you don't need to call acquire_gil since it is already known to be held any time you have a &PyAny. The py() method is on the PyNativeType trait, which is not in scope (the error message when you try obj.py() should have told you that), so you have to use the trait or call it explicitly as pyo3::PyNativeType::py(obj) etc.

[batconjurer]

Ok, I have removed the acquire_gil calls. However, instead of importing the PyNativeType to get access to the .py() method, I found it simpler to just copy the body of that function. So now we have

let py = unsafe{ Python::assume_gil_acquired() };

in all places

[davidhewitt]

Hmmm. I really would like us to avoid unsafe in the code generated by this macro if possible. I've opened #1751 which would make it possible to use .py() without the import. If merged, we should update to use that. Otherwise, we should definitely use the PyNativeType trait here, even if the imports produce a bit of noise.

[davidhewitt]

👍 looking really great! I like the way these errors look now, thank you for the repeated iterations of this PR.

Just a couple of tiny nits and then the question of which safe way to get .py() (maybe #1751 will be merged before long) and then I'd be keen to see this merged!

[birkenfeld]

@batconjurer I don't understand:

let py = pyo3::PyNativeType::py(obj);

is shorter than your solution and does not need to introduce unsafe code.

[batconjurer]

@batconjurer I don't understand:

let py = pyo3::PyNativeType::py(obj);

is shorter than your solution and does not need to introduce unsafe code.

Incorportated this. Did not realize at first that obj is in scope the whole time.

[davidhewitt]

This is looking perfect. Finger crossed CI is green - if so, I think it's good to merge! Thanks for all your efforts!

[birkenfeld]

Incorportated this. Did not realize at first that obj is in scope the whole time.

Great, thanks!