Skip to content

Add missing error handling to module_::def_submodule #3973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1173,9 +1173,16 @@ class module_ : public object {
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<module_>(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<module_>(submodule);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, wouldn't you also been able to do?

if(!result){
            throw error_already_set();
}

This looks good either way though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIU your question correctly, that would assume reinterpret_borrow<module_> is OK with nullptr, and does not care about the active error. While that is usually true, what if someone temporarily adds debugging code to reinterpret_borrow, or the module_ constructor in this case? Their fault, of course, but why build a trap like that?
Generally, I think it is bad style to separate error handling from the source of the error more than necessary.

if (doc && options::show_user_defined_docstrings()) {
result.attr("__doc__") = pybind11::str(doc);
}
Expand Down
2 changes: 2 additions & 0 deletions tests/test_modules.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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); });
}
30 changes: 30 additions & 0 deletions tests/test_modules.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)