From 4b159230d9a7b543307fe5830df8fc9f17dd6482 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Fri, 4 Aug 2017 13:05:12 -0400 Subject: [PATCH] Made module_local types take precedence over global types Attempting to mix py::module_local and non-module_local classes results in some unexpected/undesirable behaviour: - if a class is registered non-local by some other module, a later attempt to register it locally fails. It doesn't need to: it is perfectly acceptable for the local registration to simply override the external global registration. - going the other way (i.e. module `A` registers a type `T` locally, then `B` registers the same type `T` globally) causes a more serious issue: `A.T`'s constructors no longer work because the `self` argument gets converted to a `B.T`, which then fails to resolve. Changing the cast precedence to prefer local over global fixes this and makes it work more consistently, regardless of module load order. --- docs/advanced/classes.rst | 5 +++++ include/pybind11/cast.h | 17 +++++++++------ include/pybind11/pybind11.h | 2 +- tests/local_bindings.h | 4 ++++ tests/pybind11_cross_module_tests.cpp | 12 +++++++++++ tests/test_local_bindings.cpp | 12 +++++++++++ tests/test_local_bindings.py | 31 +++++++++++++++++++++++++++ 7 files changed, 76 insertions(+), 7 deletions(-) diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 71a7a8859..04eb2530f 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -752,6 +752,11 @@ you will only be able to call the function with the local module's class: Invoked with: +It is possible to use ``py::module_local()`` registrations in one module even if another module +registers the same type globally: within the module with the module-local definition, all C++ +instances will be cast to the associated bound Python type. Outside the module, any such values +are converted to the global Python type created elsewhere. + .. note:: STL bindings (as provided via the optional :file:`pybind11/stl_bind.h` diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index d7dcb6ef5..cfd565e59 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -266,17 +266,22 @@ PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type) { } /// Return the type info for a given C++ type; on lookup failure can either throw or return nullptr. +/// `check_global_types` can be specified as `false` to only check types registered locally to the +/// current module. PYBIND11_NOINLINE inline detail::type_info *get_type_info(const std::type_index &tp, - bool throw_if_missing = false) { + bool throw_if_missing = false, + bool check_global_types = true) { std::type_index type_idx(tp); - auto &types = get_internals().registered_types_cpp; - auto it = types.find(type_idx); - if (it != types.end()) - return (detail::type_info *) it->second; auto &locals = registered_local_types_cpp(); - it = locals.find(type_idx); + auto it = locals.find(type_idx); if (it != locals.end()) return (detail::type_info *) it->second; + if (check_global_types) { + auto &types = get_internals().registered_types_cpp; + it = types.find(type_idx); + if (it != types.end()) + return (detail::type_info *) it->second; + } if (throw_if_missing) { std::string tname = tp.name(); detail::clean_type_id(tname); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 96dea6551..2e6970165 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -822,7 +822,7 @@ protected: pybind11_fail("generic_type: cannot initialize type \"" + std::string(rec.name) + "\": an object with that name is already defined"); - if (get_type_info(*rec.type)) + if (get_type_info(*rec.type, false /* don't throw */, !rec.module_local)) pybind11_fail("generic_type: type \"" + std::string(rec.name) + "\" is already registered!"); diff --git a/tests/local_bindings.h b/tests/local_bindings.h index 0c5336930..4c031fb7f 100644 --- a/tests/local_bindings.h +++ b/tests/local_bindings.h @@ -16,6 +16,10 @@ using NonLocalType = LocalBase<1>; using NonLocal2 = LocalBase<2>; /// Tests within-module, different-compilation-unit local definition conflict: using LocalExternal = LocalBase<3>; +/// Mixed: registered local first, then global +using MixedLocalGlobal = LocalBase<4>; +/// Mixed: global first, then local (which fails) +using MixedGlobalLocal = LocalBase<5>; // Simple bindings (used with the above): template diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index f417a8944..dca8ca2e2 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -65,6 +65,18 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m) { py::bind_map>(m, "NonLocalMap2", py::module_local(false)); }); + // test_mixed_local_global + // We try this both with the global type registered first and vice versa (the order shouldn't + // matter). + m.def("register_mixed_global_local", [m]() { + bind_local(m, "MixedGlobalLocal", py::module_local()); + }); + m.def("register_mixed_local_global", [m]() { + bind_local(m, "MixedLocalGlobal", py::module_local(false)); + }); + m.def("get_mixed_gl", [](int i) { return MixedGlobalLocal(i); }); + m.def("get_mixed_lg", [](int i) { return MixedLocalGlobal(i); }); + // test_internal_locals_differ m.def("local_cpp_types_addr", []() { return (uintptr_t) &py::detail::registered_local_types_cpp(); }); } diff --git a/tests/test_local_bindings.cpp b/tests/test_local_bindings.cpp index d98840f62..e46f8f5f2 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -57,6 +57,18 @@ TEST_SUBMODULE(local_bindings, m) { py::bind_vector>(m, "LocalVec2", py::module_local()); py::bind_map>(m, "NonLocalMap2", py::module_local(false)); + // test_mixed_local_global + // We try this both with the global type registered first and vice versa (the order shouldn't + // matter). + m.def("register_mixed_global", [m]() { + bind_local(m, "MixedGlobalLocal", py::module_local(false)); + }); + m.def("register_mixed_local", [m]() { + bind_local(m, "MixedLocalGlobal", py::module_local()); + }); + m.def("get_mixed_gl", [](int i) { return MixedGlobalLocal(i); }); + m.def("get_mixed_lg", [](int i) { return MixedLocalGlobal(i); }); + // test_internal_locals_differ m.def("local_cpp_types_addr", []() { return (uintptr_t) &py::detail::registered_local_types_cpp(); }); } diff --git a/tests/test_local_bindings.py b/tests/test_local_bindings.py index 4c5a87409..4e356653c 100644 --- a/tests/test_local_bindings.py +++ b/tests/test_local_bindings.py @@ -103,6 +103,37 @@ def test_stl_bind_global(): assert str(excinfo.value) == 'generic_type: type "NonLocalMap2" is already registered!' +def test_mixed_local_global(): + """Local types take precedence over globally registered types: a module with a `module_local` + type can be registered even if the type is already registered globally. With the module, + casting will go to the local type; outside the module casting goes to the global type.""" + import pybind11_cross_module_tests as cm + m.register_mixed_global() + m.register_mixed_local() + + a = [] + a.append(m.MixedGlobalLocal(1)) + a.append(m.MixedLocalGlobal(2)) + a.append(m.get_mixed_gl(3)) + a.append(m.get_mixed_lg(4)) + + assert [x.get() for x in a] == [101, 1002, 103, 1004] + + cm.register_mixed_global_local() + cm.register_mixed_local_global() + a.append(m.MixedGlobalLocal(5)) + a.append(m.MixedLocalGlobal(6)) + a.append(cm.MixedGlobalLocal(7)) + a.append(cm.MixedLocalGlobal(8)) + a.append(m.get_mixed_gl(9)) + a.append(m.get_mixed_lg(10)) + a.append(cm.get_mixed_gl(11)) + a.append(cm.get_mixed_lg(12)) + + assert [x.get() for x in a] == \ + [101, 1002, 103, 1004, 105, 1006, 207, 2008, 109, 1010, 211, 2012] + + def test_internal_locals_differ(): """Makes sure the internal local type map differs across the two modules""" import pybind11_cross_module_tests as cm