Skip to content

enum: Add iteration and simple type-based check, towards PEP 435 #2744

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 61 additions & 1 deletion include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1569,6 +1569,59 @@ inline str enum_name(handle arg) {
return "???";
}

class enum_meta_info {
public:
static pybind11::object enum_meta_cls() {
return get().enum_meta_cls_;
}

static pybind11::object enum_base_cls() {
return get().enum_base_cls_;
}

private:
template <typename T>
friend T& pybind11::get_or_create_shared_data(const std::string&);

static const enum_meta_info& get() {
return pybind11::get_or_create_shared_data<enum_meta_info>(
"_pybind11_enum_meta_info");
}

enum_meta_info() {
handle copy = pybind11::module::import("copy").attr("copy");
locals_ = copy(pybind11::globals());
locals_["pybind11_meta_cls"] = reinterpret_borrow<object>(
reinterpret_cast<PyObject*>(get_internals().default_metaclass));
locals_["pybind11_base_cls"] = reinterpret_borrow<object>(
get_internals().instance_base);
// TODO: Make the base class work.
const char code[] = R"""(
pybind11_enum_base_cls = None

class pybind11_enum_meta_cls(pybind11_meta_cls):
is_pybind11_enum = True

def __iter__(cls):
return iter(cls.__members__.values())

def __len__(cls):
return len(cls.__members__)

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, didn't realize this should be done in enum.EnumMeta, not here. 🤦

Is there a way to do it without altering builtin classes?

Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI Dec 31, 2020

Choose a reason for hiding this comment

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

Not that I'm aware of :( I had gone the route of modifying builtin classes, but yuck:
master...a41944c
(I didn't get it fully working, hit some weird recursion thing, and gave up 'cause ewww)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it does look too hacky and doesn't sound like right thing to do.

Actually pybind-enum is NOT enum.Enum, so maybe we shouldn't try to make this check return true? I think it would be more appropriate to introduce collections.abc.Enum (e.g. in python 3.10) and make isinstance(pybind_enum_value, colletions.abc.Enum) recognize all PEP-complaining enum implementations? In meantime one probably have no choice other than stick to self-written check method (like one in tests in this PR).

I think such doubtful hacks would greatly reduce the PR merge chances, so I would leave attempts to introduce them, at least for now.

)""";
PyObject *result = PyRun_String(
code, Py_file_input, locals_.ptr(), locals_.ptr());
if (result == nullptr) {
throw error_already_set();
}
enum_meta_cls_ = locals_["pybind11_enum_meta_cls"];
enum_base_cls_ = locals_["pybind11_enum_base_cls"];
}

pybind11::object enum_meta_cls_;
pybind11::object enum_base_cls_;
pybind11::dict locals_;
};

struct enum_base {
enum_base(handle base, handle parent) : m_base(base), m_parent(parent) { }

Expand Down Expand Up @@ -1725,12 +1778,19 @@ template <typename Type> class enum_ : public class_<Type> {

template <typename... Extra>
enum_(const handle &scope, const char *name, const Extra&... extra)
: class_<Type>(scope, name, extra...), m_base(*this, scope) {
: class_<Type>(
scope, name,
// Can't re-declare base type???
// detail::enum_meta_info::enum_base_cls(),
pybind11::metaclass(detail::enum_meta_info::enum_meta_cls()),
extra...),
m_base(*this, scope) {
constexpr bool is_arithmetic = detail::any_of<std::is_same<arithmetic, Extra>...>::value;
constexpr bool is_convertible = std::is_convertible<Type, Scalar>::value;
m_base.init(is_arithmetic, is_convertible);

def(init([](Scalar i) { return static_cast<Type>(i); }), arg("value"));
def_property_readonly("value", [](Type value) { return (Scalar) value; });
def("__int__", [](Type value) { return (Scalar) value; });
#if PY_MAJOR_VERSION < 3
def("__long__", [](Type value) { return (Scalar) value; });
Expand Down
60 changes: 53 additions & 7 deletions tests/test_enum.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,44 @@
# -*- coding: utf-8 -*-
import pytest

import env # noqa: F401

from pybind11_tests import enums as m

if env.PY2:
enum = None
else:
import enum


def is_enum(cls):
"""Example showing how to recognize a class as pybind11 enum or a
PEP 345 enum."""
if enum is not None:
if issubclass(cls, enum.Enum):
return True
return getattr(cls, "is_pybind11_enum", False)


def test_pep435():
# See #2332.
cls = m.UnscopedEnum
names = ("EOne", "ETwo", "EThree")
values = (cls.EOne, cls.ETwo, cls.EThree)
raw_values = (1, 2, 3)

assert len(cls) == len(names)
assert list(cls) == list(values)
assert is_enum(cls)
if enum:
assert not issubclass(cls, enum.Enum)
for name, value, raw_value in zip(names, values, raw_values):
assert isinstance(value, cls)
if enum:
assert not isinstance(value, enum.Enum)
assert value.name == name
assert value.value == raw_value
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Self-note: Should try to implement Flag based interface. (i.e. closure under arithmetic operations)



def test_unscoped_enum():
assert str(m.UnscopedEnum.EOne) == "UnscopedEnum.EOne"
Expand All @@ -13,15 +50,24 @@ def test_unscoped_enum():

# name property
assert m.UnscopedEnum.EOne.name == "EOne"
assert m.UnscopedEnum.EOne.value == 1
assert m.UnscopedEnum.ETwo.name == "ETwo"
assert m.EOne.name == "EOne"
# name readonly
assert m.UnscopedEnum.ETwo.value == 2
assert m.EOne is m.UnscopedEnum.EOne
# name, value readonly
with pytest.raises(AttributeError):
m.UnscopedEnum.EOne.name = ""
# name returns a copy
foo = m.UnscopedEnum.EOne.name
foo = "bar"
with pytest.raises(AttributeError):
m.UnscopedEnum.EOne.value = 10
# name, value returns a copy
# TODO: Neither the name nor value tests actually check against aliasing.
# Use a mutable type that has reference semantics.
nonaliased_name = m.UnscopedEnum.EOne.name
nonaliased_name = "bar" # noqa: F841
assert m.UnscopedEnum.EOne.name == "EOne"
nonaliased_value = m.UnscopedEnum.EOne.value
nonaliased_value = 10 # noqa: F841
assert m.UnscopedEnum.EOne.value == 1

# __members__ property
assert m.UnscopedEnum.__members__ == {
Expand All @@ -33,8 +79,8 @@ def test_unscoped_enum():
with pytest.raises(AttributeError):
m.UnscopedEnum.__members__ = {}
# __members__ returns a copy
foo = m.UnscopedEnum.__members__
foo["bar"] = "baz"
nonaliased_members = m.UnscopedEnum.__members__
nonaliased_members["bar"] = "baz"
assert m.UnscopedEnum.__members__ == {
"EOne": m.UnscopedEnum.EOne,
"ETwo": m.UnscopedEnum.ETwo,
Expand Down