Derive macro for `IntoPyObject` by Icxolu · Pull Request #4495 · PyO3/pyo3

Icxolu · 2024-08-26T19:59:21Z

Initial implementation of #[derive(IntoPyObject)]. #[derive(FromPyObject)] was used as a base/reference for this implementation.

This currently supports #[pyo3(transparent)] as the only real option, which just forwards to the inner IntoPyObject impl.

Newtype structs (#[pyo3(transparent)] named and single tuple): forward impl.
Named structs: PyDict with the field names as keys
Tuple structs: PyTuple in declaration order
Enums: matches on the variant and treats it as an anonymous struct with the rules above.
Lifetimes and Generics are supported, 'py is treated special and is used a the Python<'py> lifetime if given.

We still need lots of tests here, but I thought I'll put this up as a POC already. Hopefully this can smoothen the migration to IntoPyObject a bit.

One thing not needed initially, but may be a nice to have at some point is some like #[pyo3(into_py_with = "...")] analogous to #[pyo3(from_py_with = "...")]

Closes #4458

davidhewitt

This looks brilliant, thank you so much! I am travelling with family for the next few days so it might be the weekend before I have a chance to do any reviewing... 🙏

Icxolu · 2024-08-28T22:07:37Z

No worries, enjoy your trip.

davidhewitt

Overall this looks spot on, thanks very much. I have given this initial review from my phone so haven't read the implementation in full detail, looks great in general.

I have a bunch of various suggestions, mostly related to interactions with FromPyObject.

davidhewitt · 2024-09-01T09:13:40Z

+    body: TokenStream,
+}
+
+struct NamedStructField<'a> {


We should consider the interaction with FromPyObject's options like #[pyo3(item)] and #[pyo3(attribute)]. I am fine if we just parse them and reject cases where we don't want to support them for IntoPyObject (yet); all I'd like to avoid is that we don't silently ignore those options.

If there is an unsupported case then users can split their FromPyObject and IntoPyObject derives onto different structs, and they are then aware their conversions will not round-trip.

Hmm, we could definitely parse them, but I'm not sure what we would do with them in this case. Due to the PyDict return type this always uses set_item. So I think it makes only sense to parse them, if there is a way (I'm not aware of any) to create an anonymous object and set arbitrary attributes on it. I guess we could accept item to allow renaming and to make a round-trip possible if it is given.

Good point. Let's handle item round trips here and error on attribute? Users can always make two separate types if they need #[attribute] for their FromPyObject implementation for some reason.

I added parsing for #[pyo3(item)] and #[pyo3(attribute)] (like for FromPyObject), roundtrip tests in test_frompy_intopy_roundtrip.rs and ui tests for the unsupported cases.

davidhewitt

Sorry for the long review cycle, this is looking great and I think we're nearly there! Main change I would like to see is just on the interaction with the FromPyObject attributes, where I think it makes our future development simpler if we handle that before releasing.

davidhewitt · 2024-10-20T07:27:34Z

+    Crate(CrateAttribute),
+}
+
+impl Parse for ContainerPyO3Attribute {


Reading all these structures it feels like there's a ton of boilerplate which might be interesting to refactor into common macro_rules! patterns (maybe). Not for this PR, but maybe a non-urgent follow up to explore sometime.

davidhewitt · 2024-10-20T07:53:24Z

+            _ => bail_spanned!(
+                fields.span() => "cannot derive `IntoPyObject` for empty structs"
+            ),


I wonder, does it make sense for it to be empty tuple and/or empty dict? Perhaps again we leave this for a follow up and consider FromPyObject at the same time.

Yeah, I think revisiting this in a followup (perhaps after 0.23) makes sense.

davidhewitt · 2024-10-20T08:01:07Z

+    body: TokenStream,
+}
+
+struct NamedStructField<'a> {


Good point. Let's handle item round trips here and error on attribute? Users can always make two separate types if they need #[attribute] for their FromPyObject implementation for some reason.

Icxolu · 2024-10-20T18:55:00Z

No worries, thanks for the comments. I'll have a look in a few days.

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

davidhewitt

This looks perfect, thanks so much! I'm really optimistic this will help users have a good experience upgrading to 0.23! 🚀

Icxolu force-pushed the derive-intopyobject branch from ffc4c81 to 68e57f4 Compare August 28, 2024 21:41

davidhewitt reviewed Aug 28, 2024

View reviewed changes

Icxolu marked this pull request as ready for review August 29, 2024 22:18

davidhewitt reviewed Sep 1, 2024

View reviewed changes

Icxolu mentioned this pull request Sep 13, 2024

0.23 Release #4554

Closed

3 tasks

This was referenced Oct 4, 2024

deprecate ToPyObject in favor or IntoPyObject #4595

Merged

Contribution opportunity: improved conversion traits #4041

Closed

davidhewitt mentioned this pull request Oct 11, 2024

ToPyObject and IntoPy<PyObject> boilerplates #1595

Closed

davidhewitt added this to the 0.23 milestone Oct 18, 2024

davidhewitt reviewed Oct 20, 2024

View reviewed changes

Icxolu mentioned this pull request Oct 24, 2024

fix #[derive(FromPyObject)] expansion with trait bounds #4645

Merged

davidhewitt mentioned this pull request Oct 25, 2024

release: 0.23.0 #4651

Merged

5 tasks

Icxolu and others added 16 commits October 25, 2024 18:10

initial IntoPyObject derive (transparent newtype structs)

a3de1fb

derive intopyobject for structs and tuple structs

a370be1

support lifetimes

76001ed

support enums

d2f25a7

fix spans

5b7b7d0

compile error tests

a011c96

add some tests

4f42e62

add hygiene tests

730d6ce

add newsfragment

7f88a37

don't drop additional trait bound from the struct/enum definition

0728153

add migration guide entry

39543bb

fix typo

7a8a193

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

improve unit variant spans

7f0ff71

update gudie

c354ffe

apply doc suggestions

a10c5c6

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

fix tuple expansion

9de2ff5

Icxolu force-pushed the derive-intopyobject branch from 2ed88ed to 49d9ce3 Compare October 25, 2024 16:48

parse pyo3(item) and pyo3(attribute)

748bad3

Icxolu force-pushed the derive-intopyobject branch from 49d9ce3 to 748bad3 Compare October 25, 2024 16:54

davidhewitt approved these changes Oct 26, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue Oct 26, 2024

Merged via the queue into PyO3:main with commit c8eb45b Oct 26, 2024

Icxolu deleted the derive-intopyobject branch October 26, 2024 08:24

Uh oh!

Conversation

Icxolu commented Aug 26, 2024

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Icxolu commented Aug 28, 2024

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Icxolu commented Oct 20, 2024

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants