bpo-42327: Add PyModule_Add() (smaller).

[serhiy-storchaka]

Also fix possible leaks in initialization of modules: _curses_panel, _decimal, _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time, xxsubtype.

https://bugs.python.org/issue42327

#86493

[serhiy-storchaka]

This is the same as #23240, but without large changes to modules _ssl, _testbuffer, _testcapi, posix, select, socket, _msi, msvcrt. They will be extracted to separate PRs.

[vstinner]

I like the new functon, but I prefer PyModule_AddObjectRef(obj) rather than Py_INCREF(obj); PyModule_Add(obj). In some cases, you removed a test for NULL, but I don't think that it's worth it (I prefer an explicit test with PyModule_AddObjectRef, but that's a personal preference, I guess). At least, when not test for == NULL is needed, PyModule_AddObjectRef() should be preferred IMO.

[vstinner]

Could you split this PR in two parts? The first uncontroversial PR which only adds the function, and then a second PR which modify PyInit functions to use it.

These days, there are dozens of PRs modifying PyInit functions which would be in conflict with the second part. See bpo-40077 and bpo-1635741.

[tiran]

I still prefer https://github.com/python/cpython/pull/23286/files over PyModule_Add. The three new APIs from my PR will simplify a lot of common cases:

• initialize and add new type to module dict
• initialize and add new exception to module dict
• add int/str/float/bool constants from a static array

How about we land PR #23286 first, port more code to the new API, and then see if we still need PyModule_Add?

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[vstinner]

I dislike APIs stealing references. IMO we should stop adding such API. I would prefer regular (boring and verbose) reference counting to make the C API easier to understand. Currently, developers have to check the documentation of each function to check if it returns a borrowed or strong reference, if it adds or removes a reference (and leave the ref count unchanged), etc.

[serhiy-storchaka]

It is a convenient API. How else would you fix Modules/mathmodule.c, for example?

The "N" code in Py_BuildValue() serves the same purpose. It allows to write in one line the code which would require 7 lines otherwise:

return Py_BuildValue("Nl", _PyLong_FromTime_t(sec), nsec);

instead of:

PyObject *tmp = _PyLong_FromTime_t(sec);
if (tmp == NULL) {
    return NULL;
}
PyObject *res = Py_BuildValue("Ol", tmp, nsec);
Py_DECREF(tmp);
return res;

[vstinner]

It is a convenient API. How else would you fix Modules/mathmodule.c, for example?

Usually, a macro is written to create a temporary object and add it to the module. If you use PyModule_AddObjectRef(), the macro would also clear the reference.

I dislike the fact that macro is "required" to write correct code.

In 2020, @tiran proposed designing a new API to make this work easier with a declarative syntax: https://discuss.python.org/t/define-module-constants-in-pymoduledef/5749

[serhiy-storchaka]

Macro is convenient because it can include "goto error" or "return NULL". But having to write such macro in every module is tiring. PyModule_AddNew() does not need to be a macro, it can be a regular function. It is the original PyModule_AddObject() written right.

[serhiy-storchaka]

We can make it a private function at first if you so dislike adding it to the public API. Then we can add new @tiran's API and decide whether it is worth to add public PyModule_AddNew(). But I think that it is better to have PyModule_AddNew() than not have neither PyModule_AddNew() nor new module initialization API.

[encukou]

I dislike APIs stealing references. IMO we should stop adding such API.

IMO, that's a good guideline, but PyModule_AddNew is a great example of when we should break this guideline. (Doesn't beat Py_DecRef, but it's close!)

But it should be very clear from the name. I don't think New is clear enough, and would prefer to one of Steal, Consume, Take, Move (see this draft) -- but we should settle on one of those and use it consistently for new API of this kind.

[vstinner]

To be clear, I dislike APIs stealing references, but I don't plan to block this PR. I will just not endorse it :-)

If using PyModule_AddObjectRef() is not convenient, maybe we need to provide "something" to make it easier to use. Sadly, the C language is limited in terms of macros/templates :-( The approach based on a description is promising, but I also know that there are many technical challenges which may not be possible to be solved.

By the way, I'm really unhappy on the bad state of our PyInit functions: many of them simply have no error checking, or just one final PyErr_Occurred() check. That's bad. The trend is more to disallow calling functions with an exception set (it might be enforced later). If this new function makes the situation better: go ahead ;-)

[vstinner]

What does "New" stand for? This function does not create a new reference, but steals a reference. Is is "New" because its a new function replacing an old one? What if tomorrow, there is a newer function? Should we call it PyModule_AddNewer or PyModule_AddNewEx?

I suggest to just call it PyModule_Add().

[serhiy-storchaka]

It adds a newly created object.
I think that the format unit "N" in Py_BuildValue() means "new", because it is purposed to add newly created object, for which we do not need to keep other reference. I was guided by the same reasons for choosing name PyModule_AddNew(). Initially I named it simply PyModule_Add(). I can rename it back.

[vstinner]

It may be easier to review this big PR if you could move code fixing PyInit functions into a separated PR.

[vstinner]

PyModule_AddObjectRef() must not be called with NULL. Can you please elaborate the behavior when value is NULL? Something like:

If value is NULL, just return -1.

[serhiy-storchaka]

PyModule_AddObjectRef() can be called with NULL if an exception is set. Actually, some of my fixes which use PyModule_AddObjectRef() rely on this.

[vstinner]

I suggest to add "and value can be NULL".

[vstinner]

It's not similar to PyModule_AddObjectRef(): value can be NULL.

Suggested change

	// Similar to PyModule_AddObjectRef() and PyModule_Add() but steal
	// Similar to PyModule_Add() but steal

[vstinner]

IMO it's important important to put it in the documentation. This kind of definition fits perfectly with the Soft Deprecated status. Maybe also soft deprecate this API, similar to getopt and optparse soft deprecation. Example with getopt:

.. deprecated:: 3.13
   The :mod:`getopt` module is :term:`soft deprecated` and will not be
   developed further; development will continue with the :mod:`argparse`
   module.

[serhiy-storchaka]

Do you suggest to convert the note block into deprecated?

[serhiy-storchaka]

It may be easier to review this big PR if you could move code fixing PyInit functions into a separated PR.

Already did this. #106767 and #106768.

[vstinner]

Ok, but this PR changes Modules/_curses_panel.c for example, and the change doesn't even use PyModule_Add(). Can you maybe revert these changes? If not, tell me, I will try to get more time to through the whole change.

[vstinner]

LGTM.

If I have further comments, I may propose them as a separated PR.

[serhiy-storchaka]

Thanks Victor.

[vstinner]

Even if I dislike stealing references, the API seems to help simplifying PyInit code a lot.

[vstinner]

I added the PyModule_Add() function to the pythoncapi-compat project: python/pythoncapi-compat@e3f0eba

[vstinner]

I wrote PR #106859 to document PyModule_AddObject() soft deprecation in What's New in Python 3.13.