From f0a398ee244e3bf264cf801e669ffcda963aba68 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Nov 2022 02:01:36 -0800 Subject: [PATCH 01/35] First approximation. --- tests/test_native_enum.cpp | 80 ++++++++++++++++++++++++++++++++++++++ tests/test_native_enum.py | 45 +++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 tests/test_native_enum.cpp create mode 100644 tests/test_native_enum.py diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp new file mode 100644 index 0000000000..0b37a3092d --- /dev/null +++ b/tests/test_native_enum.cpp @@ -0,0 +1,80 @@ +// #include + +#include "pybind11_tests.h" + +namespace test_native_enum { + +// https://en.cppreference.com/w/cpp/language/enum + +// enum that takes 16 bits +enum smallenum : std::int16_t { a, b, c }; + +// color may be red (value 0), yellow (value 1), green (value 20), or blue (value 21) +enum color { red, yellow, green = 20, blue }; + +// altitude may be altitude::high or altitude::low +enum class altitude : char { + high = 'h', + low = 'l', // trailing comma only allowed after CWG518 +}; + +// the constant d is 0, the constant e is 1, the constant f is 3 +enum { d, e, f = e + 2 }; + +int pass_color(color e) { return static_cast(e); } +color return_color(int i) { return static_cast(i); } + +py::handle wrap_color(py::module_ m) { + auto enum_module = py::module_::import("enum"); + auto int_enum = enum_module.attr("IntEnum"); + using u_t = std::underlying_type::type; + auto members = py::make_tuple(py::make_tuple("red", static_cast(color::red)), + py::make_tuple("yellow", static_cast(color::yellow)), + py::make_tuple("green", static_cast(color::green)), + py::make_tuple("blue", static_cast(color::blue))); + auto int_enum_color = int_enum("color", members); + int_enum_color.attr("__module__") = m; + m.attr("color") = int_enum_color; + return int_enum_color.release(); // Intentionally leak Python reference. +} + +} // namespace test_native_enum + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +using namespace test_native_enum; + +template <> +struct type_caster { + static handle native_type; + + static handle cast(const color &src, return_value_policy /* policy */, handle /* parent */) { + auto u_v = static_cast::type>(src); + return native_type(u_v).release(); + } + + bool load(handle src, bool /* convert */) { + if (!isinstance(src, native_type)) { + return false; + } + value = static_cast(py::cast::type>(src.attr("value"))); + return true; + } + + PYBIND11_TYPE_CASTER(color, const_name("")); +}; + +handle type_caster::native_type = nullptr; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) + +TEST_SUBMODULE(native_enum, m) { + using namespace test_native_enum; + + py::detail::type_caster::native_type = wrap_color(m); + + m.def("pass_color", pass_color); + m.def("return_color", return_color); +} diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py new file mode 100644 index 0000000000..85aa5937e6 --- /dev/null +++ b/tests/test_native_enum.py @@ -0,0 +1,45 @@ +import enum + +import pytest + +from pybind11_tests import native_enum as m + +COLOR_MEMBERS = ( + ("red", 0), + ("yellow", 1), + ("green", 20), + ("blue", 21), +) + + +def test_enum_color_type(): + assert isinstance(m.color, enum.EnumMeta) + + +@pytest.mark.parametrize("name,value", COLOR_MEMBERS) +def test_enum_color_members(name, value): + assert m.color[name] == value + + +@pytest.mark.parametrize("name,value", COLOR_MEMBERS) +def test_pass_color_success(name, value): + assert m.pass_color(m.color[name]) == value + + +def test_pass_color_fail(): + with pytest.raises(TypeError) as excinfo: + m.pass_color(None) + assert "" in str(excinfo.value) + + +@pytest.mark.parametrize("name,value", COLOR_MEMBERS) +def test_return_color_success(name, value): + assert m.return_color(value) == m.color[name] + + +def test_return_color_fail(): + with pytest.raises(ValueError) as excinfo_direct: + m.color(2) + with pytest.raises(ValueError) as excinfo_cast: + m.return_color(2) + assert str(excinfo_cast.value) == str(excinfo_direct.value) From 2da6779d1766a016db94d34f2061f8b1b9088362 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Nov 2022 14:42:16 -0800 Subject: [PATCH 02/35] Add pybind11/native_enum.h, building the native enum type. Not used by any type_caster yet. --- include/pybind11/detail/internals.h | 5 +- include/pybind11/native_enum.h | 67 ++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/extra_python_package/test_files.py | 1 + tests/test_native_enum.cpp | 8 ++- tests/test_native_enum.py | 6 +++ 6 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 include/pybind11/native_enum.h diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 6fd61098c4..d6cfa20b54 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -34,7 +34,7 @@ /// further ABI-incompatible changes may be made before the ABI is officially /// changed to the new version. #ifndef PYBIND11_INTERNALS_VERSION -# define PYBIND11_INTERNALS_VERSION 4 +# define PYBIND11_INTERNALS_VERSION 5 #endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -209,6 +209,9 @@ struct internals { PYBIND11_TLS_FREE(tstate); } #endif +#if PYBIND11_INTERNALS_VERSION > 4 + type_map native_enum_types; +#endif }; /// Additional type information which does not fit into the PyTypeObject. diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h new file mode 100644 index 0000000000..56091a4945 --- /dev/null +++ b/include/pybind11/native_enum.h @@ -0,0 +1,67 @@ +// Copyright (c) 2022 The pybind Community. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "pybind11.h" + +#include + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) + +/// Conversions between Python's native (stdlib) enum types and C++ enums. +template +class native_enum { +public: + using Underlying = typename std::underlying_type::type; + // Scalar is the integer representation of underlying type + using Scalar = detail::conditional_t, + std::is_same>::value, + detail::equivalent_integer_t, + Underlying>; + + template + native_enum(const handle &scope, const char *name, const Extra &.../*extra*/) + : m_scope(reinterpret_borrow(scope)), m_name(name) { + constexpr bool is_arithmetic = detail::any_of...>::value; + constexpr bool is_convertible = std::is_convertible::value; + if (is_arithmetic || is_convertible) { + // IGNORED. + } + } + + /// Export enumeration entries into the parent scope + native_enum &export_values() { return *this; } + + /// Add an enumeration entry + native_enum &value(char const *name, Type value, const char *doc = nullptr) { + if (doc) { + // IGNORED. + } + m_members.append(make_tuple(name, static_cast(value))); + return *this; + } + + native_enum(const native_enum &) = delete; + native_enum &operator=(const native_enum &) = delete; + + ~native_enum() { + // Any exception here will terminate the process. + auto enum_module = module_::import("enum"); + auto int_enum = enum_module.attr("IntEnum"); + auto int_enum_color = int_enum(m_name, m_members); + int_enum_color.attr("__module__") = m_scope; + m_scope.attr(m_name) = int_enum_color; + // Intentionally leak Python reference. + detail::get_internals().native_enum_types[std::type_index(typeid(Type))] + = int_enum_color.release().ptr(); + } + +private: + object m_scope; + str m_name; + list m_members; +}; + +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 491f215cef..1e92d8b353 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -142,6 +142,7 @@ set(PYBIND11_TEST_FILES test_methods_and_attributes test_modules test_multiple_inheritance + test_native_enum test_numpy_array test_numpy_dtypes test_numpy_vectorize diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 9a9bb1556a..00ba7aaa07 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -36,6 +36,7 @@ "include/pybind11/functional.h", "include/pybind11/gil.h", "include/pybind11/iostream.h", + "include/pybind11/native_enum.h", "include/pybind11/numpy.h", "include/pybind11/operators.h", "include/pybind11/options.h", diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 0b37a3092d..4f49141709 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -1,4 +1,4 @@ -// #include +#include #include "pybind11_tests.h" @@ -77,4 +77,10 @@ TEST_SUBMODULE(native_enum, m) { m.def("pass_color", pass_color); m.def("return_color", return_color); + + py::native_enum(m, "WIPcolor") + .value("red", color::red) + .value("yellow", color::yellow) + .value("green", color::green) + .value("blue", color::blue); } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 85aa5937e6..6334048b78 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -43,3 +43,9 @@ def test_return_color_fail(): with pytest.raises(ValueError) as excinfo_cast: m.return_color(2) assert str(excinfo_cast.value) == str(excinfo_direct.value) + + +def test_wip_color(): + assert isinstance(m.WIPcolor, enum.EnumMeta) + for name, value in COLOR_MEMBERS: + assert m.WIPcolor[name] == value From 13247691334c622764ef919f75dc11088abe0c22 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Nov 2022 18:43:52 -0800 Subject: [PATCH 03/35] Remove unused code entirely, to not trigger MSVC C4127 with the placeholder code. --- include/pybind11/native_enum.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 56091a4945..945e2037a7 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -23,13 +23,7 @@ class native_enum { template native_enum(const handle &scope, const char *name, const Extra &.../*extra*/) - : m_scope(reinterpret_borrow(scope)), m_name(name) { - constexpr bool is_arithmetic = detail::any_of...>::value; - constexpr bool is_convertible = std::is_convertible::value; - if (is_arithmetic || is_convertible) { - // IGNORED. - } - } + : m_scope(reinterpret_borrow(scope)), m_name(name) {} /// Export enumeration entries into the parent scope native_enum &export_values() { return *this; } From 4ab579ec107e74e4bddb1dc7a95c3f8d5c792b69 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Nov 2022 22:35:18 -0800 Subject: [PATCH 04/35] Insert `type_caster`, simply forwarding to `type_caster_base` in this development iteration. --- include/pybind11/cast.h | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 430c62f357..ac99f09b6e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -48,6 +48,41 @@ cast_op(make_caster &&caster) { template cast_op_type::type>(); } +template +class type_caster::value>> { +public: + static constexpr auto name = const_name(); + + template + static handle cast(SrcType &&src, return_value_policy, handle parent) { + return type_caster_base::cast( + std::forward(src), + // Fixes https://github.com/pybind/pybind11/pull/3643#issuecomment-1022987818: + return_value_policy::copy, + parent); + } + + bool load(handle src, bool convert) { + if (!pybind11_enum_) { + pybind11_enum_.reset(new type_caster_base()); + } + return pybind11_enum_->load(src, convert); + } + + template + using cast_op_type = detail::cast_op_type; + + // NOLINTNEXTLINE(google-explicit-constructor) + operator EnumType *() { return pybind11_enum_->operator EnumType *(); } + + // NOLINTNEXTLINE(google-explicit-constructor) + operator EnumType &() { return pybind11_enum_->operator EnumType &(); } + +private: + std::unique_ptr> pybind11_enum_; + EnumType value; +}; + template class type_caster> { private: From 4cb7da06657d00b9819305de7176990c5be70e0c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Nov 2022 23:39:52 -0800 Subject: [PATCH 05/35] Fit `native_enum` functionality into `type_caster` --- include/pybind11/cast.h | 35 ++++++++++++++++++++++-- tests/test_native_enum.cpp | 54 +++----------------------------------- tests/test_native_enum.py | 8 +----- 3 files changed, 38 insertions(+), 59 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index ac99f09b6e..c140168560 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -50,11 +50,19 @@ cast_op(make_caster &&caster) { template class type_caster::value>> { +private: + using Underlying = typename std::underlying_type::type; + public: static constexpr auto name = const_name(); template static handle cast(SrcType &&src, return_value_policy, handle parent) { + auto const &natives = get_internals().native_enum_types; + auto found = natives.find(std::type_index(typeid(EnumType))); + if (found != natives.end()) { + return handle(found->second)(static_cast(src)).release(); + } return type_caster_base::cast( std::forward(src), // Fixes https://github.com/pybind/pybind11/pull/3643#issuecomment-1022987818: @@ -63,6 +71,19 @@ class type_caster::value>> } bool load(handle src, bool convert) { + auto const &natives = get_internals().native_enum_types; + auto found = natives.find(std::type_index(typeid(EnumType))); + if (found != natives.end()) { + if (!isinstance(src, found->second)) { + return false; + } + type_caster underlying_caster; + if (!underlying_caster.load(src.attr("value"), convert)) { + pybind11_fail("native_enum internal consistency failure."); + } + value = static_cast(static_cast(underlying_caster)); + return true; + } if (!pybind11_enum_) { pybind11_enum_.reset(new type_caster_base()); } @@ -73,10 +94,20 @@ class type_caster::value>> using cast_op_type = detail::cast_op_type; // NOLINTNEXTLINE(google-explicit-constructor) - operator EnumType *() { return pybind11_enum_->operator EnumType *(); } + operator EnumType *() { + if (!pybind11_enum_) { + return &value; + } + return pybind11_enum_->operator EnumType *(); + } // NOLINTNEXTLINE(google-explicit-constructor) - operator EnumType &() { return pybind11_enum_->operator EnumType &(); } + operator EnumType &() { + if (!pybind11_enum_) { + return value; + } + return pybind11_enum_->operator EnumType &(); + } private: std::unique_ptr> pybind11_enum_; diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 4f49141709..54701917d8 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -24,63 +24,17 @@ enum { d, e, f = e + 2 }; int pass_color(color e) { return static_cast(e); } color return_color(int i) { return static_cast(i); } -py::handle wrap_color(py::module_ m) { - auto enum_module = py::module_::import("enum"); - auto int_enum = enum_module.attr("IntEnum"); - using u_t = std::underlying_type::type; - auto members = py::make_tuple(py::make_tuple("red", static_cast(color::red)), - py::make_tuple("yellow", static_cast(color::yellow)), - py::make_tuple("green", static_cast(color::green)), - py::make_tuple("blue", static_cast(color::blue))); - auto int_enum_color = int_enum("color", members); - int_enum_color.attr("__module__") = m; - m.attr("color") = int_enum_color; - return int_enum_color.release(); // Intentionally leak Python reference. -} - } // namespace test_native_enum -PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) -PYBIND11_NAMESPACE_BEGIN(detail) - -using namespace test_native_enum; - -template <> -struct type_caster { - static handle native_type; - - static handle cast(const color &src, return_value_policy /* policy */, handle /* parent */) { - auto u_v = static_cast::type>(src); - return native_type(u_v).release(); - } - - bool load(handle src, bool /* convert */) { - if (!isinstance(src, native_type)) { - return false; - } - value = static_cast(py::cast::type>(src.attr("value"))); - return true; - } - - PYBIND11_TYPE_CASTER(color, const_name("")); -}; - -handle type_caster::native_type = nullptr; - -PYBIND11_NAMESPACE_END(detail) -PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) - TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; - py::detail::type_caster::native_type = wrap_color(m); - - m.def("pass_color", pass_color); - m.def("return_color", return_color); - - py::native_enum(m, "WIPcolor") + py::native_enum(m, "color") .value("red", color::red) .value("yellow", color::yellow) .value("green", color::green) .value("blue", color::blue); + + m.def("pass_color", pass_color); + m.def("return_color", return_color); } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 6334048b78..9bece65cfd 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -29,7 +29,7 @@ def test_pass_color_success(name, value): def test_pass_color_fail(): with pytest.raises(TypeError) as excinfo: m.pass_color(None) - assert "" in str(excinfo.value) + assert "test_native_enum::color" in str(excinfo.value) @pytest.mark.parametrize("name,value", COLOR_MEMBERS) @@ -43,9 +43,3 @@ def test_return_color_fail(): with pytest.raises(ValueError) as excinfo_cast: m.return_color(2) assert str(excinfo_cast.value) == str(excinfo_direct.value) - - -def test_wip_color(): - assert isinstance(m.WIPcolor, enum.EnumMeta) - for name, value in COLOR_MEMBERS: - assert m.WIPcolor[name] == value From 03ed301397003a00674a694af8cb509476180fb0 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 13 Nov 2022 15:53:40 -0800 Subject: [PATCH 06/35] Add `type_caster_enum_type_enabled` with test. --- include/pybind11/cast.h | 7 +++++- tests/test_native_enum.cpp | 46 ++++++++++++++++++++++++++++++++++---- tests/test_native_enum.py | 6 +++++ 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c140168560..1c87ed90f3 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -48,8 +48,13 @@ cast_op(make_caster &&caster) { template cast_op_type::type>(); } +template +struct type_caster_enum_type_enabled : std::true_type {}; + template -class type_caster::value>> { +class type_caster::value + && type_caster_enum_type_enabled::value>> { private: using Underlying = typename std::underlying_type::type; diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 54701917d8..bec1877b61 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -21,11 +21,46 @@ enum class altitude : char { // the constant d is 0, the constant e is 1, the constant f is 3 enum { d, e, f = e + 2 }; -int pass_color(color e) { return static_cast(e); } -color return_color(int i) { return static_cast(i); } +// https://github.com/protocolbuffers/protobuf/blob/d70b5c5156858132decfdbae0a1103e6a5cb1345/src/google/protobuf/generated_enum_util.h#L52-L53 +template +struct is_proto_enum : std::false_type {}; + +enum some_proto_enum : int { Zero, One }; + +template <> +struct is_proto_enum : std::true_type {}; } // namespace test_native_enum +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +template +struct type_caster_enum_type_enabled< + ProtoEnumType, + detail::enable_if_t::value>> : std::false_type { +}; + +// https://github.com/pybind/pybind11_protobuf/blob/a50899c2eb604fc5f25deeb8901eff6231b8b3c0/pybind11_protobuf/enum_type_caster.h#L101-L105 +template +struct type_caster::value>> { + static handle + cast(const ProtoEnumType & /*src*/, return_value_policy /*policy*/, handle /*parent*/) { + return py::none(); + } + + bool load(handle /*src*/, bool /*convert*/) { + value = static_cast(0); + return true; + } + + PYBIND11_TYPE_CASTER(ProtoEnumType, const_name()); +}; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) + TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; @@ -35,6 +70,9 @@ TEST_SUBMODULE(native_enum, m) { .value("green", color::green) .value("blue", color::blue); - m.def("pass_color", pass_color); - m.def("return_color", return_color); + m.def("pass_color", [](color e) { return static_cast(e); }); + m.def("return_color", [](int i) { return static_cast(i); }); + + m.def("pass_some_proto_enum", [](some_proto_enum) { return py::none(); }); + m.def("return_some_proto_enum", []() { return some_proto_enum::Zero; }); } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 9bece65cfd..28636f837f 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -43,3 +43,9 @@ def test_return_color_fail(): with pytest.raises(ValueError) as excinfo_cast: m.return_color(2) assert str(excinfo_cast.value) == str(excinfo_direct.value) + + +def test_type_caster_enum_type_enabled_false(): + # This is really only a "does it compile" test. + assert m.pass_some_proto_enum(None) is None + assert m.return_some_proto_enum() is None From 2c9d5a027cd33b2c982ceb671dd66613faa5955a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 13 Nov 2022 20:38:23 -0800 Subject: [PATCH 07/35] Additional tests based on global testing failures: `test_pybind11_isinstance_color`, `test_obj_cast_color` --- tests/test_native_enum.cpp | 18 ++++++++++++++++++ tests/test_native_enum.py | 13 +++++++++++++ 2 files changed, 31 insertions(+) diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index bec1877b61..e8ecb214a6 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -70,9 +70,27 @@ TEST_SUBMODULE(native_enum, m) { .value("green", color::green) .value("blue", color::blue); + m.def("isinstance_color", [](const py::object &obj) { return py::isinstance(obj); }); + m.def("pass_color", [](color e) { return static_cast(e); }); m.def("return_color", [](int i) { return static_cast(i); }); m.def("pass_some_proto_enum", [](some_proto_enum) { return py::none(); }); m.def("return_some_proto_enum", []() { return some_proto_enum::Zero; }); + + m.def("obj_cast_color", [](const py::object &obj) { + if ( + // GOOD: + obj.cast() + // ERROR static_cast(!cast_is_temporary_value_reference): + // *obj.cast + // GOOD: + // py::cast(obj) + // ERROR static_cast(!cast_is_temporary_value_reference): + // *py::cast(obj) + == color::green) { + return 1; + } + return 0; + }); } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 28636f837f..a17197e863 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -16,6 +16,11 @@ def test_enum_color_type(): assert isinstance(m.color, enum.EnumMeta) +def test_pybind11_isinstance_color(): + assert not m.isinstance_color(None) + assert not m.isinstance_color(m.color) # TODO: NEEDS FIXING + + @pytest.mark.parametrize("name,value", COLOR_MEMBERS) def test_enum_color_members(name, value): assert m.color[name] == value @@ -49,3 +54,11 @@ def test_type_caster_enum_type_enabled_false(): # This is really only a "does it compile" test. assert m.pass_some_proto_enum(None) is None assert m.return_some_proto_enum() is None + + +def test_obj_cast_color(): + assert m.obj_cast_color(m.color.green) == 1 + assert m.obj_cast_color(m.color.blue) == 0 + with pytest.raises(RuntimeError) as excinfo: + m.obj_cast_color(None) + assert str(excinfo.value).startswith("Unable to cast Python instance of type ") From 70848263372174d9a2755651a62021b85fc2837d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 13 Nov 2022 20:58:20 -0800 Subject: [PATCH 08/35] Add `py::native_enum` --- tests/test_native_enum.cpp | 8 +++++--- tests/test_native_enum.py | 20 +++++++++++++++----- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index e8ecb214a6..2c062557b6 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -18,9 +18,6 @@ enum class altitude : char { low = 'l', // trailing comma only allowed after CWG518 }; -// the constant d is 0, the constant e is 1, the constant f is 3 -enum { d, e, f = e + 2 }; - // https://github.com/protocolbuffers/protobuf/blob/d70b5c5156858132decfdbae0a1103e6a5cb1345/src/google/protobuf/generated_enum_util.h#L52-L53 template struct is_proto_enum : std::false_type {}; @@ -64,6 +61,11 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; + py::native_enum(m, "smallenum") + .value("a", smallenum::a) + .value("b", smallenum::b) + .value("c", smallenum::c); + py::native_enum(m, "color") .value("red", color::red) .value("yellow", color::yellow) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index a17197e863..76510485cc 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -4,6 +4,12 @@ from pybind11_tests import native_enum as m +SMALLENUM_MEMBERS = ( + ("a", 0), + ("b", 1), + ("c", 2), +) + COLOR_MEMBERS = ( ("red", 0), ("yellow", 1), @@ -12,8 +18,9 @@ ) -def test_enum_color_type(): - assert isinstance(m.color, enum.EnumMeta) +@pytest.mark.parametrize("enum_type", (m.smallenum, m.color)) +def test_enum_color_type(enum_type): + assert isinstance(enum_type, enum.EnumMeta) def test_pybind11_isinstance_color(): @@ -21,9 +28,12 @@ def test_pybind11_isinstance_color(): assert not m.isinstance_color(m.color) # TODO: NEEDS FIXING -@pytest.mark.parametrize("name,value", COLOR_MEMBERS) -def test_enum_color_members(name, value): - assert m.color[name] == value +@pytest.mark.parametrize( + "enum_type,members", ((m.smallenum, SMALLENUM_MEMBERS), (m.color, COLOR_MEMBERS)) +) +def test_enum_color_members(enum_type, members): + for name, value in members: + assert enum_type[name] == value @pytest.mark.parametrize("name,value", COLOR_MEMBERS) From 1a1112c7b2342f6f95ee8279741c200f7e52a7d0 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 13 Nov 2022 21:09:09 -0800 Subject: [PATCH 09/35] Shorten expected "Unable to cast" message for compatibility with non-debug builds. --- tests/test_native_enum.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 76510485cc..084b08816f 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -71,4 +71,4 @@ def test_obj_cast_color(): assert m.obj_cast_color(m.color.blue) == 0 with pytest.raises(RuntimeError) as excinfo: m.obj_cast_color(None) - assert str(excinfo.value).startswith("Unable to cast Python instance of type ") + assert str(excinfo.value).startswith("Unable to cast Python instance ") From 0a49b5b2cc0343d8e0c3ab7d9a1161f1fca14adb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 15:45:34 -0800 Subject: [PATCH 10/35] Update `cast_is_temporary_value_reference` to exclude `type_caster_enum_type` Also introducing `EnumType *value_ptr`, although that's not critical to fixing the `*obj.cast()` use case. --- include/pybind11/cast.h | 47 +++++++++++++++++++------------------- tests/test_native_enum.cpp | 12 ++-------- 2 files changed, 26 insertions(+), 33 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1c87ed90f3..76703e9782 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -48,13 +48,8 @@ cast_op(make_caster &&caster) { template cast_op_type::type>(); } -template -struct type_caster_enum_type_enabled : std::true_type {}; - template -class type_caster::value - && type_caster_enum_type_enabled::value>> { +class type_caster_enum_type { private: using Underlying = typename std::underlying_type::type; @@ -87,38 +82,43 @@ class type_caster(static_cast(underlying_caster)); + value_ptr = &value; return true; } if (!pybind11_enum_) { pybind11_enum_.reset(new type_caster_base()); } - return pybind11_enum_->load(src, convert); + if (pybind11_enum_->load(src, convert)) { + value_ptr = pybind11_enum_->operator EnumType *(); + return true; + } + return false; } template using cast_op_type = detail::cast_op_type; // NOLINTNEXTLINE(google-explicit-constructor) - operator EnumType *() { - if (!pybind11_enum_) { - return &value; - } - return pybind11_enum_->operator EnumType *(); - } + operator EnumType *() { return value_ptr; } // NOLINTNEXTLINE(google-explicit-constructor) - operator EnumType &() { - if (!pybind11_enum_) { - return value; - } - return pybind11_enum_->operator EnumType &(); - } + operator EnumType &() { return *value_ptr; } private: std::unique_ptr> pybind11_enum_; EnumType value; + EnumType *value_ptr = nullptr; }; +template +struct type_caster_enum_type_enabled : std::true_type {}; + +template +class type_caster::value + && type_caster_enum_type_enabled::value>> + : public type_caster_enum_type {}; + template class type_caster> { private: @@ -1053,10 +1053,11 @@ using move_never = none_of, move_if_unreferenced>; // non-reference/pointer `type`s and reference/pointers from a type_caster_generic are safe; // everything else returns a reference/pointer to a local variable. template -using cast_is_temporary_value_reference - = bool_constant<(std::is_reference::value || std::is_pointer::value) - && !std::is_base_of>::value - && !std::is_same, void>::value>; +using cast_is_temporary_value_reference = bool_constant< + (std::is_reference::value || std::is_pointer::value) + && !std::is_base_of>::value + && !std::is_base_of>, make_caster>::value + && !std::is_same, void>::value>; // When a value returned from a C++ function is being cast back to Python, we almost always want to // force `policy = move`, regardless of the return value policy the function/method was declared diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 2c062557b6..8d67ca28ca 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -81,16 +81,8 @@ TEST_SUBMODULE(native_enum, m) { m.def("return_some_proto_enum", []() { return some_proto_enum::Zero; }); m.def("obj_cast_color", [](const py::object &obj) { - if ( - // GOOD: - obj.cast() - // ERROR static_cast(!cast_is_temporary_value_reference): - // *obj.cast - // GOOD: - // py::cast(obj) - // ERROR static_cast(!cast_is_temporary_value_reference): - // *py::cast(obj) - == color::green) { + // https://github.com/OpenImageIO/oiio/blob/30ea4ebdfab11aec291befbaff446f2a7d24835b/src/python/py_oiio.h#L300 + if (*obj.cast() == color::green) { return 1; } return 0; From fdbd5606f68a6b29e5f44fd3e9fa6d419a41a90b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 17:48:31 -0800 Subject: [PATCH 11/35] Move cast ptr test from test_native_enum to test_enum, undo change to cast_is_temporary_value_reference, replace static check in cast<> with runtime check --- include/pybind11/cast.h | 47 +++++++++++++++++++++++++------------- tests/test_enum.cpp | 11 +++++++++ tests/test_enum.py | 6 +++++ tests/test_native_enum.cpp | 8 ------- tests/test_native_enum.py | 8 ------- 5 files changed, 48 insertions(+), 32 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 76703e9782..5359ef319f 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -82,32 +82,36 @@ class type_caster_enum_type { pybind11_fail("native_enum internal consistency failure."); } value = static_cast(static_cast(underlying_caster)); - value_ptr = &value; return true; } if (!pybind11_enum_) { pybind11_enum_.reset(new type_caster_base()); } - if (pybind11_enum_->load(src, convert)) { - value_ptr = pybind11_enum_->operator EnumType *(); - return true; - } - return false; + return pybind11_enum_->load(src, convert); } template using cast_op_type = detail::cast_op_type; // NOLINTNEXTLINE(google-explicit-constructor) - operator EnumType *() { return value_ptr; } + operator EnumType *() { + if (!pybind11_enum_) { + return &value; + } + return pybind11_enum_->operator EnumType *(); + } // NOLINTNEXTLINE(google-explicit-constructor) - operator EnumType &() { return *value_ptr; } + operator EnumType &() { + if (!pybind11_enum_) { + return value; + } + return pybind11_enum_->operator EnumType &(); + } private: std::unique_ptr> pybind11_enum_; EnumType value; - EnumType *value_ptr = nullptr; }; template @@ -1053,11 +1057,10 @@ using move_never = none_of, move_if_unreferenced>; // non-reference/pointer `type`s and reference/pointers from a type_caster_generic are safe; // everything else returns a reference/pointer to a local variable. template -using cast_is_temporary_value_reference = bool_constant< - (std::is_reference::value || std::is_pointer::value) - && !std::is_base_of>::value - && !std::is_base_of>, make_caster>::value - && !std::is_same, void>::value>; +using cast_is_temporary_value_reference + = bool_constant<(std::is_reference::value || std::is_pointer::value) + && !std::is_base_of>::value + && !std::is_same, void>::value>; // When a value returned from a C++ function is being cast back to Python, we almost always want to // force `policy = move`, regardless of the return value policy the function/method was declared @@ -1109,9 +1112,21 @@ PYBIND11_NAMESPACE_END(detail) template ::value, int> = 0> T cast(const handle &handle) { using namespace detail; - static_assert(!cast_is_temporary_value_reference::value, + constexpr bool is_enum_cast + = std::is_base_of>, make_caster>::value; + static_assert(!cast_is_temporary_value_reference::value || is_enum_cast, "Unable to cast type to reference: value is local to type caster"); - return cast_op(load_type(handle)); + auto lt = load_type(handle); +#ifndef NDEBUG + if (is_enum_cast) { + auto const &natives = get_internals().native_enum_types; + auto found = natives.find(std::type_index(typeid(intrinsic_type))); + if (found != natives.end()) { + pybind11_fail("Unable to cast native enum type to reference"); + } + } +#endif + return cast_op(std::move(lt)); } // pytype -> pytype (calls converting constructor) diff --git a/tests/test_enum.cpp b/tests/test_enum.cpp index 2597b275ef..b980229fae 100644 --- a/tests/test_enum.cpp +++ b/tests/test_enum.cpp @@ -130,4 +130,15 @@ TEST_SUBMODULE(enums, m) { py::enum_(m, "ScopedBoolEnum") .value("FALSE", ScopedBoolEnum::FALSE) .value("TRUE", ScopedBoolEnum::TRUE); + + m.def("obj_cast_UnscopedEnum_ptr", [](const py::object &obj) { + // https://github.com/OpenImageIO/oiio/blob/30ea4ebdfab11aec291befbaff446f2a7d24835b/src/python/py_oiio.h#L300 + if (py::isinstance(obj)) { + if (*obj.cast() == UnscopedEnum::ETwo) { + return 2; + } + return 1; + } + return 0; + }); } diff --git a/tests/test_enum.py b/tests/test_enum.py index f14a72398f..cbf72c99e9 100644 --- a/tests/test_enum.py +++ b/tests/test_enum.py @@ -262,3 +262,9 @@ def test_docstring_signatures(): for attr in enum_type.__dict__.values(): # Issue #2623/PR #2637: Add argument names to enum_ methods assert "arg0" not in (attr.__doc__ or "") + + +def test_obj_cast_unscoped_enum_ptr(): + assert m.obj_cast_UnscopedEnum_ptr(m.UnscopedEnum.ETwo) == 2 + assert m.obj_cast_UnscopedEnum_ptr(m.UnscopedEnum.EOne) == 1 + assert m.obj_cast_UnscopedEnum_ptr(None) == 0 diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 8d67ca28ca..bec760c85c 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -79,12 +79,4 @@ TEST_SUBMODULE(native_enum, m) { m.def("pass_some_proto_enum", [](some_proto_enum) { return py::none(); }); m.def("return_some_proto_enum", []() { return some_proto_enum::Zero; }); - - m.def("obj_cast_color", [](const py::object &obj) { - // https://github.com/OpenImageIO/oiio/blob/30ea4ebdfab11aec291befbaff446f2a7d24835b/src/python/py_oiio.h#L300 - if (*obj.cast() == color::green) { - return 1; - } - return 0; - }); } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 084b08816f..12cfd3b54a 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -64,11 +64,3 @@ def test_type_caster_enum_type_enabled_false(): # This is really only a "does it compile" test. assert m.pass_some_proto_enum(None) is None assert m.return_some_proto_enum() is None - - -def test_obj_cast_color(): - assert m.obj_cast_color(m.color.green) == 1 - assert m.obj_cast_color(m.color.blue) == 0 - with pytest.raises(RuntimeError) as excinfo: - m.obj_cast_color(None) - assert str(excinfo.value).startswith("Unable to cast Python instance ") From 1cec429cc9b8726e1494a89437897c8c0574f67e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 18:20:26 -0800 Subject: [PATCH 12/35] Fix oversight: forgot to undo unneeded change. --- include/pybind11/cast.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 5359ef319f..2028f32373 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1116,7 +1116,6 @@ T cast(const handle &handle) { = std::is_base_of>, make_caster>::value; static_assert(!cast_is_temporary_value_reference::value || is_enum_cast, "Unable to cast type to reference: value is local to type caster"); - auto lt = load_type(handle); #ifndef NDEBUG if (is_enum_cast) { auto const &natives = get_internals().native_enum_types; @@ -1126,7 +1125,7 @@ T cast(const handle &handle) { } } #endif - return cast_op(std::move(lt)); + return cast_op(load_type(handle)); } // pytype -> pytype (calls converting constructor) From 75bb57651dfcc27a010ffa26a5ecaac59e7b6aa3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 18:49:04 -0800 Subject: [PATCH 13/35] Bug fix and test for "Unable to cast native enum type to reference" error. --- include/pybind11/cast.h | 2 +- tests/test_native_enum.cpp | 6 ++++++ tests/test_native_enum.py | 9 +++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 2028f32373..a5a4b8af14 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1119,7 +1119,7 @@ T cast(const handle &handle) { #ifndef NDEBUG if (is_enum_cast) { auto const &natives = get_internals().native_enum_types; - auto found = natives.find(std::type_index(typeid(intrinsic_type))); + auto found = natives.find(std::type_index(typeid(intrinsic_t))); if (found != natives.end()) { pybind11_fail("Unable to cast native enum type to reference"); } diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index bec760c85c..443d178568 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -79,4 +79,10 @@ TEST_SUBMODULE(native_enum, m) { m.def("pass_some_proto_enum", [](some_proto_enum) { return py::none(); }); m.def("return_some_proto_enum", []() { return some_proto_enum::Zero; }); + +#ifndef NDEBUG + m.def("obj_cast_color_ptr", [](const py::object &obj) { obj.cast(); }); +#else + m.attr("obj_cast_color_ptr") = py::none(); +#endif } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 12cfd3b54a..7af84bbc77 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -64,3 +64,12 @@ def test_type_caster_enum_type_enabled_false(): # This is really only a "does it compile" test. assert m.pass_some_proto_enum(None) is None assert m.return_some_proto_enum() is None + + +@pytest.mark.skipif( + m.obj_cast_color_ptr is None, reason="NDEBUG disables cast safety check" +) +def test_obj_cast_color_ptr(): + with pytest.raises(RuntimeError) as excinfo: + m.obj_cast_color_ptr(m.color.red) + assert str(excinfo.value) == "Unable to cast native enum type to reference" From 1f3d94d17facba2add1f0496d9a7cd1cc5b6f2d2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 20:55:08 -0800 Subject: [PATCH 14/35] Add `isinstance_native_enum()` and plug into `isinstance(handle)` --- include/pybind11/cast.h | 20 ++++++++++++++++++++ include/pybind11/pytypes.h | 6 +++++- tests/test_native_enum.py | 7 ++++++- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index a5a4b8af14..c23a290c54 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -123,6 +123,26 @@ class type_caster::value>> : public type_caster_enum_type {}; +template ::value, int> = 0> +bool isinstance_native_enum_impl(handle obj, const std::type_info &tp) { + auto const &natives = get_internals().native_enum_types; + auto found = natives.find(tp); + if (found == natives.end()) { + return false; + } + return isinstance(obj, found->second); +} + +template ::value, int> = 0> +bool isinstance_native_enum_impl(handle, const std::type_info &) { + return false; +} + +template +bool isinstance_native_enum(handle obj, const std::type_info &tp) { + return isinstance_native_enum_impl>(obj, tp); +} + template class type_caster> { private: diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 80b49ec397..1023358bdb 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -46,6 +46,9 @@ PYBIND11_NAMESPACE_BEGIN(detail) class args_proxy; bool isinstance_generic(handle obj, const std::type_info &tp); +template +bool isinstance_native_enum(handle obj, const std::type_info &tp); + // Accessor forward declarations template class accessor; @@ -736,7 +739,8 @@ bool isinstance(handle obj) { template ::value, int> = 0> bool isinstance(handle obj) { - return detail::isinstance_generic(obj, typeid(T)); + return detail::isinstance_native_enum(obj, typeid(T)) + || detail::isinstance_generic(obj, typeid(T)); } template <> diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 7af84bbc77..1760b4e8e1 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -24,8 +24,13 @@ def test_enum_color_type(enum_type): def test_pybind11_isinstance_color(): + for name, _ in COLOR_MEMBERS: + assert m.isinstance_color(m.color[name]) + assert not m.isinstance_color(m.color) + for name, _ in SMALLENUM_MEMBERS: + assert not m.isinstance_color(m.smallenum[name]) + assert not m.isinstance_color(m.smallenum) assert not m.isinstance_color(None) - assert not m.isinstance_color(m.color) # TODO: NEEDS FIXING @pytest.mark.parametrize( From b478a55c260d735fad603094dd5fb75ea77770c5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 21:36:34 -0800 Subject: [PATCH 15/35] Make test_native_enum.py output less noisy. --- tests/test_native_enum.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 1760b4e8e1..d2818dc8fe 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -41,9 +41,9 @@ def test_enum_color_members(enum_type, members): assert enum_type[name] == value -@pytest.mark.parametrize("name,value", COLOR_MEMBERS) -def test_pass_color_success(name, value): - assert m.pass_color(m.color[name]) == value +def test_pass_color_success(): + for name, value in COLOR_MEMBERS: + assert m.pass_color(m.color[name]) == value def test_pass_color_fail(): @@ -52,9 +52,9 @@ def test_pass_color_fail(): assert "test_native_enum::color" in str(excinfo.value) -@pytest.mark.parametrize("name,value", COLOR_MEMBERS) -def test_return_color_success(name, value): - assert m.return_color(value) == m.color[name] +def test_return_color_success(): + for name, value in COLOR_MEMBERS: + assert m.return_color(value) == m.color[name] def test_return_color_fail(): From ae953a78f06e023b51b686f4cce51b620612ce89 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 21:46:55 -0800 Subject: [PATCH 16/35] Add `enum.Enum` support, for non-integer underlying types. --- include/pybind11/native_enum.h | 16 ++++++---------- tests/test_native_enum.cpp | 4 ++++ tests/test_native_enum.py | 28 +++++++++++++++++++--------- 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 945e2037a7..0fd592c50e 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -15,15 +15,8 @@ template class native_enum { public: using Underlying = typename std::underlying_type::type; - // Scalar is the integer representation of underlying type - using Scalar = detail::conditional_t, - std::is_same>::value, - detail::equivalent_integer_t, - Underlying>; - template - native_enum(const handle &scope, const char *name, const Extra &.../*extra*/) - : m_scope(reinterpret_borrow(scope)), m_name(name) {} + native_enum(const object &scope, const char *name) : m_scope(scope), m_name(name) {} /// Export enumeration entries into the parent scope native_enum &export_values() { return *this; } @@ -33,7 +26,7 @@ class native_enum { if (doc) { // IGNORED. } - m_members.append(make_tuple(name, static_cast(value))); + m_members.append(make_tuple(name, static_cast(value))); return *this; } @@ -43,7 +36,10 @@ class native_enum { ~native_enum() { // Any exception here will terminate the process. auto enum_module = module_::import("enum"); - auto int_enum = enum_module.attr("IntEnum"); + constexpr bool use_int_enum = std::numeric_limits::is_integer + && !std::is_same::value + && !detail::is_std_char_type::value; + auto int_enum = enum_module.attr(use_int_enum ? "IntEnum" : "Enum"); auto int_enum_color = int_enum(m_name, m_members); int_enum_color.attr("__module__") = m_scope; m_scope.attr(m_name) = int_enum_color; diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 443d178568..bde49a9144 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -72,6 +72,10 @@ TEST_SUBMODULE(native_enum, m) { .value("green", color::green) .value("blue", color::blue); + py::native_enum(m, "altitude") + .value("high", altitude::high) + .value("low", altitude::low); + m.def("isinstance_color", [](const py::object &obj) { return py::isinstance(obj); }); m.def("pass_color", [](color e) { return static_cast(e); }); diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index d2818dc8fe..b40c36063d 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -17,12 +17,30 @@ ("blue", 21), ) +ALTITUDE_MEMBERS = ( + ("high", "h"), + ("low", "l"), +) + -@pytest.mark.parametrize("enum_type", (m.smallenum, m.color)) +@pytest.mark.parametrize("enum_type", (m.smallenum, m.color, m.altitude)) def test_enum_color_type(enum_type): assert isinstance(enum_type, enum.EnumMeta) +@pytest.mark.parametrize( + "enum_type,members", + ( + (m.smallenum, SMALLENUM_MEMBERS), + (m.color, COLOR_MEMBERS), + (m.altitude, ALTITUDE_MEMBERS), + ), +) +def test_enum_color_members(enum_type, members): + for name, value in members: + assert enum_type[name].value == value + + def test_pybind11_isinstance_color(): for name, _ in COLOR_MEMBERS: assert m.isinstance_color(m.color[name]) @@ -33,14 +51,6 @@ def test_pybind11_isinstance_color(): assert not m.isinstance_color(None) -@pytest.mark.parametrize( - "enum_type,members", ((m.smallenum, SMALLENUM_MEMBERS), (m.color, COLOR_MEMBERS)) -) -def test_enum_color_members(enum_type, members): - for name, value in members: - assert enum_type[name] == value - - def test_pass_color_success(): for name, value in COLOR_MEMBERS: assert m.pass_color(m.color[name]) == value From 90044fb9da3a2897f00adbdd9f946d42363897e7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 22:51:07 -0800 Subject: [PATCH 17/35] Disable `test_obj_cast_unscoped_enum_ptr` & `test_obj_cast_color_ptr` under MinGW --- tests/test_enum.cpp | 5 +++++ tests/test_enum.py | 3 +++ tests/test_native_enum.cpp | 9 ++++++--- tests/test_native_enum.py | 4 +--- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/test_enum.cpp b/tests/test_enum.cpp index b980229fae..4ec0af7245 100644 --- a/tests/test_enum.cpp +++ b/tests/test_enum.cpp @@ -131,6 +131,10 @@ TEST_SUBMODULE(enums, m) { .value("FALSE", ScopedBoolEnum::FALSE) .value("TRUE", ScopedBoolEnum::TRUE); +#if defined(__MINGW32__) + m.attr("obj_cast_UnscopedEnum_ptr") = "MinGW: dangling pointer to an unnamed temporary may be " + "used [-Werror=dangling-pointer=]"; +#else m.def("obj_cast_UnscopedEnum_ptr", [](const py::object &obj) { // https://github.com/OpenImageIO/oiio/blob/30ea4ebdfab11aec291befbaff446f2a7d24835b/src/python/py_oiio.h#L300 if (py::isinstance(obj)) { @@ -141,4 +145,5 @@ TEST_SUBMODULE(enums, m) { } return 0; }); +#endif } diff --git a/tests/test_enum.py b/tests/test_enum.py index cbf72c99e9..64c898b2d1 100644 --- a/tests/test_enum.py +++ b/tests/test_enum.py @@ -264,6 +264,9 @@ def test_docstring_signatures(): assert "arg0" not in (attr.__doc__ or "") +@pytest.mark.skipif( + isinstance(m.obj_cast_UnscopedEnum_ptr, str), reason=m.obj_cast_UnscopedEnum_ptr +) def test_obj_cast_unscoped_enum_ptr(): assert m.obj_cast_UnscopedEnum_ptr(m.UnscopedEnum.ETwo) == 2 assert m.obj_cast_UnscopedEnum_ptr(m.UnscopedEnum.EOne) == 1 diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index bde49a9144..498d4ceafb 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -84,9 +84,12 @@ TEST_SUBMODULE(native_enum, m) { m.def("pass_some_proto_enum", [](some_proto_enum) { return py::none(); }); m.def("return_some_proto_enum", []() { return some_proto_enum::Zero; }); -#ifndef NDEBUG - m.def("obj_cast_color_ptr", [](const py::object &obj) { obj.cast(); }); +#if defined(__MINGW32__) + m.attr("obj_cast_color_ptr") = "MinGW: dangling pointer to an unnamed temporary may be used " + "[-Werror=dangling-pointer=]"; +#elif defined(NDEBUG) + m.attr("obj_cast_color_ptr") = "NDEBUG disables cast safety check"; #else - m.attr("obj_cast_color_ptr") = py::none(); + m.def("obj_cast_color_ptr", [](const py::object &obj) { obj.cast(); }); #endif } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index b40c36063d..190df2f477 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -81,9 +81,7 @@ def test_type_caster_enum_type_enabled_false(): assert m.return_some_proto_enum() is None -@pytest.mark.skipif( - m.obj_cast_color_ptr is None, reason="NDEBUG disables cast safety check" -) +@pytest.mark.skipif(isinstance(m.obj_cast_color_ptr, str), reason=m.obj_cast_color_ptr) def test_obj_cast_color_ptr(): with pytest.raises(RuntimeError) as excinfo: m.obj_cast_color_ptr(m.color.red) From daa515ad3654d834006c7152bb70f4a98c6050c6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 14 Nov 2022 23:14:43 -0800 Subject: [PATCH 18/35] Add `.export_values()` and member `__doc__` implementations with tests. --- include/pybind11/native_enum.h | 30 ++++++++++++++++++++++-------- tests/test_native_enum.cpp | 14 ++++++++++++++ tests/test_native_enum.py | 33 ++++++++++++++++++++++++++++++--- 3 files changed, 66 insertions(+), 11 deletions(-) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 0fd592c50e..c27399c0ea 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -19,14 +19,17 @@ class native_enum { native_enum(const object &scope, const char *name) : m_scope(scope), m_name(name) {} /// Export enumeration entries into the parent scope - native_enum &export_values() { return *this; } + native_enum &export_values() { + m_export_values = true; + return *this; + } /// Add an enumeration entry native_enum &value(char const *name, Type value, const char *doc = nullptr) { + m_members.append(make_tuple(name, static_cast(value))); if (doc) { - // IGNORED. + m_docs.append(make_tuple(name, doc)); } - m_members.append(make_tuple(name, static_cast(value))); return *this; } @@ -39,19 +42,30 @@ class native_enum { constexpr bool use_int_enum = std::numeric_limits::is_integer && !std::is_same::value && !detail::is_std_char_type::value; - auto int_enum = enum_module.attr(use_int_enum ? "IntEnum" : "Enum"); - auto int_enum_color = int_enum(m_name, m_members); - int_enum_color.attr("__module__") = m_scope; - m_scope.attr(m_name) = int_enum_color; + auto py_enum_type = enum_module.attr(use_int_enum ? "IntEnum" : "Enum"); + auto py_enum = py_enum_type(m_name, m_members); + py_enum.attr("__module__") = m_scope; + m_scope.attr(m_name) = py_enum; + if (m_export_values) { + for (auto member : m_members) { + auto member_name = member[int_(0)]; + m_scope.attr(member_name) = py_enum[member_name]; + } + } + for (auto doc : m_docs) { + py_enum[doc[int_(0)]].attr("__doc__") = doc[int_(1)]; + } // Intentionally leak Python reference. detail::get_internals().native_enum_types[std::type_index(typeid(Type))] - = int_enum_color.release().ptr(); + = py_enum.release().ptr(); } private: object m_scope; str m_name; + bool m_export_values = false; list m_members; + list m_docs; }; PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 498d4ceafb..83dd3665bf 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -18,6 +18,10 @@ enum class altitude : char { low = 'l', // trailing comma only allowed after CWG518 }; +enum class export_values { exv0, exv1 }; + +enum class member_doc { mem0, mem1, mem2 }; + // https://github.com/protocolbuffers/protobuf/blob/d70b5c5156858132decfdbae0a1103e6a5cb1345/src/google/protobuf/generated_enum_util.h#L52-L53 template struct is_proto_enum : std::false_type {}; @@ -76,6 +80,16 @@ TEST_SUBMODULE(native_enum, m) { .value("high", altitude::high) .value("low", altitude::low); + py::native_enum(m, "export_values") + .value("exv0", export_values::exv0) + .value("exv1", export_values::exv1) + .export_values(); + + py::native_enum(m, "member_doc") + .value("mem0", member_doc::mem0, "docA") + .value("mem1", member_doc::mem1) + .value("mem2", member_doc::mem2, "docC"); + m.def("isinstance_color", [](const py::object &obj) { return py::isinstance(obj); }); m.def("pass_color", [](color e) { return static_cast(e); }); diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 190df2f477..2138ab635f 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -22,9 +22,22 @@ ("low", "l"), ) +EXPORT_VALUES_MEMBERS = ( + ("exv0", 0), + ("exv1", 1), +) + +MEMBER_DOC_MEMBERS = ( + ("mem0", 0), + ("mem1", 1), + ("mem2", 2), +) -@pytest.mark.parametrize("enum_type", (m.smallenum, m.color, m.altitude)) -def test_enum_color_type(enum_type): + +@pytest.mark.parametrize( + "enum_type", (m.smallenum, m.color, m.altitude, m.export_values, m.member_doc) +) +def test_enum_type(enum_type): assert isinstance(enum_type, enum.EnumMeta) @@ -34,13 +47,27 @@ def test_enum_color_type(enum_type): (m.smallenum, SMALLENUM_MEMBERS), (m.color, COLOR_MEMBERS), (m.altitude, ALTITUDE_MEMBERS), + (m.export_values, EXPORT_VALUES_MEMBERS), + (m.member_doc, MEMBER_DOC_MEMBERS), ), ) -def test_enum_color_members(enum_type, members): +def test_enum_members(enum_type, members): for name, value in members: assert enum_type[name].value == value +def test_export_values(): + assert m.exv0 is m.export_values.exv0 + assert m.exv1 is m.export_values.exv1 + + +def test_member_doc(): + pure_native = enum.IntEnum("pure_native", (("mem", 0),)) + assert m.member_doc.mem0.__doc__ == "docA" + assert m.member_doc.mem1.__doc__ == pure_native.mem.__doc__ + assert m.member_doc.mem2.__doc__ == "docC" + + def test_pybind11_isinstance_color(): for name, _ in COLOR_MEMBERS: assert m.isinstance_color(m.color[name]) From 962ebf9311ff3366de953f3ed6a38fd820745be3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 15 Nov 2022 00:05:47 -0800 Subject: [PATCH 19/35] Add missing ``, while at it, also `` for completeness. --- include/pybind11/native_enum.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index c27399c0ea..8b6b79b64f 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -6,7 +6,9 @@ #include "pybind11.h" +#include #include +#include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) From 703372c46af7daf700b0b73093f8509290dd2e5f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 01:14:54 -0800 Subject: [PATCH 20/35] Move code for building the native enum type from `native_enum<>` dtor to `module::operator+=` --- CMakeLists.txt | 2 + include/pybind11/detail/native_enum_data.h | 28 ++++++++++++ include/pybind11/native_enum.h | 50 ++++++---------------- include/pybind11/pybind11.h | 21 +++++++++ tests/extra_python_package/test_files.py | 1 + tests/test_native_enum.cpp | 48 ++++++++++----------- 6 files changed, 89 insertions(+), 61 deletions(-) create mode 100644 include/pybind11/detail/native_enum_data.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 0d93203881..21beea24f3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -116,6 +116,7 @@ set(PYBIND11_HEADERS include/pybind11/detail/descr.h include/pybind11/detail/init.h include/pybind11/detail/internals.h + include/pybind11/detail/native_enum_data.h include/pybind11/detail/type_caster_base.h include/pybind11/detail/typeid.h include/pybind11/attr.h @@ -133,6 +134,7 @@ set(PYBIND11_HEADERS include/pybind11/gil.h include/pybind11/iostream.h include/pybind11/functional.h + include/pybind11/native_enum.h include/pybind11/numpy.h include/pybind11/operators.h include/pybind11/pybind11.h diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h new file mode 100644 index 0000000000..a0990fd273 --- /dev/null +++ b/include/pybind11/detail/native_enum_data.h @@ -0,0 +1,28 @@ +// Copyright (c) 2022 The pybind Community. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "../pytypes.h" +#include "common.h" + +#include + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +struct native_enum_data { + native_enum_data(const char *name, const std::type_index &enum_type_index, bool use_int_enum) + : name{name}, enum_type_index{enum_type_index}, use_int_enum{use_int_enum} {} + + str name; + std::type_index enum_type_index; + bool use_int_enum; + bool export_values_flag = false; + list members; + list docs; +}; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 8b6b79b64f..10f23e9b9e 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -4,7 +4,9 @@ #pragma once -#include "pybind11.h" +#include "detail/common.h" +#include "detail/native_enum_data.h" +#include "cast.h" #include #include @@ -14,60 +16,34 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) /// Conversions between Python's native (stdlib) enum types and C++ enums. template -class native_enum { +class native_enum : public detail::native_enum_data { public: using Underlying = typename std::underlying_type::type; - native_enum(const object &scope, const char *name) : m_scope(scope), m_name(name) {} + 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) {} /// Export enumeration entries into the parent scope native_enum &export_values() { - m_export_values = true; + export_values_flag = true; return *this; } /// Add an enumeration entry native_enum &value(char const *name, Type value, const char *doc = nullptr) { - m_members.append(make_tuple(name, static_cast(value))); + members.append(make_tuple(name, static_cast(value))); if (doc) { - m_docs.append(make_tuple(name, doc)); + docs.append(make_tuple(name, doc)); } return *this; } native_enum(const native_enum &) = delete; native_enum &operator=(const native_enum &) = delete; - - ~native_enum() { - // Any exception here will terminate the process. - auto enum_module = module_::import("enum"); - constexpr bool use_int_enum = std::numeric_limits::is_integer - && !std::is_same::value - && !detail::is_std_char_type::value; - auto py_enum_type = enum_module.attr(use_int_enum ? "IntEnum" : "Enum"); - auto py_enum = py_enum_type(m_name, m_members); - py_enum.attr("__module__") = m_scope; - m_scope.attr(m_name) = py_enum; - if (m_export_values) { - for (auto member : m_members) { - auto member_name = member[int_(0)]; - m_scope.attr(member_name) = py_enum[member_name]; - } - } - for (auto doc : m_docs) { - py_enum[doc[int_(0)]].attr("__doc__") = doc[int_(1)]; - } - // Intentionally leak Python reference. - detail::get_internals().native_enum_types[std::type_index(typeid(Type))] - = py_enum.release().ptr(); - } - -private: - object m_scope; - str m_name; - bool m_export_values = false; - list m_members; - list m_docs; }; PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 76d6eadcae..475e1f05a7 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -12,6 +12,7 @@ #include "detail/class.h" #include "detail/init.h" +#include "detail/native_enum_data.h" #include "attr.h" #include "gil.h" #include "options.h" @@ -1268,6 +1269,26 @@ class module_ : public object { // For Python 2, reinterpret_borrow was correct. return reinterpret_borrow(m); } + + module_ &operator+=(const detail::native_enum_data &data) { + auto enum_module = import("enum"); + auto py_enum_type = enum_module.attr(data.use_int_enum ? "IntEnum" : "Enum"); + auto py_enum = py_enum_type(data.name, data.members); + py_enum.attr("__module__") = *this; + this->attr(data.name) = py_enum; + if (data.export_values_flag) { + for (auto member : data.members) { + auto member_name = member[int_(0)]; + this->attr(member_name) = py_enum[member_name]; + } + } + for (auto doc : data.docs) { + py_enum[doc[int_(0)]].attr("__doc__") = doc[int_(1)]; + } + // Intentionally leak Python reference. + detail::get_internals().native_enum_types[data.enum_type_index] = py_enum.release().ptr(); + return *this; + } }; // When inside a namespace (or anywhere as long as it's not the first item on a line), diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 00ba7aaa07..86b60cbac5 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -52,6 +52,7 @@ "include/pybind11/detail/descr.h", "include/pybind11/detail/init.h", "include/pybind11/detail/internals.h", + "include/pybind11/detail/native_enum_data.h", "include/pybind11/detail/type_caster_base.h", "include/pybind11/detail/typeid.h", } diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 83dd3665bf..b34be03f91 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -65,30 +65,30 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; - py::native_enum(m, "smallenum") - .value("a", smallenum::a) - .value("b", smallenum::b) - .value("c", smallenum::c); - - py::native_enum(m, "color") - .value("red", color::red) - .value("yellow", color::yellow) - .value("green", color::green) - .value("blue", color::blue); - - py::native_enum(m, "altitude") - .value("high", altitude::high) - .value("low", altitude::low); - - py::native_enum(m, "export_values") - .value("exv0", export_values::exv0) - .value("exv1", export_values::exv1) - .export_values(); - - py::native_enum(m, "member_doc") - .value("mem0", member_doc::mem0, "docA") - .value("mem1", member_doc::mem1) - .value("mem2", member_doc::mem2, "docC"); + m += py::native_enum("smallenum") + .value("a", smallenum::a) + .value("b", smallenum::b) + .value("c", smallenum::c); + + m += py::native_enum("color") + .value("red", color::red) + .value("yellow", color::yellow) + .value("green", color::green) + .value("blue", color::blue); + + m += py::native_enum("altitude") + .value("high", altitude::high) + .value("low", altitude::low); + + m += py::native_enum("export_values") + .value("exv0", export_values::exv0) + .value("exv1", export_values::exv1) + .export_values(); + + m += py::native_enum("member_doc") + .value("mem0", member_doc::mem0, "docA") + .value("mem1", member_doc::mem1) + .value("mem2", member_doc::mem2, "docC"); m.def("isinstance_color", [](const py::object &obj) { return py::isinstance(obj); }); From 252524b69150b017c717de82b6ad75c44572e790 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 01:48:13 -0800 Subject: [PATCH 21/35] Change variable name to avoid MSVC C4458 warning. ``` D:\a\pybind11\pybind11\include\pybind11/native_enum.h(37,1): warning C4458: declaration of 'name' hides class member ``` --- include/pybind11/detail/native_enum_data.h | 8 +++++--- include/pybind11/pybind11.h | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index a0990fd273..b2c27fa6f2 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -13,10 +13,12 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) struct native_enum_data { - native_enum_data(const char *name, const std::type_index &enum_type_index, bool use_int_enum) - : name{name}, enum_type_index{enum_type_index}, use_int_enum{use_int_enum} {} + native_enum_data(const char *enum_name, + const std::type_index &enum_type_index, + bool use_int_enum) + : enum_name{enum_name}, enum_type_index{enum_type_index}, use_int_enum{use_int_enum} {} - str name; + str enum_name; std::type_index enum_type_index; bool use_int_enum; bool export_values_flag = false; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 475e1f05a7..023684ceeb 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1273,9 +1273,9 @@ class module_ : public object { module_ &operator+=(const detail::native_enum_data &data) { auto enum_module = import("enum"); auto py_enum_type = enum_module.attr(data.use_int_enum ? "IntEnum" : "Enum"); - auto py_enum = py_enum_type(data.name, data.members); + auto py_enum = py_enum_type(data.enum_name, data.members); py_enum.attr("__module__") = *this; - this->attr(data.name) = py_enum; + this->attr(data.enum_name) = py_enum; if (data.export_values_flag) { for (auto member : data.members) { auto member_name = member[int_(0)]; From e0a4eb690c53a0f0809027076fb04472d1b4d218 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 01:50:33 -0800 Subject: [PATCH 22/35] Resolve clang-tidy error (missing `explicit`). --- include/pybind11/native_enum.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 10f23e9b9e..6055744411 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -20,7 +20,7 @@ class native_enum : public detail::native_enum_data { public: using Underlying = typename std::underlying_type::type; - native_enum(const char *name) + explicit native_enum(const char *name) : detail::native_enum_data(name, std::type_index(typeid(Type)), std::numeric_limits::is_integer From f3ece13dbca8a0fb1d89c84fd7987cc211b8cbcf Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 09:23:28 -0800 Subject: [PATCH 23/35] Add user-friendly correct-use check (activated in Debug builds only). --- include/pybind11/detail/native_enum_data.h | 32 ++++++++++++++++++++-- include/pybind11/pybind11.h | 6 ++-- tests/test_native_enum.cpp | 8 ++++++ tests/test_native_enum.py | 8 ++++++ 4 files changed, 50 insertions(+), 4 deletions(-) diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index b2c27fa6f2..336cd93dce 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -12,18 +12,46 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) -struct native_enum_data { +class native_enum_data { +public: native_enum_data(const char *enum_name, const std::type_index &enum_type_index, bool use_int_enum) : enum_name{enum_name}, enum_type_index{enum_type_index}, use_int_enum{use_int_enum} {} - str enum_name; + std::string enum_name; std::type_index enum_type_index; bool use_int_enum; bool export_values_flag = false; list members; list docs; + + native_enum_data(const native_enum_data &) = delete; + native_enum_data &operator=(const native_enum_data &) = delete; + + // This is a separate public function only to enable easy unit testing. + std::string was_not_added_error_message() { + return "`native_enum` was not added to any module." + " Use e.g. `m += native_enum<...>(\"" + + enum_name + "\")` to fix."; + } + +#if defined(NDEBUG) + void set_was_added_to_module() const {}; +#else + void set_was_added_to_module() const { was_added_to_module = true; } + +private: + mutable bool was_added_to_module = false; + +public: + // This dtor cannot easily be unit tested because it terminates the process. + ~native_enum_data() { + if (!was_added_to_module) { + pybind11_fail(was_not_added_error_message()); + } + } +#endif }; PYBIND11_NAMESPACE_END(detail) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 023684ceeb..6516defae5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1271,11 +1271,13 @@ class module_ : public object { } module_ &operator+=(const detail::native_enum_data &data) { + data.set_was_added_to_module(); auto enum_module = import("enum"); auto py_enum_type = enum_module.attr(data.use_int_enum ? "IntEnum" : "Enum"); - auto py_enum = py_enum_type(data.enum_name, data.members); + pybind11::str py_enum_name(data.enum_name); + auto py_enum = py_enum_type(py_enum_name, data.members); py_enum.attr("__module__") = *this; - this->attr(data.enum_name) = py_enum; + this->attr(py_enum_name) = py_enum; if (data.export_values_flag) { for (auto member : data.members) { auto member_name = member[int_(0)]; diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index b34be03f91..7e105b79cb 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -2,6 +2,8 @@ #include "pybind11_tests.h" +#include + namespace test_native_enum { // https://en.cppreference.com/w/cpp/language/enum @@ -106,4 +108,10 @@ TEST_SUBMODULE(native_enum, m) { #else m.def("obj_cast_color_ptr", [](const py::object &obj) { obj.cast(); }); #endif + + m.def("native_enum_data_was_not_added_error_message", [](const char *enum_name) { + py::detail::native_enum_data data(enum_name, std::type_index(typeid(void)), false); + data.set_was_added_to_module(); + return data.was_not_added_error_message(); + }); } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 2138ab635f..8b55cdece6 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -113,3 +113,11 @@ def test_obj_cast_color_ptr(): with pytest.raises(RuntimeError) as excinfo: m.obj_cast_color_ptr(m.color.red) assert str(excinfo.value) == "Unable to cast native enum type to reference" + + +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.' + ) From 50fae46f53a6d701c138eab6a41bfe2d95043b3f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 10:34:47 -0800 Subject: [PATCH 24/35] Resolve clang-tidy error (missing `const`). --- 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 336cd93dce..de3a7f6e05 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -30,7 +30,7 @@ class native_enum_data { native_enum_data &operator=(const native_enum_data &) = delete; // This is a separate public function only to enable easy unit testing. - std::string was_not_added_error_message() { + 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 + "\")` to fix."; From 0344e90a49a980c2f35bfd1dc00f42755acb4fa0 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 14:19:44 -0800 Subject: [PATCH 25/35] Catch & test all double-registration or name clash situations. Fix up correct use check and add test. --- include/pybind11/detail/native_enum_data.h | 39 +++++++------ include/pybind11/native_enum.h | 16 +++++- include/pybind11/pybind11.h | 25 ++++++-- tests/test_native_enum.cpp | 51 +++++++++++++++- tests/test_native_enum.py | 67 ++++++++++++++++++++++ 5 files changed, 173 insertions(+), 25 deletions(-) diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index de3a7f6e05..2e0a3c4870 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -17,41 +17,42 @@ class native_enum_data { native_enum_data(const char *enum_name, const std::type_index &enum_type_index, bool use_int_enum) - : enum_name{enum_name}, enum_type_index{enum_type_index}, use_int_enum{use_int_enum} {} - - std::string enum_name; - std::type_index enum_type_index; - bool use_int_enum; - bool export_values_flag = false; - list members; - list docs; + : correct_use_check{false}, enum_name_encoded{enum_name}, enum_type_index{enum_type_index}, + use_int_enum{use_int_enum}, export_values_flag{false}, enum_name{enum_name} {} native_enum_data(const native_enum_data &) = delete; native_enum_data &operator=(const native_enum_data &) = delete; + void disarm_correct_use_check() const { correct_use_check = false; } + void arm_correct_use_check() const { correct_use_check = true; } + // This is a separate public function only to enable easy unit testing. 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 + "\")` to fix."; + + enum_name_encoded + "\")` to fix."; } -#if defined(NDEBUG) - void set_was_added_to_module() const {}; -#else - void set_was_added_to_module() const { was_added_to_module = true; } - -private: - mutable bool was_added_to_module = false; - -public: +#if !defined(NDEBUG) // This dtor cannot easily be unit tested because it terminates the process. ~native_enum_data() { - if (!was_added_to_module) { + if (correct_use_check) { pybind11_fail(was_not_added_error_message()); } } #endif + +private: + mutable bool correct_use_check; + +public: + std::string enum_name_encoded; + std::type_index enum_type_index; + bool use_int_enum; + bool export_values_flag; + str enum_name; + list members; + list docs; }; PYBIND11_NAMESPACE_END(detail) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 6055744411..138a367908 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -25,7 +25,19 @@ class native_enum : public detail::native_enum_data { std::type_index(typeid(Type)), std::numeric_limits::is_integer && !std::is_same::value - && !detail::is_std_char_type::value) {} + && !detail::is_std_char_type::value) { + if (detail::get_local_type_info(typeid(Type)) + || detail::get_global_type_info(typeid(Type))) { + pybind11_fail( + "pybind11::native_enum<...>(\"" + enum_name_encoded + + "\") is already registered as a `pybind11::enum_` or `pybind11::class_`!"); + } + if (detail::get_internals().native_enum_types.count(enum_type_index)) { + pybind11_fail("pybind11::native_enum<...>(\"" + enum_name_encoded + + "\") is already registered!"); + } + arm_correct_use_check(); + } /// Export enumeration entries into the parent scope native_enum &export_values() { @@ -35,10 +47,12 @@ class native_enum : public detail::native_enum_data { /// Add an enumeration entry native_enum &value(char const *name, Type value, const char *doc = nullptr) { + disarm_correct_use_check(); members.append(make_tuple(name, static_cast(value))); if (doc) { docs.append(make_tuple(name, doc)); } + arm_correct_use_check(); return *this; } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 6516defae5..d68cfd3132 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1271,16 +1271,24 @@ class module_ : public object { } module_ &operator+=(const detail::native_enum_data &data) { - data.set_was_added_to_module(); + data.disarm_correct_use_check(); + if (hasattr(*this, data.enum_name)) { + pybind11_fail("pybind11::native_enum<...>(\"" + data.enum_name_encoded + + "\"): an object with that name is already defined"); + } auto enum_module = import("enum"); auto py_enum_type = enum_module.attr(data.use_int_enum ? "IntEnum" : "Enum"); - pybind11::str py_enum_name(data.enum_name); - auto py_enum = py_enum_type(py_enum_name, data.members); + auto py_enum = py_enum_type(data.enum_name, data.members); py_enum.attr("__module__") = *this; - this->attr(py_enum_name) = py_enum; + this->attr(data.enum_name) = py_enum; if (data.export_values_flag) { for (auto member : data.members) { auto member_name = member[int_(0)]; + if (hasattr(*this, member_name)) { + pybind11_fail("pybind11::native_enum<...>(\"" + data.enum_name_encoded + + "\").value(\"" + member_name.cast() + + "\"): an object with that name is already defined"); + } this->attr(member_name) = py_enum[member_name]; } } @@ -2195,6 +2203,15 @@ class enum_ : public class_ { template enum_(const handle &scope, const char *name, const Extra &...extra) : class_(scope, name, extra...), m_base(*this, scope) { + { + auto const &natives = detail::get_internals().native_enum_types; + auto found = natives.find(std::type_index(typeid(Type))); + if (found != natives.end()) { + pybind11_fail("pybind11::enum_ \"" + std::string(name) + + "\" is already registered as a pybind11::native_enum!"); + } + } + constexpr bool is_arithmetic = detail::any_of...>::value; constexpr bool is_convertible = std::is_convertible::value; m_base.init(is_arithmetic, is_convertible); diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 7e105b79cb..0cb11c14c5 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -111,7 +111,56 @@ TEST_SUBMODULE(native_enum, m) { m.def("native_enum_data_was_not_added_error_message", [](const char *enum_name) { py::detail::native_enum_data data(enum_name, std::type_index(typeid(void)), false); - data.set_was_added_to_module(); + data.disarm_correct_use_check(); return data.was_not_added_error_message(); }); + + m.def("native_enum_ctor_malformed_utf8", [](const char *malformed_utf8) { + enum fake { x }; + py::native_enum{malformed_utf8}; + }); + + m.def("native_enum_value_malformed_utf8", [](const char *malformed_utf8) { + enum fake { x }; + py::native_enum("fake").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.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.def("native_enum_value_name_clash", [](py::module_ m) { + enum fake { x }; + m += py::native_enum("fake_native_enum_value_name_clash") + .value("fake_native_enum_value_name_clash_x", fake::x) + .export_values(); + }); + + 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); + }); + + 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); + 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); + }); +#else + m.attr("native_enum_correct_use_failure") = "For local testing only: terminates process"; +#endif } diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 8b55cdece6..d078e1a371 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -121,3 +121,70 @@ def test_native_enum_data_was_not_added_error_message(): "`native_enum` was not added to any module." ' Use e.g. `m += native_enum<...>("Fake")` to fix.' ) + + +@pytest.mark.parametrize( + "func", (m.native_enum_ctor_malformed_utf8, m.native_enum_value_malformed_utf8) +) +def test_native_enum_malformed_utf8(func): + malformed_utf8 = b"\x80" + with pytest.raises(UnicodeDecodeError): + func(malformed_utf8) + + +def test_double_registration_native_enum(): + with pytest.raises(RuntimeError) as excinfo: + m.double_registration_native_enum(m) + assert ( + str(excinfo.value) + == 'pybind11::native_enum<...>("fake_double_registration_native_enum") is already registered!' + ) + + +def test_native_enum_name_clash(): + m.fake_native_enum_name_clash = None + with pytest.raises(RuntimeError) as excinfo: + m.native_enum_name_clash(m) + assert ( + str(excinfo.value) + == 'pybind11::native_enum<...>("fake_native_enum_name_clash"):' + " an object with that name is already defined" + ) + + +def test_native_enum_value_name_clash(): + m.fake_native_enum_value_name_clash_x = None + with pytest.raises(RuntimeError) as excinfo: + m.native_enum_value_name_clash(m) + assert ( + str(excinfo.value) + == 'pybind11::native_enum<...>("fake_native_enum_value_name_clash")' + '.value("fake_native_enum_value_name_clash_x"):' + " an object with that name is already defined" + ) + + +def test_double_registration_enum_before_native_enum(): + with pytest.raises(RuntimeError) as excinfo: + m.double_registration_enum_before_native_enum(m) + assert ( + str(excinfo.value) + == 'pybind11::native_enum<...>("fake_enum_first") is already registered' + " as a `pybind11::enum_` or `pybind11::class_`!" + ) + + +def test_double_registration_native_enum_before_enum(): + with pytest.raises(RuntimeError) as excinfo: + m.double_registration_native_enum_before_enum(m) + assert ( + str(excinfo.value) + == 'pybind11::enum_ "name_must_be_different_to_reach_desired_code_path"' + " is already registered as a pybind11::native_enum!" + ) + + +def test_native_enum_correct_use_failure(): + if not isinstance(m.native_enum_correct_use_failure, str): + m.native_enum_correct_use_failure() + pytest.fail("Process termination expected.") From a0f43c920fe1ff13cc88880802d17ba34ac4959c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 16:18:25 -0800 Subject: [PATCH 26/35] Resolve clang-tidy error (missing `!= nullptr`). --- include/pybind11/native_enum.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 138a367908..2d306981a1 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -26,8 +26,8 @@ class native_enum : public detail::native_enum_data { std::numeric_limits::is_integer && !std::is_same::value && !detail::is_std_char_type::value) { - if (detail::get_local_type_info(typeid(Type)) - || detail::get_global_type_info(typeid(Type))) { + if (detail::get_local_type_info(typeid(Type)) != nullptr + || detail::get_global_type_info(typeid(Type)) != nullptr) { pybind11_fail( "pybind11::native_enum<...>(\"" + enum_name_encoded + "\") is already registered as a `pybind11::enum_` or `pybind11::class_`!"); From c2e6b38c9376cae59fb803793a6b49f8f8a3351f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 21:20:00 -0800 Subject: [PATCH 27/35] Modified version of PR #4293 by @wjakob Modifications are: * Backward compatibility (no ABI break), as originally under PR #4307. * Naming: `get_python_state_dict()`, `has_pybind11_internals_capsule()` * Report error retrieving `internals**` from capsule instead of clearing it. Locally tested with ASAN, MSAN, TSAN, UBSAN (Google-internal toolchain). --- include/pybind11/detail/internals.h | 44 +++++++++++++++++++++++---- tests/test_embed/test_interpreter.cpp | 19 ++++++------ 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index d6cfa20b54..7b4be56071 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -424,9 +424,30 @@ inline void translate_local_exception(std::exception_ptr p) { } #endif +inline object get_python_state_dict() { + object state_dict; +#if (PYBIND11_INTERNALS_VERSION <= 4 && PY_VERSION_HEX < 0x030C0000) \ + || PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) + state_dict = reinterpret_borrow(PyEval_GetBuiltins()); +#else +# if PY_VERSION_HEX < 0x03090000 + PyInterpreterState *istate = _PyInterpreterState_Get(); +# else + PyInterpreterState *istate = PyInterpreterState_Get(); +# endif + if (istate) { + state_dict = reinterpret_borrow(PyInterpreterState_GetDict(istate)); + } +#endif + if (!state_dict) { + raise_from(PyExc_SystemError, "pybind11::detail::get_python_state_dict() FAILED"); + } + return state_dict; +} + /// Return a reference to the current `internals` data PYBIND11_NOINLINE internals &get_internals() { - auto **&internals_pp = get_internals_pp(); + internals **&internals_pp = get_internals_pp(); if (internals_pp && *internals_pp) { return **internals_pp; } @@ -448,11 +469,22 @@ PYBIND11_NOINLINE internals &get_internals() { #endif error_scope err_scope; - PYBIND11_STR_TYPE id(PYBIND11_INTERNALS_ID); - auto builtins = handle(PyEval_GetBuiltins()); - if (builtins.contains(id) && isinstance(builtins[id])) { - internals_pp = static_cast(capsule(builtins[id])); + constexpr const char *id_cstr = PYBIND11_INTERNALS_ID; + str id(id_cstr); + dict state_dict = get_python_state_dict(); + + if (state_dict.contains(id_cstr)) { + void *raw_ptr = PyCapsule_GetPointer(state_dict[id].ptr(), id_cstr); + if (raw_ptr == nullptr) { + raise_from( + PyExc_SystemError, + "pybind11::detail::get_internals(): Retrieve internals** from capsule FAILED"); + } + internals_pp = static_cast(raw_ptr); + } + + if (internals_pp && *internals_pp) { // We loaded builtins through python's builtins, which means that our `error_already_set` // and `builtin_exception` may be different local classes than the ones set up in the // initial exception translator, below, so add another for our local exception classes. @@ -488,7 +520,7 @@ PYBIND11_NOINLINE internals &get_internals() { # endif internals_ptr->istate = tstate->interp; #endif - builtins[id] = capsule(internals_pp); + state_dict[id] = capsule(internals_pp, id_cstr); internals_ptr->registered_exception_translators.push_front(&translate_exception); internals_ptr->static_property_type = make_static_property_type(); internals_ptr->default_metaclass = make_default_metaclass(); diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 44dcd1fdb0..e49f9adfc8 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -168,9 +168,8 @@ TEST_CASE("There can be only one interpreter") { py::initialize_interpreter(); } -bool has_pybind11_internals_builtin() { - auto builtins = py::handle(PyEval_GetBuiltins()); - return builtins.contains(PYBIND11_INTERNALS_ID); +bool has_pybind11_internals_capsule() { + return py::detail::get_python_state_dict().contains(PYBIND11_INTERNALS_ID); }; bool has_pybind11_internals_static() { @@ -181,7 +180,7 @@ bool has_pybind11_internals_static() { TEST_CASE("Restart the interpreter") { // Verify pre-restart state. REQUIRE(py::module_::import("widget_module").attr("add")(1, 2).cast() == 3); - REQUIRE(has_pybind11_internals_builtin()); + REQUIRE(has_pybind11_internals_capsule()); REQUIRE(has_pybind11_internals_static()); REQUIRE(py::module_::import("external_module").attr("A")(123).attr("value").cast() == 123); @@ -198,10 +197,10 @@ TEST_CASE("Restart the interpreter") { REQUIRE(Py_IsInitialized() == 1); // Internals are deleted after a restart. - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_capsule()); REQUIRE_FALSE(has_pybind11_internals_static()); pybind11::detail::get_internals(); - REQUIRE(has_pybind11_internals_builtin()); + REQUIRE(has_pybind11_internals_capsule()); REQUIRE(has_pybind11_internals_static()); REQUIRE(reinterpret_cast(*py::detail::get_internals_pp()) == py::module_::import("external_module").attr("internals_at")().cast()); @@ -216,13 +215,13 @@ TEST_CASE("Restart the interpreter") { py::detail::get_internals(); *static_cast(ran) = true; }); - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_capsule()); REQUIRE_FALSE(has_pybind11_internals_static()); REQUIRE_FALSE(ran); py::finalize_interpreter(); REQUIRE(ran); py::initialize_interpreter(); - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_capsule()); REQUIRE_FALSE(has_pybind11_internals_static()); // C++ modules can be reloaded. @@ -244,7 +243,7 @@ TEST_CASE("Subinterpreter") { REQUIRE(m.attr("add")(1, 2).cast() == 3); } - REQUIRE(has_pybind11_internals_builtin()); + REQUIRE(has_pybind11_internals_capsule()); REQUIRE(has_pybind11_internals_static()); /// Create and switch to a subinterpreter. @@ -254,7 +253,7 @@ TEST_CASE("Subinterpreter") { // Subinterpreters get their own copy of builtins. detail::get_internals() still // works by returning from the static variable, i.e. all interpreters share a single // global pybind11::internals; - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_capsule()); REQUIRE(has_pybind11_internals_static()); // Modules tags should be gone. From bb6c00336a757c966fa8ca4f4d7514e153484d20 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 22:36:25 -0800 Subject: [PATCH 28/35] Snapshot of .github/workflows/python312.yml (PR #4342) --- .github/workflows/python312.yml | 131 ++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 .github/workflows/python312.yml diff --git a/.github/workflows/python312.yml b/.github/workflows/python312.yml new file mode 100644 index 0000000000..5a153938a5 --- /dev/null +++ b/.github/workflows/python312.yml @@ -0,0 +1,131 @@ +name: Python312 + +on: + workflow_dispatch: + pull_request: + +concurrency: + group: python312-${{ github.ref }} + cancel-in-progress: false + +env: + PYTEST_TIMEOUT: 300 + +jobs: + standard: + name: "🐍 3.12 latest • ubuntu-latest • x64" + runs-on: ubuntu-latest + # if: "contains(github.event.pull_request.labels.*.name, 'python dev')" + + steps: + - name: Show env + run: env + + - uses: actions/checkout@v3 + + - name: Setup Python 3.12 + uses: actions/setup-python@v4 + with: + python-version: "3.12-dev" + + - name: Setup Boost + run: sudo apt-get install libboost-dev + + - name: Update CMake + uses: jwlawson/actions-setup-cmake@v1.13 + + - name: Run pip installs + run: | + python -m pip install --upgrade pip + python -m pip install --prefer-binary -r tests/requirements.txt + python -m pip install --prefer-binary numpy + python -m pip install --prefer-binary scipy + + - name: Show platform info + run: python -m platform + + - name: Show CMake version + run: cmake --version + + # FIRST BUILD + - name: Configure C++11 + run: > + cmake -S . -B build11 + -DCMAKE_VERBOSE_MAKEFILE=ON + -DPYBIND11_WERROR=ON + -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_EIGEN=ON + -DCMAKE_CXX_STANDARD=11 + -DCMAKE_BUILD_TYPE=Debug + + - name: Build C++11 + run: cmake --build build11 -j 2 + + - name: Python tests C++11 + run: cmake --build build11 --target pytest -j 2 + + - name: C++ tests C++11 + run: cmake --build build11 --target cpptest -j 2 + + - name: Interface test C++11 + run: cmake --build build11 --target test_cmake_build + + - name: Clean directory + run: git clean -fdx + + # SECOND BUILD + - name: Configure C++17 + run: > + cmake -S . -B build17 + -DCMAKE_VERBOSE_MAKEFILE=ON + -DPYBIND11_WERROR=ON + -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_EIGEN=ON + -DCMAKE_CXX_STANDARD=17 + -DCMAKE_BUILD_TYPE=Debug + + - name: Build C++17 + run: cmake --build build17 -j 2 + + - name: Python tests C++17 + run: cmake --build build17 --target pytest + + - name: C++ tests C++17 + run: cmake --build build17 --target cpptest + + - name: Interface test C++17 + run: cmake --build build17 --target test_cmake_build + + - name: Clean directory + run: git clean -fdx + + # THIRD BUILD + - name: Configure C++17 max DPYBIND11_INTERNALS_VERSION + run: > + cmake -S . -B build17max + -DCMAKE_VERBOSE_MAKEFILE=ON + -DPYBIND11_WERROR=ON + -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_EIGEN=ON + -DCMAKE_CXX_STANDARD=17 + -DCMAKE_BUILD_TYPE=Debug + -DPYBIND11_INTERNALS_VERSION=10000000 + + - name: Build C++17 max DPYBIND11_INTERNALS_VERSION + run: cmake --build build17max -j 2 + + - name: Python tests C++17 max DPYBIND11_INTERNALS_VERSION + run: cmake --build build17max --target pytest + + - name: C++ tests C++17 max DPYBIND11_INTERNALS_VERSION + run: cmake --build build17max --target cpptest + + - name: Interface test + run: cmake --build build17max --target test_cmake_build + + # Ensure the setup_helpers module can build packages using setuptools + - name: Setuptools helpers test + run: pytest tests/extra_setuptools + + - name: Clean directory + run: git clean -fdx From dd0147abe3af65795db9bfbe10dd92cdabe6e0fb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 22:49:04 -0800 Subject: [PATCH 29/35] Update .github/workflows/python312.yml (PR #4342) --- .github/workflows/python312.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/python312.yml b/.github/workflows/python312.yml index 5a153938a5..c97dbdb0d9 100644 --- a/.github/workflows/python312.yml +++ b/.github/workflows/python312.yml @@ -39,7 +39,7 @@ jobs: python -m pip install --upgrade pip python -m pip install --prefer-binary -r tests/requirements.txt python -m pip install --prefer-binary numpy - python -m pip install --prefer-binary scipy + # python -m pip install --prefer-binary scipy - name: Show platform info run: python -m platform @@ -120,7 +120,7 @@ jobs: - name: C++ tests C++17 max DPYBIND11_INTERNALS_VERSION run: cmake --build build17max --target cpptest - - name: Interface test + - name: Interface test C++17 max DPYBIND11_INTERNALS_VERSION run: cmake --build build17max --target test_cmake_build # Ensure the setup_helpers module can build packages using setuptools From 8737beaad503ce5136cb6c6119254e201132d60d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 16 Nov 2022 23:38:08 -0800 Subject: [PATCH 30/35] Split out `get_native_enum_type_map()`, leaving `struct internals` untouched (compared to master). Retested with all sanitizers. --- include/pybind11/cast.h | 8 +-- include/pybind11/detail/internals.h | 100 +++++++++++++++++++++------- include/pybind11/native_enum.h | 2 +- include/pybind11/pybind11.h | 4 +- tests/test_native_enum.cpp | 2 + tests/test_native_enum.py | 9 +++ 6 files changed, 93 insertions(+), 32 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c23a290c54..e8fe376450 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -58,7 +58,7 @@ class type_caster_enum_type { template static handle cast(SrcType &&src, return_value_policy, handle parent) { - auto const &natives = get_internals().native_enum_types; + auto const &natives = get_native_enum_type_map(); auto found = natives.find(std::type_index(typeid(EnumType))); if (found != natives.end()) { return handle(found->second)(static_cast(src)).release(); @@ -71,7 +71,7 @@ class type_caster_enum_type { } bool load(handle src, bool convert) { - auto const &natives = get_internals().native_enum_types; + auto const &natives = get_native_enum_type_map(); auto found = natives.find(std::type_index(typeid(EnumType))); if (found != natives.end()) { if (!isinstance(src, found->second)) { @@ -125,7 +125,7 @@ class type_caster::value, int> = 0> bool isinstance_native_enum_impl(handle obj, const std::type_info &tp) { - auto const &natives = get_internals().native_enum_types; + auto const &natives = get_native_enum_type_map(); auto found = natives.find(tp); if (found == natives.end()) { return false; @@ -1138,7 +1138,7 @@ T cast(const handle &handle) { "Unable to cast type to reference: value is local to type caster"); #ifndef NDEBUG if (is_enum_cast) { - auto const &natives = get_internals().native_enum_types; + auto const &natives = get_native_enum_type_map(); auto found = natives.find(std::type_index(typeid(intrinsic_t))); if (found != natives.end()) { pybind11_fail("Unable to cast native enum type to reference"); diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 7b4be56071..878a23ded0 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -34,7 +34,7 @@ /// further ABI-incompatible changes may be made before the ABI is officially /// changed to the new version. #ifndef PYBIND11_INTERNALS_VERSION -# define PYBIND11_INTERNALS_VERSION 5 +# define PYBIND11_INTERNALS_VERSION 4 #endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -209,9 +209,6 @@ struct internals { PYBIND11_TLS_FREE(tstate); } #endif -#if PYBIND11_INTERNALS_VERSION > 4 - type_map native_enum_types; -#endif }; /// Additional type information which does not fit into the PyTypeObject. @@ -300,15 +297,16 @@ struct type_info { # endif #endif +#define PYBIND11_ABI_ID \ + PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI \ + PYBIND11_BUILD_TYPE + #define PYBIND11_INTERNALS_ID \ - "__pybind11_internals_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) \ - PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI \ - PYBIND11_BUILD_TYPE "__" + "__pybind11_internals_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) PYBIND11_ABI_ID "__" #define PYBIND11_MODULE_LOCAL_ID \ - "__pybind11_module_local_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) \ - PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI \ - PYBIND11_BUILD_TYPE "__" + "__pybind11_module_local_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) PYBIND11_ABI_ID "_" \ + "_" /// Each module locally stores a pointer to the `internals` data. The data /// itself is shared among modules with the same `PYBIND11_INTERNALS_ID`. @@ -445,6 +443,21 @@ inline object get_python_state_dict() { return state_dict; } +#if defined(WITH_THREAD) +# if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +using gil_scoped_acquire_simple = gil_scoped_acquire; +# else +// Cannot use py::gil_scoped_acquire here since that constructor calls get_internals. +struct gil_scoped_acquire_simple { + gil_scoped_acquire_simple() : state(PyGILState_Ensure()) {} + gil_scoped_acquire_simple(const gil_scoped_acquire_simple &) = delete; + gil_scoped_acquire_simple &operator=(const gil_scoped_acquire_simple &) = delete; + ~gil_scoped_acquire_simple() { PyGILState_Release(state); } + const PyGILState_STATE state; +}; +# endif +#endif + /// Return a reference to the current `internals` data PYBIND11_NOINLINE internals &get_internals() { internals **&internals_pp = get_internals_pp(); @@ -452,21 +465,7 @@ PYBIND11_NOINLINE internals &get_internals() { return **internals_pp; } -#if defined(WITH_THREAD) -# if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) - gil_scoped_acquire gil; -# else - // Ensure that the GIL is held since we will need to make Python calls. - // Cannot use py::gil_scoped_acquire here since that constructor calls get_internals. - struct gil_scoped_acquire_local { - gil_scoped_acquire_local() : state(PyGILState_Ensure()) {} - gil_scoped_acquire_local(const gil_scoped_acquire_local &) = delete; - gil_scoped_acquire_local &operator=(const gil_scoped_acquire_local &) = delete; - ~gil_scoped_acquire_local() { PyGILState_Release(state); } - const PyGILState_STATE state; - } gil; -# endif -#endif + gil_scoped_acquire_simple gil; error_scope err_scope; constexpr const char *id_cstr = PYBIND11_INTERNALS_ID; @@ -646,4 +645,55 @@ T &get_or_create_shared_data(const std::string &name) { return *ptr; } +PYBIND11_NAMESPACE_BEGIN(detail) + +#define PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID \ + "__pybind11_native_enum_type_map_v1" PYBIND11_ABI_ID "__" + +using native_enum_type_map = type_map; + +inline native_enum_type_map **&get_native_enum_types_pp() { + static native_enum_type_map **native_enum_types_pp = nullptr; + return native_enum_types_pp; +} + +PYBIND11_NOINLINE native_enum_type_map &get_native_enum_type_map() { + native_enum_type_map **&native_enum_type_map_pp = get_native_enum_types_pp(); + if (native_enum_type_map_pp && *native_enum_type_map_pp) { + return **native_enum_type_map_pp; + } + + gil_scoped_acquire_simple gil; + error_scope err_scope; + + constexpr const char *id_cstr = PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID; + str id(id_cstr); + + dict state_dict = get_python_state_dict(); + + if (state_dict.contains(id_cstr)) { + void *raw_ptr = PyCapsule_GetPointer(state_dict[id].ptr(), id_cstr); + if (raw_ptr == nullptr) { + raise_from(PyExc_SystemError, + "pybind11::detail::get_native_enum_type_map(): Retrieve " + "native_enum_type_map** from capsule FAILED"); + } + native_enum_type_map_pp = static_cast(raw_ptr); + } + + if (native_enum_type_map_pp && *native_enum_type_map_pp) { + return **native_enum_type_map_pp; + } + + if (!native_enum_type_map_pp) { + native_enum_type_map_pp = new native_enum_type_map *(); + } + auto *&native_enum_type_map_ptr = *native_enum_type_map_pp; + native_enum_type_map_ptr = new native_enum_type_map(); + state_dict[id] = capsule(native_enum_type_map_pp, id_cstr); + return **native_enum_type_map_pp; +} + +PYBIND11_NAMESPACE_END(detail) + PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 2d306981a1..9a17439edf 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -32,7 +32,7 @@ class native_enum : public detail::native_enum_data { "pybind11::native_enum<...>(\"" + enum_name_encoded + "\") is already registered as a `pybind11::enum_` or `pybind11::class_`!"); } - if (detail::get_internals().native_enum_types.count(enum_type_index)) { + if (detail::get_native_enum_type_map().count(enum_type_index)) { pybind11_fail("pybind11::native_enum<...>(\"" + enum_name_encoded + "\") is already registered!"); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d68cfd3132..7b7da1f559 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1296,7 +1296,7 @@ class module_ : public object { py_enum[doc[int_(0)]].attr("__doc__") = doc[int_(1)]; } // Intentionally leak Python reference. - detail::get_internals().native_enum_types[data.enum_type_index] = py_enum.release().ptr(); + detail::get_native_enum_type_map()[data.enum_type_index] = py_enum.release().ptr(); return *this; } }; @@ -2204,7 +2204,7 @@ class enum_ : public class_ { enum_(const handle &scope, const char *name, const Extra &...extra) : class_(scope, name, extra...), m_base(*this, scope) { { - auto const &natives = detail::get_internals().native_enum_types; + auto const &natives = detail::get_native_enum_type_map(); auto found = natives.find(std::type_index(typeid(Type))); if (found != natives.end()) { pybind11_fail("pybind11::enum_ \"" + std::string(name) diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 0cb11c14c5..5818edd491 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -67,6 +67,8 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; + m.attr("PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID") = PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID; + m += py::native_enum("smallenum") .value("a", smallenum::a) .value("b", smallenum::b) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index d078e1a371..18e4070ca8 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -1,9 +1,18 @@ import enum +import re import pytest from pybind11_tests import native_enum as m + +def test_abi_id(): + assert re.match( + "__pybind11_native_enum_type_map_v1_.*__$", + m.PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID, + ) + + SMALLENUM_MEMBERS = ( ("a", 0), ("b", 1), From 3debb517d08aa056093363904e4e3bdae3932a78 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 18 Nov 2022 08:55:52 -0800 Subject: [PATCH 31/35] Organize internals-like functionality as `struct native_enum_type_map_v1`, including cleanup in embed.h (no more leaking). --- include/pybind11/cast.h | 8 +-- include/pybind11/detail/internals.h | 95 +++++++++++++++++++---------- include/pybind11/embed.h | 2 + include/pybind11/native_enum.h | 2 +- include/pybind11/pybind11.h | 5 +- tests/conftest.py | 8 ++- tests/test_native_enum.cpp | 2 +- tests/test_native_enum.py | 3 +- 8 files changed, 81 insertions(+), 44 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index e8fe376450..107df68e95 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -58,7 +58,7 @@ class type_caster_enum_type { template static handle cast(SrcType &&src, return_value_policy, handle parent) { - auto const &natives = get_native_enum_type_map(); + auto const &natives = native_enum_type_map::get(); auto found = natives.find(std::type_index(typeid(EnumType))); if (found != natives.end()) { return handle(found->second)(static_cast(src)).release(); @@ -71,7 +71,7 @@ class type_caster_enum_type { } bool load(handle src, bool convert) { - auto const &natives = get_native_enum_type_map(); + auto const &natives = native_enum_type_map::get(); auto found = natives.find(std::type_index(typeid(EnumType))); if (found != natives.end()) { if (!isinstance(src, found->second)) { @@ -125,7 +125,7 @@ class type_caster::value, int> = 0> bool isinstance_native_enum_impl(handle obj, const std::type_info &tp) { - auto const &natives = get_native_enum_type_map(); + auto const &natives = native_enum_type_map::get(); auto found = natives.find(tp); if (found == natives.end()) { return false; @@ -1138,7 +1138,7 @@ T cast(const handle &handle) { "Unable to cast type to reference: value is local to type caster"); #ifndef NDEBUG if (is_enum_cast) { - auto const &natives = get_native_enum_type_map(); + auto const &natives = native_enum_type_map::get(); auto found = natives.find(std::type_index(typeid(intrinsic_t))); if (found != natives.end()) { pybind11_fail("Unable to cast native enum type to reference"); diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 878a23ded0..6adcacdbbf 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -311,6 +311,8 @@ struct type_info { /// Each module locally stores a pointer to the `internals` data. The data /// itself is shared among modules with the same `PYBIND11_INTERNALS_ID`. inline internals **&get_internals_pp() { + // The reason for the double-indirection is documented here: + // https://github.com/pybind/pybind11/pull/1092 static internals **internals_pp = nullptr; return internals_pp; } @@ -647,52 +649,81 @@ T &get_or_create_shared_data(const std::string &name) { PYBIND11_NAMESPACE_BEGIN(detail) -#define PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID \ - "__pybind11_native_enum_type_map_v1" PYBIND11_ABI_ID "__" +struct native_enum_type_map_v1 { + static constexpr const char *abi_id_c_str + = "__pybind11_native_enum_type_map_v1" PYBIND11_ABI_ID "__"; -using native_enum_type_map = type_map; + using native_enum_type_map = type_map; -inline native_enum_type_map **&get_native_enum_types_pp() { - static native_enum_type_map **native_enum_types_pp = nullptr; - return native_enum_types_pp; -} - -PYBIND11_NOINLINE native_enum_type_map &get_native_enum_type_map() { - native_enum_type_map **&native_enum_type_map_pp = get_native_enum_types_pp(); - if (native_enum_type_map_pp && *native_enum_type_map_pp) { - return **native_enum_type_map_pp; + static native_enum_type_map **&native_enum_type_map_pp() { + static native_enum_type_map **pp; + return pp; } - gil_scoped_acquire_simple gil; - error_scope err_scope; + static native_enum_type_map *get_existing() { + if (native_enum_type_map_pp() && *native_enum_type_map_pp()) { + return *native_enum_type_map_pp(); + } - constexpr const char *id_cstr = PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID; - str id(id_cstr); + gil_scoped_acquire_simple gil; + error_scope err_scope; - dict state_dict = get_python_state_dict(); + str abi_id_str(abi_id_c_str); + dict state_dict = get_python_state_dict(); + if (!state_dict.contains(abi_id_str)) { + return nullptr; + } - if (state_dict.contains(id_cstr)) { - void *raw_ptr = PyCapsule_GetPointer(state_dict[id].ptr(), id_cstr); + void *raw_ptr = PyCapsule_GetPointer(state_dict[abi_id_str].ptr(), abi_id_c_str); if (raw_ptr == nullptr) { raise_from(PyExc_SystemError, - "pybind11::detail::get_native_enum_type_map(): Retrieve " - "native_enum_type_map** from capsule FAILED"); + "pybind11::detail::native_enum_type_map::get_existing():" + " Retrieve native_enum_type_map** from capsule FAILED"); } - native_enum_type_map_pp = static_cast(raw_ptr); + native_enum_type_map_pp() = static_cast(raw_ptr); + return *native_enum_type_map_pp(); } - if (native_enum_type_map_pp && *native_enum_type_map_pp) { - return **native_enum_type_map_pp; + static native_enum_type_map &get() { + if (get_existing() != nullptr) { + return **native_enum_type_map_pp(); + } + if (native_enum_type_map_pp() == nullptr) { + native_enum_type_map_pp() = new native_enum_type_map *(); + } + *native_enum_type_map_pp() = new native_enum_type_map(); + get_python_state_dict()[abi_id_c_str] = capsule(native_enum_type_map_pp(), abi_id_c_str); + return **native_enum_type_map_pp(); } - if (!native_enum_type_map_pp) { - native_enum_type_map_pp = new native_enum_type_map *(); - } - auto *&native_enum_type_map_ptr = *native_enum_type_map_pp; - native_enum_type_map_ptr = new native_enum_type_map(); - state_dict[id] = capsule(native_enum_type_map_pp, id_cstr); - return **native_enum_type_map_pp; -} + struct scoped_clear { + // To be called BEFORE Py_Finalize(). + scoped_clear() { + if (get_existing() != nullptr) { + for (auto it : **native_enum_type_map_pp()) { + Py_DECREF(it.second); + } + (*native_enum_type_map_pp())->clear(); + arm_dtor = true; + } + } + + // To be called AFTER Py_Finalize(). + ~scoped_clear() { + if (arm_dtor) { + delete *native_enum_type_map_pp(); + *native_enum_type_map_pp() = nullptr; + } + } + + scoped_clear(const scoped_clear &) = delete; + scoped_clear &operator=(const scoped_clear &) = delete; + + bool arm_dtor = false; + }; +}; + +using native_enum_type_map = native_enum_type_map_v1; PYBIND11_NAMESPACE_END(detail) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 0ac609e0f1..58f5bc0958 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -213,6 +213,8 @@ inline void initialize_interpreter(bool init_signal_handlers = true, \endrst */ inline void finalize_interpreter() { + detail::native_enum_type_map::scoped_clear native_enum_type_map_clear; + handle builtins(PyEval_GetBuiltins()); const char *id = PYBIND11_INTERNALS_ID; diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 9a17439edf..283fd44170 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -32,7 +32,7 @@ class native_enum : public detail::native_enum_data { "pybind11::native_enum<...>(\"" + enum_name_encoded + "\") is already registered as a `pybind11::enum_` or `pybind11::class_`!"); } - if (detail::get_native_enum_type_map().count(enum_type_index)) { + if (detail::native_enum_type_map::get().count(enum_type_index)) { pybind11_fail("pybind11::native_enum<...>(\"" + enum_name_encoded + "\") is already registered!"); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 7b7da1f559..b5a74064e4 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1295,8 +1295,7 @@ class module_ : public object { for (auto doc : data.docs) { py_enum[doc[int_(0)]].attr("__doc__") = doc[int_(1)]; } - // Intentionally leak Python reference. - detail::get_native_enum_type_map()[data.enum_type_index] = py_enum.release().ptr(); + detail::native_enum_type_map::get()[data.enum_type_index] = py_enum.release().ptr(); return *this; } }; @@ -2204,7 +2203,7 @@ class enum_ : public class_ { enum_(const handle &scope, const char *name, const Extra &...extra) : class_(scope, name, extra...), m_base(*this, scope) { { - auto const &natives = detail::get_native_enum_type_map(); + auto const &natives = detail::native_enum_type_map::get(); auto found = natives.find(std::type_index(typeid(Type))); if (found != natives.end()) { pybind11_fail("pybind11::enum_ \"" + std::string(name) diff --git a/tests/conftest.py b/tests/conftest.py index f5ddb9f129..639b57f474 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,11 +9,17 @@ import gc import re import textwrap +import traceback import pytest # Early diagnostic for failed imports -import pybind11_tests +try: + import pybind11_tests +except Exception: + # pytest does not show the traceback without this. + traceback.print_exc() + raise _long_marker = re.compile(r"([0-9])L") _hexadecimal = re.compile(r"0x[0-9a-fA-F]+") diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 5818edd491..d5d8d6ed24 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -67,7 +67,7 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; - m.attr("PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID") = PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID; + m.attr("native_enum_type_map_abi_id_c_str") = py::detail::native_enum_type_map::abi_id_c_str; m += py::native_enum("smallenum") .value("a", smallenum::a) diff --git a/tests/test_native_enum.py b/tests/test_native_enum.py index 18e4070ca8..a0ce8dba7e 100644 --- a/tests/test_native_enum.py +++ b/tests/test_native_enum.py @@ -8,8 +8,7 @@ def test_abi_id(): assert re.match( - "__pybind11_native_enum_type_map_v1_.*__$", - m.PYBIND11_NATIVE_ENUM_TYPE_MAP_ABI_ID, + "__pybind11_native_enum_type_map_v1_.*__$", m.native_enum_type_map_abi_id_c_str ) From 61925d2297820ee1ce67460ab7e856a980b39262 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 18 Nov 2022 11:58:42 -0800 Subject: [PATCH 32/35] Make the internals-like mechanism reusable as `cross_extension_shared_state` --- include/pybind11/detail/internals.h | 77 ++++++++++++++++++----------- tests/test_native_enum.cpp | 2 +- 2 files changed, 48 insertions(+), 31 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 6adcacdbbf..00d32d8404 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -18,6 +18,7 @@ #include "../pytypes.h" #include +#include /// Tracks the `internals` and `type_info` ABI version independent of the main library version. /// @@ -311,8 +312,6 @@ struct type_info { /// Each module locally stores a pointer to the `internals` data. The data /// itself is shared among modules with the same `PYBIND11_INTERNALS_ID`. inline internals **&get_internals_pp() { - // The reason for the double-indirection is documented here: - // https://github.com/pybind/pybind11/pull/1092 static internals **internals_pp = nullptr; return internals_pp; } @@ -649,61 +648,63 @@ T &get_or_create_shared_data(const std::string &name) { PYBIND11_NAMESPACE_BEGIN(detail) -struct native_enum_type_map_v1 { - static constexpr const char *abi_id_c_str - = "__pybind11_native_enum_type_map_v1" PYBIND11_ABI_ID "__"; +template +struct cross_extension_shared_state { + static constexpr const char *abi_id() { return AdapterType::abi_id(); } - using native_enum_type_map = type_map; + using payload_type = typename AdapterType::payload_type; - static native_enum_type_map **&native_enum_type_map_pp() { - static native_enum_type_map **pp; + static payload_type **&payload_pp() { + // The reason for the double-indirection is documented here: + // https://github.com/pybind/pybind11/pull/1092 + static payload_type **pp; return pp; } - static native_enum_type_map *get_existing() { - if (native_enum_type_map_pp() && *native_enum_type_map_pp()) { - return *native_enum_type_map_pp(); + static payload_type *get_existing() { + if (payload_pp() && *payload_pp()) { + return *payload_pp(); } gil_scoped_acquire_simple gil; error_scope err_scope; - str abi_id_str(abi_id_c_str); + str abi_id_str(AdapterType::abi_id()); dict state_dict = get_python_state_dict(); if (!state_dict.contains(abi_id_str)) { return nullptr; } - void *raw_ptr = PyCapsule_GetPointer(state_dict[abi_id_str].ptr(), abi_id_c_str); + void *raw_ptr = PyCapsule_GetPointer(state_dict[abi_id_str].ptr(), AdapterType::abi_id()); if (raw_ptr == nullptr) { raise_from(PyExc_SystemError, - "pybind11::detail::native_enum_type_map::get_existing():" - " Retrieve native_enum_type_map** from capsule FAILED"); + ("pybind11::detail::cross_extension_shared_state::get_existing():" + " Retrieve payload_type** from capsule FAILED for ABI ID \"" + + std::string(AdapterType::abi_id()) + "\"") + .c_str()); } - native_enum_type_map_pp() = static_cast(raw_ptr); - return *native_enum_type_map_pp(); + payload_pp() = static_cast(raw_ptr); + return *payload_pp(); } - static native_enum_type_map &get() { + static payload_type &get() { if (get_existing() != nullptr) { - return **native_enum_type_map_pp(); + return **payload_pp(); } - if (native_enum_type_map_pp() == nullptr) { - native_enum_type_map_pp() = new native_enum_type_map *(); + if (payload_pp() == nullptr) { + payload_pp() = new payload_type *(); } - *native_enum_type_map_pp() = new native_enum_type_map(); - get_python_state_dict()[abi_id_c_str] = capsule(native_enum_type_map_pp(), abi_id_c_str); - return **native_enum_type_map_pp(); + *payload_pp() = new payload_type(); + get_python_state_dict()[AdapterType::abi_id()] + = capsule(payload_pp(), AdapterType::abi_id()); + return **payload_pp(); } struct scoped_clear { // To be called BEFORE Py_Finalize(). scoped_clear() { if (get_existing() != nullptr) { - for (auto it : **native_enum_type_map_pp()) { - Py_DECREF(it.second); - } - (*native_enum_type_map_pp())->clear(); + AdapterType::payload_clear(**payload_pp()); arm_dtor = true; } } @@ -711,8 +712,8 @@ struct native_enum_type_map_v1 { // To be called AFTER Py_Finalize(). ~scoped_clear() { if (arm_dtor) { - delete *native_enum_type_map_pp(); - *native_enum_type_map_pp() = nullptr; + delete *payload_pp(); + *payload_pp() = nullptr; } } @@ -723,6 +724,22 @@ struct native_enum_type_map_v1 { }; }; +struct native_enum_type_map_v1_adapter { + static constexpr const char *abi_id() { + return "__pybind11_native_enum_type_map_v1" PYBIND11_ABI_ID "__"; + } + + using payload_type = type_map; + + static void payload_clear(payload_type &payload) { + for (auto it : payload) { + Py_DECREF(it.second); + } + payload.clear(); + } +}; + +using native_enum_type_map_v1 = cross_extension_shared_state; using native_enum_type_map = native_enum_type_map_v1; PYBIND11_NAMESPACE_END(detail) diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index d5d8d6ed24..3c9ad40ddf 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -67,7 +67,7 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; - m.attr("native_enum_type_map_abi_id_c_str") = py::detail::native_enum_type_map::abi_id_c_str; + m.attr("native_enum_type_map_abi_id_c_str") = py::detail::native_enum_type_map::abi_id(); m += py::native_enum("smallenum") .value("a", smallenum::a) From 3a9ecef2b42742a7b991c170684ee7262cb338f1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 18 Nov 2022 20:58:02 -0800 Subject: [PATCH 33/35] Rearrange code slightly to resolve clang-tidy warning. ``` /__w/pybind11/pybind11/include/pybind11/detail/internals.h:707:17: error: Forming reference to null pointer [clang-analyzer-core.NonNullParamChecker,-warnings-as-errors] AdapterType::payload_clear(**payload_pp()); ^ ``` --- include/pybind11/detail/internals.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 00d32d8404..169b2405c2 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -688,8 +688,9 @@ struct cross_extension_shared_state { } static payload_type &get() { - if (get_existing() != nullptr) { - return **payload_pp(); + payload_type *existing = get_existing(); + if (existing != nullptr) { + return *existing; } if (payload_pp() == nullptr) { payload_pp() = new payload_type *(); @@ -703,8 +704,9 @@ struct cross_extension_shared_state { struct scoped_clear { // To be called BEFORE Py_Finalize(). scoped_clear() { - if (get_existing() != nullptr) { - AdapterType::payload_clear(**payload_pp()); + payload_type *existing = get_existing(); + if (existing != nullptr) { + AdapterType::payload_clear(*existing); arm_dtor = true; } } From f29f6e2f459ca6491f97e59de362b6fb1a319773 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 18 Nov 2022 21:46:45 -0800 Subject: [PATCH 34/35] Use `detail::native_enum_type_map::get().count()` to simplify code slightly. --- include/pybind11/cast.h | 4 +--- include/pybind11/pybind11.h | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 107df68e95..8ed2c799b6 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1138,9 +1138,7 @@ T cast(const handle &handle) { "Unable to cast type to reference: value is local to type caster"); #ifndef NDEBUG if (is_enum_cast) { - auto const &natives = native_enum_type_map::get(); - auto found = natives.find(std::type_index(typeid(intrinsic_t))); - if (found != natives.end()) { + if (native_enum_type_map::get().count(std::type_index(typeid(intrinsic_t))) != 0) { pybind11_fail("Unable to cast native enum type to reference"); } } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b5a74064e4..9cc2716467 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2203,9 +2203,7 @@ class enum_ : public class_ { enum_(const handle &scope, const char *name, const Extra &...extra) : class_(scope, name, extra...), m_base(*this, scope) { { - auto const &natives = detail::native_enum_type_map::get(); - auto found = natives.find(std::type_index(typeid(Type))); - if (found != natives.end()) { + if (detail::native_enum_type_map::get().count(std::type_index(typeid(Type))) != 0) { pybind11_fail("pybind11::enum_ \"" + std::string(name) + "\" is already registered as a pybind11::native_enum!"); } From ad1d258f76d62ae0e869101dce224fc03ec9ea1c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 19 Nov 2022 11:21:42 -0800 Subject: [PATCH 35/35] Factor out pybind11/detail/abi_platform_id.h, type_map.h, cross_extension_shared_state.h from internals.h IWYU cleanup in the new files and internals.h --- CMakeLists.txt | 3 + include/pybind11/cast.h | 11 +- include/pybind11/detail/abi_platform_id.h | 93 ++++++ .../detail/cross_extension_shared_state.h | 142 +++++++++ include/pybind11/detail/internals.h | 271 ++---------------- include/pybind11/detail/native_enum_data.h | 28 ++ include/pybind11/detail/type_map.h | 72 +++++ include/pybind11/embed.h | 2 +- include/pybind11/native_enum.h | 3 +- include/pybind11/pybind11.h | 7 +- tests/extra_python_package/test_files.py | 3 + tests/test_native_enum.cpp | 3 +- 12 files changed, 374 insertions(+), 264 deletions(-) create mode 100644 include/pybind11/detail/abi_platform_id.h create mode 100644 include/pybind11/detail/cross_extension_shared_state.h create mode 100644 include/pybind11/detail/type_map.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 21beea24f3..ae85cf4e73 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -111,13 +111,16 @@ cmake_dependent_option(PYBIND11_FINDPYTHON "Force new FindPython" OFF # NB: when adding a header don't forget to also add it to setup.py set(PYBIND11_HEADERS + include/pybind11/detail/abi_platform_id.h include/pybind11/detail/class.h include/pybind11/detail/common.h + include/pybind11/detail/cross_extension_shared_state.h include/pybind11/detail/descr.h include/pybind11/detail/init.h include/pybind11/detail/internals.h include/pybind11/detail/native_enum_data.h include/pybind11/detail/type_caster_base.h + include/pybind11/detail/type_map.h include/pybind11/detail/typeid.h include/pybind11/attr.h include/pybind11/buffer_info.h diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8ed2c799b6..d3bf67190a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -12,6 +12,7 @@ #include "detail/common.h" #include "detail/descr.h" +#include "detail/native_enum_data.h" #include "detail/type_caster_base.h" #include "detail/typeid.h" #include "pytypes.h" @@ -58,7 +59,7 @@ class type_caster_enum_type { template static handle cast(SrcType &&src, return_value_policy, handle parent) { - auto const &natives = native_enum_type_map::get(); + auto const &natives = cross_extension_shared_states::native_enum_type_map::get(); auto found = natives.find(std::type_index(typeid(EnumType))); if (found != natives.end()) { return handle(found->second)(static_cast(src)).release(); @@ -71,7 +72,7 @@ class type_caster_enum_type { } bool load(handle src, bool convert) { - auto const &natives = native_enum_type_map::get(); + auto const &natives = cross_extension_shared_states::native_enum_type_map::get(); auto found = natives.find(std::type_index(typeid(EnumType))); if (found != natives.end()) { if (!isinstance(src, found->second)) { @@ -125,7 +126,7 @@ class type_caster::value, int> = 0> bool isinstance_native_enum_impl(handle obj, const std::type_info &tp) { - auto const &natives = native_enum_type_map::get(); + auto const &natives = cross_extension_shared_states::native_enum_type_map::get(); auto found = natives.find(tp); if (found == natives.end()) { return false; @@ -1138,7 +1139,9 @@ T cast(const handle &handle) { "Unable to cast type to reference: value is local to type caster"); #ifndef NDEBUG if (is_enum_cast) { - if (native_enum_type_map::get().count(std::type_index(typeid(intrinsic_t))) != 0) { + if (cross_extension_shared_states::native_enum_type_map::get().count( + std::type_index(typeid(intrinsic_t))) + != 0) { pybind11_fail("Unable to cast native enum type to reference"); } } diff --git a/include/pybind11/detail/abi_platform_id.h b/include/pybind11/detail/abi_platform_id.h new file mode 100644 index 0000000000..913ba4d20f --- /dev/null +++ b/include/pybind11/detail/abi_platform_id.h @@ -0,0 +1,93 @@ +// Copyright (c) 2022 The pybind Community. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "common.h" + +/// On MSVC, debug and release builds are not ABI-compatible! +#if defined(_MSC_VER) && defined(_DEBUG) +# define PYBIND11_BUILD_TYPE "_debug" +#else +# define PYBIND11_BUILD_TYPE "" +#endif + +/// Let's assume that different compilers are ABI-incompatible. +/// A user can manually set this string if they know their +/// compiler is compatible. +#ifndef PYBIND11_COMPILER_TYPE +# if defined(_MSC_VER) +# define PYBIND11_COMPILER_TYPE "_msvc" +# elif defined(__INTEL_COMPILER) +# define PYBIND11_COMPILER_TYPE "_icc" +# elif defined(__clang__) +# define PYBIND11_COMPILER_TYPE "_clang" +# elif defined(__PGI) +# define PYBIND11_COMPILER_TYPE "_pgi" +# elif defined(__MINGW32__) +# define PYBIND11_COMPILER_TYPE "_mingw" +# elif defined(__CYGWIN__) +# define PYBIND11_COMPILER_TYPE "_gcc_cygwin" +# elif defined(__GNUC__) +# define PYBIND11_COMPILER_TYPE "_gcc" +# else +# define PYBIND11_COMPILER_TYPE "_unknown" +# endif +#endif + +/// Also standard libs +#ifndef PYBIND11_STDLIB +# if defined(_LIBCPP_VERSION) +# define PYBIND11_STDLIB "_libcpp" +# elif defined(__GLIBCXX__) || defined(__GLIBCPP__) +# define PYBIND11_STDLIB "_libstdcpp" +# else +# define PYBIND11_STDLIB "" +# endif +#endif + +/// On Linux/OSX, changes in __GXX_ABI_VERSION__ indicate ABI incompatibility. +#ifndef PYBIND11_BUILD_ABI +# if defined(__GXX_ABI_VERSION) +# define PYBIND11_BUILD_ABI "_cxxabi" PYBIND11_TOSTRING(__GXX_ABI_VERSION) +# else +# define PYBIND11_BUILD_ABI "" +# endif +#endif + +#ifndef PYBIND11_INTERNALS_KIND +# if defined(WITH_THREAD) +# define PYBIND11_INTERNALS_KIND "" +# else +# define PYBIND11_INTERNALS_KIND "_without_thread" +# endif +#endif + +/* NOTE - ATTENTION - WARNING - EXTREME CAUTION + Changing this will break compatibility with `PYBIND11_INTERNALS_VERSION 4` + See pybind11/detail/type_map.h for more information. + */ +#define PYBIND11_PLATFORM_ABI_ID_V4 \ + PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI \ + PYBIND11_BUILD_TYPE + +/// LEGACY "ABI-breaking" APPROACH, ORIGINAL COMMENT +/// ------------------------------------------------ +/// Tracks the `internals` and `type_info` ABI version independent of the main library version. +/// +/// Some portions of the code use an ABI that is conditional depending on this +/// version number. That allows ABI-breaking changes to be "pre-implemented". +/// Once the default version number is incremented, the conditional logic that +/// no longer applies can be removed. Additionally, users that need not +/// maintain ABI compatibility can increase the version number in order to take +/// advantage of any functionality/efficiency improvements that depend on the +/// newer ABI. +/// +/// WARNING: If you choose to manually increase the ABI version, note that +/// pybind11 may not be tested as thoroughly with a non-default ABI version, and +/// further ABI-incompatible changes may be made before the ABI is officially +/// changed to the new version. +#ifndef PYBIND11_INTERNALS_VERSION +# define PYBIND11_INTERNALS_VERSION 4 +#endif diff --git a/include/pybind11/detail/cross_extension_shared_state.h b/include/pybind11/detail/cross_extension_shared_state.h new file mode 100644 index 0000000000..150df0b052 --- /dev/null +++ b/include/pybind11/detail/cross_extension_shared_state.h @@ -0,0 +1,142 @@ +// Copyright (c) 2022 The pybind Community. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "common.h" + +#if defined(WITH_THREAD) && defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +# include "../gil.h" +#endif + +#include "../pytypes.h" +#include "abi_platform_id.h" + +#include + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +inline object get_python_state_dict() { + object state_dict; +#if (PYBIND11_INTERNALS_VERSION <= 4 && PY_VERSION_HEX < 0x030C0000) \ + || PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) + state_dict = reinterpret_borrow(PyEval_GetBuiltins()); +#else +# if PY_VERSION_HEX < 0x03090000 + PyInterpreterState *istate = _PyInterpreterState_Get(); +# else + PyInterpreterState *istate = PyInterpreterState_Get(); +# endif + if (istate) { + state_dict = reinterpret_borrow(PyInterpreterState_GetDict(istate)); + } +#endif + if (!state_dict) { + raise_from(PyExc_SystemError, "pybind11::detail::get_python_state_dict() FAILED"); + } + return state_dict; +} + +#if defined(WITH_THREAD) +# if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +using gil_scoped_acquire_simple = gil_scoped_acquire; +# else +// Cannot use py::gil_scoped_acquire here since that constructor calls get_internals. +struct gil_scoped_acquire_simple { + gil_scoped_acquire_simple() : state(PyGILState_Ensure()) {} + gil_scoped_acquire_simple(const gil_scoped_acquire_simple &) = delete; + gil_scoped_acquire_simple &operator=(const gil_scoped_acquire_simple &) = delete; + ~gil_scoped_acquire_simple() { PyGILState_Release(state); } + const PyGILState_STATE state; +}; +# endif +#endif + +/* NOTE: struct cross_extension_shared_state is in + namespace pybind11::detail + but all types using this struct are meant to live in + namespace pybind11::cross_extension_shared_states + to make them easy to discover and reason about. + */ +template +struct cross_extension_shared_state { + static constexpr const char *abi_id() { return AdapterType::abi_id(); } + + using payload_type = typename AdapterType::payload_type; + + static payload_type **&payload_pp() { + // The reason for the double-indirection is documented here: + // https://github.com/pybind/pybind11/pull/1092 + static payload_type **pp; + return pp; + } + + static payload_type *get_existing() { + if (payload_pp() && *payload_pp()) { + return *payload_pp(); + } + + gil_scoped_acquire_simple gil; + error_scope err_scope; + + str abi_id_str(AdapterType::abi_id()); + dict state_dict = get_python_state_dict(); + if (!state_dict.contains(abi_id_str)) { + return nullptr; + } + + void *raw_ptr = PyCapsule_GetPointer(state_dict[abi_id_str].ptr(), AdapterType::abi_id()); + if (raw_ptr == nullptr) { + raise_from(PyExc_SystemError, + ("pybind11::detail::cross_extension_shared_state::get_existing():" + " Retrieve payload_type** from capsule FAILED for ABI ID \"" + + std::string(AdapterType::abi_id()) + "\"") + .c_str()); + } + payload_pp() = static_cast(raw_ptr); + return *payload_pp(); + } + + static payload_type &get() { + payload_type *existing = get_existing(); + if (existing != nullptr) { + return *existing; + } + if (payload_pp() == nullptr) { + payload_pp() = new payload_type *(); + } + *payload_pp() = new payload_type(); + get_python_state_dict()[AdapterType::abi_id()] + = capsule(payload_pp(), AdapterType::abi_id()); + return **payload_pp(); + } + + struct scoped_clear { + // To be called BEFORE Py_Finalize(). + scoped_clear() { + payload_type *existing = get_existing(); + if (existing != nullptr) { + AdapterType::payload_clear(*existing); + arm_dtor = true; + } + } + + // To be called AFTER Py_Finalize(). + ~scoped_clear() { + if (arm_dtor) { + delete *payload_pp(); + *payload_pp() = nullptr; + } + } + + scoped_clear(const scoped_clear &) = delete; + scoped_clear &operator=(const scoped_clear &) = delete; + + bool arm_dtor = false; + }; +}; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 169b2405c2..4aaf138960 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -16,27 +16,21 @@ #endif #include "../pytypes.h" +#include "abi_platform_id.h" +#include "cross_extension_shared_state.h" +#include "type_map.h" #include +#include +#include +#include #include - -/// Tracks the `internals` and `type_info` ABI version independent of the main library version. -/// -/// Some portions of the code use an ABI that is conditional depending on this -/// version number. That allows ABI-breaking changes to be "pre-implemented". -/// Once the default version number is incremented, the conditional logic that -/// no longer applies can be removed. Additionally, users that need not -/// maintain ABI compatibility can increase the version number in order to take -/// advantage of any functionality/efficiency improvements that depend on the -/// newer ABI. -/// -/// WARNING: If you choose to manually increase the ABI version, note that -/// pybind11 may not be tested as thoroughly with a non-default ABI version, and -/// further ABI-incompatible changes may be made before the ABI is officially -/// changed to the new version. -#ifndef PYBIND11_INTERNALS_VERSION -# define PYBIND11_INTERNALS_VERSION 4 -#endif +#include +#include +#include +#include +#include +#include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -109,42 +103,6 @@ inline void tls_replace_value(PYBIND11_TLS_KEY_REF key, void *value) { # define PYBIND11_TLS_FREE(key) (void) key #endif -// Python loads modules by default with dlopen with the RTLD_LOCAL flag; under libc++ and possibly -// other STLs, this means `typeid(A)` from one module won't equal `typeid(A)` from another module -// even when `A` is the same, non-hidden-visibility type (e.g. from a common include). Under -// libstdc++, this doesn't happen: equality and the type_index hash are based on the type name, -// which works. If not under a known-good stl, provide our own name-based hash and equality -// functions that use the type name. -#if defined(__GLIBCXX__) -inline bool same_type(const std::type_info &lhs, const std::type_info &rhs) { return lhs == rhs; } -using type_hash = std::hash; -using type_equal_to = std::equal_to; -#else -inline bool same_type(const std::type_info &lhs, const std::type_info &rhs) { - return lhs.name() == rhs.name() || std::strcmp(lhs.name(), rhs.name()) == 0; -} - -struct type_hash { - size_t operator()(const std::type_index &t) const { - size_t hash = 5381; - const char *ptr = t.name(); - while (auto c = static_cast(*ptr++)) { - hash = (hash * 33) ^ c; - } - return hash; - } -}; - -struct type_equal_to { - bool operator()(const std::type_index &lhs, const std::type_index &rhs) const { - return lhs.name() == rhs.name() || std::strcmp(lhs.name(), rhs.name()) == 0; - } -}; -#endif - -template -using type_map = std::unordered_map; - struct override_hash { inline size_t operator()(const std::pair &v) const { size_t value = std::hash()(v.first); @@ -240,74 +198,13 @@ struct type_info { bool module_local : 1; }; -/// On MSVC, debug and release builds are not ABI-compatible! -#if defined(_MSC_VER) && defined(_DEBUG) -# define PYBIND11_BUILD_TYPE "_debug" -#else -# define PYBIND11_BUILD_TYPE "" -#endif - -/// Let's assume that different compilers are ABI-incompatible. -/// A user can manually set this string if they know their -/// compiler is compatible. -#ifndef PYBIND11_COMPILER_TYPE -# if defined(_MSC_VER) -# define PYBIND11_COMPILER_TYPE "_msvc" -# elif defined(__INTEL_COMPILER) -# define PYBIND11_COMPILER_TYPE "_icc" -# elif defined(__clang__) -# define PYBIND11_COMPILER_TYPE "_clang" -# elif defined(__PGI) -# define PYBIND11_COMPILER_TYPE "_pgi" -# elif defined(__MINGW32__) -# define PYBIND11_COMPILER_TYPE "_mingw" -# elif defined(__CYGWIN__) -# define PYBIND11_COMPILER_TYPE "_gcc_cygwin" -# elif defined(__GNUC__) -# define PYBIND11_COMPILER_TYPE "_gcc" -# else -# define PYBIND11_COMPILER_TYPE "_unknown" -# endif -#endif - -/// Also standard libs -#ifndef PYBIND11_STDLIB -# if defined(_LIBCPP_VERSION) -# define PYBIND11_STDLIB "_libcpp" -# elif defined(__GLIBCXX__) || defined(__GLIBCPP__) -# define PYBIND11_STDLIB "_libstdcpp" -# else -# define PYBIND11_STDLIB "" -# endif -#endif - -/// On Linux/OSX, changes in __GXX_ABI_VERSION__ indicate ABI incompatibility. -#ifndef PYBIND11_BUILD_ABI -# if defined(__GXX_ABI_VERSION) -# define PYBIND11_BUILD_ABI "_cxxabi" PYBIND11_TOSTRING(__GXX_ABI_VERSION) -# else -# define PYBIND11_BUILD_ABI "" -# endif -#endif - -#ifndef PYBIND11_INTERNALS_KIND -# if defined(WITH_THREAD) -# define PYBIND11_INTERNALS_KIND "" -# else -# define PYBIND11_INTERNALS_KIND "_without_thread" -# endif -#endif - -#define PYBIND11_ABI_ID \ - PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI \ - PYBIND11_BUILD_TYPE - #define PYBIND11_INTERNALS_ID \ - "__pybind11_internals_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) PYBIND11_ABI_ID "__" + "__pybind11_internals_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) \ + PYBIND11_PLATFORM_ABI_ID_V4 "__" #define PYBIND11_MODULE_LOCAL_ID \ - "__pybind11_module_local_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) PYBIND11_ABI_ID "_" \ - "_" + "__pybind11_module_local_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) \ + PYBIND11_PLATFORM_ABI_ID_V4 "__" /// Each module locally stores a pointer to the `internals` data. The data /// itself is shared among modules with the same `PYBIND11_INTERNALS_ID`. @@ -423,42 +320,6 @@ inline void translate_local_exception(std::exception_ptr p) { } #endif -inline object get_python_state_dict() { - object state_dict; -#if (PYBIND11_INTERNALS_VERSION <= 4 && PY_VERSION_HEX < 0x030C0000) \ - || PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) - state_dict = reinterpret_borrow(PyEval_GetBuiltins()); -#else -# if PY_VERSION_HEX < 0x03090000 - PyInterpreterState *istate = _PyInterpreterState_Get(); -# else - PyInterpreterState *istate = PyInterpreterState_Get(); -# endif - if (istate) { - state_dict = reinterpret_borrow(PyInterpreterState_GetDict(istate)); - } -#endif - if (!state_dict) { - raise_from(PyExc_SystemError, "pybind11::detail::get_python_state_dict() FAILED"); - } - return state_dict; -} - -#if defined(WITH_THREAD) -# if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) -using gil_scoped_acquire_simple = gil_scoped_acquire; -# else -// Cannot use py::gil_scoped_acquire here since that constructor calls get_internals. -struct gil_scoped_acquire_simple { - gil_scoped_acquire_simple() : state(PyGILState_Ensure()) {} - gil_scoped_acquire_simple(const gil_scoped_acquire_simple &) = delete; - gil_scoped_acquire_simple &operator=(const gil_scoped_acquire_simple &) = delete; - ~gil_scoped_acquire_simple() { PyGILState_Release(state); } - const PyGILState_STATE state; -}; -# endif -#endif - /// Return a reference to the current `internals` data PYBIND11_NOINLINE internals &get_internals() { internals **&internals_pp = get_internals_pp(); @@ -646,104 +507,4 @@ T &get_or_create_shared_data(const std::string &name) { return *ptr; } -PYBIND11_NAMESPACE_BEGIN(detail) - -template -struct cross_extension_shared_state { - static constexpr const char *abi_id() { return AdapterType::abi_id(); } - - using payload_type = typename AdapterType::payload_type; - - static payload_type **&payload_pp() { - // The reason for the double-indirection is documented here: - // https://github.com/pybind/pybind11/pull/1092 - static payload_type **pp; - return pp; - } - - static payload_type *get_existing() { - if (payload_pp() && *payload_pp()) { - return *payload_pp(); - } - - gil_scoped_acquire_simple gil; - error_scope err_scope; - - str abi_id_str(AdapterType::abi_id()); - dict state_dict = get_python_state_dict(); - if (!state_dict.contains(abi_id_str)) { - return nullptr; - } - - void *raw_ptr = PyCapsule_GetPointer(state_dict[abi_id_str].ptr(), AdapterType::abi_id()); - if (raw_ptr == nullptr) { - raise_from(PyExc_SystemError, - ("pybind11::detail::cross_extension_shared_state::get_existing():" - " Retrieve payload_type** from capsule FAILED for ABI ID \"" - + std::string(AdapterType::abi_id()) + "\"") - .c_str()); - } - payload_pp() = static_cast(raw_ptr); - return *payload_pp(); - } - - static payload_type &get() { - payload_type *existing = get_existing(); - if (existing != nullptr) { - return *existing; - } - if (payload_pp() == nullptr) { - payload_pp() = new payload_type *(); - } - *payload_pp() = new payload_type(); - get_python_state_dict()[AdapterType::abi_id()] - = capsule(payload_pp(), AdapterType::abi_id()); - return **payload_pp(); - } - - struct scoped_clear { - // To be called BEFORE Py_Finalize(). - scoped_clear() { - payload_type *existing = get_existing(); - if (existing != nullptr) { - AdapterType::payload_clear(*existing); - arm_dtor = true; - } - } - - // To be called AFTER Py_Finalize(). - ~scoped_clear() { - if (arm_dtor) { - delete *payload_pp(); - *payload_pp() = nullptr; - } - } - - scoped_clear(const scoped_clear &) = delete; - scoped_clear &operator=(const scoped_clear &) = delete; - - bool arm_dtor = false; - }; -}; - -struct native_enum_type_map_v1_adapter { - static constexpr const char *abi_id() { - return "__pybind11_native_enum_type_map_v1" PYBIND11_ABI_ID "__"; - } - - using payload_type = type_map; - - static void payload_clear(payload_type &payload) { - for (auto it : payload) { - Py_DECREF(it.second); - } - payload.clear(); - } -}; - -using native_enum_type_map_v1 = cross_extension_shared_state; -using native_enum_type_map = native_enum_type_map_v1; - -PYBIND11_NAMESPACE_END(detail) - PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/detail/native_enum_data.h b/include/pybind11/detail/native_enum_data.h index 2e0a3c4870..e639f545b0 100644 --- a/include/pybind11/detail/native_enum_data.h +++ b/include/pybind11/detail/native_enum_data.h @@ -5,8 +5,12 @@ #pragma once #include "../pytypes.h" +#include "abi_platform_id.h" #include "common.h" +#include "cross_extension_shared_state.h" +#include "type_map.h" +#include #include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -56,4 +60,28 @@ class native_enum_data { }; PYBIND11_NAMESPACE_END(detail) + +PYBIND11_NAMESPACE_BEGIN(cross_extension_shared_states) + +struct native_enum_type_map_v1_adapter { + static constexpr const char *abi_id() { + return "__pybind11_native_enum_type_map_v1" PYBIND11_PLATFORM_ABI_ID_V4 "__"; + } + + using payload_type = detail::type_map; + + static void payload_clear(payload_type &payload) { + for (auto it : payload) { + Py_DECREF(it.second); + } + payload.clear(); + } +}; + +using native_enum_type_map_v1 + = detail::cross_extension_shared_state; +using native_enum_type_map = native_enum_type_map_v1; + +PYBIND11_NAMESPACE_END(cross_extension_shared_states) + PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/detail/type_map.h b/include/pybind11/detail/type_map.h new file mode 100644 index 0000000000..e899f3eadc --- /dev/null +++ b/include/pybind11/detail/type_map.h @@ -0,0 +1,72 @@ +// Copyright (c) 2022 The pybind Community. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "common.h" + +#include +#include +#include +#include + +/* NOTE - ATTENTION - WARNING - EXTREME CAUTION + + Almost any changes here will break compatibility with `PYBIND11_INTERNALS_VERSION 4` + + Recommendation: + To not break compatibility with many existing PyPI wheels (https://pypi.org/), + DO NOT MAKE CHANGES HERE + until Python 3.11 (at least) has reached EOL. Then remove this file entirely. + + To evolve this code, start with a copy of this file and move the code to + namespace pybind11::cross_extension_shared_states + with new, versioned names, e.g. type_map_v2. + */ + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +// Python loads modules by default with dlopen with the RTLD_LOCAL flag; under libc++ and possibly +// other STLs, this means `typeid(A)` from one module won't equal `typeid(A)` from another module +// even when `A` is the same, non-hidden-visibility type (e.g. from a common include). Under +// libstdc++, this doesn't happen: equality and the type_index hash are based on the type name, +// which works. If not under a known-good stl, provide our own name-based hash and equality +// functions that use the type name. +#if defined(__GLIBCXX__) + +inline bool same_type(const std::type_info &lhs, const std::type_info &rhs) { return lhs == rhs; } +using type_hash = std::hash; +using type_equal_to = std::equal_to; + +#else + +inline bool same_type(const std::type_info &lhs, const std::type_info &rhs) { + return lhs.name() == rhs.name() || std::strcmp(lhs.name(), rhs.name()) == 0; +} + +struct type_hash { + size_t operator()(const std::type_index &t) const { + size_t hash = 5381; + const char *ptr = t.name(); + while (auto c = static_cast(*ptr++)) { + hash = (hash * 33) ^ c; + } + return hash; + } +}; + +struct type_equal_to { + bool operator()(const std::type_index &lhs, const std::type_index &rhs) const { + return lhs.name() == rhs.name() || std::strcmp(lhs.name(), rhs.name()) == 0; + } +}; + +#endif + +template +using type_map = std::unordered_map; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 58f5bc0958..2138fce9ce 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -213,7 +213,7 @@ inline void initialize_interpreter(bool init_signal_handlers = true, \endrst */ inline void finalize_interpreter() { - detail::native_enum_type_map::scoped_clear native_enum_type_map_clear; + cross_extension_shared_states::native_enum_type_map::scoped_clear native_enum_type_map_clear; handle builtins(PyEval_GetBuiltins()); const char *id = PYBIND11_INTERNALS_ID; diff --git a/include/pybind11/native_enum.h b/include/pybind11/native_enum.h index 283fd44170..13c15ab905 100644 --- a/include/pybind11/native_enum.h +++ b/include/pybind11/native_enum.h @@ -6,6 +6,7 @@ #include "detail/common.h" #include "detail/native_enum_data.h" +#include "detail/type_caster_base.h" #include "cast.h" #include @@ -32,7 +33,7 @@ class native_enum : public detail::native_enum_data { "pybind11::native_enum<...>(\"" + enum_name_encoded + "\") is already registered as a `pybind11::enum_` or `pybind11::class_`!"); } - if (detail::native_enum_type_map::get().count(enum_type_index)) { + if (cross_extension_shared_states::native_enum_type_map::get().count(enum_type_index)) { pybind11_fail("pybind11::native_enum<...>(\"" + enum_name_encoded + "\") is already registered!"); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 9cc2716467..8f4f7269af 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1295,7 +1295,8 @@ class module_ : public object { for (auto doc : data.docs) { py_enum[doc[int_(0)]].attr("__doc__") = doc[int_(1)]; } - detail::native_enum_type_map::get()[data.enum_type_index] = py_enum.release().ptr(); + cross_extension_shared_states::native_enum_type_map::get()[data.enum_type_index] + = py_enum.release().ptr(); return *this; } }; @@ -2203,7 +2204,9 @@ class enum_ : public class_ { enum_(const handle &scope, const char *name, const Extra &...extra) : class_(scope, name, extra...), m_base(*this, scope) { { - if (detail::native_enum_type_map::get().count(std::type_index(typeid(Type))) != 0) { + if (cross_extension_shared_states::native_enum_type_map::get().count( + std::type_index(typeid(Type))) + != 0) { pybind11_fail("pybind11::enum_ \"" + std::string(name) + "\" is already registered as a pybind11::native_enum!"); } diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 86b60cbac5..a9b09cd613 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -47,13 +47,16 @@ } detail_headers = { + "include/pybind11/detail/abi_platform_id.h", "include/pybind11/detail/class.h", "include/pybind11/detail/common.h", + "include/pybind11/detail/cross_extension_shared_state.h", "include/pybind11/detail/descr.h", "include/pybind11/detail/init.h", "include/pybind11/detail/internals.h", "include/pybind11/detail/native_enum_data.h", "include/pybind11/detail/type_caster_base.h", + "include/pybind11/detail/type_map.h", "include/pybind11/detail/typeid.h", } diff --git a/tests/test_native_enum.cpp b/tests/test_native_enum.cpp index 3c9ad40ddf..d634f54db1 100644 --- a/tests/test_native_enum.cpp +++ b/tests/test_native_enum.cpp @@ -67,7 +67,8 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) TEST_SUBMODULE(native_enum, m) { using namespace test_native_enum; - m.attr("native_enum_type_map_abi_id_c_str") = py::detail::native_enum_type_map::abi_id(); + 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") .value("a", smallenum::a)