Skip to content

Commit 47dcbdf

Browse files
IcxoluIcxolu
committed
improve PyCapsule constructors
1 parent d07a1ab commit 47dcbdf

5 files changed

Lines changed: 135 additions & 32 deletions

File tree

‎newsfragments/5881.added.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
added `PyCapsule::new_with_value` and `PyCapsule::new_with_value_and_destructor`

‎src/instance.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2753,10 +2753,10 @@ a = A()
27532753
method: impl FnOnce(*mut ffi::PyObject) -> Bound<'py, PyAny>,
27542754
) {
27552755
let mut dropped = false;
2756-
let capsule = PyCapsule::new_with_destructor(
2756+
let capsule = PyCapsule::new_with_value_and_destructor(
27572757
py,
27582758
(&mut dropped) as *mut _ as usize,
2759-
None,
2759+
c"bound_from_borrowed_ptr_constructors",
27602760
|ptr, _| unsafe { std::ptr::write(ptr as *mut bool, true) },
27612761
)
27622762
.unwrap();
@@ -2795,10 +2795,10 @@ a = A()
27952795
method: impl FnOnce(&*mut ffi::PyObject) -> Borrowed<'_, 'py, PyAny>,
27962796
) {
27972797
let mut dropped = false;
2798-
let capsule = PyCapsule::new_with_destructor(
2798+
let capsule = PyCapsule::new_with_value_and_destructor(
27992799
py,
28002800
(&mut dropped) as *mut _ as usize,
2801-
None,
2801+
c"borrowed_ptr_constructors",
28022802
|ptr, _| unsafe { std::ptr::write(ptr as *mut bool, true) },
28032803
)
28042804
.unwrap();

‎src/types/capsule.rs‎

Lines changed: 122 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ use std::ptr::{self, NonNull};
3535
///
3636
/// let r = Python::attach(|py| -> PyResult<()> {
3737
/// let foo = Foo { val: 123 };
38-
/// let capsule = PyCapsule::new(py, foo, Some(c"builtins.capsule".to_owned()))?;
38+
/// let capsule = PyCapsule::new_with_value(py, foo, c"builtins.capsule")?;
3939
///
4040
/// let module = PyModule::import(py, "builtins")?;
4141
/// module.add("capsule", capsule)?;
@@ -69,27 +69,77 @@ impl PyCapsule {
6969
/// // this can be c"foo" on Rust 1.77+
7070
/// const NAME: &CStr = c"foo";
7171
///
72+
/// # fn main() -> PyResult<()> {
7273
/// Python::attach(|py| {
73-
/// let capsule = PyCapsule::new(py, 123_u32, Some(NAME.to_owned())).unwrap();
74-
/// let val: NonNull<u32> = capsule.pointer_checked(Some(NAME)).unwrap().cast();
74+
/// let capsule = PyCapsule::new_with_value(py, 123_u32, NAME)?;
75+
/// let val: NonNull<u32> = capsule.pointer_checked(Some(NAME))?.cast();
7576
/// assert_eq!(unsafe { *val.as_ref() }, 123);
76-
/// });
77+
/// # Ok(())
78+
/// })
79+
/// # }
7780
/// ```
7881
///
7982
/// However, attempting to construct a `PyCapsule` with a zero-sized type will not compile:
8083
///
8184
/// ```compile_fail
8285
/// use pyo3::{prelude::*, types::PyCapsule};
8386
///
87+
/// # fn main() -> PyResult<()> {
8488
/// Python::attach(|py| {
85-
/// let capsule = PyCapsule::new(py, (), None).unwrap(); // Oops! `()` is zero sized!
86-
/// });
89+
/// let capsule = PyCapsule::new_with_value(py, (), c"foo")?; // Oops! `()` is zero sized!
90+
/// # Ok(())
91+
/// })
92+
/// # }
8793
/// ```
94+
pub fn new_with_value<'py, T>(
95+
py: Python<'py>,
96+
value: T,
97+
name: &'static CStr,
98+
) -> PyResult<Bound<'py, Self>>
99+
where
100+
T: 'static + Send + AssertNotZeroSized,
101+
{
102+
AssertNotZeroSized::assert_not_zero_sized(&value);
103+
104+
// NOTE: Not implemented in terms of `new_with_value_and_destructor` as this allows for
105+
// storing the `Box<T>` directly without allocating additionally for the destructor
106+
let val = Box::into_raw(Box::new(value));
107+
108+
unsafe extern "C" fn destructor<T>(capsule: *mut ffi::PyObject) {
109+
// SAFETY: `capsule` is known to be a borrowed reference to the capsule being destroyed
110+
let name = unsafe { ffi::PyCapsule_GetName(capsule) };
111+
112+
// SAFETY:
113+
// - `capsule` is known to be a borrowed reference to the capsule being destroyed
114+
// - `name` is known to be the capsule's name
115+
let ptr = unsafe { ffi::PyCapsule_GetPointer(capsule, name) };
116+
117+
// SAFETY: `capsule` was knowingly constructed from a `Box<T>` and is now being
118+
// destroyed, so we reconstruct the Box and drop it.
119+
let _ = unsafe { Box::<T>::from_raw(ptr.cast()) };
120+
}
121+
122+
// SAFETY:
123+
// - `val` is a non-null pointer to valid capsule data
124+
// - `name` is a static valid C string
125+
// - `destructor` will delete this data when called
126+
// - thread is attached to the Python interpreter
127+
// - `PyCapsule_New` returns a new reference or null on error
128+
unsafe {
129+
ffi::PyCapsule_New(val.cast(), name.as_ptr(), Some(destructor::<T>))
130+
.assume_owned_or_err(py)
131+
.cast_into_unchecked()
132+
}
133+
}
134+
135+
/// See [`PyCapsule::new_with_value`]
136+
#[deprecated(since = "0.29.0", note = "use `PyCapsule::new_with_value` instead")]
88137
pub fn new<T: 'static + Send + AssertNotZeroSized>(
89138
py: Python<'_>,
90139
value: T,
91140
name: Option<CString>,
92141
) -> PyResult<Bound<'_, Self>> {
142+
#[allow(deprecated)]
93143
Self::new_with_destructor(py, value, name, |_, _| {})
94144
}
95145

@@ -100,6 +150,45 @@ impl PyCapsule {
100150
///
101151
/// The `destructor` must be `Send`, because there is no guarantee which thread it will eventually
102152
/// be called from.
153+
pub fn new_with_value_and_destructor<'py, T, F>(
154+
py: Python<'py>,
155+
value: T,
156+
name: &'static CStr,
157+
destructor: F,
158+
) -> PyResult<Bound<'py, Self>>
159+
where
160+
T: 'static + Send + AssertNotZeroSized,
161+
F: FnOnce(T, *mut c_void) + Send,
162+
{
163+
AssertNotZeroSized::assert_not_zero_sized(&value);
164+
165+
// Sanity check for capsule layout
166+
debug_assert_eq!(offset_of!(CapsuleContents::<T, F>, value), 0);
167+
168+
let val = Box::into_raw(Box::new(CapsuleContents {
169+
value,
170+
destructor,
171+
name: None,
172+
}));
173+
174+
// SAFETY:
175+
// - `val` is a non-null pointer to valid capsule data
176+
// - `name` is a static valid C string
177+
// - `destructor` will delete this data when called
178+
// - thread is attached to the Python interpreter
179+
// - `PyCapsule_New` returns a new reference or null on error
180+
unsafe {
181+
ffi::PyCapsule_New(val.cast(), name.as_ptr(), Some(capsule_destructor::<T, F>))
182+
.assume_owned_or_err(py)
183+
.cast_into_unchecked()
184+
}
185+
}
186+
187+
/// See [`PyCapsule::new_with_value_and_destructor`]
188+
#[deprecated(
189+
since = "0.29.0",
190+
note = "use `PyCapsule::new_with_value_and_destructor` instead"
191+
)]
103192
pub fn new_with_destructor<
104193
T: 'static + Send + AssertNotZeroSized,
105194
F: FnOnce(T, *mut c_void) + Send,
@@ -307,6 +396,7 @@ pub trait PyCapsuleMethods<'py>: crate::sealed::Sealed {
307396
/// use std::sync::mpsc::{channel, Sender};
308397
/// use pyo3::{prelude::*, types::PyCapsule};
309398
///
399+
/// # fn main() -> PyResult<()> {
310400
/// let (tx, rx) = channel::<String>();
311401
///
312402
/// fn destructor(val: u32, context: *mut c_void) {
@@ -315,15 +405,21 @@ pub trait PyCapsuleMethods<'py>: crate::sealed::Sealed {
315405
/// }
316406
///
317407
/// Python::attach(|py| {
318-
/// let capsule =
319-
/// PyCapsule::new_with_destructor(py, 123, None, destructor as fn(u32, *mut c_void))
320-
/// .unwrap();
408+
/// let capsule = PyCapsule::new_with_value_and_destructor(
409+
/// py,
410+
/// 123,
411+
/// c"foo",
412+
/// destructor as fn(u32, *mut c_void)
413+
/// )?;
321414
/// let context = Box::new(tx); // `Sender<String>` is our context, box it up and ship it!
322-
/// capsule.set_context(Box::into_raw(context).cast()).unwrap();
415+
/// capsule.set_context(Box::into_raw(context).cast())?;
323416
/// // This scope will end, causing our destructor to be called...
324-
/// });
417+
/// # Ok::<_, PyErr>(())
418+
/// })?;
325419
///
326420
/// assert_eq!(rx.recv(), Ok("Destructor called!".to_string()));
421+
/// # Ok(())
422+
/// }
327423
/// ```
328424
fn set_context(&self, context: *mut c_void) -> PyResult<()>;
329425

@@ -524,6 +620,7 @@ struct CapsuleContents<T: 'static + Send, D: FnOnce(T, *mut c_void) + Send> {
524620
/// Destructor to be used by the capsule
525621
destructor: D,
526622
/// Name used when creating the capsule
623+
// TODO: remove this field when `PyCapsule::new` and `PyCapsule::new_with_destructor` are removed
527624
name: Option<CString>,
528625
}
529626

@@ -634,7 +731,7 @@ mod tests {
634731
Python::attach(|py| {
635732
let foo = Foo { val: 123 };
636733

637-
let cap = PyCapsule::new(py, foo, Some(NAME.to_owned())).unwrap();
734+
let cap = PyCapsule::new_with_value(py, foo, NAME).unwrap();
638735
assert!(cap.is_valid_checked(Some(NAME)));
639736

640737
let foo_capi = cap.pointer_checked(Some(NAME)).unwrap().cast::<Foo>();
@@ -659,7 +756,7 @@ mod tests {
659756
}
660757

661758
let cap: Py<PyCapsule> = Python::attach(|py| {
662-
let cap = PyCapsule::new(py, foo as fn(u32) -> u32, Some(NAME.to_owned())).unwrap();
759+
let cap = PyCapsule::new_with_value(py, foo as fn(u32) -> u32, NAME).unwrap();
663760
cap.into()
664761
});
665762

@@ -677,7 +774,7 @@ mod tests {
677774
#[test]
678775
fn test_pycapsule_context() {
679776
Python::attach(|py| {
680-
let cap = PyCapsule::new(py, 0, Some(NAME.to_owned())).unwrap();
777+
let cap = PyCapsule::new_with_value(py, 0, NAME).unwrap();
681778

682779
let c = cap.context().unwrap();
683780
assert!(c.is_null());
@@ -703,7 +800,7 @@ mod tests {
703800
let foo = Foo { val: 123 };
704801
let name = c"builtins.capsule";
705802

706-
let capsule = PyCapsule::new(py, foo, Some(name.to_owned())).unwrap();
803+
let capsule = PyCapsule::new_with_value(py, foo, name).unwrap();
707804

708805
let module = PyModule::import(py, "builtins").unwrap();
709806
module.add("capsule", capsule).unwrap();
@@ -724,7 +821,7 @@ mod tests {
724821
fn test_vec_storage() {
725822
let cap: Py<PyCapsule> = Python::attach(|py| {
726823
let stuff: Vec<u8> = vec![1, 2, 3, 4];
727-
let cap = PyCapsule::new(py, stuff, Some(NAME.to_owned())).unwrap();
824+
let cap = PyCapsule::new_with_value(py, stuff, NAME).unwrap();
728825
cap.into()
729826
});
730827

@@ -744,7 +841,7 @@ mod tests {
744841
let context: Vec<u8> = vec![1, 2, 3, 4];
745842

746843
let cap: Py<PyCapsule> = Python::attach(|py| {
747-
let cap = PyCapsule::new(py, 0, Some(NAME.to_owned())).unwrap();
844+
let cap = PyCapsule::new_with_value(py, 0, NAME).unwrap();
748845
cap.set_context(Box::into_raw(Box::new(&context)).cast())
749846
.unwrap();
750847

@@ -771,8 +868,7 @@ mod tests {
771868
}
772869

773870
Python::attach(move |py| {
774-
let cap =
775-
PyCapsule::new_with_destructor(py, 0, Some(NAME.to_owned()), destructor).unwrap();
871+
let cap = PyCapsule::new_with_value_and_destructor(py, 0, NAME, destructor).unwrap();
776872
cap.set_context(Box::into_raw(Box::new(tx)).cast()).unwrap();
777873
});
778874

@@ -781,6 +877,7 @@ mod tests {
781877
}
782878

783879
#[test]
880+
#[allow(deprecated)]
784881
fn test_pycapsule_no_name() {
785882
Python::attach(|py| {
786883
let cap = PyCapsule::new(py, 0usize, None).unwrap();
@@ -869,7 +966,7 @@ mod tests {
869966
#[test]
870967
fn test_pycapsule_pointer_checked_wrong_name() {
871968
Python::attach(|py| {
872-
let cap = PyCapsule::new(py, 123u32, Some(c"correct.name".to_owned())).unwrap();
969+
let cap = PyCapsule::new_with_value(py, 123u32, c"correct.name").unwrap();
873970

874971
// Requesting with wrong name should fail
875972
let result = cap.pointer_checked(Some(c"wrong.name"));
@@ -882,6 +979,7 @@ mod tests {
882979
}
883980

884981
#[test]
982+
#[allow(deprecated)]
885983
fn test_pycapsule_pointer_checked_none_vs_some() {
886984
Python::attach(|py| {
887985
// Capsule with no name
@@ -899,7 +997,7 @@ mod tests {
899997
#[test]
900998
fn test_pycapsule_is_valid_checked_wrong_name() {
901999
Python::attach(|py| {
902-
let cap = PyCapsule::new(py, 123u32, Some(c"correct.name".to_owned())).unwrap();
1000+
let cap = PyCapsule::new_with_value(py, 123u32, c"correct.name").unwrap();
9031001

9041002
// Should be valid with correct name
9051003
assert!(cap.is_valid_checked(Some(c"correct.name")));
@@ -913,6 +1011,7 @@ mod tests {
9131011
}
9141012

9151013
#[test]
1014+
#[allow(deprecated)]
9161015
fn test_pycapsule_is_valid_checked_no_name() {
9171016
Python::attach(|py| {
9181017
let cap = PyCapsule::new(py, 123u32, None).unwrap();
@@ -928,7 +1027,7 @@ mod tests {
9281027
#[test]
9291028
fn test_pycapsule_context_on_invalid_capsule() {
9301029
Python::attach(|py| {
931-
let cap = PyCapsule::new(py, 123u32, Some(NAME.to_owned())).unwrap();
1030+
let cap = PyCapsule::new_with_value(py, 123u32, NAME).unwrap();
9321031

9331032
// Invalidate the capsule
9341033
// SAFETY: intentionally breaking the capsule for testing
@@ -957,7 +1056,7 @@ mod tests {
9571056
fn test_pycapsule_import_wrong_attribute() {
9581057
Python::attach(|py| {
9591058
// Create a capsule and register it
960-
let cap = PyCapsule::new(py, 123u32, Some(c"builtins.test_cap".to_owned())).unwrap();
1059+
let cap = PyCapsule::new_with_value(py, 123u32, c"builtins.test_cap").unwrap();
9611060
let module = crate::prelude::PyModule::import(py, "builtins").unwrap();
9621061
module.add("test_cap", cap).unwrap();
9631062

‎src/types/function.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,13 +91,13 @@ impl PyCFunction {
9191
pymethods::PyMethodDef::cfunction_with_keywords(name, run_closure::<F, R>, doc);
9292
let def = method_def.into_raw();
9393

94-
let capsule = PyCapsule::new(
94+
let capsule = PyCapsule::new_with_value(
9595
py,
9696
ClosureDestructor::<F> {
9797
closure,
9898
def: UnsafeCell::new(def),
9999
},
100-
Some(CLOSURE_CAPSULE_NAME.to_owned()),
100+
CLOSURE_CAPSULE_NAME,
101101
)?;
102102

103103
let data: NonNull<ClosureDestructor<F>> =

‎tests/test_inheritance.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -253,9 +253,12 @@ mod inheriting_native_type {
253253
Python::attach(|py| {
254254
let dropped = Arc::new(AtomicBool::new(false));
255255
let destructor_drop = Arc::clone(&dropped);
256-
let item = PyCapsule::new_with_destructor(py, 0, None, move |_, _| {
257-
destructor_drop.store(true, Ordering::Relaxed)
258-
})
256+
let item = PyCapsule::new_with_value_and_destructor(
257+
py,
258+
0,
259+
c"inherit_dict_drop",
260+
move |_, _| destructor_drop.store(true, Ordering::Relaxed),
261+
)
259262
.unwrap();
260263

261264
let dict_sub = pyo3::Py::new(py, DictWithName::new()).unwrap();

0 commit comments

Comments
 (0)