Skip to content

Commit c96ee64

Browse files
committed
Importing pybind11 smart_holder PR #2886 from GitHub and adjusting PyCLIF smart_ptrs_test accordingly.
pybind/pybind11#2886 High-level summary of the two critical behavior changes: * Context: handling of smart pointers for instances of Python-derived classes with pybind11-wrapped bases. * Keywords: trampoline, also known as alias class, virtual functions with Python overrides. * Minimal example: `class PyDrvd(m.Abase)` in test_class_sh_with_alias.py * When a `shared_ptr<base>` is passed from Python to C++ (as a C++ function argument), the `shared_ptr` is built specifically for the function call, using a custom deleter tying the lifetime of the Python instance to the lifetime of the `shared_ptr`. This solves what's sometimes referred to as "inheritance slicing" issue in connection with the pybind11 trampoline feature. Note that there are ~10 open pybind11 issues related to this problem, enumerated in the description of [PR #2839](pybind/pybind11#2839). * Passing a `unique_ptr<base>` is disabled. A `ValueError` is raised, with message: "Ownership of instance with virtual overrides in Python cannot be transferred to C++." For full details see the description of PR #2886. - 97a7fb722a9842cae4d91be82897dd863f6b607d Porting/adapting Dustin's PR #2839 to smart_holder branch... by Ralf W. Grosse-Kunstleve <[email protected]> PiperOrigin-RevId: 360966763
1 parent 6b2309f commit c96ee64

File tree

2 files changed

+25
-10
lines changed

2 files changed

+25
-10
lines changed

clif/pybind11/staging/smart_ptrs_test.py

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,14 @@ def testSmartPtrs(self):
6969
smart_ptrs.Func(a)
7070

7171
add = Add(120, 3)
72-
# TODO: Temporarily disable the test because pybind11 throws
73-
# `RuntimeError: Tried to call pure virtual function` when calling
74-
# smart_ptrs.PerformUP.
75-
# self.assertEqual(smart_ptrs.PerformUP(add), 123)
76-
# # Previous call to Perform invalidated |add|
77-
# with self.assertRaises(ValueError):
78-
# smart_ptrs.PerformUP(add)
72+
# pybind11 (with smart_holder) deliberatly does not support this
73+
# inherently unsafe transfer of ownership.
74+
with self.assertRaises(ValueError) as ctx:
75+
smart_ptrs.PerformUP(add)
76+
self.assertEqual(
77+
str(ctx.exception),
78+
'Ownership of instance with virtual overrides in Python cannot be'
79+
' transferred to C++.')
7980

8081
add = Add(1230, 4)
8182
self.assertEqual(smart_ptrs.PerformSP(add), 1234)

clif/testing/python/smart_ptrs_test.py

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,15 +99,29 @@ def testSmartPtrs(self, wrapper_lib):
9999

100100
add = AddParameterized(wrapper_lib)(120, 3)
101101
if wrapper_lib is smart_ptrs:
102-
# TODO: Temporarily disabling test for pybind11, which
103-
# throws `RuntimeError: Tried to call pure virtual function` when calling
104-
# smart_ptrs.PerformUP.
102+
# This test (and the test for `PerformSP` below) can work safely only
103+
# because PyCLIF leaks the reference count for `add` (use
104+
# testInfiniteLoopAddVirtualOverride below to reproduce the leak), which
105+
# keeps the Python instance alive beyond the time of transferring
106+
# ownership from Python to C++.
105107
self.assertEqual(wrapper_lib.PerformUP(add), 123)
106108
# Previous call to Perform invalidated |add|
107109
with self.assertRaises(ValueError):
108110
wrapper_lib.PerformUP(add)
111+
else:
112+
# pybind11 (with smart_holder) deliberatly does not support this
113+
# inherently unsafe transfer of ownership. (Knowingly leaking the
114+
# reference count is not an option.)
115+
with self.assertRaises(ValueError) as ctx:
116+
wrapper_lib.PerformUP(add)
117+
self.assertEqual(
118+
str(ctx.exception),
119+
'Ownership of instance with virtual overrides in Python cannot be'
120+
' transferred to C++.')
109121

110122
add = AddParameterized(wrapper_lib)(1230, 4)
123+
# This works with the C-API code generator only because the reference count
124+
# for `add` is leaked. It works cleanly in pybind11 after PR #2886.
111125
self.assertEqual(wrapper_lib.PerformSP(add), 1234)
112126
# Calls to PerformSP should not invalidate |add|.
113127
self.assertEqual(wrapper_lib.PerformSP(add), 1234)

0 commit comments

Comments
 (0)