[smart_holder] Unique ptr deleter roundtrip tests and fix

[iwanders]

Description

@rwgk , as promised; followup to #4850, this PR ensures custom deleters are propagated correctly and keep their state.

I only saw the note about conventional commits when I filed this PR, so their descriptions aren't that good, the work is mostly split out though in individual commits;

• 7cc1e1f a failing unit test was introduced showing the deleter got lost.
• 5b00921 subsequently fixes that.
• ad7eaa4 adds a unit test for std::unique_ptr<atyp const, custom_deleter>, similar how the other default delete unit tests had that.
• a49a7ce Fixes that new unit test for const atyp.
• eb05f97 Removes the assignment in case the 'use_del_fun' is false, I don't think this is necessary as the code in smart_holder_poc guarantees that the del_fun is used in case there's anything but the default deleter, and for the default deleters no special handling is necessary.

Unit tests make check all pass, I ran clang format 13 on the files I touched, but didn't see a make format or anything along those lines, so it wouldn't surprise me if some linters still fail.

Happy to iterate, but feel free to push commits to this branch as you see fit. Filing as draft first before involving other code owners.

Somewhat related observation

One thing that stood out to me was this line;

pybind11/include/pybind11/detail/smart_holder_poc.h

Line 106 in edfaaed

delete static_cast<T *>(raw_ptr);

I feel that shouldn't be doing a delete, but instead do:

   std::default_delete<T>{}(raw_ptr);

Because the trait system that std::default_delete provides can be considered an extension point of the standard library, and calling delete would bypass any specializations that are provided.

Suggested changelog entry:

Custom deleters in unique_ptrs are stored and extracted correctly from the smart holder.

[rwgk]

@iwanders wrote (in the PR description):

---

One thing that stood out to me was this line;

pybind11/include/pybind11/detail/smart_holder_poc.h

Line 106 in edfaaed

delete static_cast<T *>(raw_ptr);

I feel that shouldn't be doing a delete, but instead do:

   std::default_delete<T>{}(raw_ptr);

---

I agree. I didn't think deeply at all about that line.

Do you want to fix that in a separate small PR on the side?

[iwanders]

Oh, oops, I forgot std::make_unique is c++14 and not 11 😬 let me fix that.

Edit, should be fixed with 25c2e40

[iwanders]

Previous CI failed on c++20, the explicit string constructor is a requirement if we remove the default constructor, otherwise we can't instantiate the unique pointer in the unit test. Quick minimal example with compile error.

And force pushed again to fix clang tidy with f2f87f0, 🤞 should be in the clear now...

[rwgk]

I'll run some internal testing with this PR as is. My suggestions don't change anything substantial. Thanks!

[rwgk]

The latest commits all look good to me, thanks!

I'm not surprised by the test failures, most likely they're very easy to fix, like this:

pybind11/tests/test_class_sh_basic.py

Line 113 in e02fe00

assert re.match("passenger(_MvCtor)*_MvCtor", m.get_mtxt(recycled))

[rwgk]

I'll run some internal testing with this PR as is. My suggestions don't change anything substantial. Thanks!

Unit testing with all sanitizers in the Google toolchain passes. (as expected)

Global testing just changing pybind11 passes. (as expected; there are only a handful of smart_holder uses this way)

None of the remaining PyCLIF-pybind11 global test failures is fixed by this PR. (I had some wishful thinking some may be fixed.) — This was a "rerun" of global testing, after adding in this PR.

Fresh global testing — with this PR included from the start — with PyCLIF-pybind11 exercises the smart_holder functionality in vastly more situations, but it is very expensive. I'll do that next weekend at the latest. (It might need a little bit of trickery to determine for certain if this PR fixed latent bugs.)

[iwanders]

I'm not surprised by the test failures, most likely they're very easy to fix, like this:

Awesome, thanks for that suggestion, I was surprised tbh, I would've expected the amount of move constructors to be equal on between the compilers, but guess there's still some variation in how they implement the language. Pipeline is running with that suggested fix now 🤞

None of the remaining PyCLIF-pybind11 global test failures is fixed by this PR. (I had some wishful thinking some may be fixed.) — This was a "rerun" of global testing, after adding in this PR.

Heh, well, I don't think too many people use a unique pointer with a custom deleter, so not too surprised. Regardless, it's a good improvement and I had fun doing some C++ for a good cause :)

[iwanders]

This looks like a flakey test, at least, we didn't see this error before; https://github.com/pybind/pybind11/actions/runs/6791647051/job/18463525701?pr=4921

         FAILED test_iostream.py::test_flush - AssertionError: assert '### TestFact...(not flushed)' == '(not flushed)'
           + ### TestFactory2 @ 0x227bbf9d0a0 destroyed
             (not flushed)
         = 1 failed, 760 passed, 50 skipped, 26 xfailed, 7 xpassed in 200.07s (0:03:20) =

Going to incorporate the clang tidy noexcept suggestion, but thought I'd point it out in case it doesn't occur again.

[rwgk]

That's a very common flake.

…

On Tue, Nov 7, 2023 at 16:07 Ivor Wanders ***@***.***> wrote: This looks like a flakey test, at least, we didn't see this error before; https://github.com/pybind/pybind11/actions/runs/6791647051/job/18463525701?pr=4921 FAILED test_iostream.py::test_flush - AssertionError: assert '### TestFact...(not flushed)' == '(not flushed)' + ### TestFactory2 @ 0x227bbf9d0a0 destroyed (not flushed) = 1 failed, 760 passed, 50 skipped, 26 xfailed, 7 xpassed in 200.07s (0:03:20) = Going to incorporate the clang tidy noexcept suggestion, but thought I'd point it out in case it doesn't occur again. — Reply to this email directly, view it on GitHub <#4921 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAG56L3YVYRZNOC6XP3YDLEMXAVCNFSM6AAAAAA66OFMGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBQGYYDINZRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rwgk]

Looks like it's working now (121 successful already, no failures).

Could you do me a small favor and click update branch here, or git merge smart_holder, git push interactively? (If I click the button here it'll mark me a collaborator just for that. I think it's better if it's clear that it's all your work.)

[iwanders]

Done, I've also removed the 'draft' label, good to go in from my side, but if you want to wait until your big weekend run of tests that's fine by me of course, no rush.

[rwgk]

Done, I've also removed the 'draft' label, good to go in from my side, but if you want to wait until your big weekend run of tests that's fine by me of course, no rush.

Thanks for the great work!

I'll wait for the GHA to finish, then merge. For the weekend run I'd want to include this PR from the start anyway. Big maybe: if I see a reason to back it out for a rerun (to see if that resolves any failures), I can do that easily with a local revert. In the worst case (very much unexpected) we can revert here, or fix forward.

[iwanders]

Please do let me know if any of the test runs uncover any issues, happy to help tackle them if related to this piece of work.

[rwgk]

For the weekend run I'd want to include this PR from the start anyway.

Done. No surprises.

Big maybe: if I see a reason to back it out for a rerun (to see if that resolves any failures), I can do that easily with a local revert.

I didn't really see a reason, but for completeness, and because it was easy, I tested with the local revert anyway. Very clearly: this PR did not break anything.

In summary (see #4921 (comment)): This PR did not fix anything in the wild, but also did not break anything.

I'm very thankful though that this bug was fixed, so that we no longer have it hanging over our heads as an uncertainty.

[iwanders]

I didn't really see a reason, but for completeness, and because it was easy, I tested with the local revert anyway. Very clearly: this PR did not break anything.

Thanks for letting me know, glad we didn't discover any new bugs.

I'm very thankful though that this bug was fixed, so that we no longer have it hanging over our heads as an uncertainty.

Agreed, this is one of those 'would lead to an annoying bug in production' type of things.