Repository navigation
C API: Add PyImport_AddModuleRef() function #105922
Description
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:
- "Functions must not return borrowed references" says https://devguide.python.org/developer-workflow/c-api/index.html
- Unclear lifetimes of borrowed references capi-workgroup/problems#5
I proposed to:
- Add a new
PyImport_AddModuleRef(const char *name)function, similar toPyImport_AddModule()but return a strong reference - Deprecate
PyImport_AddModule()andPyImport_AddModuleObject()
Activity
pythongh-105922: Add PyImport_AddModuleRef() function
pythongh-105922: Add PyImport_AddModuleRef() function
pythongh-105922: Add PyImport_AddModuleRef() function
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
gh-105922: Refactor PyRun_InteractiveOneObjectEx() (#105925)
pythongh-105922: Add PyImport_AddModuleRef() function
pythongh-105922: Add PyImport_AddModuleRef() function
gh-105922: Add PyImport_AddModuleRef() function (#105923)
erlend-aasland commentedon Jun 20, 2023
The PRs are merged; looks like this can be closed.
vstinner commentedon Jun 21, 2023
Well, this part is not done yet:
vstinner commentedon Jun 21, 2023
I added PyImport_AddModuleRef() to pythoncapi-compat: commit python/pythoncapi-compat@8ba8db3
encukou commentedon 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_AddModuleRefas 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 commentedon Jun 22, 2023
@encukou:
I'm not sure. Here is a code search on PyPI top 5,000 projcts: 31 projects use it.
pythongh-105922: Use PyImport_AddModuleRef() function
pythongh-105922: PyImport_AddModule() uses Py_DECREF()
pythongh-105922: PyImport_AddModule() uses Py_DECREF()
gh-105922: Use PyImport_AddModuleRef() function (#105999)
[-]C API: PyImport_AddModule() returns a borrowed reference[/-][+]C API: Add PyImport_AddModuleRef() function[/+]vstinner commentedon Jul 10, 2023
The initial issue, adding PyImport_AddModuleRef() function, was done, so I close the issue.
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.moduleswhich holds a strong reference. Moreover, I checked and it looks impossible to use a type other than dict forsys.modulesin practice, since PyImport_AddModuleObject() access directly toPyInterpreterState.imports.moduleswhich 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 commentedon Jul 20, 2023
Let me bring the discussion from python/steering-council#201 (comment) here:
No, there is no such request. Neither is there a user request for a
PyImport_AddModuleRef.I opened an issue here: #106915