Unify Python 2 & 3 py::module constructor, and make contructor with pre-allocated PyModuleDef private

[YannickJadoul]

Up for discussion, whether this is worth the addition (@rwgk, @bstaletic, @henryiii, ... anyone else?). As far as I'm concerned, keeping the public, non-detail API clean, is something that might pay off in the future? And being able to use the pybind11 API from Python 3 and at least having it compile in 2 (before CI is getting tested), is a nice thing to have?

First stab at the general idea (as suggested here: #2413 (comment)). I do hope we can make it a little bit cleaner.

I'll add this to the 2.6.0 milestone, since it would be nice to have a decision on it, so we don't release an API that we later change.

[rwgk]

Is the new PyModuleDef() actually useful? If this is used there will be leak check errors again. — I'm just trying to understand. I intentionally side-stepped / deferred this question in #2413 (see "potential follow-up" note in top-level comment), to not have it block the straightforward solution, but if we invest more time to get things as clean as possible, the question seems important to consider.

[YannickJadoul]

Is the new PyModuleDef() actually useful?

Good point; I don't really know. Things worked like that before, so I just kept it, but it's worth an investigation.

Is there any point to constructing a module without PYBIND11_MODULE? Nested submodules are created with module_::def_submodule, modules are imported through module_::import, and extra embedded modules are added through PYBIND11_EMBEDDED_MODULE. So I don't know if there a valid use case for constructing a loose, non-nested, standalone module which is not the top-level module of an extension?

If not, maybe the module constructors ought to just be private (providing access through pybind11::detail::create_module for internal purposes)?

If there is a use case and this is used, we might not want to break old code? Correct me if I'm wrong, but the new vs. static doesn't actually matter a lot, and is just to shut up the leak detectors? In practice, both a static PyModuleDefas well as a new-allocated one will just stay alive for as long as Python is running.
So I saw #2413 more as a way to make valgrind and other sanitizers happy (which is a perfectly valid thing to do; don't get me wrong! :-) ), rather than plugging an actual leak.

[rwgk]

A quick update: some pybind11 unit tests fail if I make the change suggested in the #2413 top-level comment.
But I will run our global testing anyway, to see if there are any other uses of module_::module_.

For easy references, this is the change I'm trying:

- if (!def) def = new PyModuleDef();
+ if (!def) pybind11_fail("PyModuleDef *def must not be a nullptr.");

So I saw #2413 more as a way to make valgrind and other sanitizers happy

Yes, that's true.

[rwgk]

But I will run our global testing anyway, to see if there are any other uses of module_::module_.

@YannickJadoul No failures!
That observation strongly suggests that it's very unlikely that anyone outside needs the public module_::module_ constructor. I think if we can fix up the unit tests we can make it private; my suggestion is to try that after the 2.6.0 release is out.

[rwgk]

There are only two call sites that need the module_::module_ constructor:

diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h
index eae86c7..eee1f45 100644
--- a/include/pybind11/embed.h
+++ b/include/pybind11/embed.h
@@ -48,7 +48,7 @@
 #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));                   \
+        auto m = pybind11::module(PYBIND11_TOSTRING(name), nullptr, nullptr); \
         try {                                                                 \
             PYBIND11_CONCAT(pybind11_init_, name)(m);                         \
             return m.ptr();                                                   \
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 0cde4fa..a909e52 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -863,7 +863,7 @@ public:
 
     /// Create a new top-level Python module with the given name and docstring
 #if PY_MAJOR_VERSION >= 3
-    explicit module_(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
+    explicit module_(const char *name, const char *doc, PyModuleDef *def) {
         if (!def) def = new PyModuleDef();
         def = new (def) PyModuleDef {  // Placement new (not an allocation).
             /* m_base */     PyModuleDef_HEAD_INIT,
diff --git a/tests/test_modules.cpp b/tests/test_modules.cpp
index c1475fa..d5b2b43 100644
--- a/tests/test_modules.cpp
+++ b/tests/test_modules.cpp
@@ -60,7 +60,7 @@ TEST_SUBMODULE(modules, m) {
         class Dupe3 { };
         class DupeException { };
 
-        auto dm = py::module("dummy");
+        auto dm = py::module("dummy", nullptr, nullptr);
         auto failures = py::list();
 
         py::class_<Dupe1>(dm, "Dupe1");

[YannickJadoul]

Great news! Let's go back a bit in history and check if there are any related issues or PRs that would say otherwise. If not, I like the idea of just making this constructor private and having access through pybind11::detail::create_top_level_module! :-)

I am in the middle of moving, though, so it'll be only this weekend or early next week.

[YannickJadoul]

I think if we can fix up the unit tests we can make it private; my suggestion is to try that after the 2.6.0 release is out.

Any reason to wait for this until after 2.6.0, though? My initial suggestion would be to merge the current state of this PR (as to nót release a change to the public API that was introduced in #2413, and that would be annoying to deprecate again, later), and potentially add a deprecation warning to the public module_ constructors. Once people start using it, we can see if we get complaints and revert the deprecation, or go on with it?

But especially about the latter part, I can be persuaded to change our approach. I would really like to úndo the change to the public API before officially releasing it, though; your tests have given me a bit more confidence that this should actually be possible :-)

[rwgk]

Any reason to wait for this until after 2.6.0, though?

My only concern is that it adds a little bit of complexity, but I'm OK if you feel merging this is best, on the way to making the entire interface private.

[henryiii]

(I forgot to approve it too 😊 )

[YannickJadoul]

Thanks, all! :-) Next stop: deprecating the public constructors?