feat: change PYBIND11_MODULE to use multi-phase init (PEP 489)

[b-pass]

Description

Changed PYBIND11_MODULE to use multi-phase init. Multi-phase init (slots) is required for sub-interpreter support in (see #5564). This splits the init portion from that PR out into this smaller PR as requested by @rwgk .

This PR Adds a new function init_module_def to module_, similar to the existing (unchanged) create_extension_module. init_module_def takes a variadic final argument to allow for future initialization tags (no tags are added in this PR, the py::mod_gil_not_used tag works with this new function).

This also has the minor side benefit of not needing to use the unstable API (PyUnstable_Module_SetGIL) for the Py_MOD_GIL_NOT_USED option. However, the module_::create_extension_module function has not been changed and so it still uses the unstable API.

No tests are changed, the arguments to the macro are unchanged, and all behavior is expected to be the same. Multi-phase init has been supported since Python 3.5, so no compatibility issues are expected either.

Suggested changelog entry:

* Changed ``PYBIND11_MODULE`` macro implementation to perform multi-phase module initialization (PEP 489) behind the scenes.

[rwgk]

We're always using squash-merge.

To make reviewing easier (especially in follow-on PRs), could you please never force push?

[rwgk]

Slight worries: Could we get into trouble during process teardown?

Concretely, could there be a situation in which slots.data() is referenced after this static variable was deleted already?

Probably not ... but maybe?

I'd want to err on the side of "abundance of caution".

I think we should use the standard trick of leaking a pointer. It's really simple, all doubts removed. Untested:

    static auto *PYBIND11_CONCAT(pybind11_module_slots_, name) = new std::vector<PyModuleDef_Slot>();

[henryiii]

I'd prefer not to leak a pointer unless we are sure it is needed. Isn't this already the lifetime of the module, though?

[b-pass]

Yes, this is a static global for the lifetime of the module... it ends up referenced inside the initialized PyModuleDef, which is defined right above it. So I don't think there's any way that the PyModuleDef could outlive it.

[rwgk]

It's standard best practice for non-POD static objects.

There is no good reason for taking any risks (running destructors during process teardown; or freeing in the wrong order). The memory will be freed by the kernel for sure.

[b-pass]

Ok, I switched it to an std::array of slots, which is POD (since PyModuleDef_Slot is POD), so it has no destructor.... it's more along the lines of the original suggestion of using a named constant instead of a magic number for the size.... the size is part of the typedef (with a comment, ofc).

[henryiii]

PS: Generally, I would not force push to answer a review comment, only to rebase or squash unimportant details. For those, I strongly recommend and encourage force pushing normally, so we aren't all of the same opinion.¹ For addressing a review, though, you want the commit history clearly displayed. But @rwgk prefers avoiding it entirely, and he's the main reviewer, so please avoid in pybind11. :)

Footnotes

1. In pypa/build we lost a great PR because it was constantly merged instead of rebased, and became an absolute spaghetti mess that we couldn't recover to a state where it was mergable after a few months. I wasted several hours trying to make it mergable. ↩

[rwgk]

But @rwgk prefers avoiding it entirely

Overall that works best for me, but I don't want to be an extremist :-)
Occasionally I force push, too.

[b-pass]

But @rwgk prefers avoiding it entirely

Overall that works best for me, but I don't want to be an extremist :-) Occasionally I force push, too.

I prefer not to force push but I've contributed to other projects where they really want everything squashed all the time... 🤷‍♂️

[rwgk]

Looks great to me, thanks!

[rwgk]

Thanks @b-pass!

[rwgk]

I made a slight edit to the PR description. This sure is worth mentioning in the changelog.