enum WIP #1045

stillinbeta · 2020-07-17T20:34:53Z

Thank you for contributing to pyo3!

Here are some things you should check for submitting your pull request:

Run cargo fmt (This is checked by travis ci)
Run cargo clippy and check there are no hard errors (There are a bunch of existing warnings; This is also checked by travis)
If applicable, add an entry in the changelog.
If applicable, add documentation to all new items and extend the guide.
If applicable, add tests for all new or fixed functions
If you changed any python code, run black .. You can install black with pip install black)

You might want to run tox (pip install tox) locally to check compatibility with all supported python versions. If you're using linux or mac you might find the Makefile helpful for testing.

davidhewitt · 2020-07-17T21:19:14Z

@stillinbeta many thanks for kicking this off, I'm thrilled to see this! With respect to your comment on the other thread:

I made an attempt (#1045), but I got frustrated by the amount of boilerplate and traits required.

If we were to work together to design this functionality, have you got interest in continuing onwards with this PR?

If you've lost interest that's also fine, I'll pick this up eventually when I get round to thinking about this feature harder.

Reading through what you've started here, I have a few initial thoughts:

The variant parsing code you've written looks clean and straightforward for these "C-style" enums, which is nice to see. 👍
I'm wondering whether we need to implement PyClass and all its gubbins for #[pyenum], or whether we should go down a different direction.
- I don't think that we'll need either inheritance or being able to have &mut self receivers in #[pymethods] for these "C-style" enums. These are two of the main reasons PyClass is so messy, so maybe we can design something leaner here.
- On the other hand, at the moment the #[pyproto] implementations are heavily coupled to PyClass. That's not necessarily an issue, but it does mean that if we want to use #[pyproto] with #[pyenum] then we're either forced to use PyClass, or we'll have to do some refactoring...
It would likely be nice to add each enum variant as a #[classattr] automatically.

kngwyu · 2020-07-18T12:59:47Z

Thanks!

I don't think that we'll need either inheritance or being able to have &mut self receivers in #[pymethods] for these "C-style" enums.

Yeah, but I don't think both inheritance and &mut self don't contribute the code complexity here.
I think what we need is just to use the common code as pyclass.

stillinbeta · 2020-07-18T13:28:02Z

My initial intent was to inherit from enum, but that blocked on #991. I agree that modularizing the pyclass macro is probably the way forward, since we're definitely going to need some of it (`EnumName.EnumVariant` was the UX I was going for, a la the enum modules, and I think that basically necessitates a class)

…

On Sat, 18 Jul 2020, 09:00 Yuji Kanagawa, ***@***.***> wrote: Thanks! I don't think that we'll need either inheritance or being able to have &mut self receivers in #[pymethods] for these "C-style" enums. Yeah, but I don't think both inheritance and &mut self don't contribute the code complexity here. I think what we need is just to use the common code as pyclass. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD724CBE7DAMNMQN7L4N5DR4GME7ANCNFSM4O63GGYA> .

davidhewitt · 2020-07-18T20:28:33Z

Agreed. Perhaps if I drafted up a solution to #991, we could get that merged which would pave the way for inheriting from enum.Enum in this PR?

I guess we can continue building out the other elements of this PR in the meanwhile!

EnumName.EnumVariant was the UX I was going for, a la the enum modules, and I think that basically necessitates a class

If you haven't seen it yet, you should check out the #[classattr] annotation we support in #[pymethods] blocks. We already went through a lot of trouble to allow that to safely support attributes that are instances of the containing class, so you should definitely be able to re-use that to expose the variants.

stillinbeta · 2020-07-19T20:49:50Z

I took a stab at getting rid of all the PyClass constraint, but it's going to require a slot of boilerplate on its own:

We need a Layout, and the only blanket implementation is for PyClass: https://docs.rs/pyo3/0.11.1/pyo3/type_object/trait.PyLayout.html#impl-PyLayout%3CT%3E
So does Initializer, which is only implemented in for PyClass and native types (which this definitely isn't). https://docs.rs/pyo3/0.11.1/pyo3/pyclass_init/struct.PyClassInitializer.html

I managed to get enough of the traits in there to compile (was just missing one, it turns out), but it feels... janky. We don't actually need the Inventory or the ProtoRegistry, but the constraints on PyClass mandate them. Do we think we could loosen some of those constraints, or would that break other things?

davidhewitt · 2020-07-19T22:21:01Z

Thanks for continuing to iterate on this!

Regarding the layout, I think that we could try adding a new struct like this:

struct PyEnumLayout<T> {
    base: PyAny,
    value: T
}

This new type could get a new blanket layout impl?

As for the initializer, I think that we can use PyClassInitializer by adding a new function similar to create_cell_from_subtype. The new function would instead return PyResult<*mut PyEnumLayout<T>>.

We don't actually need the Inventory or the ProtoRegistry

I actually think we probably do want to support #[pymethods] and #[pyproto] for enum classes? So I think we do actually need the inventories.

kngwyu · 2020-07-20T03:18:31Z

I took a stab at getting rid of all the PyClass constraint, but it's going to require a slot of boilerplate on its own:

I really don't think we should do so.
So now we're going to introduce new lots of boilerplates for reducing boilerplates for pyclass?
I don't think it's so meaningful and believe that reusing all pyclass components is the simplest way and also easy to maintain.

kngwyu · 2020-07-20T03:22:52Z

I think what we really need here is to provide a nice API for our Rustic enum. How to implement it does not matter so much. So I recommend implementing it internally just as pyclass.

gilescope · 2020-07-20T16:09:03Z

I keep wondering if another way to think about enums is if we supported static getters on pyclasses? That would look from the python side very similar esp if we could subclass Enum...

stillinbeta · 2020-07-20T17:12:44Z

I actually think we probably do want to support #[pymethods] and #[pyproto] for enum classes? So I think we do actually need the inventories.

I'm not sure I agree with this. Do we have examples of Enums in python with methods on them? It seems like usually they're just sigils in Python.

programmerjake · 2020-07-20T17:47:47Z

I'm not sure I agree with this. Do we have examples of Enums in python with methods on them? It seems like usually they're just sigils in Python.

Example:
https://git.libre-soc.org/?p=ieee754fpu.git;a=blob;f=src/ieee754/div_rem_sqrt_rsqrt/core.py;h=9f925b871493a6885d92366d648ea6c57bb39c20;hb=HEAD#l28

class DivPipeCoreOperation(enum.Enum):
    """ Operation for ``DivPipeCore``.

    :attribute UDivRem: unsigned divide/remainder.
    :attribute SqrtRem: square-root/remainder.
    :attribute RSqrtRem: reciprocal-square-root/remainder.
    """

    SqrtRem = 0
    UDivRem = 1
    RSqrtRem = 2

    def __int__(self):
        """ Convert to int. """
        return self.value

    @classmethod
    def create_signal(cls, *, src_loc_at=0, **kwargs):
        """ Create a signal that can contain a ``DivPipeCoreOperation``. """
        return Signal(range(min(map(int, cls)), max(map(int, cls)) + 2),
                      src_loc_at=(src_loc_at + 1),
                      decoder=lambda v: str(cls(v)),
                      **kwargs)

stillinbeta · 2020-07-20T18:07:30Z

oh yep, fair enough. In that case, I think PyClass is the way to go. Though I think perhaps initially I'll try to land the change without pymethod support, then we can add it.

gilescope · 2020-07-24T16:53:06Z

Any luck @stillinbeta ?

stillinbeta · 2020-07-24T16:54:56Z

Sorry, I don't have a lot of time during the week to work on this (day job drains my programming energy). Will give it a shot tomorrow!

…

On Fri, 24 Jul 2020 at 12:53, Squirrel ***@***.***> wrote: Any luck @stillinbeta <https://github.com/stillinbeta> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD724BCC2Y2TR2A43LXN4DR5G4AFANCNFSM4O63GGYA> .

kngwyu · 2020-07-24T17:07:10Z

Don't worry, that’s the order of OSS

stillinbeta · 2020-07-26T22:58:59Z

Made some progress this weekend! You can see that you can now return enums from pyfunctions, and they'll be Python-ified properly. However, I ran into issues when trying to pass one back:

error[E0119]: conflicting implementations of trait `pyo3::FromPyObject<'_>` for type `MyEnum`:
 --> tests/test_enum.rs:6:1
  |
6 | #[pyenum]
  | ^^^^^^^^^
  |
  = note: conflicting implementation in crate `pyo3`:
          - impl<'a, T> pyo3::FromPyObject<'a> for T
            where T: pyo3::PyClass, T: std::clone::Clone;
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

Any ideas how to get around this? Should be implementing a different trait, like PyTryInto?

kngwyu · 2020-07-27T07:21:49Z

pyo3-derive-backend/src/pyenum.rs

+                let name = cls.to_string();
+                // TODO: this should be
+                // #cls as pyo3::type_object::PyTypeObject>::type_object(py).compare()
+                // but I can't figure out how to get py inside extract()


You can use pyo3::PyNativeType::py(ob)

kngwyu · 2020-07-27T07:28:43Z

How does this work without impl FromPyObject?

stillinbeta · 2020-07-27T13:59:12Z

Do you mean “does it compile without that part?” If so, yes, and the appropriate test cases pass

…

On Mon, 27 Jul 2020, 03:28 Yuji Kanagawa, ***@***.***> wrote: How does this work without impl FromPyObject? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD724BATAES6E4KG4T4GETR5UUDRANCNFSM4O63GGYA> .

gilescope · 2020-07-31T12:21:32Z

Nice work @stillinbeta ! As an extension I wonder if it would be possible to use enums where the numeric value is not specified? I know on the rust side you could access the https://doc.rust-lang.org/std/mem/fn.discriminant.html but whether that helps at all I'm less certain.

kngwyu · 2020-08-01T07:03:05Z

If so, yes, and the appropriate test cases pass

Then why should we need that impl?

davidhewitt · 2020-08-01T08:40:13Z

pyo3-derive-backend/src/pyenum.rs

+
+        impl pyo3::pyclass::PyClassAlloc for #typ {}
+
+        // TODO: handle not in send


I think it's reasonable to expect #[pyenum] to always be Send? It wouldn't ever carry nontrivial data afaik.

Maybe go further and have pyenum always be 'Copy' as we are doing small data here rather than ADTs?

Perhaps, though I'm not sure it's our place to add #[derive(Copy, Clone)] automatically to the struct. If we don't need it for the #[pyenum] implementation, I'd rather give users the freedom to choose this themselves.

davidhewitt · 2020-08-01T08:41:18Z

Then why should we need that impl?

+1. Like for #[pyclass], I think this impl gets derived automatically if you have #[derive(Clone)].

stillinbeta · 2020-08-03T04:16:19Z

The impl is because I want passing the class in to rust to turn into the enum variant, not the a type object. I don't think that'll work automatically

…

On Sat, 1 Aug 2020, 04:41 David Hewitt, ***@***.***> wrote: Then why should we need that impl? +1. Like for #[pyclass], I think this impl gets derived automatically if you have #[derive(Clone)]. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD724F32SVXBUIATXBO7QDR6PILXANCNFSM4O63GGYA> .

davidhewitt

I think this test_enum_arg test is confirming that this behaviour works automatically?

(You can get &MyEnum always, and MyEnum as long as MyEnum: Clone)

davidhewitt · 2020-08-03T07:39:07Z

tests/test_enum.rs

+fn enum_arg(e: MyEnum) {
+    assert_eq!(MyEnum::OtherVariant, e)
+}


I suggest making this

Suggested change

fn enum_arg(e: MyEnum) {

assert_eq!(MyEnum::OtherVariant, e)

}

fn enum_arg(e: MyEnum) -> String {

format!("{}", e)

}

davidhewitt · 2020-08-03T07:40:02Z

tests/test_enum.rs

+    let f = wrap_pyfunction!(enum_arg)(py);
+    let mynum = py.get_type::<MyEnum>();
+
+    py_run!(py, f mynum, "f(mynum.Variant)")


If you take my suggestion from above, we can test both cases:

Suggested change

py_run!(py, f mynum, "f(mynum.Variant)")

py_run!(py, f mynum, "assert f(mynum.Variant) == 'Variant'")

py_run!(py, f mynum, "assert f(mynum.OtherVariant) == 'OtherVariant'")

stillinbeta · 2020-08-09T19:11:12Z

the test_enum_arg behaviour doesn't work right now, that's the problem:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: Can't convert <class 'Variant'> to MyEnum
test test_enum_arg ... FAILED

There's no way to automatically determine that I want the class Variant to map to MyEnum::Variant, I need to produce that code myself.

davidhewitt · 2020-08-09T21:02:49Z

pyo3-derive-backend/src/pyenum.rs

+        #(
+            struct #variant_names;
+        )*
+
+        #(
+            #variant_cls
+        )*


I don't think this bit is needed - instead you should just be making the variants be instances of the the enum class. (Like how it is with the enum module.)

davidhewitt · 2020-08-09T21:03:49Z

Oh, I see what you mean now! I don't think that MyClass.Variant should be a separate type? I've marked that code in the comment above.

stillinbeta · 2020-08-09T21:12:24Z

Ah yeah, that makes sense. I misinterpreted how Enum was architected.

…

On Sun, 9 Aug 2020 at 17:04, David Hewitt ***@***.***> wrote: Oh, I see what you mean now! I don't think that MyClass.Variant should be a separate type? I've marked that code in the comment above. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD724CWL5W5LZX2NUNYEMTR74FMDANCNFSM4O63GGYA> .

stillinbeta · 2020-08-09T21:59:08Z

The one advantage of every variant having its own class is it made mapping the variants to their classes fit better into the model of struct -> class. I think to create instances of Variant, I need to make some use of IntoPython, any ideas what that would look like? I'm a little lost in the type system right now.

davidhewitt · 2020-08-09T22:11:26Z

I think MyClass::Variant.into_py(py) should already do the into-python conversion you're asking for?

FWIW if you rebase on master I just merged some simplifications to the type system, which hopefully will help!

davidhewitt · 2020-08-09T22:09:26Z

pyo3-derive-backend/src/pyenum.rs

+        .collect::<syn::Result<Vec<_>>>()?;
+
+    Ok(quote! {
+        impl pyo3::FromPy<#enum_> for pyo3::PyObject {


With the rebase on master, you'll need to change this to impl IntoPy<PyObject> for #enum

stillinbeta · 2020-08-09T22:21:47Z

impl pyo3::IntoPy<pyo3::PyObject> for #enum_ {
            fn into_py(self, py: pyo3::Python) -> pyo3::PyObject {
                self.into_py(py)
            }
        }

is definitely not going to do it, that's just recursion.

davidhewitt · 2020-08-09T23:01:45Z

Oh heh I see what you're asking 😄

You probably want to use the same implementation as #[pyclass], which uses Py::new

pyo3/pyo3-derive-backend/src/pyclass.rs

Line 379 in bcb9077

pyo3::IntoPy::into_py(pyo3::Py::new(py, self).unwrap(), py)

kngwyu · 2020-08-22T07:57:09Z

I checked the problem and think each variant does not have to its own types, just as David says.

@stillinbeta
Thanks, if you cannot afford to complete this PR, please feel free to leave the rest for maintainers.

stillinbeta · 2020-08-22T18:26:14Z

I'm not likely to have time for this until next weekend, feel free to pick it up if you've got time before then.

…

On Sat, 22 Aug 2020, 03:57 Yuji Kanagawa, ***@***.***> wrote: I checked the problem and think each variant does not have to its own types, just as David says. @stillinbeta <https://github.com/stillinbeta> Thanks, if you cannot afford to complete this PR, please feel free to leave the rest for maintainers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD724GOFK5Y5X3BTRJTRALSB526BANCNFSM4O63GGYA> .

gilescope · 2020-09-05T16:00:13Z

Given #1065 is merged in, do we want to also try and get this landed so they can be in 0.12 together or does that PR also make it possible to convert simple rust enums to python enums so this PR is now not needed?

davidhewitt · 2020-09-05T16:06:06Z

#1065 enables the Python -> Rust direction, but it doesn't really solve the case of data enums like we see here (afaik).

I'm now thinking that this new macro #[pyenum] makes sense to land in 0.13, with #[pyexception]. That should also give time for this to be finished off.

gilescope · 2020-10-19T09:41:15Z

Still keen on using this and having proper python enums! What's left to do? (and can we help in any way...?)

hwchen · 2020-11-10T03:08:49Z

Same here, I'm definitely looking forward to this and I'm happy to help.

davidhewitt · 2020-11-10T06:45:50Z

I think at this point it's reasonable to assume the original PR author has unfortunately not been able to find the time to finish this off, so I guess they wouldn't mind if anyone is interested in copying this branch and reopening it as a new PR.

I think that rebasing it on master, applying the review changes I've made here, and then fixing any subsequent test failures would be a great starting point. I don't think there's particularly far to go for us to merge this!

stillinbeta · 2020-11-10T15:23:58Z

Yeah, I've mostly moved onto other projects, sorry for not un-licking this particular cookie sooner. Go forth with my blessing, and I'll try to answer questions about my code if they come up.

…

On Tue, 10 Nov 2020, 01:46 David Hewitt, ***@***.***> wrote: I think at this point it's reasonable to assume the original PR author has unfortunately not been able to find the time to finish this off, so I guess they wouldn't mind if anyone is interested in copying this branch and reopening it as a new PR. I think that rebasing it on master, applying the review changes I've made here, and then fixing any subsequent test failures would be a great starting point. I don't think there's particularly far to go for us to merge this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD724CGGUIBTJESOMBJ5U3SPDOSZANCNFSM4O63GGYA> .

davidhewitt · 2021-11-17T22:28:08Z

This is now superseded by #2002. Thanks again for kicking this off.

stillinbeta mentioned this pull request Jul 17, 2020

Support exporting Rust enums to Python (old school / without data) #834

Closed

stillinbeta force-pushed the derive-enum branch from cd8e1bd to 4c13d40 Compare July 19, 2020 20:50

stillinbeta added 6 commits July 26, 2020 16:20

enum WIP

cda5dd1

Add PyClassSend implementation

dd6741b

Derive classes for enum types as well

9d0d578

Add FromPy implementation for enums

3cc9d7f

Test returning an enum

ddb7ca5

(WIP) Passing an enum as an argument

170021e

stillinbeta force-pushed the derive-enum branch from 4c13d40 to 170021e Compare July 26, 2020 22:56

kngwyu reviewed Jul 27, 2020

View reviewed changes

kngwyu mentioned this pull request Aug 1, 2020

FromPyObject derivation for structs and enums #1065

Merged

davidhewitt reviewed Aug 1, 2020

View reviewed changes

davidhewitt reviewed Aug 3, 2020

View reviewed changes

davidhewitt reviewed Aug 9, 2020

View reviewed changes

kngwyu mentioned this pull request Aug 15, 2020

0.12 Release #1104

Closed

6 tasks

marioortizmanero mentioned this pull request Sep 25, 2020

Rust integration vidify/vidify#108

Closed

10 tasks

davidhewitt added the needs-implementer label Nov 10, 2020

davidhewitt mentioned this pull request Jan 24, 2021

Automatically implement Python getters and setters for public Rust struct fields #1375

Open

Base automatically changed from master to main March 16, 2021 22:09

jovenlin0527 mentioned this pull request Nov 17, 2021

Derive enum #2002

Merged

davidhewitt closed this Nov 17, 2021


		impl pyo3::pyclass::PyClassAlloc for #typ {}

		// TODO: handle not in send

	py_run!(py, f mynum, "f(mynum.Variant)")
	py_run!(py, f mynum, "assert f(mynum.Variant) == 'Variant'")
	py_run!(py, f mynum, "assert f(mynum.OtherVariant) == 'OtherVariant'")

enum WIP #1045

enum WIP #1045

Conversation

stillinbeta commented Jul 17, 2020

davidhewitt commented Jul 17, 2020

kngwyu commented Jul 18, 2020

stillinbeta commented Jul 18, 2020 via email

davidhewitt commented Jul 18, 2020

stillinbeta commented Jul 19, 2020

davidhewitt commented Jul 19, 2020 • edited

kngwyu commented Jul 20, 2020

kngwyu commented Jul 20, 2020 • edited

gilescope commented Jul 20, 2020

stillinbeta commented Jul 20, 2020

programmerjake commented Jul 20, 2020

stillinbeta commented Jul 20, 2020

gilescope commented Jul 24, 2020

stillinbeta commented Jul 24, 2020 via email

kngwyu commented Jul 24, 2020

stillinbeta commented Jul 26, 2020

kngwyu Jul 27, 2020

Choose a reason for hiding this comment

kngwyu commented Jul 27, 2020

stillinbeta commented Jul 27, 2020 via email

gilescope commented Jul 31, 2020

kngwyu commented Aug 1, 2020

davidhewitt Aug 1, 2020

Choose a reason for hiding this comment

gilescope Aug 11, 2020

Choose a reason for hiding this comment

davidhewitt Aug 11, 2020 • edited

Choose a reason for hiding this comment

davidhewitt commented Aug 1, 2020

stillinbeta commented Aug 3, 2020 via email

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Aug 3, 2020

Choose a reason for hiding this comment

davidhewitt Aug 3, 2020

Choose a reason for hiding this comment

stillinbeta commented Aug 9, 2020

davidhewitt Aug 9, 2020

Choose a reason for hiding this comment

davidhewitt commented Aug 9, 2020

stillinbeta commented Aug 9, 2020 via email

stillinbeta commented Aug 9, 2020

davidhewitt commented Aug 9, 2020

davidhewitt Aug 9, 2020

Choose a reason for hiding this comment

stillinbeta commented Aug 9, 2020

davidhewitt commented Aug 9, 2020

kngwyu commented Aug 22, 2020

stillinbeta commented Aug 22, 2020 via email

gilescope commented Sep 5, 2020

davidhewitt commented Sep 5, 2020

gilescope commented Oct 19, 2020 • edited

hwchen commented Nov 10, 2020

davidhewitt commented Nov 10, 2020

stillinbeta commented Nov 10, 2020 via email

davidhewitt commented Nov 17, 2021

davidhewitt commented Jul 19, 2020 •

edited

kngwyu commented Jul 20, 2020 •

edited

davidhewitt Aug 11, 2020 •

edited

gilescope commented Oct 19, 2020 •

edited