From 748ae2270b333d7024d9ef7ffb7f2ca947b2491f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 28 May 2022 16:40:57 -0700 Subject: [PATCH 1/2] Add missing error handling to `module_::def_submodule` (#3973) * Add missing error handling to module_::def_submodule * Add test_def_submodule_failures * PyPy only: Skip test with trigger for PyModule_GetName() failure. * Reapply minor fix that accidentally got lost in transfer from PR #3964 --- include/pybind11/pybind11.h | 13 ++++++++++--- tests/test_modules.cpp | 2 ++ tests/test_modules.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4c392713c..ea97ed072 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1173,9 +1173,16 @@ public: py::module_ m3 = m2.def_submodule("subsub", "A submodule of 'example.sub'"); \endrst */ module_ def_submodule(const char *name, const char *doc = nullptr) { - std::string full_name - = std::string(PyModule_GetName(m_ptr)) + std::string(".") + std::string(name); - auto result = reinterpret_borrow(PyImport_AddModule(full_name.c_str())); + const char *this_name = PyModule_GetName(m_ptr); + if (this_name == nullptr) { + throw error_already_set(); + } + std::string full_name = std::string(this_name) + '.' + name; + handle submodule = PyImport_AddModule(full_name.c_str()); + if (!submodule) { + throw error_already_set(); + } + auto result = reinterpret_borrow(submodule); if (doc && options::show_user_defined_docstrings()) { result.attr("__doc__") = pybind11::str(doc); } diff --git a/tests/test_modules.cpp b/tests/test_modules.cpp index 8aa2a4bdb..18a7ec74c 100644 --- a/tests/test_modules.cpp +++ b/tests/test_modules.cpp @@ -120,4 +120,6 @@ TEST_SUBMODULE(modules, m) { return failures; }); + + m.def("def_submodule", [](py::module_ m, const char *name) { return m.def_submodule(name); }); } diff --git a/tests/test_modules.py b/tests/test_modules.py index 15db8355e..e11d68e78 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -1,3 +1,6 @@ +import pytest + +import env from pybind11_tests import ConstructorStats from pybind11_tests import modules as m from pybind11_tests.modules import subsubmodule as ms @@ -89,3 +92,30 @@ def test_builtin_key_type(): keys = __builtins__.__dict__.keys() assert {type(k) for k in keys} == {str} + + +@pytest.mark.xfail("env.PYPY", reason="PyModule_GetName()") +def test_def_submodule_failures(): + sm = m.def_submodule(m, b"ScratchSubModuleName") # Using bytes to show it works. + assert sm.__name__ == m.__name__ + "." + "ScratchSubModuleName" + malformed_utf8 = b"\x80" + if env.PYPY: + # It is not worth the effort finding a trigger for a failure when running with PyPy. + pytest.skip("Sufficiently exercised on platforms other than PyPy.") + else: + # Meant to trigger PyModule_GetName() failure: + sm_name_orig = sm.__name__ + sm.__name__ = malformed_utf8 + try: + with pytest.raises(Exception): + # Seen with Python 3.9: SystemError: nameless module + # But we do not want to exercise the internals of PyModule_GetName(), which could + # change in future versions of Python, but a bad __name__ is very likely to cause + # some kind of failure indefinitely. + m.def_submodule(sm, b"SubSubModuleName") + finally: + # Clean up to ensure nothing gets upset by a module with an invalid __name__. + sm.__name__ = sm_name_orig # Purely precautionary. + # Meant to trigger PyImport_AddModule() failure: + with pytest.raises(UnicodeDecodeError): + m.def_submodule(sm, malformed_utf8) From 8da58da53996e9aa40d051b3b84cb3220fdbbb58 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Sat, 28 May 2022 19:58:15 -0400 Subject: [PATCH 2/2] chore: perfectly forward all make_iterator args (#3980) * Perfectly forward all make_iterator args * Try emplace back --- include/pybind11/pybind11.h | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index ea97ed072..be206e1a6 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2333,7 +2333,7 @@ template -iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) { +iterator make_iterator_impl(Iterator &&first, Sentinel &&last, Extra &&...extra) { using state = detail::iterator_state; // TODO: state captures only the types of Extra, not the values @@ -2359,7 +2359,7 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) { Policy); } - return cast(state{first, last, true}); + return cast(state{std::forward(first), std::forward(last), true}); } PYBIND11_NAMESPACE_END(detail) @@ -2370,13 +2370,15 @@ template ::result_type, typename... Extra> -iterator make_iterator(Iterator first, Sentinel last, Extra &&...extra) { +iterator make_iterator(Iterator &&first, Sentinel &&last, Extra &&...extra) { return detail::make_iterator_impl, Policy, Iterator, Sentinel, ValueType, - Extra...>(first, last, std::forward(extra)...); + Extra...>(std::forward(first), + std::forward(last), + std::forward(extra)...); } /// Makes a python iterator over the keys (`.first`) of a iterator over pairs from a @@ -2386,13 +2388,15 @@ template ::result_type, typename... Extra> -iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) { +iterator make_key_iterator(Iterator &&first, Sentinel &&last, Extra &&...extra) { return detail::make_iterator_impl, Policy, Iterator, Sentinel, KeyType, - Extra...>(first, last, std::forward(extra)...); + Extra...>(std::forward(first), + std::forward(last), + std::forward(extra)...); } /// Makes a python iterator over the values (`.second`) of a iterator over pairs from a @@ -2402,13 +2406,15 @@ template ::result_type, typename... Extra> -iterator make_value_iterator(Iterator first, Sentinel last, Extra &&...extra) { +iterator make_value_iterator(Iterator &&first, Sentinel &&last, Extra &&...extra) { return detail::make_iterator_impl, Policy, Iterator, Sentinel, ValueType, - Extra...>(first, last, std::forward(extra)...); + Extra...>(std::forward(first), + std::forward(last), + std::forward(extra)...); } /// Makes an iterator over values of an stl container or other container supporting @@ -2467,7 +2473,7 @@ void implicitly_convertible() { }; if (auto *tinfo = detail::get_type_info(typeid(OutputType))) { - tinfo->implicit_conversions.push_back(implicit_caster); + tinfo->implicit_conversions.emplace_back(std::move(implicit_caster)); } else { pybind11_fail("implicitly_convertible: Unable to find type " + type_id()); }