feat: deprecate public constructors of module_ class (#2552)

* Deprecated public constructors of module

* Turn documentation comment of module_::add_object into valid doxygen documentation

* Move definition of PYBIND11_DETAIL_MODULE_STATIC_DEF and PYBIND11_DETAIL_MODULE_CREATE macros up

* Move detail::create_top_level_module to module_::create_extension_module, and unify Python 2 and 3 signature again

* Throw error_already_set if module creation fails in module_::create_extension_module

* Mention module_::create_extension_module in deprecation warning message of module_::module_

docs/Doxyfile

@@ -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

include/pybind11/detail/common.h

@@ -309,32 +309,23 @@ 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)                                    \
+    static ::pybind11::module_::module_def                                     \
+        PYBIND11_CONCAT(pybind11_module_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                                        \
-        PYBIND11_DETAIL_MODULE_CREATE(name)                                    \
+        auto m = ::pybind11::module_::create_extension_module(                 \
+            PYBIND11_TOSTRING(name), nullptr,                                  \
+            &PYBIND11_CONCAT(pybind11_module_def_, name));                     \
         try {                                                                  \
             PYBIND11_CONCAT(pybind11_init_, name)(m);                          \
             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)

include/pybind11/embed.h

@@ -46,13 +46,17 @@
         }
  \endrst */
 #define PYBIND11_EMBEDDED_MODULE(name, variable)                                \
-    static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &);    \
+    static ::pybind11::module_::module_def                                      \
+        PYBIND11_CONCAT(pybind11_module_def_, name);                            \
+    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_::create_extension_module(                  \
+            PYBIND11_TOSTRING(name), nullptr,                                   \
+            &PYBIND11_CONCAT(pybind11_module_def_, name));                      \
         try {                                                                   \
             PYBIND11_CONCAT(pybind11_init_, name)(m);                           \
             return m.ptr();                                                     \
-        } catch (pybind11::error_already_set &e) {                            \
+        } catch (::pybind11::error_already_set &e) {                            \
             PyErr_SetString(PyExc_ImportError, e.what());                       \
             return nullptr;                                                     \
         } catch (const std::exception &e) {                                     \
@@ -61,10 +65,10 @@
         }                                                                       \
     }                                                                           \
     PYBIND11_EMBEDDED_MODULE_IMPL(name)                                         \
-    pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, 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)
+    void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable)
 
 
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

include/pybind11/pybind11.h

@@ -868,32 +868,20 @@ protected:
     }
 };
 
-
-#if PY_MAJOR_VERSION >= 3
-class module_;
-
-PYBIND11_NAMESPACE_BEGIN(detail)
-inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def);
-PYBIND11_NAMESPACE_END(detail)
-#endif
-
 /// 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 or module_::create_extension_module instead")
+    explicit module_(const char *name, const char *doc = nullptr) {
 #if PY_MAJOR_VERSION >= 3
-        : module_(name, doc, new PyModuleDef()) {}
+        *this = create_extension_module(name, doc, new PyModuleDef());
 #else
-    {
+        *this = create_extension_module(name, doc, nullptr);
-        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();
-    }
 #endif
+    }
 
     /** \rst
         Create Python binding for a new function within the module scope. ``Func``
@@ -946,11 +934,13 @@ public:
         *this = reinterpret_steal<module_>(obj);
     }
 
-    // Adds an object to the module using the given name.  Throws if an object with the given name
+    /** \rst
-    // already exists.
+        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.
+        ``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 \"" +
@@ -959,11 +949,21 @@ public:
         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 *);
+    using module_def = PyModuleDef;
+#else
+    struct module_def {};
+#endif
 
-    explicit module_(const char *name, const char *doc, PyModuleDef *def) {
+    /** \rst
+        Create a new top-level module that can be used as the main module of a C extension.
+
+        For Python 3, ``def`` should point to a staticly allocated module_def.
+        For Python 2, ``def`` can be a nullptr and is completely ignored.
+    \endrst */
+    static module_ create_extension_module(const char *name, const char *doc, module_def *def) {
+#if PY_MAJOR_VERSION >= 3
+        // module_def is PyModuleDef
         def = new (def) PyModuleDef {  // Placement new (not an allocation).
             /* m_base */     PyModuleDef_HEAD_INIT,
             /* m_name */     name,
@@ -975,12 +975,21 @@ private:
             /* m_clear */    nullptr,
             /* m_free */     nullptr
         };
-        m_ptr = PyModule_Create(def);
+        auto m = PyModule_Create(def);
-        if (m_ptr == nullptr)
+#else
-            pybind11_fail("Internal error in module_::module_()");
+        // Ignore module_def *def; only necessary for Python 3
-        inc_ref();
+        (void) def;
-    }
+        auto m = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
 #endif
+        if (m == nullptr) {
+            if (PyErr_Occurred())
+                throw error_already_set();
+            pybind11_fail("Internal error in module_::create_extension_module()");
+        }
+        // TODO: Sould be reinterpret_steal for Python 3, but Python also steals it again when returned from PyInit_...
+        //       For Python 2, reinterpret_borrow is correct.
+        return reinterpret_borrow<module_>(m);
+    }
 };
 
 // When inside a namespace (or anywhere as long as it's not the first item on a line),
@@ -988,14 +997,6 @@ private:
 // 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)
-inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def) {
-    return module_(name, doc, def);
-}
-PYBIND11_NAMESPACE_END(detail)
-#endif
-
 /// \ingroup python_builtins
 /// Return a dictionary representing the global variables in the current execution frame,
 /// or ``__main__.__dict__`` if there is no frame (usually when the interpreter is embedded).

tests/test_modules.cpp

@@ -62,7 +62,8 @@ TEST_SUBMODULE(modules, m) {
         class Dupe3 { };
         class DupeException { };
 
-        auto dm = py::module_("dummy");
+        // Go ahead and leak, until we have a non-leaking py::module_ constructor
+        auto dm = py::module_::create_extension_module("dummy", nullptr, new py::module_::module_def);
         auto failures = py::list();
 
         py::class_<Dupe1>(dm, "Dupe1");