Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support pyclass on tuple enums (closer but not there yet) #4135

Closed
wants to merge 9 commits into from

Conversation

carderne
Copy link
Contributor

This is an extension of this PR, which hasn't been updated in two weeks:
#4072

Aiming to close this issue:
#3748

@newcomertv in that PR added support for tuple enums but only with ._0 ._1 access to elements.

This PR attempts to do the following:

I've got the Rust-side macros working but haven't yet managed to get them wired up on the Python side... Main complication is that the __getitem__() implementation has to return a Box<dyn Any> (as the type won't be known) and I haven't figured out how to deal with that on the other side.

Will give it another go soon but putting this up in case anyone has suggestions...

@Icxolu
Copy link
Contributor

Icxolu commented Apr 29, 2024

Thanks for continuing the work here! These double underscore methods are a bit special in how their code generation works.
I think we can reuse

//  `slot` is `__GETITEM__`/`__LEN__` from `pyo3-macros-backend/src/pymethods` 
slot.generate_type_slot(variant_cls_type, &spec, &name, ctx)

to generate these special definitions and thread them back up into PyClassImplsBuilder::new slots argument. Then in impl_complex_enum_tuple_variant_cls we can just generate the corresponding functions

 fn __getitem__(slf: PyRef<Self>, idx: usize) -> PyResult<PyObject> {...}
 fn __len__(slf: PyRef<Self>) -> usize {...}

There should be no need to use Box<dyn Any>, because they need to be convertible to a PyObject anyway. Maybe this helps you a bit while continuing.

Just for future reference: We might also want to generate __match_args__ to allow tuple pattern matching on the Python side.

@carderne
Copy link
Contributor Author

carderne commented May 11, 2024

Closing in favour of #4072 which has picked this up again!
Thanks @Icxolu and @newcomertv!

@carderne carderne closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants