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

C API: Add PyImport_AddModuleRef() function #105922

Closed
vstinner opened this issue Jun 19, 2023 · 7 comments
Closed

C API: Add PyImport_AddModuleRef() function #105922

vstinner opened this issue Jun 19, 2023 · 7 comments
Labels
topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

vstinner commented Jun 19, 2023

The C API PyImport_AddModule() returns a borrowed reference using a special dance added by commit 4db8988 of issue #86160:

        PyObject *ref = PyWeakref_NewRef(mod, NULL);
        Py_DECREF(mod);
        if (ref == NULL) {
            return NULL;
        }
        mod = PyWeakref_GetObject(ref);
        Py_DECREF(ref);

Borrowed references are bad:

I proposed to:

  • Add a new PyImport_AddModuleRef(const char *name) function, similar to PyImport_AddModule() but return a strong reference
  • Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

Linked PRs

@vstinner vstinner added type-bug An unexpected behavior, bug, or error topic-C-API labels Jun 19, 2023
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
Add tests on PyImport_AddModuleRef(), PyImport_AddModule() and
PyImport_AddModuleObject().
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
* 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.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
* 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.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
Refactor PyRun_InteractiveOneObjectEx(), _PyRun_SimpleFileObject()
and PyRun_SimpleStringFlags():

* Declare variables closer to where they are defined
* Rename variables to use name longer than 1 character
* Use PyImport_AddModule() with Py_XNewRef()
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
Refactor PyRun_InteractiveOneObjectEx(), _PyRun_SimpleFileObject()
and PyRun_SimpleStringFlags():

* Keep a strong reference to the __main__ module while using its
  dictionary (PyModule_GetDict()).
* Declare variables closer to where they are defined
* Rename variables to use name longer than 1 character
* Use PyImport_AddModule() with Py_XNewRef()
* Add pyrun_one_parse_ast() sub-function.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
Refactor PyRun_InteractiveOneObjectEx(), _PyRun_SimpleFileObject()
and PyRun_SimpleStringFlags():

* Keep a strong reference to the __main__ module while using its
  dictionary (PyModule_GetDict()).
* Declare variables closer to where they are defined
* Rename variables to use name longer than 1 character
* Use PyImport_AddModule() with Py_XNewRef()
* Add pyrun_one_parse_ast() sub-function.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
Refactor PyRun_InteractiveOneObjectEx(), _PyRun_SimpleFileObject()
and PyRun_SimpleStringFlags():

* Keep a strong reference to the __main__ module while using its
  dictionary (PyModule_GetDict()). Use PyImport_AddModule() with
  Py_XNewRef().
* Declare variables closer to where they are defined.
* Rename variables to use name longer than 1 character.
* Add pyrun_one_parse_ast() sub-function.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
Refactor PyRun_InteractiveOneObjectEx(), _PyRun_SimpleFileObject()
and PyRun_SimpleStringFlags():

* Keep a strong reference to the __main__ module while using its
  dictionary (PyModule_GetDict()). Use PyImport_AddModule() with
  Py_XNewRef().
* Declare variables closer to where they are defined.
* Rename variables to use name longer than 1 character.
* Add pyrun_one_parse_ast() sub-function.
vstinner added a commit that referenced this issue Jun 19, 2023
Refactor PyRun_InteractiveOneObjectEx(), _PyRun_SimpleFileObject()
and PyRun_SimpleStringFlags():

* Keep a strong reference to the __main__ module while using its
  dictionary (PyModule_GetDict()). Use PyImport_AddModule() with
  Py_XNewRef().
* Declare variables closer to where they are defined.
* Rename variables to use name longer than 1 character.
* Add pyrun_one_parse_ast() sub-function.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2023
* Add tests on PyImport_AddModuleRef(), PyImport_AddModule() and
  PyImport_AddModuleObject().
* pythonrun.c: Replace Py_XNewRef(PyImport_AddModule(name)) with
  PyImport_AddModuleRef(name).
vstinner added a commit to vstinner/cpython that referenced this issue Jun 20, 2023
* Add tests on PyImport_AddModuleRef(), PyImport_AddModule() and
  PyImport_AddModuleObject().
* pythonrun.c: Replace Py_XNewRef(PyImport_AddModule(name)) with
  PyImport_AddModuleRef(name).
vstinner added a commit that referenced this issue Jun 20, 2023
* Add tests on PyImport_AddModuleRef(), PyImport_AddModule() and
  PyImport_AddModuleObject().
* pythonrun.c: Replace Py_XNewRef(PyImport_AddModule(name)) with
  PyImport_AddModuleRef(name).
@erlend-aasland
Copy link
Contributor

The PRs are merged; looks like this can be closed.

@vstinner
Copy link
Member Author

The PRs are merged; looks like this can be closed.

Well, this part is not done yet:

Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

@vstinner
Copy link
Member Author

I added PyImport_AddModuleRef() to pythoncapi-compat: commit python/pythoncapi-compat@8ba8db3

@encukou
Copy link
Member

encukou commented Jun 21, 2023

Is this function actually useful for anything except the __main__ module?
It returns an already loaded module, or a new empty module, but doesn't give any indication of which one of those it did. IMO, that means you can't really use it for anything except a module that's supposed to stay empty.

Would it be better to deprecate the old functions without direct replacement, and add PyImport_AddModuleRef as a recipe in the docs that users could adapt to their needs?

Or, should we change the function to indicate whether the returned module is new or not?

@vstinner
Copy link
Member Author

@encukou:

Is this function actually useful for anything except the main module?

I'm not sure. Here is a code search on PyPI top 5,000 projcts: 31 projects use it.

cffi-1.15.1.tar.gz: cffi-1.15.1/cffi/_cffi_errors.h: m = PyImport_AddModule("_cffi_error_capture");
matplotlib-3.7.1.tar.gz: matplotlib-3.7.1/src/_macosx.m: if (!(builtins = PyImport_AddModule("builtins"))  // borrowed.
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule(__Pyx_BUILTIN_MODULE_NAME); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule((char *) "cython_runtime"); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule("%s"); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Includes/cpython/module.pxd: PyObject* PyImport_AddModule(const char *name) except NULL
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/CommonStructures.c: fake_module = PyImport_AddModule((char*) "_cython_" CYTHON_ABI);
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/CommonStructures.c: fake_module = PyImport_AddModule((char*) "_cython_" CYTHON_ABI);
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/ModuleSetupCode.c: abi_module = PyImport_AddModule(__Pyx_FastGIL_ABI_module);
cloudpickle-2.2.1.tar.gz: cloudpickle-2.2.1/tests/cloudpickle_testpkg/_cloudpickle_testpkg/mod.py: # it will use the Python/C API ``PyImport_AddModule(submodule_qualified_name)``
cloudpickle-2.2.1.tar.gz: cloudpickle-2.2.1/tests/cloudpickle_testpkg/build/lib/_cloudpickle_testpkg/mod.py: # it will use the Python/C API ``PyImport_AddModule(submodule_qualified_name)``
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: pub fn PyImport_AddModuleObject(name: *mut PyObject) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: #[cfg_attr(PyPy, link_name = "PyPyImport_AddModule")]
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: pub fn PyImport_AddModule(name: *const c_char) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: pub fn PyImport_AddModuleObject(name: *mut PyObject) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: #[cfg_attr(PyPy, link_name = "PyPyImport_AddModule")]
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: pub fn PyImport_AddModule(name: *const c_char) -> *mut PyObject;
sentencepiece-0.1.98.tar.gz: sentencepiece-0.1.98/src/sentencepiece/sentencepiece_wrap.cxx: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
opencv-python-4.7.0.72.tar.gz: opencv-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-python-4.7.0.72.tar.gz: opencv-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/include/jp_primitive_accessor.h: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_booleantype.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_booleantype.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_context.cpp: JPPyObject import = JPPyObject::use(PyImport_AddModule("importlib.util"));
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_functional.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
pybind11-2.10.4.tar.gz: pybind11-2.10.4/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
opencv-python-headless-4.7.0.72.tar.gz: opencv-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-python-headless-4.7.0.72.tar.gz: opencv-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
onnx-1.13.1.tar.gz: onnx-1.13.1/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: if (!PyImport_AddModule("dummy_threading"))
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: py_file_module = PyImport_AddModule(name);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: new_uwsgi_module = PyImport_AddModule("uwsgi");
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
python-crfsuite-0.9.9.tar.gz: python-crfsuite-0.9.9/crfsuite/swig/python/export_wrap.cpp: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_launch.c: __main__ = PI_PyImport_AddModule("__main__");
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.c: DECLPROC(PyImport_AddModule);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.c: GETPROC(dll, PyImport_AddModule);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.h: EXTDECLPROC(PyObject *, PyImport_AddModule, (char *));
opencv-contrib-python-4.7.0.72.tar.gz: opencv-contrib-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-contrib-python-4.7.0.72.tar.gz: opencv-contrib-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
PyMuPDF-1.21.1.tar.gz: PyMuPDF-1.21.1/fitz/helper-other.i: PyObject *this_module = PyImport_AddModule("fitz");  // get our module
cvxpy-1.3.1.tar.gz: cvxpy-1.3.1/cvxpy/cvxcore/python/cvxcore_wrap.cxx: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
mecab-python3-1.0.6.tar.gz: mecab-python3-1.0.6/src/MeCab/MeCab_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
M2Crypto-0.38.0.tar.gz: M2Crypto-0.38.0/src/SWIG/_m2crypto_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygraphviz-1.10.zip: pygraphviz-1.10/pygraphviz/graphviz_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
opencv-contrib-python-headless-4.7.0.72.tar.gz: opencv-contrib-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-contrib-python-headless-4.7.0.72.tar.gz: opencv-contrib-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdal_array_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdal_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdalconst_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gnm_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/ogr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/osr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/PythonEngine.cs: var module = Runtime.PyImport_AddModule(name);
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.Delegates.cs: PyImport_AddModule = (delegate* unmanaged[Cdecl]<StrPtr, BorrowedReference>)GetFunctionByName(nameof(PyImport_AddModule), GetUnmanagedDll(_PythonDll));
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.Delegates.cs: internal static delegate* unmanaged[Cdecl]<StrPtr, BorrowedReference> PyImport_AddModule { get; }
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.cs: internal static BorrowedReference PyImport_AddModule(string name)
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.cs: return Delegates.PyImport_AddModule(namePtr);
pyarmor-8.1.3.zip: pyarmor-8.1.3/src/helper/buildext.py: m = PyImport_AddModule("__main__");
dlib-19.24.1.tar.gz: dlib-19.24.1/dlib/external/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
osmium-3.6.0.tar.gz: osmium-3.6.0/contrib/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
osmium-3.6.0.tar.gz: osmium-3.6.0/contrib/pybind11/tests/test_modules.py: # Meant to trigger PyImport_AddModule() failure:
onnxoptimizer-0.3.10.tar.gz: onnxoptimizer-0.3.10/third_party/onnx/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/casadi.i: PyObject* module = PyImport_AddModule("casadi");
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule(SWIG_module_name);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule("casadi");
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule(SWIG_module_name);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule("casadi");
onnxsim-0.4.19.tar.gz: onnxsim-0.4.19/third_party/onnx-optimizer/third_party/onnx/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
onnxsim-0.4.19.tar.gz: onnxsim-0.4.19/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdal_array_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdal_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdalconst_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/ogr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/osr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
mercurial-6.4.1.tar.gz: mercurial-6.4.1/contrib/fuzz/pyutil.cc: mainmod = PyImport_AddModule("__main__");

vstinner added a commit to vstinner/cpython that referenced this issue Jun 22, 2023
Replace PyImport_AddModuleObject() + Py_XNewRef() with
PyImport_AddModuleRef() to get directly a strong reference.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 22, 2023
Rewrite PyImport_AddModule() to simply call Py_DECREF(), rather than
creating a weak reference,  to get a borrowed reference to the
module.

In the documentation, add a link to sys.modules to explicit which
"modules" are checked.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 22, 2023
Rewrite PyImport_AddModule() to simply call Py_DECREF(), rather than
creating a weak reference,  to get a borrowed reference to the
module.
vstinner added a commit that referenced this issue Jun 22, 2023
Replace PyImport_AddModuleObject() + Py_XNewRef() with
PyImport_AddModuleRef() to get directly a strong reference.
@vstinner vstinner changed the title C API: PyImport_AddModule() returns a borrowed reference C API: Add PyImport_AddModuleRef() function Jun 23, 2023
@vstinner
Copy link
Member Author

The initial issue, adding PyImport_AddModuleRef() function, was done, so I close the issue.

Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

IMO PyWeakref_GetObject() API is unsafe since it returns a borrowed reference from a weak reference, so I chose to deprecate it as soon as I added PyWeakref_GetRef().

For PyImport_AddModuleObject(), it's less obvious since the module must be stored in sys.modules which holds a strong reference. Moreover, I checked and it looks impossible to use a type other than dict for sys.modules in practice, since PyImport_AddModuleObject() access directly to PyInterpreterState.imports.modules which cannot be overriden.

See PR #105998 and PR #106001 for technical details.

Moreover, the Steering Council is still discussing the topic: python/steering-council#201

For now, I prefer to not deprecate the old API. We can still deprecate it later once the situation will be more clear.

@encukou
Copy link
Member

encukou commented Jul 20, 2023

Let me bring the discussion from python/steering-council#201 (comment) here:

We will want to yet another function to fix that wart, and if no one gets to it in time, the *Ref iteration will be hard to get rid of.

Is there any user request for such feature? Can't you get the behavior you want with existing functions, like checking if the name is in sys.modules?

No, there is no such request. Neither is there a user request for a PyImport_AddModuleRef.

One minor change would be to provide the information if the module was imported or created (add an optional parameter for that). You can open an issue if you want that.

I opened an issue here: #106915

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants