fix: PyCapsule_GetDestructor is allowed to return a nullptr destructor

[galv]

Description

Previously, this code would error out if the destructor happened to be a nullptr. This is incorrect. nullptrs are allowed for capsule destructors.

"It is legal for a capsule to have a NULL destructor. This makes a NULL return code somewhat ambiguous; use PyCapsule_IsValid() or PyErr_Occurred() to disambiguate."

See:

https://docs.python.org/3/c-api/capsule.html#c.PyCapsule_GetDestructor

I noticed this while working on a type caster related to #3858 DLPack happens to allow the destructor not to be defined on a capsule, and I encountered such a case. See:

https://github.com/dmlc/dlpack/blob/e2bdd3bee8cb6501558042633fa59144cc8b7f5f/include/dlpack/dlpack.h#L219

Suggested changelog entry:

* Allow pybind11::capsule constructor to take null destructor pointers.

[Skylion007]

Thanks @galv, oversight on my part when rewriting the error handling of capsules a few months ago.

We also have an open issue to include dlpack casters as officially supported in pybind11 so consider making a PR once you finish writing the casters.

[rwgk]

Thanks for the fix! (Sorry I missed this, too.)

[rwgk]

Oh, wait, I'll add a test (to be sure we don't miss this again).

[rwgk]

f626d7d adds a simple test.

Remark regarding implementation:

I tried to use reinterpet_cast but that did not work:

clang++ -o pybind11/tests/test_pytypes.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_pytypes.cpp
/usr/local/google/home/rwgk/forked/pybind11/tests/test_pytypes.cpp:294:43: error: reinterpret_cast from 'std::nullptr_t' to 'void (*)(void *)' is not allowed
return py::capsule((void *) 1234, reinterpret_cast<void (*)(void *)>(nullptr)); // #4221
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

To be sure the test is actually exercising the fix:

I cherry-picked f626d7d on top of master, then ran test_pytypes:

Running tests in directory "/usr/local/google/home/rwgk/forked/pybind11/tests":
=========================================================== test session starts ============================================================
platform linux -- Python 3.10.7, pytest-7.1.2, pluggy-1.0.0
C++ Info: Debian Clang 14.0.6 C++17 __pybind11_internals_v4_clang_libstdcpp_cxxabi1002__
rootdir: /usr/local/google/home/rwgk/forked/pybind11/tests, configfile: pytest.ini
collected 79 items
test_pytypes.py ....................terminate called after throwing an instance of 'std::runtime_error'
what(): Unable to get capsule context
Fatal Python error: Aborted
Current thread 0x00007fe523f4e740 (most recent call first):
File "/usr/local/google/home/rwgk/forked/pybind11/tests/test_pytypes.py", line 304 in test_capsule
... (rest of trace omitted) ...

[rwgk]

@Skylion007 Does the added test look good to you?

[rwgk]

Thanks! Waiting a moment before merging this, until we have #4201 stable again.

[rwgk]

Thanks for the fix!

[Skylion007]

@galv@rwgk Out of curiosity, I noticed we store the destructor in the context, when PyCapsule's appear to have a dedicated slot for the destructor. Any idea why? @rwgk@galv@wjakob ?

[rwgk]

@galv@rwgk Out of curiosity, I noticed we store the destructor in the context, when PyCapsule's appear to have a dedicated slot for the destructor. Any idea why? @rwgk@galv@wjakob ?

Oh, same question, sorry I didn't see this before.

https://github.com/pybind/pybind11/pull/4232/files#r991640142