Derive enum #2002

jovenlin0527 · 2021-11-17T13:10:54Z

This PR is about #834. Actually it's just #1045 rebased on the current main branch.

However, due to PyO3 internal API changes, I have to rewrite how pyenum implements PyO3 traits, and I am not sure whether I got it right. I am sure I omitted docstrings and descriptors, but maybe there are some other traits that I need to implement, or maybe I implemented something wrong.

The origin implement tries to create a subclass for each variant. This isn't compatible with current PyO3 API, because we don't have a way to convert the subclass back to the enum. To make PyO3 support this is beyond my capability, so this PR will use a simpler approach. But to support #417 and #1714 we will eventually have to do that.

This PR will only support enums with unit variants, by implement the enum as a PyClass with an __eq__ method, and add a classattr for each variant.

Unsolved issues:

~~How should we treat custom discriminant values? (Add an __int__ implementation?)~~ Will not touch it at this PR.
How should we implement __repr__?
How does docstring on each variant work?

davidhewitt

Thank you, I'm really excited to see progress being made for this again!

I've posted a few comments to get started. I think that may lead to a fair few changes so I've held off on more detailed review for now.

src/types/enum.rs

davidhewitt · 2021-11-17T22:36:50Z

pyo3-macros-backend/src/pyenum.rs

+        #[pymethods]
+        impl #enum_ {
+            #(
+                #[allow(non_upper_case_globals)]
+                #variant_consts
+            )*
+        }


This is clever, but I think the downside of this is that without multiple-pymethods feature it won't be possible for users to add their own #[pymethods] block.

You should instead use gen_py_const to create "method definitions" which then need to be fed into the collection machinery via a "methods trait" - probably PyClassDescriptors can be "abused" here (that's the one which collects all the #[pyo3(get, set)] annotations from regular #[pyclass]).

Let me know if you want me to give some more pointers - this code is rather messy and not well documented as it's in a permanent state of evolution / hackery :)

Let me try to figure that out. Meanwhile there's a new problem: I need to implement __eq__ (via __richcmp__) for the enum to be usable, but I'll need a way to do that.

Mmm ultimately you'll need to generate the code to create a PyType_Slot struct which points to the implementation of __richcmp__ you want, which then gets fed into the collection machinery as part of the for_each_proto_slot function.

There's some code in pyo3-macros-backend/src/pymethod.rs to which handles the magic methods as part of pymethods which might be useful if you need to generate a __richcmp__ method. Or alternatively maybe you're able to hard-code an "enum protocols trait" in src/class/impl_.rs which you can then add to enum classes only using the PyClassImplBuilder.

Hopefully again those are some notes to get started reading the right code - please do ask more questions as you need!

I have an idea, how about we derive from the enum and provide __richcmp__ in the subclass? That's less complex, and we don't have to handle the situation where the user want to provide their own __richcmp__.
Equivalent code in Python:

class Enum: @classmethod @property def Var1(cls): return EnumVar1() ... class EnumVar1(Enum): def __eq__(self, other): # Return True only if `other` is an `Enum` that represents Var1

Perhaps, I think if the user provides their own __richcmp__ and it overrides our default one that's fine. Let's see how this goes.

Another thought could be to enforce singletons for each variant, this removing need for __richcmp__

I wanted to use subclass approach because I didn't understand how PyO3 collects methods and thought if we provided __richcmp__, the user couldn't create their own. Now I've read the implementation, I know it's not the case and I think I like introducing a new trait.
I don't know how to enforce singletons.

pyo3-macros-backend/src/pyenum.rs

pyo3-macros-backend/src/pyimpl.rs

davidhewitt · 2021-11-17T22:42:27Z

tests/test_enum.rs

+#[pyenum]
+#[derive(Debug, PartialEq, Clone)]
+pub enum MyEnum {


Crazy idea, does this need to be a separate #[pyenum] macro?

This still creates a Python class, so maybe this can just be a new feature for the #[pyclass] macro?

I think that's a great idea. I'm not sure how do that interact with customizing-the-class though. Maybe we just disable extends, subclass and dict? I'll implement this PR as an extension to #[pyclass] if you are okay with it.

Implementing as #[pyclass] sounds great to me!

Maybe we just disable extends, subclass and dict?

I think extends makes sense to disable, however subclass and dict could reasonably be things that users want to add to customize their enum? I don't think they'll add much complexity to this PR, we can see as we go and decide that later.

I just want to be future-proof. In the future when we add ADT support, each variant will be represented by a subclass (I explained why in another comment of #417), and we want them to be the only subclasses of this enum, otherwise pattern matching will not work. (Python interpreter cannot recognize other subclasses as a variant of the enum.)

I am now okay with enabling dict, as I cannot think of a potential reason why we would wan't to disable dict in the future.

and we want them to be the only subclasses of this enum

Ahh good point, ok yes let's disable subclass too.

Well, I just want to stop user from writing #[pyclass(subclass)]. We still need to secretly enable that in order to write our variant subclasses. (Will not enable that in this PR)

I see. I'm not sure it can work like that; as far as Python is concerned either it's legal to subclass this or it isn't.

Might be able to control this a bit via a metaclass? Could be overkill.

Python's Enum also makes use of metaclass to make sure its enums are immutable. ~~I think that's a good reason to implement an enum metaclass.~~
~~I'll try to implement that after I finish __eq__.~~
There's no appropriate PyNativeType for metaclass instances to implement PyClass. I'm not familiar enough with PyO3 to change that.

I'm sorry, for some reasons I thought PyClass::BaseNativeType must be a class created by the metaclass ... I found I was wrong after reading create_type_object and a bunch of CPython code... 😅😅
So currently PyO3 does not support subclass from PyType, because it doesn't implement PyClassBaseType... I wonder if we can implement that using pyobject_native_type_sized... Maybe we should open another issue for supporting metaclasses? We can discuss whether this use case is important enough there.

Indeed there isn't yet a way to set metaclasses - #906.

Agreed, let's complete this PR without solving the metaclass issue and we can always explore that as a follow-up.

davidhewitt · 2021-11-18T07:25:57Z

How should we treat custom discriminant values? (Add an __int__ implementation?)

Can you elaborate a little more what you mean by this?

How should we implement __repr__?

How does docstring on each variant work?

I guess that for these two it would make sense to look at how Python does it (maybe Python 3.10?) and aim for a similar end result for users.

jovenlin0527 · 2021-11-18T08:11:08Z

How should we treat custom discriminant values? (Add an __int__ implementation?)

Can you elaborate a little more what you mean by this?

I think when the user writes

enum Foo{
Bar = 1,
}

, they will expect some relationship between Foo.Bar and 1 at the Python side. I'm not sure what should that be.

How should we implement __repr__?

How does docstring on each variant work?

I guess that for these two it would make sense to look at how Python does it (maybe Python 3.10?) and aim for a similar end result for users.

Hmmm.. I have an idea how to implement docstring. I'll push it when I'm done and you can see whether it's reasonable.

davidhewitt · 2021-11-18T08:16:36Z

they will expect some relationship between Foo.Bar and 1 at the Python side. I'm not sure what should that be.

Perhaps like Python IntEnum, Foo.Bar.value should be 1 and also Foo.Bar == 1?

jovenlin0527 · 2021-11-18T08:51:54Z

they will expect some relationship between Foo.Bar and 1 at the Python side. I'm not sure what should that be.

Perhaps like Python IntEnum, Foo.Bar.value should be 1 and also Foo.Bar == 1?

https://docs.python.org/3/library/enum.html#enum.IntEnum

That's implemented by subclassing int... Not sure if we should & how to do that.

I think I'll forbid that for now, and leave that for another PR.

jovenlin0527 · 2021-11-18T18:13:05Z

Hi, I did a major refactoring to implementations of #[pyclass]. You might want to review that.

pyo3-macros-backend/src/pyclass.rs

davidhewitt · 2021-11-19T17:27:32Z

👍 will try to re-review in the next couple days when I get a chance!

davidhewitt

👍 this is looking really solid to me, and nice refactoring work on the #[pyclass] implementation.

I'm leaning towards we tidy this PR up just a tiny bit and then merge it, because it's got a lot of changes which might conflict with other work going on.

We can create an issue to record all the pieces of follow-up work to do (such as __eq__ implementation and guide contents). Hopefully you'd be interested in carrying on with the future work on this? It's an exciting piece of new functionality, thank you for picking this up!

pyo3-macros-backend/src/pyclass.rs

jovenlin0527 · 2021-11-21T03:52:35Z

We can create an issue to record all the pieces of follow-up work to do (such as __eq__ implementation and guide contents).

I'm okay with that, but don't we already have #417 and #834?

Hopefully you'd be interested in carrying on with the future work on this? It's an exciting piece of new functionality, thank you for picking this up!

I'd love to implement rest of this feature.

davidhewitt · 2021-11-21T09:16:54Z

I'm okay with that, but don't we already have #417 and #834?

Yes, but I think once this merges we should aim to complete this initial functionality (e.g. __eq__ and guide entry) before 0.16 releases, so it would be useful to have an issue to remind us of those bits.

davidhewitt

Created #2013 for follow-up work. Post on there if you think anything is missing.

Would you mind rebasing and force-pushing this PR (and potentially squashing it into one or two commits)? The CI seems to have glitched out and so I can't merge it 😅

There's no functionality since it does not generate __richcmp__. Also it only works on enums with only variants, and does not support C-like enums.

jovenlin0527 · 2021-11-21T12:15:12Z

Rebase done. Sorry there are some hiccups...

davidhewitt · 2021-11-21T14:00:50Z

Brilliant, thanks again!

davidhewitt mentioned this pull request Nov 17, 2021

enum WIP #1045

Closed

davidhewitt reviewed Nov 17, 2021

View reviewed changes

davidhewitt mentioned this pull request Nov 17, 2021

Support exporting Rust enums to Python (full ADTs) #417

Open

jovenlin0527 force-pushed the derive-enum branch 2 times, most recently from fd73d79 to 215ee61 Compare November 18, 2021 15:26

jovenlin0527 commented Nov 18, 2021

View reviewed changes

pyo3-macros-backend/src/pyclass.rs Show resolved Hide resolved

jovenlin0527 force-pushed the derive-enum branch from 92af972 to 0ffc436 Compare November 18, 2021 22:56

davidhewitt reviewed Nov 20, 2021

View reviewed changes

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved

pyo3-macros-backend/src/pyclass.rs Show resolved Hide resolved

jovenlin0527 marked this pull request as ready for review November 21, 2021 04:06

davidhewitt mentioned this pull request Nov 21, 2021

#[pyclass] on enums MVP #2013

Closed

6 tasks

davidhewitt approved these changes Nov 21, 2021

View reviewed changes

jovenlin0527 force-pushed the derive-enum branch 2 times, most recently from 9aaa67e to e381815 Compare November 21, 2021 10:56

jovenlin0527 marked this pull request as draft November 21, 2021 11:48

Refactor #[pyclass] and now it supports enum.

b7419b5

There's no functionality since it does not generate __richcmp__. Also it only works on enums with only variants, and does not support C-like enums.

jovenlin0527 force-pushed the derive-enum branch from e381815 to b7419b5 Compare November 21, 2021 12:08

jovenlin0527 marked this pull request as ready for review November 21, 2021 12:11

davidhewitt merged commit b329f1b into PyO3:main Nov 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derive enum #2002

Derive enum #2002

jovenlin0527 commented Nov 17, 2021 •

edited

davidhewitt left a comment

davidhewitt Nov 17, 2021 •

edited

jovenlin0527 Nov 18, 2021 •

edited

davidhewitt Nov 18, 2021

jovenlin0527 Nov 18, 2021 •

edited

davidhewitt Nov 21, 2021

davidhewitt Nov 21, 2021

jovenlin0527 Nov 21, 2021

davidhewitt Nov 17, 2021

jovenlin0527 Nov 18, 2021 •

edited

davidhewitt Nov 18, 2021

jovenlin0527 Nov 18, 2021 •

edited

davidhewitt Nov 18, 2021

jovenlin0527 Nov 18, 2021 •

edited

davidhewitt Nov 18, 2021

jovenlin0527 Nov 18, 2021 •

edited

jovenlin0527 Nov 19, 2021 •

edited

davidhewitt Nov 19, 2021

davidhewitt commented Nov 18, 2021

jovenlin0527 commented Nov 18, 2021 •

edited

davidhewitt commented Nov 18, 2021

jovenlin0527 commented Nov 18, 2021

jovenlin0527 commented Nov 18, 2021

davidhewitt commented Nov 19, 2021

davidhewitt left a comment •

edited

jovenlin0527 commented Nov 21, 2021 •

edited

davidhewitt commented Nov 21, 2021

davidhewitt left a comment

jovenlin0527 commented Nov 21, 2021

davidhewitt commented Nov 21, 2021

Derive enum #2002

Derive enum #2002

Conversation

jovenlin0527 commented Nov 17, 2021 • edited

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Nov 17, 2021 • edited

Choose a reason for hiding this comment

jovenlin0527 Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovenlin0527 Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovenlin0527 Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovenlin0527 Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovenlin0527 Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovenlin0527 Nov 18, 2021 • edited

Choose a reason for hiding this comment

jovenlin0527 Nov 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented Nov 18, 2021

jovenlin0527 commented Nov 18, 2021 • edited

davidhewitt commented Nov 18, 2021

jovenlin0527 commented Nov 18, 2021

jovenlin0527 commented Nov 18, 2021

davidhewitt commented Nov 19, 2021

davidhewitt left a comment • edited

Choose a reason for hiding this comment

jovenlin0527 commented Nov 21, 2021 • edited

davidhewitt commented Nov 21, 2021

davidhewitt left a comment

Choose a reason for hiding this comment

jovenlin0527 commented Nov 21, 2021

davidhewitt commented Nov 21, 2021

jovenlin0527 commented Nov 17, 2021 •

edited

davidhewitt Nov 17, 2021 •

edited

jovenlin0527 Nov 18, 2021 •

edited

jovenlin0527 Nov 18, 2021 •

edited

jovenlin0527 Nov 18, 2021 •

edited

jovenlin0527 Nov 18, 2021 •

edited

jovenlin0527 Nov 18, 2021 •

edited

jovenlin0527 Nov 18, 2021 •

edited

jovenlin0527 Nov 19, 2021 •

edited

jovenlin0527 commented Nov 18, 2021 •

edited

davidhewitt left a comment •

edited

jovenlin0527 commented Nov 21, 2021 •

edited