FromPyObject derivation for structs and enums

[sebpuetz]

This is a draft addressing a bit of both topics in #301 and #1055.

I have not written proc-macro code before, so this might be quite ugly. This is mostly proof-of-concept, but I think it'd be a great quality-of-life improvement if something like this was available.

This produces the following behaviour:

>>> union.foo(["Test"])
["Test"]
>>> union.foo("Test")
Test
>>> union.foo(1)
1
>>> union.foo(None)
TypeError: Can't convert NoneType to Union[Str, Int, StringList]

This implementation covers both the cheap downcast variant and extraction of proper Rust types. Although I'm not sure whether relying on the variant holding a reference is the proper heuristic to apply here.

---

Todos are marked in the code, a proper implementation should in my opinion:

• make the reported names of the variants in the error configurable through an attribute on the variant, i.e.:

  #[union]
  pub enum MyUnion {
      #[rename(str)]
      SomeVariant(String)
  } 

• support named fields & variants with multiple fields

[kngwyu]

Thank you, but...
Should this go this way?

Some complaints about the #[union]

• The conversion result depends on the order of the enum elements.
  E.g., we get different result from #[union] enum<'a> { A(&'a PyAny), B(&'a PyDict) } and #[union] enum<'a> { B(&'a PyDict), A(&'a PyAny) }.
• It's not clear what the union means for who don't use typing usually.
• It's not full-featured attribute. Just derive.

So, how about just making it #[derive(FromPyObject)]?

[sebpuetz]

Thanks for the feedback. As mentioned, this is just a rough draft to figure out whether this is the right path or not.

Thank you, but...
Should this go this way?

Some complaints about the #[union]

• The conversion result depends on the order of the enum elements.
  E.g., we get different result from #[union] enum<'a> { A(&'a PyAny), B(&'a PyDict) } and
  #[union] enum<'a> { B(&'a PyDict), A(&'a PyAny) }.

That's a valid point although I'm not sure how to address this or whether this needs to be addressed.

• It's not clear what the union means for who don't use typing usually.

I chose that name since it's the most specific term within Python, also it was the one discussed over at #301. Different naming would be fine, too.

• It's not full-featured attribute. Just derive.
  So, how about just making it #[derive(FromPyObject)]?

Sure, at this point that's all that the macro is doing.

---

Before proceeding, we should probably discuss whether deriving FromPyObject is something that should be offered for enums and whether dependence on order is an issue.

[davidhewitt]

Thanks for this.

I think @kngwyu is right that because #[union] is just deriving FromPyObject, it should really be #[derive(FromPyObject)].

However, I think that if we proceed to add this #[derive] macro to PyO3, we should probably consider also offering it for structs in the same way that the dict-derive crate does. And in addition, we should aim to provide #[derive] for IntoPyObject in the same way.

Order dependence - I think this is a desirable feature and should be documented. e.g. consider this:

#[pyclass] struct Base { }
#[pyclass(extends = "Base")] struct Child { };

#[derive(FromPyObject)]
enum ChildOrBase<'a> {
    Child(PyRef<'a, Child>),
    Base(PyRef<'a, Base>),
}

In the above, ChildOrBase::Base can always be extracted from any input value that would also successfully extract the Child variant. So I think the only sensible strategy for this example to work is to state that variants are tested in order.

[kngwyu]

And another question is how should we distinguish this from the enum type introduced in #1045

[davidhewitt]

IMO that's a question for documentation and easy enough for us to solve:

• if you want to just convert Rust data back and forth with Python, use #[derive(FromPyObject, IntoPy)]

• if you want to share data between Rust and Python, use #[pyclass] or #[pyenum]

[sebpuetz]

I've been working a bit on this on and off:

Should the FromPyObject impl handle structs / variants with multiple fields? E.g., struct AllTheStrings<'a>(String, &'a PyString, &'a PyAny) could all be extracted from the same object. My current (local) implementation handles all that but I'm unsure whether we want this. What do you think?

Wrt. unit structs / variants I have a check that rules those out since it wouldn't be possible to decide what to extract, or rather, any unit struct can always be extracted.

#[derive(Debug, FromPyObject)]
pub struct A<'a>
{
    s: String,
    t: &'a PyString,
    p: &'a PyAny,
}

//generated
impl<'a> ::pyo3::FromPyObject<'a> for A<'a> {
    fn extract(ob: &'a ::pyo3::PyAny) -> ::pyo3::PyResult<Self> {
        Ok(Self {
            s: ::pyo3::FromPyObject::extract(ob)?,
            t: ::pyo3::FromPyObject::extract(ob)?,
            p: ::pyo3::FromPyObject::extract(ob)?,
        })
    }
}

for structs and for enums:

pub enum Union<'a> {
    #[err_rename = "str"]
    Named {
        a: B,
        b: String,
        c: &'a PyString,
    },
    Int(&'a PyLong),
    #[err_rename = "List[str]"]
    StringList(Vec<String>),
}

// generated
impl<'a> ::pyo3::FromPyObject<'a> for Union<'a> {
    fn extract(ob: &'a ::pyo3::PyAny) -> ::pyo3::PyResult<Self> {
        if let (Ok(a), Ok(b), Ok(c)) = (
            ::pyo3::FromPyObject::extract(ob),
            ::pyo3::FromPyObject::extract(ob),
            ::pyo3::FromPyObject::extract(ob),
        ) {
            return Ok(Union::Named { a, b, c });
        };
        if let (Ok(_field_0)) = (::pyo3::FromPyObject::extract(ob)) {
            return Ok(Union::Int(_field_0));
        };
        if let (Ok(_field_0)) = (::pyo3::FromPyObject::extract(ob)) {
            return Ok(Union::StringList(_field_0));
        };
        let type_name = ob.get_type().name();
        let from = ob
            .repr()
            .map(|s| {
                let res = ::alloc::fmt::format(::core::fmt::Arguments::new_v1(
                    &["", " (", ")"],
                    &match (&s.to_string_lossy(), &type_name) {
                        (arg0, arg1) => [
                            ::core::fmt::ArgumentV1::new(arg0, ::core::fmt::Display::fmt),
                            ::core::fmt::ArgumentV1::new(arg1, ::core::fmt::Display::fmt),
                        ],
                    },
                ));
                res
            })
            .unwrap_or_else(|_| type_name.to_string());
        let err_msg = {
            let res = ::alloc::fmt::format(::core::fmt::Arguments::new_v1(
                &["Can\'t convert ", " to "],
                &match (&from, &"str, Int, List[str]") {
                    (arg0, arg1) => [
                        ::core::fmt::ArgumentV1::new(arg0, ::core::fmt::Display::fmt),
                        ::core::fmt::ArgumentV1::new(arg1, ::core::fmt::Display::fmt),
                    ],
                },
            ));
            res
        };
        Err(::pyo3::exceptions::PyTypeError::py_err(err_msg))
    }
}

[davidhewitt]

Should the FromPyObject impl handle structs / variants with multiple fields? E.g., struct AllTheStrings<'a>(String, &'a PyString, &'a PyAny) could all be extracted from the same object. My current (local) implementation handles all that but I'm unsure whether we want this. What do you think?

It seems reasonable to me that the Python "equivalent" of AllTheThings<'a> is Tuple[str, str, Any], so I'm happy for tuple structs & variants to be handled in that fashion in a first draft. We can see how this feels in practice.

#[err_rename = "List[str]"]

Can I propose that we instead name this attribute #[pyo3(annotation = "List[str]")]? In the future I'd hope we can generate type annotations from these impls so let's make a forward-compatible name.

//generated
impl<'a> ::pyo3::FromPyObject<'a> for A<'a> {
    fn extract(ob: &'a ::pyo3::PyAny) -> ::pyo3::PyResult<Self> {
        Ok(Self {
            s: ::pyo3::FromPyObject::extract(ob)?,
            t: ::pyo3::FromPyObject::extract(ob)?,
            p: ::pyo3::FromPyObject::extract(ob)?,
        })
    }
}

I'm not sure I agree with this implementation - it looks to me that A<'a> can basically only be extracted from a Python str, and that s, t, p will all end up with the same data?

I think we need to consider what A -> (python) -> A conversion should look like. I think that the conversion from A to Python will probably be a dict with keys "s", "t", and "p", so I think the from-python conversion should probably be similar?

If I were to write it as a Python type annotation, I think the Python "type" that A<'a> above should map to / from would be:

class A(TypedDict):
    s: str
    t: str
    p: Any

[sebpuetz]

Should the FromPyObject impl handle structs / variants with multiple fields? E.g., struct AllTheStrings<'a>(String, &'a PyString, &'a PyAny) could all be extracted from the same object. My current (local) implementation handles all that but I'm unsure whether we want this. What do you think?

It seems reasonable to me that the Python "equivalent" of AllTheThings<'a> is Tuple[str, str, Any], so I'm happy for tuple structs & variants to be handled in that fashion in a first draft. We can see how this feels in practice.

The Python equivalent would actually be str or something for that on the Rust side extractions to String ,&'a PyString and &'a PyAny has been implemented. FromPyObject just gets a single argument, if that argument is a Tuple[str, str, Any], then the corresponding type would be (String, String, &'a PyAny) (although I don't think extraction is implemented for tuples).

#[derive(FromPyObject)]
struct Tuple<'a>((String, String, &'a PyAny));

#[err_rename = "List[str]"]

Can I propose that we instead name this attribute #[pyo3(annotation = "List[str]")]? In the future I'd hope we can generate type annotations from these impls so let's make a forward-compatible name.

Sure that was just whatever I came up with at the time.

I'm not sure I agree with this implementation - it looks to me that A<'a> can basically only be extracted from a Python str, and that s, t, p will all end up with the same data?

I think we need to consider what A -> (python) -> A conversion should look like. I think that the conversion from A to Python will probably be a dict with keys "s", "t", and "p", so I think the from-python conversion should probably be similar?

If I were to write it as a Python type annotation, I think the Python "type" that A<'a> above should map to / from would be:

class A(TypedDict):
    s: str
    t: str
    p: Any

This is more or less the same as above: If a dict should be extracted, that needs to be guided from the type of the field that it gets extracted to. I'm not sure how this would work otherwise...

[davidhewitt]

For example I think the implementation for A<'a> could be:

impl<'a> ::pyo3::FromPyObject<'a> for A<'a> {
    fn extract(ob: &'a ::pyo3::PyAny) -> ::pyo3::PyResult<Self> {
        Ok(Self {
            s: ob.get_item("s")?.extract()?,
            t: ob.get_item("t")?.extract()?,
            p: ob.get_item("p")?.extract()?,
        })
    }
}

and for AllTheThings<'a>:

impl<'a> ::pyo3::FromPyObject<'a> for AllTheThings<'a> {
    fn extract(ob: &'a ::pyo3::PyAny) -> ::pyo3::PyResult<Self> {
        let tup: (String, &'a PyString, &'a PyAny) = ob.extract()?;
        Ok(Self(tup.0, tup.1, tup.2))
    }
}

[gilescope]

Any news? Can't wait to try out pyo3 with ADTs. Is there anything we can do to help?

[sebpuetz]

I got caught up in some other stuff, I might find some time later this week / towards the weekend to get back to this.

I'm not completely sold on the assumed dict-struct relation in the derivation. IMO, a more intuitive parallel for struct fields would be properties / attributes in Python classes, so what's your opinion on getattr instead of getitem?

The PyTuple-to-tuple-struct extraction makes sense to me as proposed by @davidhewitt

[davidhewitt]

a more intuitive parallel for struct fields would be properties / attributes in Python classes

This is a fair view and I don't disagree that getattr seems more pure than get_item, but at the same time what type would that Python class be?

I think that whatever design we build here should have an equivalent to-python implementation we can also derive in pyo3. (Even if we don't implement the to-python conversion as part of this PR.)

I can't think of an obvious class type for the to-python conversion, so I think struct -> dict seems best for that direction. And equivalently this makes me think the from-python conversion should be dict->struct.

Another viewpoint I've been thinking on: how to implement a serde serializer & deserializer that maps Rust <-> Python. It seems to me that for a serde implementation, the following should be true:

1.Python -> Rust -> Python roundrips always produce the original result.
2. Rust -> JSON is equivalent to Rust -> Python -> JSON, i.e. the output of such a serializer should be Python objects that would json-serialize the same way as serde_json would serialize the original Rust type.

I think that both of these two rules should probably apply to the design here?

[sebpuetz]

a more intuitive parallel for struct fields would be properties / attributes in Python classes

This is a fair view and I don't disagree that getattr seems more pure than get_item, but at the same time what type would that Python class be?

I think that whatever design we build here should have an equivalent to-python implementation we can also derive in pyo3. (Even if we don't implement the to-python conversion as part of this PR.)

I can't think of an obvious class type for the to-python conversion, so I think struct -> dict seems best for that direction. And equivalently this makes me think the from-python conversion should be dict->struct.

Another viewpoint I've been thinking on: how to implement a serde serializer & deserializer that maps Rust <-> Python. It seems to me that for a serde implementation, the following should be true:

1.Python -> Rust -> Python roundrips always produce the original result.
2. Rust -> JSON is equivalent to Rust -> Python -> JSON, i.e. the output of such a serializer should be Python objects that would json-serialize the same way as serde_json would serialize the original Rust type.

I think that both of these two rules should probably apply to the design here?

I don't think serde's derive(Serialize, Deserialize) guarantees that all data survives roundtrips. And I don't think that deriving FromPyObject / ToPyObject should make such guarantees either. Restricting extraction to dicts will quite likely cause some odd code on the Python side where you'd naturally have some class Foo with field _bar exposed through a property bar. Being forced to store this in a dictionary as {"bar": value} in Python seems like a strange constraint to me.

I wouldn't expect derive(ToPyObject) to implement conversion to Python dictionaries, I'd expect that from derive(ToPyDict). A much more involved solution could generate a pyclass and expose the fields as properties / attributes. Or maybe we can find a middleground and use namedtuples in this case? Although I have no clue whether we can easily generate that in PyO3.

[davidhewitt]

I don't think serde's derive(Serialize, Deserialize) guarantees that all data survives roundtrips.

True, but I think in practice if data does not roundtrip, it's extremely suprising to unexpecting users. If it's possible for us to make roundtripping work, I think it should. See e.g. RReverser/serde-xml-rs#130

A much more involved solution could generate a pyclass and expose the fields as properties / attributes.

This is true, but I'm not convinced that adding a #[derive] macro which expands to #[pyclass] under the hood is a good design. That kinda defeats the rule from this comment.

Or maybe we can find a middleground and use namedtuples in this case?

I'd be much more happy with this solution. There's still questions of how to construct the namedtuple type, though perhaps we can find a way to overcome that.

---

A possible interesting case of discussion: I've now started a WIP implementation on such a serde/python implementation. See https://github.com/davidhewitt/pythonize

I've not quite settled on the final design, so I'd welcome any feedback on that.

I kinda see these #[derive(IntoPyObject, FromPyObject)] macros as being roughly equivalent to the serde implementation but without requiring all the overhead of the serde machinery. And so I would hope they behave the same as that crate. But I could be convinced otherwise.

[davidhewitt]

(See also #566 and #884 which are kind of related to this problem area.)

[sebpuetz]

I tried implementing some of the suggestions but I think there's some issues to smooth out:

#[derive(FromPyObject)]
struct Wrapper(String);

With such a struct it doesn't seem intuitive to extract the String from a PySequence / PyTuple. So we'd probably want some option that switches to simply String::from_pyobject(obj)?, i.e. something like:

#[derive(FromPyObject)]
#[pyo3(wrapper)]
struct Wrapper(String);

The same applies to a wrapper struct with a named field:

#[derive(FromPyObject)]
struct Wrapper { wrapped: String }

This also extends to enums.

---

Regarding the extraction of named fields, I think it'd be nice to decide this based on a field attribute, i.e.:

#[derive(FromPyObject)]
struct WithNamedFields {
    #[extract(getattr)]
    from_attr: String,
    #[extract(get_item)]
    from_item: String,
}

// expands to

impl<'a> FromPyObject<'a> for WithNamedFields {
    fn extract(obj: &'a PyAny) -> PyResult<Self> {
        Ok(WithNamedFields {
            from_attr: obj.getattr("from_attr")?.extract()?,
            from_item: obj.get_item("from_item")?.extract()?
        }
    }
}

It'd probably be nice to untie the field name from the Rust implementation. Otherwise trying to derive FromPyObject for some type from a foreign Python package would force the Rust struct to use the name the Python package chose.

[davidhewitt]

Thanks for proceeding forwards with this implementation! I think we can can draw inspiration to make configurable attributes from serde's set of attributes.

With such a struct it doesn't seem intuitive to extract the String from a PySequence / PyTuple. So we'd probably want some option that switches to simply String::from_pyobject(obj)?

Let's make #[pyo3(transparent)], the same as serde has #[serde(transparent)]?

It'd probably be nice to untie the field name from the Rust implementation. Otherwise trying to derive FromPyObject for some type from a foreign Python package would force the Rust struct to use the name the Python package chose.

Serde does this with #[serde(rename = "...")]. In other places in pyo3 we've used #[name = "..."], though I think that was a mistake and it should have been at least #[pyo3(name = "...")].

So perhaps #[pyo3(name = "...")] here? I could also accept rename if you prefer it, though think name is just as clear when documented. (And then I might make a PR to change the attribute in other contexts.)

Regarding the extraction of named fields, I think it'd be nice to decide this based on a field attribute

Agreed, though I think probably should go on the struct rather than the fields - probably you want either all getattr, or all get_item?

As it's not clear right now whether attributes or items is a better default: how about making it required for one of #[pyo3(from_attributes)] or #[pyo3(from_items)] to be specified for now, if we're in one of these cases where the strategy needs to be specified?

If you have ideas for better names for this pair of attributes I'm happy for you to use something else.

Later we could make it optional when we figure out the better default strategy.

[kngwyu]

How about making it only for enum so far?
It can be too hard to make all functionality in this PR

[sebpuetz]

I'm on vacation starting monday, so I should find time to turn this into a proper PR next week.

[davidhewitt]

How about making it only for enum so far?

I think unfortunately because struct variants and tuple variants encode all the possibilites, solving for enum pretty much requires solving for structs too.

If the scope was to reduce on this PR I think we'd have to dial it back pretty much to just the original design (i.e. single-item tuple variants).

I'm happy with whatever @sebpuetz is planning to build! 😄

[sebpuetz]

I have an ugly WIP solution that handles most of the desired cases for both enums and structs (I haven't handled the wrapper-struct-variant yet).

It's pretty hard to abstract over tuple structs, tuple variants, proper structs and struct variants, at least I'm getting lost with all the different syn types. Before this is reviewable, I'll definitely have to factor out some common logic.

I'm pushing the current state to this branch so this doesn't look all that stale. Perhaps you can voice your opinion on some of the decisions, too.

#[pyfunction]
pub fn foo(mut inp: A) {
    println!("{:?}", inp);
}
#[pyfunction]
pub fn bar(mut inp: Foo) {
    println!("{:?}", inp);
}

#[pyfunction]
pub fn baz(mut inp: B) {
    println!("{:?}", inp);
}

#[derive(Debug, FromPyObject)]
pub struct A<'a> {
    #[extract(getattr)]
    s: String,
    #[extract(get_item)]
    t: &'a PyString,
    #[extract(getattr)]
    #[name = "foo"]
    p: &'a PyAny,
}

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

#[derive(Debug, FromPyObject)]
pub struct C {
    #[extract(getattr)]
    #[name = "bla"]
    test: String
}

#[derive(Debug, FromPyObject)]
pub enum Foo<'a> {
    #[err_rename = "bla"]
    FF(&'a PyAny, String),
    #[wrapper]
    F(&'a PyAny)
}

>>>import from_py_object
>>> from_py_object.bar(("test", "test"))                                                                                                                                                                       
F('test')
>>> from_py_object.bar(("test", "test"))                                                                                                                                                                       
FF('test', "test")
>>> class Foo:
...     def __init__(self):
...         self.s = "test"
...         self.foo = None
...     def __getitem__(self, key):
...         return "Foo"
>>> from_py_object.foo(Foo())
A { s: "test", t: 'Foo', p: None }
>>> from_py_object.baz("test")                                                                                                                                                                                
B { test: "test" }

[davidhewitt]

Awesome! 👍

I'll try to find time to review whatever state this PR is in at some point in the next few days. Perhaps that can help with some of the refactoring.

[davidhewitt]

(I just merged #1121.)

[sebpuetz]

All green, do we want UI tests & docs done within this PR?

[davidhewitt]

🎉

UI tests - I think it's desirable to get as much test coverage as possible in the original PR, so if you have ideas what you would make for UI tests it would be great to have here.

Docs - I'm okay with merging PRs without docs, but especially for a new feature like this nobody will use it until we write good documentation. So if we choose to wait on docs we should open an issue when closing this PR so that we remind ourselves docs are needed.

[sebpuetz]

I'm working on the UI tests, docs are up next. Should be done in the afternoon!

[sebpuetz]

If everything turns out green, this should be reviewable!

[kngwyu]

LGTM, thank you for the hard work!
My suggestions are mostly for reducing LOC and not critical. Please address them if you like.

[sebpuetz]

Thanks for the suggestions, I pushed the sugested changes. Hoping for all-green CI!

[davidhewitt]

This is looking absolutely excellent; great tests and docs! I just have a few final suggestions/questions and then let's merge it!

[sebpuetz]

Once again, thanks for the suggestions. CI is slowly turning green, too!

edit had to force-push again, the attribute list ended up under the wrong section in the guide...

[davidhewitt]

Looks absolutely excellent to me. Thanks so much! 🚀

[sebpuetz]

Is there still something left to be addressed or is this good to go?

[davidhewitt]

I'm happy, just leaving some time for @kngwyu to have a final chance to review and make the merge.

[kngwyu]

Thanks, I'm happy to see this became a reality in such a nice way.