Organize internals-like functionality as struct native_enum_type_map_v1, including cleanup in embed.h (no more leaking).

Author: rwgk

include/pybind11/cast.h

@@ -58,7 +58,7 @@ class type_caster_enum_type {
 
     template <typename SrcType>
     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<Underlying>(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<EnumType,
 
 template <typename T, detail::enable_if_t<std::is_enum<T>::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<T>)));
         if (found != natives.end()) {
             pybind11_fail("Unable to cast native enum type to reference");

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<PyObject *>;
+    using native_enum_type_map = type_map<PyObject *>;
 
-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<native_enum_type_map **>(raw_ptr);
+        native_enum_type_map_pp() = static_cast<native_enum_type_map **>(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)
 

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;
 

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!");
         }

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_<Type> {
     enum_(const handle &scope, const char *name, const Extra &...extra)
         : class_<Type>(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)

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]+")

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>("smallenum")
              .value("a", smallenum::a)

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
     )