From 2f3cc6738a34faf1e74cbe598b500eb5ad41be30 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 31 Oct 2023 17:22:31 +0100 Subject: [PATCH 1/6] gh-106915: Add PyImport_ImportOrAddModule() function Remove PyImport_AddModuleRef() function. --- Doc/c-api/import.rst | 30 +++++++----- Doc/data/refcounts.dat | 3 -- Doc/data/stable_abi.dat | 2 +- Doc/whatsnew/3.13.rst | 7 +-- Include/import.h | 15 +++++- Lib/test/test_import/__init__.py | 11 +++-- Lib/test/test_stable_abi_ctypes.py | 2 +- ...-10-31-17-40-34.gh-issue-106915.XVWI8X.rst | 2 + Misc/stable_abi.toml | 2 +- Modules/_testcapimodule.c | 12 +++-- PC/python3dll.c | 2 +- Python/import.c | 48 ++++++++++++------- Python/pythonrun.c | 13 ++--- 13 files changed, 95 insertions(+), 54 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2023-10-31-17-40-34.gh-issue-106915.XVWI8X.rst diff --git a/Doc/c-api/import.rst b/Doc/c-api/import.rst index 137780cc359cf9..1bef07f6ce34f2 100644 --- a/Doc/c-api/import.rst +++ b/Doc/c-api/import.rst @@ -98,16 +98,22 @@ Importing Modules an exception set on failure (the module still exists in this case). -.. c:function:: PyObject* PyImport_AddModuleRef(const char *name) +.. c:function:: int PyImport_ImportOrAddModule(const char *name, PyObject **module) - Return the module object corresponding to a module name. + Create a new module and store it in :data:`sys.modules`, or get an already + imported module from :data:`sys.modules`. - 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. + 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 *name* argument may be of the form ``package.module``. + + - If the module does not exist, create a module, store it in + :data:`sys.modules`, set *\*module* to a :term:`strong reference` to the + module, and return 1. + - If the module was already imported, set *\*module* to a :term:`strong + reference` to the existing module, and return 0. + - On error, raise an exception, set *\*module* to NULL, and return -1. The module name *name* is decoded from UTF-8. @@ -122,16 +128,18 @@ Importing Modules .. 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. + Similar to :c:func:`PyImport_ImportOrAddModule`, but return a :term:`borrowed + reference`, *name* is a Python :class:`str` object, and don't provide the + information if the module was created or was already imported. .. versionadded:: 3.3 .. c:function:: PyObject* PyImport_AddModule(const char *name) - Similar to :c:func:`PyImport_AddModuleRef`, but return a :term:`borrowed - reference`. + Similar to :c:func:`PyImport_ImportOrAddModule`, but return a :term:`borrowed + reference`, and don't provide the information if the module was created or + was already imported. .. c:function:: PyObject* PyImport_ExecCodeModule(const char *name, PyObject *co) diff --git a/Doc/data/refcounts.dat b/Doc/data/refcounts.dat index ef9ac1617a284b..7bb1d2c5b9e2aa 100644 --- a/Doc/data/refcounts.dat +++ b/Doc/data/refcounts.dat @@ -974,9 +974,6 @@ 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 index 52d6d967d66327..489152e8a16622 100644 --- a/Doc/data/stable_abi.dat +++ b/Doc/data/stable_abi.dat @@ -300,7 +300,6 @@ type,PyGetSetDef,3.2,,full-abi var,PyGetSetDescr_Type,3.2,, function,PyImport_AddModule,3.2,, function,PyImport_AddModuleObject,3.7,, -function,PyImport_AddModuleRef,3.13,, function,PyImport_AppendInittab,3.2,, function,PyImport_ExecCodeModule,3.2,, function,PyImport_ExecCodeModuleEx,3.2,, @@ -318,6 +317,7 @@ function,PyImport_ImportModule,3.2,, function,PyImport_ImportModuleLevel,3.2,, function,PyImport_ImportModuleLevelObject,3.7,, function,PyImport_ImportModuleNoBlock,3.2,, +function,PyImport_ImportOrAddModule,3.13,, function,PyImport_ReloadModule,3.2,, function,PyIndex_Check,3.8,, type,PyInterpreterState,3.2,,opaque diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index ef83f662788fe4..21db8b29dd6c94 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -1018,9 +1018,10 @@ 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`: similar to - :c:func:`PyImport_AddModule`, but return a :term:`strong reference` instead - of a :term:`borrowed reference`. +* Add :c:func:`PyImport_ImportOrAddModule`: similar to + :c:func:`PyImport_AddModule`, but get a :term:`strong reference` instead + of a :term:`borrowed reference` and return 0 if the module was already + imported. (Contributed by Victor Stinner in :gh:`105922`.) * Add :c:func:`PyWeakref_GetRef` function: similar to diff --git a/Include/import.h b/Include/import.h index 24b23b9119196f..1c05b093118948 100644 --- a/Include/import.h +++ b/Include/import.h @@ -43,11 +43,22 @@ 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 */ +// Create a new module and store it in sys.modules, or get an already imported +// module from sys.modules. +// +// - If the module does not exist, create a module, store it in sys.modules, +// set '*module' to a strong reference to the module, and return 1. +// - If the module was already imported, set '*module' to a strong reference +// to the existing module, and return 0. +// - On error, raise an exception, set '*module' to NULL, and return -1. +PyAPI_FUNC(int) PyImport_ImportOrAddModule( + const char *name, // UTF-8 encoded string + PyObject **module ); #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 index aa465c70dfbcd0..a6bd6bab7e7822 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -2669,24 +2669,25 @@ def test_basic_multiple_interpreters_reset_each(self): @cpython_only class CAPITests(unittest.TestCase): def test_pyimport_addmodule(self): - # gh-105922: Test PyImport_AddModuleRef(), PyImport_AddModule() - # and PyImport_AddModuleObject() + # gh-105922: Test PyImport_ImportOrAddModule(), PyImport_AddModule() + # and PyImport_AddModuleObject(): module already exists. import _testcapi for name in ( 'sys', # frozen module 'test', # package __name__, # package.module ): - _testcapi.check_pyimport_addmodule(name) + self.assertIn(name, sys.modules) + _testcapi.check_pyimport_addmodule(name, False) def test_pyimport_addmodule_create(self): - # gh-105922: Test PyImport_AddModuleRef(), create a new module + # gh-105922: Test PyImport_ImportOrAddModule(): create a new module import _testcapi name = 'dontexist' self.assertNotIn(name, sys.modules) self.addCleanup(unload, name) - mod = _testcapi.check_pyimport_addmodule(name) + mod = _testcapi.check_pyimport_addmodule(name, True) self.assertIs(mod, sys.modules[name]) diff --git a/Lib/test/test_stable_abi_ctypes.py b/Lib/test/test_stable_abi_ctypes.py index 85a63c44b49431..4ec7d54a34008c 100644 --- a/Lib/test/test_stable_abi_ctypes.py +++ b/Lib/test/test_stable_abi_ctypes.py @@ -330,7 +330,6 @@ def test_windows_feature_macros(self): "PyGetSetDescr_Type", "PyImport_AddModule", "PyImport_AddModuleObject", - "PyImport_AddModuleRef", "PyImport_AppendInittab", "PyImport_ExecCodeModule", "PyImport_ExecCodeModuleEx", @@ -348,6 +347,7 @@ def test_windows_feature_macros(self): "PyImport_ImportModuleLevel", "PyImport_ImportModuleLevelObject", "PyImport_ImportModuleNoBlock", + "PyImport_ImportOrAddModule", "PyImport_ReloadModule", "PyIndex_Check", "PyInterpreterState_Clear", diff --git a/Misc/NEWS.d/next/C API/2023-10-31-17-40-34.gh-issue-106915.XVWI8X.rst b/Misc/NEWS.d/next/C API/2023-10-31-17-40-34.gh-issue-106915.XVWI8X.rst new file mode 100644 index 00000000000000..d31785b927c9ff --- /dev/null +++ b/Misc/NEWS.d/next/C API/2023-10-31-17-40-34.gh-issue-106915.XVWI8X.rst @@ -0,0 +1,2 @@ +Add :c:func:`PyImport_ImportOrAddModule` function and remove +:c:func:`PyImport_AddModuleRef` function. Patch by Victor Stinner. diff --git a/Misc/stable_abi.toml b/Misc/stable_abi.toml index b55bb599d71dcc..6e4255a006504a 100644 --- a/Misc/stable_abi.toml +++ b/Misc/stable_abi.toml @@ -2428,7 +2428,7 @@ added = '3.12' [const.Py_TPFLAGS_ITEMS_AT_END] added = '3.12' -[function.PyImport_AddModuleRef] +[function.PyImport_ImportOrAddModule] added = '3.13' [function.PyWeakref_GetRef] added = '3.13' diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 300025ce3705fe..1e2d43138a7f80 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3036,15 +3036,19 @@ static PyObject * check_pyimport_addmodule(PyObject *self, PyObject *args) { const char *name; - if (!PyArg_ParseTuple(args, "s", &name)) { + int is_new; + if (!PyArg_ParseTuple(args, "si", &name, &is_new)) { return NULL; } + // name must be the name of a module which is already in sys.modules - // test PyImport_AddModuleRef() - PyObject *module = PyImport_AddModuleRef(name); - if (module == NULL) { + // test PyImport_ImportOrAddModule() + PyObject *module = UNINITIALIZED_PTR; + int res = PyImport_ImportOrAddModule(name, &module); + if (res < 0) { return NULL; } + assert(res == is_new); assert(PyModule_Check(module)); // module is a strong reference diff --git a/PC/python3dll.c b/PC/python3dll.c index fa6bf1f0282b0a..ab425f4576cbea 100755 --- a/PC/python3dll.c +++ b/PC/python3dll.c @@ -292,7 +292,6 @@ EXPORT_FUNC(PyGILState_GetThisThreadState) EXPORT_FUNC(PyGILState_Release) EXPORT_FUNC(PyImport_AddModule) EXPORT_FUNC(PyImport_AddModuleObject) -EXPORT_FUNC(PyImport_AddModuleRef) EXPORT_FUNC(PyImport_AppendInittab) EXPORT_FUNC(PyImport_ExecCodeModule) EXPORT_FUNC(PyImport_ExecCodeModuleEx) @@ -310,6 +309,7 @@ EXPORT_FUNC(PyImport_ImportModule) EXPORT_FUNC(PyImport_ImportModuleLevel) EXPORT_FUNC(PyImport_ImportModuleLevelObject) EXPORT_FUNC(PyImport_ImportModuleNoBlock) +EXPORT_FUNC(PyImport_ImportOrAddModule) EXPORT_FUNC(PyImport_ReloadModule) EXPORT_FUNC(PyIndex_Check) EXPORT_FUNC(PyInterpreterState_Clear) diff --git a/Python/import.c b/Python/import.c index b6ffba5c5746e2..64e833c231f021 100644 --- a/Python/import.c +++ b/Python/import.c @@ -291,7 +291,7 @@ PyImport_GetModule(PyObject *name) if not, create a new one and insert it in the modules dictionary. */ static PyObject * -import_add_module(PyThreadState *tstate, PyObject *name) +import_add_module(PyThreadState *tstate, PyObject *name, int *is_new) { PyObject *modules = MODULES(tstate->interp); if (modules == NULL) { @@ -305,31 +305,47 @@ import_add_module(PyThreadState *tstate, PyObject *name) return NULL; } if (m != NULL && PyModule_Check(m)) { + if (is_new) { + *is_new = 0; + } return m; } Py_XDECREF(m); + m = PyModule_NewObject(name); - if (m == NULL) + if (m == NULL) { return NULL; + } if (PyObject_SetItem(modules, name, m) != 0) { Py_DECREF(m); return NULL; } + if (is_new) { + *is_new = 1; + } return m; } -PyObject * -PyImport_AddModuleRef(const char *name) +int +PyImport_ImportOrAddModule(const char *name, PyObject **pmodule) { PyObject *name_obj = PyUnicode_FromString(name); if (name_obj == NULL) { - return NULL; + *pmodule = NULL; + return -1; } PyThreadState *tstate = _PyThreadState_GET(); - PyObject *module = import_add_module(tstate, name_obj); + int is_new; + PyObject *module = import_add_module(tstate, name_obj, &is_new); Py_DECREF(name_obj); - return module; + + *pmodule = module; + if (module == NULL) { + assert(PyErr_Occurred()); + return -1; + } + return is_new; } @@ -337,7 +353,7 @@ PyObject * PyImport_AddModuleObject(PyObject *name) { PyThreadState *tstate = _PyThreadState_GET(); - PyObject *mod = import_add_module(tstate, name); + PyObject *mod = import_add_module(tstate, name, NULL); if (!mod) { return NULL; } @@ -351,7 +367,7 @@ PyImport_AddModuleObject(PyObject *name) // unknown. With weakref we can be sure that we get either a reference to // live object or NULL. // - // Use PyImport_AddModuleRef() to avoid these issues. + // Use PyImport_ImportOrAddModule() to avoid these issues. PyObject *ref = PyWeakref_NewRef(mod, NULL); Py_DECREF(mod); if (ref == NULL) { @@ -1258,7 +1274,7 @@ import_find_extension(PyThreadState *tstate, PyObject *name, return NULL; } } - mod = import_add_module(tstate, name); + mod = import_add_module(tstate, name, NULL); if (mod == NULL) { return NULL; } @@ -1386,7 +1402,7 @@ 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") */ - return import_add_module(tstate, name); + return import_add_module(tstate, name, NULL); } mod = (*p->initfunc)(); if (mod == NULL) { @@ -1671,7 +1687,7 @@ module_dict_for_exec(PyThreadState *tstate, PyObject *name) { PyObject *m, *d; - m = import_add_module(tstate, name); + m = import_add_module(tstate, name, NULL); if (m == NULL) return NULL; /* If the module is being reloaded, we get the old module back @@ -2110,7 +2126,7 @@ PyImport_ImportFrozenModuleObject(PyObject *name) if (info.is_package) { /* Set __path__ to the empty list */ PyObject *l; - m = import_add_module(tstate, name); + m = import_add_module(tstate, name, NULL); if (m == NULL) goto err_return; d = PyModule_GetDict(m); @@ -2252,8 +2268,8 @@ init_importlib(PyThreadState *tstate, PyObject *sysmod) return -1; } - PyObject *importlib = PyImport_AddModuleRef("_frozen_importlib"); - if (importlib == NULL) { + PyObject *importlib; + if (PyImport_ImportOrAddModule("_frozen_importlib", &importlib) < 0) { return -1; } IMPORTLIB(interp) = importlib; @@ -3464,7 +3480,7 @@ _imp_init_frozen_impl(PyObject *module, PyObject *name) if (ret == 0) { Py_RETURN_NONE; } - return import_add_module(tstate, name); + return import_add_module(tstate, name, NULL); } /*[clinic input] diff --git a/Python/pythonrun.c b/Python/pythonrun.c index 5f305aa00e08b9..e349393f50aae8 100644 --- a/Python/pythonrun.c +++ b/Python/pythonrun.c @@ -277,8 +277,8 @@ PyRun_InteractiveOneObjectEx(FILE *fp, PyObject *filename, return parse_res; } - PyObject *main_module = PyImport_AddModuleRef("__main__"); - if (main_module == NULL) { + PyObject *main_module; + if (PyImport_ImportOrAddModule("__main__", &main_module) < 0) { _PyArena_Free(arena); return -1; } @@ -407,9 +407,10 @@ _PyRun_SimpleFileObject(FILE *fp, PyObject *filename, int closeit, { int ret = -1; - PyObject *main_module = PyImport_AddModuleRef("__main__"); - if (main_module == NULL) + PyObject *main_module; + if (PyImport_ImportOrAddModule("__main__", &main_module) < 0) { return -1; + } PyObject *dict = PyModule_GetDict(main_module); // borrowed ref int set_file_name = 0; @@ -503,8 +504,8 @@ PyRun_SimpleFileExFlags(FILE *fp, const char *filename, int closeit, int _PyRun_SimpleStringFlagsWithName(const char *command, const char* name, PyCompilerFlags *flags) { - PyObject *main_module = PyImport_AddModuleRef("__main__"); - if (main_module == NULL) { + PyObject *main_module; + if (PyImport_ImportOrAddModule("__main__", &main_module) < 0) { return -1; } PyObject *dict = PyModule_GetDict(main_module); // borrowed ref From 390fa73e11171519439002c14b159f9b3c6f3f54 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 7 Nov 2023 10:28:29 +0100 Subject: [PATCH 2/6] Rephrase the doc --- Doc/c-api/import.rst | 24 +++++++++++------------- Include/import.h | 8 ++++---- Lib/test/test_import/__init__.py | 2 +- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/Doc/c-api/import.rst b/Doc/c-api/import.rst index 1bef07f6ce34f2..af28be1f563ce2 100644 --- a/Doc/c-api/import.rst +++ b/Doc/c-api/import.rst @@ -100,28 +100,26 @@ Importing Modules .. c:function:: int PyImport_ImportOrAddModule(const char *name, PyObject **module) - Create a new module and store it in :data:`sys.modules`, or get an already - imported module from :data:`sys.modules`. - - First check the modules dictionary if there's one there, and if not, create - a new one and insert it in the modules dictionary. - - The *name* argument may be of the form ``package.module``. + Get an already imported module from :data:`sys.modules`. If *name* is not + :data:`sys.modules`, create a new module and store it in + :data:`sys.modules`. + - If the module was already imported, set *\*module* to a :term:`strong + reference` to the existing module, and return 0. - If the module does not exist, create a module, store it in :data:`sys.modules`, set *\*module* to a :term:`strong reference` to the module, and return 1. - - If the module was already imported, set *\*module* to a :term:`strong - reference` to the existing module, and return 0. - On error, raise an exception, set *\*module* to NULL, and return -1. + The *name* argument may be of the form ``package.module``. Package + structures implied by a dotted name for *name* are not created if not + already present. + 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. .. versionadded:: 3.13 @@ -138,8 +136,8 @@ Importing Modules .. c:function:: PyObject* PyImport_AddModule(const char *name) Similar to :c:func:`PyImport_ImportOrAddModule`, but return a :term:`borrowed - reference`, and don't provide the information if the module was created or - was already imported. + reference`, and don't provide the information if the module was already + imported or was created. .. c:function:: PyObject* PyImport_ExecCodeModule(const char *name, PyObject *co) diff --git a/Include/import.h b/Include/import.h index 1c05b093118948..45d5abb2022341 100644 --- a/Include/import.h +++ b/Include/import.h @@ -45,13 +45,13 @@ PyAPI_FUNC(PyObject *) PyImport_AddModule( ); #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030d0000 -// Create a new module and store it in sys.modules, or get an already imported -// module from sys.modules. +// Get an already imported module from sys.modules. If name is not in +// sys.modules, create a new module and store it in sys.modules. // -// - If the module does not exist, create a module, store it in sys.modules, -// set '*module' to a strong reference to the module, and return 1. // - If the module was already imported, set '*module' to a strong reference // to the existing module, and return 0. +// - If the module does not exist, create a module, store it in sys.modules, +// set '*module' to a strong reference to the module, and return 1. // - On error, raise an exception, set '*module' to NULL, and return -1. PyAPI_FUNC(int) PyImport_ImportOrAddModule( const char *name, // UTF-8 encoded string diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index a6bd6bab7e7822..cf64ad70191119 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -2681,7 +2681,7 @@ def test_pyimport_addmodule(self): _testcapi.check_pyimport_addmodule(name, False) def test_pyimport_addmodule_create(self): - # gh-105922: Test PyImport_ImportOrAddModule(): create a new module + # gh-105922: Test PyImport_ImportOrAddModule(): create a new module. import _testcapi name = 'dontexist' self.assertNotIn(name, sys.modules) From ac91bea644ff5f17f5774501fd9384d9174a29c3 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 7 Nov 2023 10:42:20 +0100 Subject: [PATCH 3/6] Add import_import_or_add_module() helper function --- Python/import.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/Python/import.c b/Python/import.c index 64e833c231f021..9e6eed34a3e976 100644 --- a/Python/import.c +++ b/Python/import.c @@ -289,9 +289,8 @@ PyImport_GetModule(PyObject *name) /* Get the module object corresponding to a module name. First check the modules dictionary if there's one there, if not, create a new one and insert it in the modules dictionary. */ - static PyObject * -import_add_module(PyThreadState *tstate, PyObject *name, int *is_new) +import_import_or_add_module(PyThreadState *tstate, PyObject *name, int *is_new) { PyObject *modules = MODULES(tstate->interp); if (modules == NULL) { @@ -327,6 +326,14 @@ import_add_module(PyThreadState *tstate, PyObject *name, int *is_new) return m; } + +static PyObject * +import_add_module(PyThreadState *tstate, PyObject *name) +{ + return import_import_or_add_module(tstate, name, NULL); +} + + int PyImport_ImportOrAddModule(const char *name, PyObject **pmodule) { @@ -337,7 +344,7 @@ PyImport_ImportOrAddModule(const char *name, PyObject **pmodule) } PyThreadState *tstate = _PyThreadState_GET(); int is_new; - PyObject *module = import_add_module(tstate, name_obj, &is_new); + PyObject *module = import_import_or_add_module(tstate, name_obj, &is_new); Py_DECREF(name_obj); *pmodule = module; @@ -353,7 +360,7 @@ PyObject * PyImport_AddModuleObject(PyObject *name) { PyThreadState *tstate = _PyThreadState_GET(); - PyObject *mod = import_add_module(tstate, name, NULL); + PyObject *mod = import_add_module(tstate, name); if (!mod) { return NULL; } @@ -1274,7 +1281,7 @@ import_find_extension(PyThreadState *tstate, PyObject *name, return NULL; } } - mod = import_add_module(tstate, name, NULL); + mod = import_add_module(tstate, name); if (mod == NULL) { return NULL; } @@ -1402,7 +1409,7 @@ 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") */ - return import_add_module(tstate, name, NULL); + return import_add_module(tstate, name); } mod = (*p->initfunc)(); if (mod == NULL) { @@ -1685,14 +1692,14 @@ PyImport_ExecCodeModuleWithPathnames(const char *name, PyObject *co, static PyObject * module_dict_for_exec(PyThreadState *tstate, PyObject *name) { - PyObject *m, *d; - - m = import_add_module(tstate, name, NULL); - if (m == NULL) + PyObject *m = import_add_module(tstate, name); + if (m == NULL) { return NULL; + } + /* If the module is being reloaded, we get the old module back and re-use its dict to exec the new code. */ - d = PyModule_GetDict(m); + PyObject *d = PyModule_GetDict(m); int r = PyDict_Contains(d, &_Py_ID(__builtins__)); if (r == 0) { r = PyDict_SetItem(d, &_Py_ID(__builtins__), PyEval_GetBuiltins()); @@ -2126,7 +2133,7 @@ PyImport_ImportFrozenModuleObject(PyObject *name) if (info.is_package) { /* Set __path__ to the empty list */ PyObject *l; - m = import_add_module(tstate, name, NULL); + m = import_add_module(tstate, name); if (m == NULL) goto err_return; d = PyModule_GetDict(m); @@ -3480,7 +3487,7 @@ _imp_init_frozen_impl(PyObject *module, PyObject *name) if (ret == 0) { Py_RETURN_NONE; } - return import_add_module(tstate, name, NULL); + return import_add_module(tstate, name); } /*[clinic input] From 35f862ab0a5647f54149bc29b4a73b3234e481da Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 8 Nov 2023 13:54:50 +0100 Subject: [PATCH 4/6] Address Petr's review --- Doc/c-api/import.rst | 17 ++++++++--------- Include/import.h | 12 ++++++------ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/Doc/c-api/import.rst b/Doc/c-api/import.rst index af28be1f563ce2..fc53f3250c5fcd 100644 --- a/Doc/c-api/import.rst +++ b/Doc/c-api/import.rst @@ -100,15 +100,14 @@ Importing Modules .. c:function:: int PyImport_ImportOrAddModule(const char *name, PyObject **module) - Get an already imported module from :data:`sys.modules`. If *name* is not - :data:`sys.modules`, create a new module and store it in - :data:`sys.modules`. - - - If the module was already imported, set *\*module* to a :term:`strong - reference` to the existing module, and return 0. - - If the module does not exist, create a module, store it in - :data:`sys.modules`, set *\*module* to a :term:`strong reference` to the - module, and return 1. + Get an already imported module, or create a new empty one. + + - If the module name is already present in :data:`sys.modules`, + set *\*module* to a :term:`strong reference` to the existing module, and + return 0. + - If the module does not exist in :data:`sys.modules`, create a new empty + module, store it in :data:`sys.modules`, set *\*module* to a :term:`strong + reference` to the module, and return 1. - On error, raise an exception, set *\*module* to NULL, and return -1. The *name* argument may be of the form ``package.module``. Package diff --git a/Include/import.h b/Include/import.h index 45d5abb2022341..d8186b93165616 100644 --- a/Include/import.h +++ b/Include/import.h @@ -45,13 +45,13 @@ PyAPI_FUNC(PyObject *) PyImport_AddModule( ); #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030d0000 -// Get an already imported module from sys.modules. If name is not in -// sys.modules, create a new module and store it in sys.modules. +// Get an already imported module, or create a new empty one. // -// - If the module was already imported, set '*module' to a strong reference -// to the existing module, and return 0. -// - If the module does not exist, create a module, store it in sys.modules, -// set '*module' to a strong reference to the module, and return 1. +// - If the module name is already present in sys.modules, set '*module' to +// a strong reference to the existing module, and return 0. +// - If the module does not exist in sys.modules, create a new empty module, +// store it in sys.modules, set '*module' to a strong reference to the +// module, and return 1. // - On error, raise an exception, set '*module' to NULL, and return -1. PyAPI_FUNC(int) PyImport_ImportOrAddModule( const char *name, // UTF-8 encoded string From c12b080cb07dbdfff136b332088d4e20cbeb0afa Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 9 Nov 2023 16:58:37 +0100 Subject: [PATCH 5/6] Remove outdated comment --- Modules/_testcapimodule.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 1e2d43138a7f80..340c9e6f1f59ab 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3040,7 +3040,6 @@ check_pyimport_addmodule(PyObject *self, PyObject *args) if (!PyArg_ParseTuple(args, "si", &name, &is_new)) { return NULL; } - // name must be the name of a module which is already in sys.modules // test PyImport_ImportOrAddModule() PyObject *module = UNINITIALIZED_PTR; From af8a9dfb4ab23e5dcc5861c5a04b2e62ab8b682b Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 9 Nov 2023 17:05:45 +0100 Subject: [PATCH 6/6] Return 1 if exists, 0 if created --- Doc/c-api/import.rst | 4 ++-- Lib/test/test_import/__init__.py | 4 ++-- Modules/_testcapimodule.c | 6 +++--- Python/import.c | 16 ++++++++-------- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/Doc/c-api/import.rst b/Doc/c-api/import.rst index fc53f3250c5fcd..d16adbd8c58a50 100644 --- a/Doc/c-api/import.rst +++ b/Doc/c-api/import.rst @@ -104,10 +104,10 @@ Importing Modules - If the module name is already present in :data:`sys.modules`, set *\*module* to a :term:`strong reference` to the existing module, and - return 0. + return 1. - If the module does not exist in :data:`sys.modules`, create a new empty module, store it in :data:`sys.modules`, set *\*module* to a :term:`strong - reference` to the module, and return 1. + reference` to the module, and return 0. - On error, raise an exception, set *\*module* to NULL, and return -1. The *name* argument may be of the form ``package.module``. Package diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index cf64ad70191119..257e4164d7929f 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -2678,7 +2678,7 @@ def test_pyimport_addmodule(self): __name__, # package.module ): self.assertIn(name, sys.modules) - _testcapi.check_pyimport_addmodule(name, False) + _testcapi.check_pyimport_addmodule(name, True) def test_pyimport_addmodule_create(self): # gh-105922: Test PyImport_ImportOrAddModule(): create a new module. @@ -2687,7 +2687,7 @@ def test_pyimport_addmodule_create(self): self.assertNotIn(name, sys.modules) self.addCleanup(unload, name) - mod = _testcapi.check_pyimport_addmodule(name, True) + mod = _testcapi.check_pyimport_addmodule(name, False) self.assertIs(mod, sys.modules[name]) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 340c9e6f1f59ab..f1b9d68b057cdb 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3036,8 +3036,8 @@ static PyObject * check_pyimport_addmodule(PyObject *self, PyObject *args) { const char *name; - int is_new; - if (!PyArg_ParseTuple(args, "si", &name, &is_new)) { + int expected; + if (!PyArg_ParseTuple(args, "si", &name, &expected)) { return NULL; } @@ -3047,7 +3047,7 @@ check_pyimport_addmodule(PyObject *self, PyObject *args) if (res < 0) { return NULL; } - assert(res == is_new); + assert(res == expected); assert(PyModule_Check(module)); // module is a strong reference diff --git a/Python/import.c b/Python/import.c index 9e6eed34a3e976..c166a1cc08c313 100644 --- a/Python/import.c +++ b/Python/import.c @@ -290,7 +290,7 @@ PyImport_GetModule(PyObject *name) First check the modules dictionary if there's one there, if not, create a new one and insert it in the modules dictionary. */ static PyObject * -import_import_or_add_module(PyThreadState *tstate, PyObject *name, int *is_new) +import_import_or_add_module(PyThreadState *tstate, PyObject *name, int *exists) { PyObject *modules = MODULES(tstate->interp); if (modules == NULL) { @@ -304,8 +304,8 @@ import_import_or_add_module(PyThreadState *tstate, PyObject *name, int *is_new) return NULL; } if (m != NULL && PyModule_Check(m)) { - if (is_new) { - *is_new = 0; + if (exists) { + *exists = 1; } return m; } @@ -320,8 +320,8 @@ import_import_or_add_module(PyThreadState *tstate, PyObject *name, int *is_new) return NULL; } - if (is_new) { - *is_new = 1; + if (exists) { + *exists = 0; } return m; } @@ -343,8 +343,8 @@ PyImport_ImportOrAddModule(const char *name, PyObject **pmodule) return -1; } PyThreadState *tstate = _PyThreadState_GET(); - int is_new; - PyObject *module = import_import_or_add_module(tstate, name_obj, &is_new); + int exists; + PyObject *module = import_import_or_add_module(tstate, name_obj, &exists); Py_DECREF(name_obj); *pmodule = module; @@ -352,7 +352,7 @@ PyImport_ImportOrAddModule(const char *name, PyObject **pmodule) assert(PyErr_Occurred()); return -1; } - return is_new; + return exists; }