Commit Graph

1 Commits

Author SHA1 Message Date
Noah Oblath
74b5242713 feat: add py::potentially_slicing_weak_ptr(handle) function (#5624)
* Added weak_ptr test (currently failing)

* Cleanup

* [skip ci] Simplify test case.

* Add test_class_sp_trampoline_weak_ptr.cpp,py (using std::shared_ptr as holder). Tweak test_class_sh_trampoline_weak_ptr.py to pass, with `# THIS NEEDS FIXING` comment.

* Resolve clang-tidy errors

```
/__w/pybind11/pybind11/tests/test_class_sh_trampoline_weak_ptr.cpp:23:43: error: the parameter 'sp' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
   23 |     void set_wp(std::shared_ptr<VirtBase> sp) { wp = sp; }
      |                                           ^
      |                 const                    &
4443 warnings generated.
```

```
/__w/pybind11/pybind11/tests/test_class_sp_trampoline_weak_ptr.cpp:23:43: error: the parameter 'sp' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
   23 |     void set_wp(std::shared_ptr<VirtBase> sp) { wp = sp; }
      |                                           ^
      |                 const                    &
4430 warnings generated.
```

* PYPY, GRAALPY: skip last part of test_weak_ptr_base

* Rename test_weak_ptr_base → test_weak_ptr_owner

* Add SpOwner, test_with_sp_owner, test_with_sp_and_wp_owners

* Modify py::trampoline_self_life_support semantics: if trampoline class does not inherit from this class, preserve established Inheritance Slicing behavior.

rwgk reached this point with the help of ChatGPT:

* https://chatgpt.com/share/68056498-7d94-8008-8ff0-232e2aba451c

The only production code change in this commit is:

```
diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index d4f9a41e..f3d45301 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -776,6 +776,14 @@ struct load_helper : value_and_holder_helper {
                 if (released_ptr) {
                     return std::shared_ptr<T>(released_ptr, type_raw_ptr);
                 }
+                auto *self_life_support
+                    = dynamic_raw_ptr_cast_if_possible<trampoline_self_life_support>(type_raw_ptr);
+                if (self_life_support == nullptr) {
+                    std::shared_ptr<void> void_shd_ptr = hld.template as_shared_ptr<void>();
+                    std::shared_ptr<T> to_be_released(void_shd_ptr, type_raw_ptr);
+                    vptr_gd_ptr->released_ptr = to_be_released;
+                    return to_be_released;
+                }
                 std::shared_ptr<T> to_be_released(
                     type_raw_ptr, shared_ptr_trampoline_self_life_support(loaded_v_h.inst));
                 vptr_gd_ptr->released_ptr = to_be_released;
```

* Remove debug printf in include/pybind11/pybind11.h

* Resolve MSVC error

```
    11>D:\a\pybind11\pybind11\tests\test_class_sp_trampoline_weak_ptr.cpp(44,50): error C2220: the following warning is treated as an error [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
    11>D:\a\pybind11\pybind11\tests\test_class_sp_trampoline_weak_ptr.cpp(44,50): warning C4458: declaration of 'sp' hides class member [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
             D:\a\pybind11\pybind11\tests\test_class_sp_trampoline_weak_ptr.cpp(54,31):
             see declaration of 'pybind11_tests::class_sp_trampoline_weak_ptr::SpOwner::sp'
```

* [skip ci] Undo the production code change under 4638e017b6

Also undo the corresponding test change in test_class_sh_trampoline_weak_ptr.py

But keep all extra debugging code for now.

* [skip ci] Introduce lambda in `WpOwner::set_wp` bindings, but simply cast to `std::shared_ptr<VirtBase>` for now.

* Add `py::potentially_slicing_shared_ptr()`

* Add `type_id<T>()` to `py::potentially_slicing_shared_ptr()` error message and add test.

* test_potentially_slicing_shared_ptr.cpp,py (for smart_holder only)

* Generalize test_potentially_slicing_shared_ptr.cpp,py for testing with smart_holder and std::shared_ptr as holder.

* Add back test_potentially_slicing_shared_ptr_not_convertible_error(), it got lost accidentally in commit 56d23dc478

* Add simple trampoline state assertions.

* Resolve clang-tidy errors.

```
/__w/pybind11/pybind11/tests/test_potentially_slicing_shared_ptr.cpp:30:9: error: 'magic_token' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
   29 |     trampoline_is_alive_simple(const trampoline_is_alive_simple &other) {
      |                                                                         : magic_token(other.magic_token)
   30 |         magic_token = other.magic_token;
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/pybind11/pybind11/tests/test_potentially_slicing_shared_ptr.cpp:33:9: error: 'magic_token' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
   32 |     trampoline_is_alive_simple(trampoline_is_alive_simple &&other) noexcept {
      |                                                                             : magic_token(other.magic_token)
   33 |         magic_token = other.magic_token;
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

* Add a (long) C++ comment for py::potentially_slicing_shared_ptr<>()

* [skip ci] Add new "Avoiding Inheritance Slicing and ``std::weak_ptr`` surprises" section in advanced/classes.rst

* [skip ci] Add introductory comment to test_potentially_slicing_shared_ptr.py

* Minimal (!) changes to have py::potentially_slicing_weak_ptr<T>(handle) as the public API. For CI testing, before changing the names around more widely, and the documentation.

* Rename test_potentially_slicing_shared_ptr.cpp,py → test_potentially_slicing_weak_ptr.cpp,py

* Update docs/advanced/classes.rst and C++ comments → potentially_slicing_weak_ptr

* Write "shared_ptr" instead of just "pointer" in a couple places in docs/advanced/classes.rst

* Add comment for force_potentially_slicing_shared_ptr in type_caster_base.h, to make a direct connection py::potentially_slicing_weak_ptr

---------

Co-authored-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
2025-05-13 10:07:33 -04:00