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

enum WIP #1045

Closed
wants to merge 6 commits into from
Closed

enum WIP #1045

wants to merge 6 commits into from

Conversation

stillinbeta
Copy link

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
Copy link
Member

@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
Copy link
Member

kngwyu commented Jul 18, 2020

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
Copy link
Author

stillinbeta commented Jul 18, 2020 via email

@davidhewitt
Copy link
Member

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
Copy link
Author

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
Copy link
Member

davidhewitt commented Jul 19, 2020

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
Copy link
Member

kngwyu commented Jul 20, 2020

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
Copy link
Member

kngwyu commented Jul 20, 2020

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

Any luck @stillinbeta ?

@stillinbeta
Copy link
Author

stillinbeta commented Jul 24, 2020 via email

@kngwyu
Copy link
Member

kngwyu commented Jul 24, 2020

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

@stillinbeta
Copy link
Author

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?

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kngwyu
Copy link
Member

kngwyu commented Jul 27, 2020

How does this work without impl FromPyObject?

@stillinbeta
Copy link
Author

stillinbeta commented Jul 27, 2020 via email

@gilescope
Copy link
Contributor

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
Copy link
Member

kngwyu commented Aug 1, 2020

If so, yes, and the appropriate test cases pass

Then why should we need that impl?


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

// TODO: handle not in send
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@davidhewitt davidhewitt Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Then why should we need that impl?

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

@stillinbeta
Copy link
Author

stillinbeta commented Aug 3, 2020 via email

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +38 to +40
fn enum_arg(e: MyEnum) {
assert_eq!(MyEnum::OtherVariant, e)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest making this

Suggested change
fn enum_arg(e: MyEnum) {
assert_eq!(MyEnum::OtherVariant, e)
}
fn enum_arg(e: MyEnum) -> String {
format!("{}", e)
}

let f = wrap_pyfunction!(enum_arg)(py);
let mynum = py.get_type::<MyEnum>();

py_run!(py, f mynum, "f(mynum.Variant)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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.

Comment on lines +56 to +62
#(
struct #variant_names;
)*

#(
#variant_cls
)*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Author

stillinbeta commented Aug 9, 2020 via email

@stillinbeta
Copy link
Author

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
Copy link
Member

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!

.collect::<syn::Result<Vec<_>>>()?;

Ok(quote! {
impl pyo3::FromPy<#enum_> for pyo3::PyObject {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stillinbeta
Copy link
Author

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
Copy link
Member

Oh heh I see what you're asking 😄

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

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

@kngwyu kngwyu mentioned this pull request Aug 15, 2020
6 tasks
@kngwyu
Copy link
Member

kngwyu commented Aug 22, 2020

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
Copy link
Author

stillinbeta commented Aug 22, 2020 via email

@gilescope
Copy link
Contributor

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
Copy link
Member

#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.

@marioortizmanero marioortizmanero mentioned this pull request Sep 25, 2020
10 tasks
@gilescope
Copy link
Contributor

gilescope commented Oct 19, 2020

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

@hwchen
Copy link

hwchen commented Nov 10, 2020

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

@davidhewitt
Copy link
Member

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
Copy link
Author

stillinbeta commented Nov 10, 2020 via email

@davidhewitt
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants