@serhiy-storchaka: Ingh-86160, you added this code using PyWeakref_New() + PyWeakref_GET_OBJECT() get a borrowed reference, whereas the module is known to be stored in the sys.modules dictionary. What's the rationale for using a weak reference, rather than just using Py_DECREF()?

I reverted the doc change.

Oh,./python -m test -j1 -R 3:3 test_import leaks references ("test_import leaked [11, 13, 10] references, sum=34")... but it also leaks in the main branch. Ah! It's unrelated to this PR. Also,./python -m test -R 3:3 test_import doesn't leak. It's inteesting :-)

sys.modules can be not a dict, and it is not guaranteed that it keeps reference. It can be a custom mapping with__getitem__ which returns a new object or removes returned object or__setitem__ which does nothing. There is so much unknown. With weakref we can be sure that we get either a reference to live object or NULL. Please do not change this code until you find better solution.

Also,./python -m test -j1 -R 3:3 test_import does crash, but again, it's unrelated to this PR. It does also crash in the main branch. It's a recent regression:#91095 (comment)

PyImport_AddModule() history, oldest to newest:

• In 1990, commit85a5fbb (first commit which added C code to CPython, so oldest C code tracked by the Git repository) addednew_module() function which usesmtab = sysget("modules"); to get modules.sysget() is implemented asdictlookup(sysdict, name) which looks intostatic object *sysdict;.This version respected sys.modules :-)

• In 1990, commit3f5da24 addedstatic object *modules; variable toPython/import.c, variable used byadd_module() (old name of PyImport_AddModule()).sys.modules is no longer used by add_modules().

• In 1997, commit25ce566 added PyImport_GetModuleDict() implemented asPyThreadState_Get()->interp. PyImport_AddModules() usesPyObject *modules = PyImport_GetModuleDict(); and then works on this dictionary withPyDict_GetItemString() andPyDict_SetItemString().

• In 2017, commit3f9eee6 modified import_add_module() to "Support other mappings in PyInterpreterState.modules": issue[subinterpreters] Eliminate PyInterpreterState.modules. #72597.

• In 2021, commit4db8988 changes PyImport_AddModule() to use PyWeakref_New() to create a borrowed reference.

• In 2023, commitb2fc549 changesPython/import.c to use a new MODULES(interp) macro which getsinterp->imports.modules.

In 2017, commit3f9eee6 modified import_add_module() to "Support other mappings in PyInterpreterState.modules": issueGH-72597.

I'm not convinced thatGH-72597 was fully implemented. At least, PyImport_AddModule() doesn't support overridingsys.modules at runtime since 1990... so since the creation of Python :-)

I added a test to my PR to check this special case: customsys.modules which doesn't hold a strong reference to the newly created module.

According to this test, PyImport_AddModule() always useststate->interp->imports.modules directly, it doesn't take in account thatsys.modules was overriden. Moreover, settingsys.modules at runtime doesn't update this internaltstate->interp->imports.modules member: the sys module has no custom__setattr__() function to update it.

Calling PyImport_AddModule() writes into the "original" modules dict, even ifsys.modules was overriden.

In the main branch, there is a single line which setsPyInterpreter.imports.modules:MODULES(interp) = PyDict_New(); of_PyImport_InitModules(). I don't see any C API, even internal C API, to changes this PyInterpreterState member. And the public C APIPyImport_GetModuleDict() just returns this member.

It means that all C code using directlyPyInterpreter.imports.modules can only get a dict, not a custom type.

Obviously, C code and Python code gettingsys.modules attribute can get any type, not only dict, ifsys.modules is overriden. For example, the_pickle extension (implemented in C) gets thesys.modules attribute and then... uses the PyDict C API, oops. It is likely to crash ifsys.modules type is not dict or a subclass of dict.

Well, my goal here was to get rid of thePyWeakref_GET_OBJECT() since I would like to deprecate it. So I wrote a different change: PR#106001 uses_PyWeakref_GET_REF().

But maybe this whole code was always useless and so can be removed, since it seems impossible to get a type other than dict in this code path.

I have great doubts that it was worth to add support of non-dict assys.modules. It complicated a code a lot, it is not supported everywhere in the C code, and causes issues with borrowed references, like in this code.

I have great doubts that it was worth to add support of non-dict as sys.modules. It complicated a code a lot, it is not supported everywhere in the C code, and causes issues with borrowed references, like in this code.

The problem is that modules have no__setattr__() method which could be used to prevent settingsys.modules to a non-sense value likesys.modules = 123 orsys.modules = ["my", "list"]. Maybe some developers made the wrong assumption that internal C functions really accesssys.modules attributes, whereas the implementation access directly aPyInterpreterState member and ignoressys.modules.

I created issue#106016 "Support definingsetattr() anddelattr() in a module". It would allow to either disallow setting sys.modules to a type different than dict, or at least to updatePyInterpreterState.imports.modules reference.

I fixed the issue differently: commitee52158

I don't think that PyImport_AddModuleObject() requires this complicated PyWeakref dance, since the code always gets a strong reference from a Python dict: it's not possible to overridePyInterpreterState.imports.modules, and this function access directlyPyInterpreterState.imports.modules. But I don't feel the need to change the code. I propose to continue the discussion in the issue#106016.