Do not derive enum_ from class_

[aldanor]

(This is related to work on Python 3 enums in #781)

• Using class_ API now requires calling .into_class() or doing a rvalue cast
• .value() and .export_values() now return rvalue *this instead of lvalue
• As a result of changing this, no existing tests were broken
• Added a few additional tests; updated the docs

Note: this is technically a breaking change, but the consensus seems to be that this functionality was more of a side-effect (of deriving from class_) rather than an intended feature.

[dean0x7d]

Why only rvalues? I'm a bit more worried about this breaking backward compatibility than the inherited class_ functions.

[aldanor]

Well, if you return a reference, you won't be able to call .into_class() which needs an rvalue (which is an important point, in order to avoid all kinds of edge cases). Taking an rvalue *this here, disabling copy ctor and returning by value basically helps ensure we never copy it (accidentally or not) and always move. The usual chaining-style enum definitions still all work as expected.

[dean0x7d]

Ah, right, it would be required for the new enum34. I was only looking at it for the enum_ here, which does not need to return an rvalue.

[aldanor]

I've added back the lvalue overloads which seems to work and won't break backwards compatibility if someone chose to assign enum object to a variable and not chain the calls:

// okauto e = enum_<>();
e.value().value();
// ok
enum_<>().value().into_class().def();
// not okauto e = enum_<>().value();
e.into_class().def(); // <-- compile fail// not ok eitherauto e = enum_<>().value();
e.value().into_class().def(); // <-- compile fail

[dean0x7d]

Why make it implicitly convertible?

[aldanor]

That's how I had it initially (in the other PR) -- with hindsight, this could probably be removed.

[aldanor]

This is now removed.

[dean0x7d]

Note: this is technically a breaking change, but the consensus seems to be that this functionality was more of a side-effect (of deriving from class_) rather than an intended feature.

Just to make sure we don't break anything important, I'll just cc this for others how have also contributed to enum_: @pschella, @mdcb .

The gist is that enum_ supported defining methods like class_ but this was undocumented and would also break chaining, i.e. enum_().value().def().value() would not compile. The first class_ API call would implicitly turn the enum_ into a class_. This PR makes that explicit, documented and tested, but this will break the old undocumented behavior.

[wjakob]

Thinking a bit more about this, I find problematic that enum_ is no longer a py::object. This will definitely break a few of my projects that stick aliases of enumerations into other (sub-) modules to mirror using namespace statements on the C++ side.

Why are we changing this? It seems to be making things more difficult that were simpler before. In that case, I would find it preferable to document the old behavior and keep it as is.

[aldanor]

@wjakob I've initially chosen into_ as opposed to as_ here to indicate the && requirement (same as in Rust conventions, as_ returns a reference/view on the object, where as into_ moves out of it and returns something else). Could definitely change it though, of course.

We are changing it because it was a bit broken initially (and undocumented) since class_<> is not a CRTP and its methods return class_&. So, for instance, code like this won't compile: enum_().value().def().value(). This wasn't the initial reason though -- in order to make the new py3 enums be API-compatible with the existing enum_ there are two options (see #781) -- adjust enum_ so it's no longer a class_, or split out the public part of class_ into a CRTP interface (the few latest commits in #781). The latter was deemed too intrusive, hence this PR.

[wjakob]

I think that it would be straightforward to fix the .def issue by adding a variadic template implementation that forwards its arguments to the .def implementation of the base class and then returns *this. Does this address the general issue you mentioned?

[wjakob]

(i.e. no CRTP needed)

[aldanor]

Sort of, yea. It's a bit more than def though... .def, .def_property_readonly, .def_readonly_static, and all of that. This would basically be a copy/paste-driven implementation, so to speak :)

[aldanor]

(For the CRTP example, see cf1ef95)

[wjakob]

This seems fine to me -- ~9-ish lines of code that will all be optimized away, no?

[wjakob]

That's it, no?

template <typename... Args> def(Args&&... args) { return (enum_&) Base::def(std::forward<Args>(args)...); }
template <typename... Args> def_property_readonly(Args&&... args) { return (enum_&) Base::def_property_readonly(std::forward<Args>(args)...); }
template <typename... Args> def_readonly_static(Args&&... args) { return (enum_&) Base::def_readonly_static(std::forward<Args>(args)...); }

Or am I missing something?

[aldanor]

For the new enum34 implementation - yea, something like this (x3) would work; hacky but should suffice. For enum_ as well, but now you have 18 duplicated lines and not 9 :)

If we're going this way... this is exactly what a separate CRTP would do, less the copy/paste. With a bit of effort I think it could benefit both enum implementations -- for enum_ which derives from class_ (which derives from CRTP) it could allow .value().def().value(); for enum34 (which derives directly from CRTP) it could enable .def_*() methods.

[wjakob]

I don't think it's hacky :). CRTP brings its own set of issues -- it definitely doesn't make the source easier to read ;). In one of my current projects, MSVC 2017 needs to be spoon-fed some of the CRTP-related type aliases to avoid compiler segfaults (accessing members of the derived class in template default parameters still seems somewhat fragile).

[wjakob]

If the duplication bothers you, we could add a macro:

#definePYBIND11_FORWARD(name) template <typename... Args> name(Args&&... args) { returnstatic_cast<decltype(*this)>(Base::name(std::forward<Args>(args)...)); }

I'd really prefer this to making something that should be a py::object into a non-object.

[aldanor]

A macro would be reasonable, it will certainly deal with enum_, but it won't work with enum34 as is :/ (first, because enum34 can't be derived from class_, and second, it cannot give you a class_ lvalue, only a by-value when the instance is rvalue -- to prevent defining methods on a type that may get recreated). It's possible to handle all this lvalue/rvalue mess with crtp and proper template methods, but not with macro.

[aldanor]

Btw if enum_ remains a py::object and there's code actively relying on it then enum34 cannot be made an automatic drop-in replacement (which is what @dean0x7d and @jagerman seemed to suggest), because enum34 cannot have the same API as enum_ and be a py::object at the same time.

[jagerman]

Could enum34 be derived directly from py::object (rather than indirectly through py::class_)?

[aldanor]

Btw @wjakob could you post examples of code you've have that relies on enum_ being an object (eg storing aliases to it)?

[aldanor]

@jagerman I was thinking about it too, will take a look tomorrow.

[wjakob]

One simple example are cases where an enumeration exists in separate classes. Something like this:

classA {
enum E {Value = 0};
};
classB {
using E = A::E;
};
class_<A> cls_a(m, "A");
class_<B> cls_b(m, "B");
enum_<A::E> e(cls_a, "E");
e.value("Value", A::E::Value);
cls_b.attr("E") = e;

[aldanor]

@wjakob Here's your example working :) 9c75fd0

[wjakob]

Hm -- what about @jagerman's suggestion (enum_ being an object)?

[aldanor]

I thought a little bit about it and don't think enum34 can be an object (and the whole purpose of all this is to try and unify the two as much as possible), because it has a "finalize" step and has to recreate itself multiple times, so basically you have a py::object whose m_ptr may change outside of ctor, that doesn't sound too good. At least with cast ops you can control all this business to some extent by & and && qualifiers.

[jagerman]

I'm not sure I see the problem with the m_ptr changing. If I do something like m.attr("foo") = myenum; and then add additional enum values to myenum, it's going to break in exactly the same way with the handle operator as it would with direct object inheritance.

[aldanor]

@jagerman Hmm yea you're right, in the current state handle operator is a bit leaky. However... if we remove enum_& value() & (which I didn't have initially in this PR but then added, see comments above) and leave just enum_ value() && then your example won't compile -- specifically you won't be able to "add an additional value".

[jagerman]

then your example won't compile

... nor will @wjakob's if the lvalue version is removed, which doesn't seem desirable. Requiring you to write std::move(myenum).into_class() when you want both a local myenum variable and then a class_ out of it seems pretty reasonable to me, since it means not forcing existing, simple code into a particular style (i.e. the all-at-once value definitions). (And that's even ignoring the backwards compatibility issue).

[aldanor]

Yep, you're right. Either way something will be broken it seems :) Unless there's some other, smarter way (deriving from object but not class_? idk).

If there's no middle ground, I'm ok with retracting this PR altogether since it was opened out of a suggestion voiced in enum34 PR. if everyone thinks the enum_<> is fine as is (being derived from class_ in a half-broken way, that is), then ok; as I see it, the only way enum34 could be compatible with this then would be either allowing knowingly broken constructs to compile and leave it to users discretion (e.g. .value().def().value()), or pulling class_ apart into a CRTP which was deemed too intrusive iiuc. There was also suggestion of deriving from object but not class_ but I can't immediately see how it solves this.

[dean0x7d]

I guess the entire point here was to make enum_ and enum34 API compatible and interchangeable to the point that users don't need to be aware of it and enum34 is just an invisible optimization of enum_ on compatible Python versions. But if that's not possible without breaking stuff and/or a lot of pain, I would recommend simplifying enum34 to the essentials and making it an optional header enum34.h for those that need it/target the required Python versions.

[aldanor]

Yea; far as I see now, without big changes it's only possible to make them API compatible if we accept the possibility of runtime errors thrown in enum34 API at module initialization time due to incorrect use (which gets compiled ok) -- I really wouldn't like this to be the case.

So (soon as I get back to IE...) I'll probably close this and revert all changes to pybind11.h in #781.

[aldanor]

In fact I'll close this one now so it doesn't annoy anyone anymore :)

(I still think enum_<> is a bit broken in the current state due to how .def() works; I'm not sure if we care though).