Skip to content
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

Deprecate public constructors of module_ class #2552

Merged
merged 6 commits into from
Oct 9, 2020
Merged
Show file tree
Hide file tree
Changes from 3 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
3 changes: 2 additions & 1 deletion docs/Doxyfile
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ QUIET = YES
WARNINGS = YES
WARN_IF_UNDOCUMENTED = NO
PREDEFINED = DOXYGEN_SHOULD_SKIP_THIS \
PY_MAJOR_VERSION=3
PY_MAJOR_VERSION=3 \
PYBIND11_NOINLINE
30 changes: 16 additions & 14 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,20 @@ extern "C" {
return nullptr; \
} \

#if PY_MAJOR_VERSION >= 3
#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \
static PyModuleDef PYBIND11_CONCAT(pybind11_module_def_, name);
#define PYBIND11_DETAIL_MODULE_CREATE(name) \
auto m = ::pybind11::detail::create_top_level_module( \
PYBIND11_TOSTRING(name), nullptr, \
&PYBIND11_CONCAT(pybind11_module_def_, name));
#else
#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name)
#define PYBIND11_DETAIL_MODULE_CREATE(name) \
auto m = ::pybind11::detail::create_top_level_module( \
PYBIND11_TOSTRING(name), nullptr);
#endif

/** \rst
***Deprecated in favor of PYBIND11_MODULE***

Expand Down Expand Up @@ -309,22 +323,10 @@ extern "C" {
});
}
\endrst */
#if PY_MAJOR_VERSION >= 3
#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \
static PyModuleDef PYBIND11_CONCAT(pybind11_module_def_, name);
#define PYBIND11_DETAIL_MODULE_CREATE(name) \
auto m = pybind11::detail::create_top_level_module( \
PYBIND11_TOSTRING(name), nullptr, \
&PYBIND11_CONCAT(pybind11_module_def_, name));
#else
#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name)
#define PYBIND11_DETAIL_MODULE_CREATE(name) \
auto m = pybind11::module_(PYBIND11_TOSTRING(name));
#endif
#define PYBIND11_MODULE(name, variable) \
PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \
PYBIND11_MAYBE_UNUSED \
static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &); \
static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \
PYBIND11_PLUGIN_IMPL(name) { \
PYBIND11_CHECK_PYTHON_VERSION \
PYBIND11_ENSURE_INTERNALS_READY \
Expand All @@ -334,7 +336,7 @@ extern "C" {
return m.ptr(); \
} PYBIND11_CATCH_INIT_EXCEPTIONS \
} \
void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &variable)
void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable)


PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down
41 changes: 21 additions & 20 deletions include/pybind11/embed.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,26 +45,27 @@
});
}
\endrst */
#define PYBIND11_EMBEDDED_MODULE(name, variable) \
static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &); \
static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \
auto m = pybind11::module_(PYBIND11_TOSTRING(name)); \
try { \
PYBIND11_CONCAT(pybind11_init_, name)(m); \
return m.ptr(); \
} catch (pybind11::error_already_set &e) { \
PyErr_SetString(PyExc_ImportError, e.what()); \
return nullptr; \
} catch (const std::exception &e) { \
PyErr_SetString(PyExc_ImportError, e.what()); \
return nullptr; \
} \
} \
PYBIND11_EMBEDDED_MODULE_IMPL(name) \
pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name) \
(PYBIND11_TOSTRING(name), \
PYBIND11_CONCAT(pybind11_init_impl_, name)); \
void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &variable)
#define PYBIND11_EMBEDDED_MODULE(name, variable) \
PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \
static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \
static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \
PYBIND11_DETAIL_MODULE_CREATE(name) \
try { \
PYBIND11_CONCAT(pybind11_init_, name)(m); \
return m.ptr(); \
} catch (::pybind11::error_already_set &e) { \
Copy link
Member

Choose a reason for hiding this comment

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

Can we use PYBIND11_CATCH_INIT_EXCEPTIONS here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll pour that into another PR, yes.

PyErr_SetString(PyExc_ImportError, e.what()); \
return nullptr; \
} catch (const std::exception &e) { \
PyErr_SetString(PyExc_ImportError, e.what()); \
return nullptr; \
} \
} \
PYBIND11_EMBEDDED_MODULE_IMPL(name) \
::pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name) \
(PYBIND11_TOSTRING(name), \
PYBIND11_CONCAT(pybind11_init_impl_, name)); \
void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable)


PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down
83 changes: 41 additions & 42 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -868,32 +868,30 @@ class cpp_function : public function {
}
};


#if PY_MAJOR_VERSION >= 3
class module_;

PYBIND11_NAMESPACE_BEGIN(detail)
#if PY_MAJOR_VERSION >= 3
inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def);
PYBIND11_NAMESPACE_END(detail)
#else
inline module_ create_top_level_module(const char *name, const char *doc);
#endif
PYBIND11_NAMESPACE_END(detail)

/// Wrapper for Python extension modules
class module_ : public object {
public:
PYBIND11_OBJECT_DEFAULT(module_, object, PyModule_Check)

/// Create a new top-level Python module with the given name and docstring
explicit module_(const char *name, const char *doc = nullptr)
PYBIND11_DEPRECATED("Use PYBIND11_MODULE, module_::def_submodule, or module_::import instead")
explicit module_(const char *name, const char *doc = nullptr) {
#if PY_MAJOR_VERSION >= 3
: module_(name, doc, new PyModuleDef()) {}
*this = detail::create_top_level_module(name, doc, new PyModuleDef());
#else
{
m_ptr = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
if (m_ptr == nullptr)
pybind11_fail("Internal error in module_::module_()");
inc_ref();
}
*this = detail::create_top_level_module(name, doc);
#endif
}

/** \rst
Create Python binding for a new function within the module scope. ``Func``
Expand Down Expand Up @@ -946,55 +944,56 @@ class module_ : public object {
*this = reinterpret_steal<module_>(obj);
}

// Adds an object to the module using the given name. Throws if an object with the given name
// already exists.
//
// overwrite should almost always be false: attempting to overwrite objects that pybind11 has
// established will, in most cases, break things.
/** \rst
Adds an object to the module using the given name. Throws if an object with the given name
already exists.

``overwrite`` should almost always be false: attempting to overwrite objects that pybind11 has
established will, in most cases, break things.
\endrst */
PYBIND11_NOINLINE void add_object(const char *name, handle obj, bool overwrite = false) {
if (!overwrite && hasattr(*this, name))
pybind11_fail("Error during initialization: multiple incompatible definitions with name \"" +
std::string(name) + "\"");

PyModule_AddObject(ptr(), name, obj.inc_ref().ptr() /* steals a reference */);
}

private:
#if PY_MAJOR_VERSION >= 3
friend module_ detail::create_top_level_module(const char *, const char *, PyModuleDef *);

explicit module_(const char *name, const char *doc, PyModuleDef *def) {
def = new (def) PyModuleDef { // Placement new (not an allocation).
/* m_base */ PyModuleDef_HEAD_INIT,
/* m_name */ name,
/* m_doc */ options::show_user_defined_docstrings() ? doc : nullptr,
/* m_size */ -1,
/* m_methods */ nullptr,
/* m_slots */ nullptr,
/* m_traverse */ nullptr,
/* m_clear */ nullptr,
/* m_free */ nullptr
};
m_ptr = PyModule_Create(def);
if (m_ptr == nullptr)
pybind11_fail("Internal error in module_::module_()");
inc_ref();
}
#endif
};

// When inside a namespace (or anywhere as long as it's not the first item on a line),
// C++20 allows "module" to be used. This is provided for backward compatibility, and for
// simplicity, if someone wants to use py::module for example, that is perfectly safe.
using module = module_;

#if PY_MAJOR_VERSION >= 3
PYBIND11_NAMESPACE_BEGIN(detail)
#if PY_MAJOR_VERSION >= 3
inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def) {
return module_(name, doc, def);
def = new (def) PyModuleDef { // Placement new (not an allocation).
/* m_base */ PyModuleDef_HEAD_INIT,
/* m_name */ name,
/* m_doc */ options::show_user_defined_docstrings() ? doc : nullptr,
/* m_size */ -1,
/* m_methods */ nullptr,
/* m_slots */ nullptr,
/* m_traverse */ nullptr,
/* m_clear */ nullptr,
/* m_free */ nullptr
};
auto m = PyModule_Create(def);
if (m == nullptr)
pybind11_fail("Internal error in detail::create_top_level_module()");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idea 1: does throw error_already_set() work?
Idea 2: somehow include name in the error message, to be helpful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Answered in the main thread, but that was maybe not a great idea:

I'm now just looking at the error message, to finish this PR, but I can't find if a nullptr mean the error indicator is already set?
https://docs.python.org/3/c-api/module.html#c.PyModule_Create

EDIT: If it definitely does, we should throw py::error_already_set to be fully correct!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the 3.7 sources, I see some code paths set an error for sure, but it's difficult to know if that's always the case. The bomb-proof solution would be something like if (PyErr_Occurred()) throw error_already_set(); and pybind11_fail() as a fallback, with name in the error message.
I kind of feel it's worth it: in case this fires it's most likely a difficult to debug situation, and showing the original error (root cause) or the module name could save a lot of time troubleshooting.
Unless we can convince the Python developers to document that a NULL return guarantees the error is set ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might not need to add name to the error message, if it usually or hopefully always works with error_already_set(), as that makes pybind11_fail() a last-ditch fallback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Assuming error_already_set would show the name?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Myeah, or we just trust that this part of the API works in the same way? An error_already_set won't cause things to be messed up too badly.

I can't both throw error_already_set ánd have the name in there, though. So that's only as a fall-back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Assuming error_already_set would show the name?)

I don't know; we don't control that, since that the message is already set by Python. It might be something as cryptic as "didn't manage to create a str from this const char*"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, didn't add the name, as the pybind11_fail is only our fallback option. We can add it, if we find out that (and when!) the control flow can reach that statement?

// TODO: Should be reinterpret_steal, but Python also steals it again when returned from PyInit_...
return reinterpret_borrow<module_>(m);
}
#else
inline module_ create_top_level_module(const char *name, const char *doc) {
auto m = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
if (m == nullptr)
pybind11_fail("Internal error in detail::create_top_level_module()");
return reinterpret_borrow<module_>(m);
}
PYBIND11_NAMESPACE_END(detail)
#endif
PYBIND11_NAMESPACE_END(detail)

/// \ingroup python_builtins
/// Return a dictionary representing the global variables in the current execution frame,
Expand Down
6 changes: 5 additions & 1 deletion tests/test_modules.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ TEST_SUBMODULE(modules, m) {
class Dupe3 { };
class DupeException { };

auto dm = py::module_("dummy");
#if PY_MAJOR_VERSION >= 3
auto dm = py::detail::create_top_level_module("dummy", nullptr, new PyModuleDef);
#else
auto dm = py::detail::create_top_level_module("dummy", nullptr);
#endif
auto failures = py::list();

py::class_<Dupe1>(dm, "Dupe1");
Expand Down