From 4161899c55fafe1000e7a5dbb645b60411f35856 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 Apr 2024 10:49:46 -0700 Subject: [PATCH 1/8] Fix `native_enum` `__module__` attribute: it needs to be a Python `str`. --- include/pybind11/detail/class.h | 10 +--------- include/pybind11/detail/native_enum_data.h | 8 +++----- include/pybind11/pytypes.h | 13 +++++++++++++ 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index c39ea165..174557f1 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -695,15 +695,7 @@ inline PyObject *make_new_python_type(const type_record &rec) { PyUnicode_FromFormat("%U.%U", rec.scope.attr("__qualname__").ptr(), name.ptr())); } - object module_; - if (rec.scope) { - if (hasattr(rec.scope, "__module__")) { - module_ = rec.scope.attr("__module__"); - } else if (hasattr(rec.scope, "__name__")) { - module_ = rec.scope.attr("__name__"); - } - } - + object module_ = get_module_name_if_available(rec.scope); const auto *full_name = c_str( #if !defined(PYPY_VERSION) module_ ? str(module_).cast() + "." + rec.name : diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index 11e7f792..fd75dc6a 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -102,11 +102,9 @@ inline void native_enum_add_to_parent(object parent, const detail::native_enum_d } auto py_enum_type = enum_module.attr(data.use_int_enum ? "IntEnum" : "Enum"); auto py_enum = py_enum_type(data.enum_name, data.members); - if (hasattr(parent, "__module__")) { - // Enum nested in class: - py_enum.attr("__module__") = parent.attr("__module__"); - } else { - py_enum.attr("__module__") = parent; + object module_name = get_module_name_if_available(parent); + if (module_name) { + py_enum.attr("__module__") = module_name; } parent.attr(data.enum_name) = py_enum; if (data.export_values_flag) { diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index e0c277e2..b621c75a 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -2590,5 +2590,18 @@ PYBIND11_MATH_OPERATOR_BINARY_INPLACE(operator>>=, PyNumber_InPlaceRshift) #undef PYBIND11_MATH_OPERATOR_BINARY #undef PYBIND11_MATH_OPERATOR_BINARY_INPLACE +// Meant to return a Python str, but this is not checked. +inline object get_module_name_if_available(handle scope) { + if (scope) { + if (hasattr(scope, "__module__")) { + return scope.attr("__module__"); + } + if (hasattr(scope, "__name__")) { + return scope.attr("__name__"); + } + } + return object(); +} + PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) From 5f1432669401f9c2f4a22c803a86c60595fc6b11 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 Apr 2024 11:14:14 -0700 Subject: [PATCH 2/8] Exercise `enum_type.__module__` --- tests/test_native_enum.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 53d36e33..84c62d4a 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -30,6 +30,11 @@ def test_abi_id(): ("low", "l"), ) +CLASS_WITH_ENUM_IN_CLASS_MEMBERS = ( + ("one", 0), + ("two", 1), +) + EXPORT_VALUES_MEMBERS = ( ("exv0", 0), ("exv1", 1), @@ -43,10 +48,19 @@ def test_abi_id(): @pytest.mark.parametrize( - "enum_type", [m.smallenum, m.color, m.altitude, m.export_values, m.member_doc] + "enum_type", + [ + m.smallenum, + m.color, + m.altitude, + m.export_values, + m.member_doc, + m.class_with_enum.in_class, + ], ) def test_enum_type(enum_type): assert isinstance(enum_type, enum.EnumMeta) + assert enum_type.__module__ == m.__name__ @pytest.mark.parametrize( @@ -57,6 +71,7 @@ def test_enum_type(enum_type): (m.altitude, ALTITUDE_MEMBERS), (m.export_values, EXPORT_VALUES_MEMBERS), (m.member_doc, MEMBER_DOC_MEMBERS), + (m.class_with_enum.in_class, CLASS_WITH_ENUM_IN_CLASS_MEMBERS), ], ) def test_enum_members(enum_type, members): @@ -76,11 +91,6 @@ def test_member_doc(): assert m.member_doc.mem2.__doc__ == "docC" -def test_class_with_enum(): - for value, name in enumerate(("one", "two")): - assert m.class_with_enum.in_class[name].value == value - - def test_pybind11_isinstance_color(): for name, _ in COLOR_MEMBERS: assert m.isinstance_color(m.color[name]) From 13a9d24569c0091b0aee5fe533ef2af147605f83 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 Apr 2024 11:19:49 -0700 Subject: [PATCH 3/8] Consolidate `ENUM_TYPES_AND_MEMBERS`, `ENUM_TYPES` in test. --- tests/test_native_enum.py | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 84c62d4a..89738d4a 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -46,34 +46,25 @@ def test_abi_id(): ("mem2", 2), ) - -@pytest.mark.parametrize( - "enum_type", - [ - m.smallenum, - m.color, - m.altitude, - m.export_values, - m.member_doc, - m.class_with_enum.in_class, - ], +ENUM_TYPES_AND_MEMBERS = ( + (m.smallenum, SMALLENUM_MEMBERS), + (m.color, COLOR_MEMBERS), + (m.altitude, ALTITUDE_MEMBERS), + (m.export_values, EXPORT_VALUES_MEMBERS), + (m.member_doc, MEMBER_DOC_MEMBERS), + (m.class_with_enum.in_class, CLASS_WITH_ENUM_IN_CLASS_MEMBERS), ) + +ENUM_TYPES = [_[0] for _ in ENUM_TYPES_AND_MEMBERS] + + +@pytest.mark.parametrize("enum_type", ENUM_TYPES) def test_enum_type(enum_type): assert isinstance(enum_type, enum.EnumMeta) assert enum_type.__module__ == m.__name__ -@pytest.mark.parametrize( - ("enum_type", "members"), - [ - (m.smallenum, SMALLENUM_MEMBERS), - (m.color, COLOR_MEMBERS), - (m.altitude, ALTITUDE_MEMBERS), - (m.export_values, EXPORT_VALUES_MEMBERS), - (m.member_doc, MEMBER_DOC_MEMBERS), - (m.class_with_enum.in_class, CLASS_WITH_ENUM_IN_CLASS_MEMBERS), - ], -) +@pytest.mark.parametrize(("enum_type", "members"), ENUM_TYPES_AND_MEMBERS) def test_enum_members(enum_type, members): for name, value in members: assert enum_type[name].value == value From 4ec8c00299386250fb48cbb43cda5def05f7afea Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 Apr 2024 11:31:19 -0700 Subject: [PATCH 4/8] Add test_pickle_roundtrip --- tests/test_native_enum.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 89738d4a..3e0bf7ed 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -1,4 +1,5 @@ import enum +import pickle import re import pytest @@ -70,6 +71,21 @@ def test_enum_members(enum_type, members): assert enum_type[name].value == value +@pytest.mark.parametrize(("enum_type", "members"), ENUM_TYPES_AND_MEMBERS) +def test_pickle_roundtrip(enum_type, members): + for name, _ in members: + orig = enum_type[name] + if enum_type is m.class_with_enum.in_class: + # This is a general pickle limitation. + with pytest.raises(pickle.PicklingError): + pickle.dumps(orig) + else: + # This only works if __module__ is correct. + serialized = pickle.dumps(orig) + restored = pickle.loads(serialized) + assert restored == orig + + def test_export_values(): assert m.exv0 is m.export_values.exv0 assert m.exv1 is m.export_values.exv1 From 4bd854ffcf70589b8ee82d0ae51ebc7e266e8ddd Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 Apr 2024 12:05:55 -0700 Subject: [PATCH 5/8] clang-tidy auto-fix --- include/pybind11/detail/native_enum_data.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index fd75dc6a..7055054b 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -88,7 +88,7 @@ PYBIND11_NAMESPACE_END(cross_extension_shared_states) PYBIND11_NAMESPACE_BEGIN(detail) -inline void native_enum_add_to_parent(object parent, const detail::native_enum_data &data) { +inline void native_enum_add_to_parent(const object &parent, const detail::native_enum_data &data) { data.disarm_correct_use_check(); if (hasattr(parent, data.enum_name)) { pybind11_fail("pybind11::native_enum<...>(\"" + data.enum_name_encoded From 633248c7bfeee4c0afa79995c242a392b84222d9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 Apr 2024 15:53:23 -0700 Subject: [PATCH 6/8] Warning suppression for Clang 19 (unrelated to `native_enum` changes). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐍 3 • Clang dev • C++11 • x64 ``` -- The CXX compiler identification is Clang 19.0.0 ``` ``` /__w/pybind11k/pybind11k/include/pybind11/detail/function_record_pyobject.h:38:26: error: cast from 'PyObject *(*)(PyObject *, PyObject *, PyObject *)' (aka '_object *(*)(_object *, _object *, _object *)') to 'PyCFunction' (aka '_object *(*)(_object *, _object *)') converts to incompatible function type [-Werror,-Wcast-function-type-mismatch] 38 | = {{"__reduce_ex__", (PyCFunction) reduce_ex_impl, METH_VARARGS | METH_KEYWORDS, nullptr}, ``` --- include/pybind11/detail/function_record_pyobject.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/pybind11/detail/function_record_pyobject.h b/include/pybind11/detail/function_record_pyobject.h index 9f59772f..e9d42609 100644 --- a/include/pybind11/detail/function_record_pyobject.h +++ b/include/pybind11/detail/function_record_pyobject.h @@ -34,6 +34,9 @@ PYBIND11_WARNING_PUSH #if defined(__GNUC__) && __GNUC__ >= 8 PYBIND11_WARNING_DISABLE_GCC("-Wcast-function-type") #endif +#if defined(__clang__) && !defined(__apple_build_version__) && __clang_major__ >= 19 +PYBIND11_WARNING_DISABLE_CLANG("-Wcast-function-type-mismatch") +#endif static PyMethodDef tp_methods_impl[] = {{"__reduce_ex__", (PyCFunction) reduce_ex_impl, METH_VARARGS | METH_KEYWORDS, nullptr}, {nullptr, nullptr, 0, nullptr}}; From b9f2c7c95668f00b6b6f2c64eb5c864f7880a519 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 Apr 2024 20:28:35 -0700 Subject: [PATCH 7/8] Introduce `py::native_enum_kind` (mandatory argument without a default). --- include/pybind11/detail/native_enum_data.h | 2 +- include/pybind11/native_enum.h | 11 +++--- tests/test_native_enum.cpp | 44 +++++++++++++--------- tests/test_native_enum.py | 2 +- 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index 7055054b..1bf938b4 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -36,7 +36,7 @@ class native_enum_data { std::string was_not_added_error_message() const { return "`native_enum` was not added to any module." " Use e.g. `m += native_enum<...>(\"" - + enum_name_encoded + "\")` to fix."; + + enum_name_encoded + "\", py::native_enum_kind::IntEnum)` to fix."; } #if !defined(NDEBUG) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 13c15ab9..00a317e5 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -15,18 +15,17 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +enum class native_enum_kind { Enum, IntEnum }; + /// Conversions between Python's native (stdlib) enum types and C++ enums. template class native_enum : public detail::native_enum_data { public: using Underlying = typename std::underlying_type::type; - explicit native_enum(const char *name) - : detail::native_enum_data(name, - std::type_index(typeid(Type)), - std::numeric_limits::is_integer - && !std::is_same::value - && !detail::is_std_char_type::value) { + explicit native_enum(const char *name, native_enum_kind kind) + : detail::native_enum_data( + name, std::type_index(typeid(Type)), kind == native_enum_kind::IntEnum) { if (detail::get_local_type_info(typeid(Type)) != nullptr || detail::get_global_type_info(typeid(Type)) != nullptr) { pybind11_fail( diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index b0c1aae8..385c3a42 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -74,35 +74,36 @@ TEST_SUBMODULE(native_enum, m) { m.attr("native_enum_type_map_abi_id_c_str") = py::cross_extension_shared_states::native_enum_type_map::abi_id(); - m += py::native_enum("smallenum") + m += py::native_enum("smallenum", py::native_enum_kind::IntEnum) .value("a", smallenum::a) .value("b", smallenum::b) .value("c", smallenum::c); - m += py::native_enum("color") + m += py::native_enum("color", py::native_enum_kind::IntEnum) .value("red", color::red) .value("yellow", color::yellow) .value("green", color::green) .value("blue", color::blue); - m += py::native_enum("altitude") + m += py::native_enum("altitude", py::native_enum_kind::Enum) .value("high", altitude::high) .value("low", altitude::low); - m += py::native_enum("export_values") + m += py::native_enum("export_values", py::native_enum_kind::IntEnum) .value("exv0", export_values::exv0) .value("exv1", export_values::exv1) .export_values(); - m += py::native_enum("member_doc") + m += py::native_enum("member_doc", py::native_enum_kind::IntEnum) .value("mem0", member_doc::mem0, "docA") .value("mem1", member_doc::mem1) .value("mem2", member_doc::mem2, "docC"); py::class_ py_class_with_enum(m, "class_with_enum"); - py_class_with_enum += py::native_enum("in_class") - .value("one", class_with_enum::in_class::one) - .value("two", class_with_enum::in_class::two); + py_class_with_enum + += py::native_enum("in_class", py::native_enum_kind::IntEnum) + .value("one", class_with_enum::in_class::one) + .value("two", class_with_enum::in_class::two); m.def("isinstance_color", [](const py::object &obj) { return py::isinstance(obj); }); @@ -136,28 +137,33 @@ TEST_SUBMODULE(native_enum, m) { m.def("native_enum_ctor_malformed_utf8", [](const char *malformed_utf8) { enum fake { x }; - py::native_enum{malformed_utf8}; + py::native_enum{malformed_utf8, py::native_enum_kind::IntEnum}; }); m.def("native_enum_value_malformed_utf8", [](const char *malformed_utf8) { enum fake { x }; - py::native_enum("fake").value(malformed_utf8, fake::x); + py::native_enum("fake", py::native_enum_kind::IntEnum) + .value(malformed_utf8, fake::x); }); m.def("double_registration_native_enum", [](py::module_ m) { enum fake { x }; - m += py::native_enum("fake_double_registration_native_enum").value("x", fake::x); - py::native_enum("fake_double_registration_native_enum"); + m += py::native_enum("fake_double_registration_native_enum", + py::native_enum_kind::IntEnum) + .value("x", fake::x); + py::native_enum("fake_double_registration_native_enum", py::native_enum_kind::Enum); }); m.def("native_enum_name_clash", [](py::module_ m) { enum fake { x }; - m += py::native_enum("fake_native_enum_name_clash").value("x", fake::x); + m += py::native_enum("fake_native_enum_name_clash", py::native_enum_kind::IntEnum) + .value("x", fake::x); }); m.def("native_enum_value_name_clash", [](py::module_ m) { enum fake { x }; - m += py::native_enum("fake_native_enum_value_name_clash") + m += py::native_enum("fake_native_enum_value_name_clash", + py::native_enum_kind::IntEnum) .value("fake_native_enum_value_name_clash_x", fake::x) .export_values(); }); @@ -165,19 +171,23 @@ TEST_SUBMODULE(native_enum, m) { m.def("double_registration_enum_before_native_enum", [](const py::module_ &m) { enum fake { x }; py::enum_(m, "fake_enum_first").value("x", fake::x); - py::native_enum("fake_enum_first").value("x", fake::x); + py::native_enum("fake_enum_first", py::native_enum_kind::IntEnum) + .value("x", fake::x); }); m.def("double_registration_native_enum_before_enum", [](py::module_ m) { enum fake { x }; - m += py::native_enum("fake_native_enum_first").value("x", fake::x); + m += py::native_enum("fake_native_enum_first", py::native_enum_kind::IntEnum) + .value("x", fake::x); py::enum_(m, "name_must_be_different_to_reach_desired_code_path"); }); #if defined(PYBIND11_NEGATE_THIS_CONDITION_FOR_LOCAL_TESTING) && !defined(NDEBUG) m.def("native_enum_correct_use_failure", []() { enum fake { x }; - py::native_enum("fake_native_enum_correct_use_failure").value("x", fake::x); + py::native_enum("fake_native_enum_correct_use_failure", + py::native_enum_kind::IntEnum) + .value("x", fake::x); }); #else m.attr("native_enum_correct_use_failure") = "For local testing only: terminates process"; diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 3e0bf7ed..1e1154b9 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -154,7 +154,7 @@ def test_native_enum_data_was_not_added_error_message(): msg = m.native_enum_data_was_not_added_error_message("Fake") assert msg == ( "`native_enum` was not added to any module." - ' Use e.g. `m += native_enum<...>("Fake")` to fix.' + ' Use e.g. `m += native_enum<...>("Fake", py::native_enum_kind::IntEnum)` to fix.' ) From f3b8a03f6c82e2f1b1be7546004417807e7fe134 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 Apr 2024 10:35:57 -0700 Subject: [PATCH 8/8] Change `was_not_added_error_message()` based on feedback from @metzen --- include/pybind11/detail/native_enum_data.h | 2 +- tests/test_native_enum.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index 1bf938b4..fc60f127 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -36,7 +36,7 @@ class native_enum_data { std::string was_not_added_error_message() const { return "`native_enum` was not added to any module." " Use e.g. `m += native_enum<...>(\"" - + enum_name_encoded + "\", py::native_enum_kind::IntEnum)` to fix."; + + enum_name_encoded + "\", ...)` to fix."; } #if !defined(NDEBUG) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 1e1154b9..da2d25c3 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -154,7 +154,7 @@ def test_native_enum_data_was_not_added_error_message(): msg = m.native_enum_data_was_not_added_error_message("Fake") assert msg == ( "`native_enum` was not added to any module." - ' Use e.g. `m += native_enum<...>("Fake", py::native_enum_kind::IntEnum)` to fix.' + ' Use e.g. `m += native_enum<...>("Fake", ...)` to fix.' )