feat: support pyclass on tuple enums #4072
Conversation
There was a problem hiding this comment.
Thanks for following on this. Don't worry about being busy, happens to all of us, sometimes there are more important things.
This is already looking very good to me. I left a few comments to simplify the code a bit. Regarding using Cow in FnArg, that was not my suggestion, but I agree with that. While leaking would probably be fine, I think using Cow would be the cleaner solution. If you would like open a separate PR for that, that would be fine with me.
Also just giving @carderne a ping, such that we don't duplicate work.
There was a problem hiding this comment.
Thank you very much 🙏 . I think we're getting really close now 🚀.
The slot and __match_args__ handling looks great.
I marked a few spots, where we can reduce some code and some (optional) style nits.
There are two pieces that I would like to address before merge.
For the documentation in the guide I think we should encourage using indexing and tuple match for these tuple variants. This should be a pretty minor change just adapting the examples.
The second point is to also support constructor customization here, which we just landed for struct variants in #4158. This should also be a pretty simple change and I added a note in the corresponding place.
(I hope I'm not putting too much additional work on your table 🙃 )
pyo3-macros-backend/src/pyclass.rs
Outdated
| #pytypeinfo | ||
|
|
||
| #pyclass_impls | ||
| #pyclass_impls | ||
|
|
||
| #[doc(hidden)] | ||
| #[allow(non_snake_case)] | ||
| impl #cls {} | ||
| #[doc(hidden)] | ||
| #[allow(non_snake_case)] | ||
| impl #cls {} | ||
|
|
||
| #(#variant_cls_zsts)* | ||
| #(#variant_cls_zsts)* | ||
|
|
||
| #(#variant_cls_pytypeinfos)* | ||
| #(#variant_cls_pytypeinfos)* | ||
|
|
||
| #(#variant_cls_pyclass_impls)* | ||
| #(#variant_cls_pyclass_impls)* | ||
|
|
||
| #(#variant_cls_impls)* | ||
| #(#variant_cls_impls)* |
There was a problem hiding this comment.
Personally I do prefer the indented version here. (But not a blocker)
There was a problem hiding this comment.
Like so?
| #pytypeinfo | |
| #pyclass_impls | |
| #pyclass_impls | |
| #[doc(hidden)] | |
| #[allow(non_snake_case)] | |
| impl #cls {} | |
| #[doc(hidden)] | |
| #[allow(non_snake_case)] | |
| impl #cls {} | |
| #(#variant_cls_zsts)* | |
| #(#variant_cls_zsts)* | |
| #(#variant_cls_pytypeinfos)* | |
| #(#variant_cls_pytypeinfos)* | |
| #(#variant_cls_pyclass_impls)* | |
| #(#variant_cls_pyclass_impls)* | |
| #(#variant_cls_impls)* | |
| #(#variant_cls_impls)* | |
| #pytypeinfo | |
| #pyclass_impls | |
| #[doc(hidden)] | |
| #[allow(non_snake_case)] | |
| impl #cls {} | |
| #(#variant_cls_zsts)* | |
| #(#variant_cls_pytypeinfos)* | |
| #(#variant_cls_pyclass_impls)* | |
| #(#variant_cls_impls)* |
There was a problem hiding this comment.
Yes, but with #pytypeinfo also indented to the same level. I think that makes it clearer that the whole block is part of the same statement.
- take py from function argument on get_item - make more general slot def implementation - remove unnecessary function arguments - add testcases for uncovered cases including future feature match_args
pytests/src/enums.rs
Outdated
| #[pyclass] | ||
| pub enum TupleEnum { | ||
| Full(i32, f64, bool), | ||
| EmptyTuple(), | ||
| } |
There was a problem hiding this comment.
Not sure I like the ergonomics of explicitly initializing a tuple based on arbitrary identifiers.
The alternative would be to simply define the tuple.
Haven't looked into implementation details, seems doable though.
There was a problem hiding this comment.
I assume you are referring to this?
#[pyo3(constructor = (_0 = 1, _1 = 1.0, _2 = true))]Yeah, it's not ideal, but I think it's better the nothing and we improve on it in the future if necessary.
#3748
Based on @mkovaxx 's https://github.com/PyO3/pyo3/pull/3582/files
Adds support for #[pyclass] for enums containing Tuple Variants.
Supports:
This introduces an issue that I didn't know how to solve quickly: Since the fields have no identifiers I need to create them inside the derive macro. Since they need to outlive their context where they are created as
FnArgrequires a&'a IdentI ended up creating a Box::leak based identifier.I'm not sure how bad this is in the first place since it is only during the build process (code gen) and extremely small.
Addressing the getitem issue the comments on #4135 were very helpful.
Thanks to @carderne for fixing the doc issues, trying to solve the getitem problem and seeing the value in this PR :) - I had a few busy weeks and didn't get to continue working on this PR until now.
I hope I didn't miss any further documentation requirements or am missing any testcases.
Blocking issues:
If we need this before this PR gets merged I can open a seperate PR. (Though I'm not keen on it). A tracking issue has been opened within Replace &'a Ident with Cow in FnArg Enum #4156
__match_args__needs to be generated as we already support matching through the subclass generation.