gh-105922: Add PyImport_AddModuleRef() function

vstinner · vstinner · commit 631c02cd7ea2 · 2023-06-19T20:02:18.000+02:00
* Add tests on PyImport_AddModuleRef(), PyImport_AddModule() and
  PyImport_AddModuleObject().
* PyRun_InteractiveOneObjectEx(), _PyRun_SimpleFileObject()
  and PyRun_SimpleStringFlags() now use PyImport_AddModuleRef()
  and keep the module strong reference alive longer than before.
diff --git a/Doc/c-api/import.rst b/Doc/c-api/import.rst
@@ -98,27 +98,46 @@ Importing Modules
    an exception set on failure (the module still exists in this case).
 
 
-.. c:function:: PyObject* PyImport_AddModuleObject(PyObject *name)
+.. c:function:: PyObject* PyImport_AddModuleRef(const char *name)
+
+   Return the module object corresponding to a module name.
+
+   The *name* argument may be of the form ``package.module``. First check the
+   modules dictionary if there's one there, and if not, create a new one and
+   insert it in the modules dictionary.
+
+   Return a :term:`strong reference` to the module on success. Return ``NULL``
+   with an exception set on failure.
+
+   The module name *name* is decoded from UTF-8.
+
+   This function does not load or import the module; if the module wasn't
+   already loaded, you will get an empty module object. Use
+   :c:func:`PyImport_ImportModule` or one of its variants to import a module.
+   Package structures implied by a dotted name for *name* are not created if
+   not already present.
 
-   Return the module object corresponding to a module name.  The *name* argument
-   may be of the form ``package.module``. First check the modules dictionary if
-   there's one there, and if not, create a new one and insert it in the modules
-   dictionary. Return ``NULL`` with an exception set on failure.
+   .. versionadded:: 3.13
 
-   .. note::
 
-      This function does not load or import the module; if the module wasn't already
-      loaded, you will get an empty module object. Use :c:func:`PyImport_ImportModule`
-      or one of its variants to import a module.  Package structures implied by a
-      dotted name for *name* are not created if not already present.
+.. c:function:: PyObject* PyImport_AddModuleObject(PyObject *name)
+
+   Similar to :c:func:`PyImport_AddModuleRef`, but return a :term:`borrowed
+   reference` and *name* is a Python :class:`str` object.
 
    .. versionadded:: 3.3
 
+   .. deprecated-removed:: 3.13 3.15
+      Use :c:func:`PyImport_AddModuleRef` instead.
+
 
 .. c:function:: PyObject* PyImport_AddModule(const char *name)
 
-   Similar to :c:func:`PyImport_AddModuleObject`, but the name is a UTF-8
-   encoded string instead of a Unicode object.
+   Similar to :c:func:`PyImport_AddModuleRef`, but return a :term:`borrowed
+   reference`.
+
+   .. deprecated-removed:: 3.13 3.15
+      Use :c:func:`PyImport_AddModuleRef` instead.
 
 
 .. c:function:: PyObject* PyImport_ExecCodeModule(const char *name, PyObject *co)
diff --git a/Doc/data/refcounts.dat b/Doc/data/refcounts.dat
@@ -974,6 +974,9 @@ PyCoro_New:PyFrameObject*:frame:0:
 PyCoro_New:PyObject*:name:0:
 PyCoro_New:PyObject*:qualname:0:
 
+PyImport_AddModuleRef:PyObject*::+1:
+PyImport_AddModuleRef:const char*:name::
+
 PyImport_AddModule:PyObject*::0:reference borrowed from sys.modules
 PyImport_AddModule:const char*:name::
 
diff --git a/Doc/data/stable_abi.dat b/Doc/data/stable_abi.dat
diff --git a/Doc/extending/extending.rst b/Doc/extending/extending.rst
@@ -989,9 +989,11 @@ is less clear, here, however, since a few common routines are exceptions:
 :c:func:`PyDict_GetItemString` all return references that you borrow from the
 tuple, list or dictionary.
 
-The function :c:func:`PyImport_AddModule` also returns a borrowed reference, even
-though it may actually create the object it returns: this is possible because an
-owned reference to the object is stored in ``sys.modules``.
+The deprecated function :c:func:`PyImport_AddModule` also returns a
+:term:`borrowed reference`, even though it may actually create the object it
+returns: this is possible because an owned reference to the object is stored in
+``sys.modules``. The new function :c:func:`PyImport_AddModuleRef` is
+recommended since it returns a :term:`strong reference`.
 
 When you pass an object reference into another function, in general, the
 function borrows the reference from you --- if it needs to store it, it will use
diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst
@@ -426,6 +426,11 @@ New Features
   APIs accepting the format codes always use ``Py_ssize_t`` for ``#`` formats.
   (Contributed by Inada Naoki in :gh:`104922`.)
 
+* Add :c:func:`PyImport_AddModuleRef` function to replace the deprecated
+  :c:func:`PyImport_AddModule` function: return a :term:`strong reference` to
+  the module, instead of a :term:`borrowed reference`.
+  (Contributed by Victor Stinner in :gh:`105922`.)
+
 
 Porting to Python 3.13
 ----------------------
@@ -457,6 +462,12 @@ Deprecated
   Scheduled for removal in Python 3.15.
   (Contributed by Victor Stinner in :gh:`105396`.)
 
+* Deprecate :c:func:`PyImport_AddModule` and :c:func:`PyImport_AddModuleObject`
+  functions which return a :term:`borrowed reference`. Replace them with the
+  new :c:func:`PyImport_AddModuleRef` function which returns a :term:`strong
+  reference`.
+  (Contributed by Victor Stinner in :gh:`105922`.)
+
 Removed
 -------
 
diff --git a/Include/import.h b/Include/import.h
@@ -43,6 +43,11 @@ PyAPI_FUNC(PyObject *) PyImport_AddModuleObject(
 PyAPI_FUNC(PyObject *) PyImport_AddModule(
     const char *name            /* UTF-8 encoded string */
     );
+#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030d0000
+PyAPI_FUNC(PyObject *) PyImport_AddModuleRef(
+    const char *name            /* UTF-8 encoded string */
+    );
+#endif
 PyAPI_FUNC(PyObject *) PyImport_ImportModule(
     const char *name            /* UTF-8 encoded string */
     );
diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py
@@ -2621,6 +2621,30 @@ def test_basic_multiple_interpreters_reset_each(self):
         #  * module's global state was initialized, not reset
 
 
+@cpython_only
+class CAPITests(unittest.TestCase):
+    def test_pyimport_addmodule(self):
+        # gh-105922: Test PyImport_AddModuleRef(), PyImport_AddModule()
+        # and PyImport_AddModuleObject()
+        import _testcapi
+        for name in (
+            'sys',     # frozen module
+            'test',    # package
+            __name__,  # package.module
+        ):
+            _testcapi.check_pyimport_addmodule(name)
+
+    def test_pyimport_addmodule_create(self):
+        # gh-105922: Test PyImport_AddModuleRef(), create a new module
+        import _testcapi
+        name = 'dontexist'
+        self.assertNotIn(name, sys.modules)
+        self.addCleanup(unload, name)
+
+        mod = _testcapi.check_pyimport_addmodule(name)
+        self.assertIs(mod, sys.modules[name])
+
+
 if __name__ == '__main__':
     # Test needs to be a package, so we can do relative imports.
     unittest.main()
diff --git a/Lib/test/test_stable_abi_ctypes.py b/Lib/test/test_stable_abi_ctypes.py
diff --git a/Misc/NEWS.d/next/C API/2023-06-19-20-02-16.gh-issue-105922.o4T6wO.rst b/Misc/NEWS.d/next/C API/2023-06-19-20-02-16.gh-issue-105922.o4T6wO.rst
@@ -0,0 +1,4 @@
+Add :c:func:`PyImport_AddModuleRef` function to replace the deprecated
+:c:func:`PyImport_AddModule` function: return a :term:`strong reference` to
+the module, instead of a :term:`borrowed reference`. Patch by Victor
+Stinner.
diff --git a/Misc/stable_abi.toml b/Misc/stable_abi.toml
@@ -2428,3 +2428,5 @@
     added = '3.12'
 [const.Py_TPFLAGS_ITEMS_AT_END]
     added = '3.12'
+[function.PyImport_AddModuleRef]
+    added = '3.13'
diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
@@ -3325,6 +3325,53 @@ test_atexit(PyObject *self, PyObject *Py_UNUSED(args))
 }
 
 
+static PyObject *
+check_pyimport_addmodule(PyObject *self, PyObject *args)
+{
+    const char *name;
+    if (!PyArg_ParseTuple(args, "s", &name)) {
+        return NULL;
+    }
+
+    // test PyImport_AddModuleRef()
+    PyObject *module = PyImport_AddModuleRef(name);
+    if (module == NULL) {
+        return NULL;
+    }
+    assert(PyModule_Check(module));
+    // module is a strong reference
+
+    // test PyImport_AddModule()
+    PyObject *module2 = PyImport_AddModule(name);
+    if (module2 == NULL) {
+        goto error;
+    }
+    assert(PyModule_Check(module2));
+    assert(module2 == module);
+    // module2 is a borrowed ref
+
+    // test PyImport_AddModuleObject()
+    PyObject *name_obj = PyUnicode_FromString(name);
+    if (name_obj == NULL) {
+        goto error;
+    }
+    PyObject *module3 = PyImport_AddModuleObject(name_obj);
+    Py_DECREF(name_obj);
+    if (module3 == NULL) {
+        goto error;
+    }
+    assert(PyModule_Check(module3));
+    assert(module3 == module);
+    // module3 is a borrowed ref
+
+    return module;
+
+error:
+    Py_DECREF(module);
+    return NULL;
+}
+
+
 static PyMethodDef TestMethods[] = {
     {"set_errno",               set_errno,                       METH_VARARGS},
     {"test_config",             test_config,                     METH_NOARGS},
@@ -3468,6 +3515,7 @@ static PyMethodDef TestMethods[] = {
     {"function_get_kw_defaults", function_get_kw_defaults, METH_O, NULL},
     {"function_set_kw_defaults", function_set_kw_defaults, METH_VARARGS, NULL},
     {"test_atexit", test_atexit, METH_NOARGS},
+    {"check_pyimport_addmodule", check_pyimport_addmodule, METH_VARARGS},
     {NULL, NULL} /* sentinel */
 };
 
diff --git a/PC/python3dll.c b/PC/python3dll.c
diff --git a/Python/import.c b/Python/import.c
@@ -350,20 +350,39 @@ import_add_module(PyThreadState *tstate, PyObject *name)
     return m;
 }
 
+PyObject *
+PyImport_AddModuleRef(const char *name)
+{
+    PyObject *name_obj = PyUnicode_FromString(name);
+    if (name_obj == NULL) {
+        return NULL;
+    }
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyObject *module = import_add_module(tstate, name_obj);
+    Py_DECREF(name_obj);
+    return module;
+}
+
+
 PyObject *
 PyImport_AddModuleObject(PyObject *name)
 {
     PyThreadState *tstate = _PyThreadState_GET();
     PyObject *mod = import_add_module(tstate, name);
-    if (mod) {
-        PyObject *ref = PyWeakref_NewRef(mod, NULL);
-        Py_DECREF(mod);
-        if (ref == NULL) {
-            return NULL;
-        }
-        mod = PyWeakref_GetObject(ref);
-        Py_DECREF(ref);
+    if (!mod) {
+        return NULL;
+    }
+
+    // PyImport_AddModuleObject() returns a borrowed reference
+    // https://bugs.python.org/issue41994
+    PyObject *ref = PyWeakref_NewRef(mod, NULL);
+    Py_DECREF(mod);
+    if (ref == NULL) {
+        return NULL;
     }
+
+    mod = PyWeakref_GetObject(ref);
+    Py_DECREF(ref);
     return mod; /* borrowed reference */
 }
 
@@ -1364,8 +1383,8 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec)
         if (_PyUnicode_EqualToASCIIString(name, p->name)) {
             if (p->initfunc == NULL) {
                 /* Cannot re-init internal module ("sys" or "builtins") */
-                mod = PyImport_AddModuleObject(name);
-                return Py_XNewRef(mod);
+                PyThreadState *tstate = _PyThreadState_GET();
+                return import_add_module(tstate, name);
             }
             mod = _PyImport_InitFunc_TrampolineCall(*p->initfunc);
             if (mod == NULL) {
@@ -2240,11 +2259,17 @@ init_importlib(PyThreadState *tstate, PyObject *sysmod)
     if (PyImport_ImportFrozenModule("_frozen_importlib") <= 0) {
         return -1;
     }
-    PyObject *importlib = PyImport_AddModule("_frozen_importlib"); // borrowed
+
+    PyObject *name = PyUnicode_FromString("_frozen_importlib");
+    if (name == NULL) {
+        return -1;
+    }
+    PyObject *importlib = import_add_module(tstate, name);
+    Py_DECREF(name);
     if (importlib == NULL) {
         return -1;
     }
-    IMPORTLIB(interp) = Py_NewRef(importlib);
+    IMPORTLIB(interp) = importlib;
 
     // Import the _imp module
     if (verbose) {
diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
@@ -2172,13 +2172,14 @@ _Py_IsInterpreterFinalizing(PyInterpreterState *interp)
 static PyStatus
 add_main_module(PyInterpreterState *interp)
 {
-    PyObject *m, *d, *loader, *ann_dict;
-    m = PyImport_AddModule("__main__");
-    if (m == NULL)
+    PyObject *module = PyImport_AddModuleRef("__main__");
+    if (module == NULL) {
         return _PyStatus_ERR("can't create __main__ module");
+    }
+    PyObject *d = PyModule_GetDict(module);
+    Py_DECREF(module);
 
-    d = PyModule_GetDict(m);
-    ann_dict = PyDict_New();
+    PyObject *ann_dict = PyDict_New();
     if ((ann_dict == NULL) ||
         (PyDict_SetItemString(d, "__annotations__", ann_dict) < 0)) {
         return _PyStatus_ERR("Failed to initialize __main__.__annotations__");
@@ -2204,7 +2205,7 @@ add_main_module(PyInterpreterState *interp)
      * will be set if __main__ gets further initialized later in the startup
      * process.
      */
-    loader = _PyDict_GetItemStringWithError(d, "__loader__");
+    PyObject *loader = _PyDict_GetItemStringWithError(d, "__loader__");
     if (loader == NULL || loader == Py_None) {
         if (PyErr_Occurred()) {
             return _PyStatus_ERR("Failed to test __main__.__loader__");
diff --git a/Python/pythonrun.c b/Python/pythonrun.c
