diff rust/pyo3-sharedref/src/lib.rs @ 52640:78b2894cd58c

rust-pyo3-sharedref: demonstrate SharedByPyObject borrow unsafety We take the existing doc-comment from rust-cpython and make it compile (validated by `cargo test`). With the added explanations, the ordinary comment was no longer useful, we could therefore remove it. The new explanation stresses that "not leaking the internal faked reference" is definitely not enough, because the problem is about *all references* that can be derived from it. We ended up duplicating the explanation, because that is a way to ensure that people do not miss it. Also, it was a bit misleading that the previous example was for `try_borrow_mut()`, so we made a similar, simpler one for `try_borrow()`.
author Georges Racinet <georges.racinet@cloudcrane.io>
date Mon, 16 Dec 2024 13:08:55 +0100
parents 76a0bdb0e4ca
children 189491cea922
line wrap: on
line diff
--- a/rust/pyo3-sharedref/src/lib.rs	Mon Dec 16 12:13:46 2024 +0100
+++ b/rust/pyo3-sharedref/src/lib.rs	Mon Dec 16 13:08:55 2024 +0100
@@ -469,15 +469,8 @@
 /// may be exposed to your Rust code. You must be careful to not make a bare
 /// reference outlive the actual object lifetime.
 ///
-/// ```ignore
-/// let mut outer = empty;
-/// let mut shared_iter = shared.map(py, |o| o.chars());
-/// {
-///     let mut iter = unsafe { shared_iter.try_borrow_mut(py) };
-///     let inner = iter.next();  // Good, in borrow scope
-///     outer = inner;            // Bad, &'static T may outlive
-/// }
-/// ```
+/// See [`Self::try_borrow_mut()`] for an example of the kind of trouble that
+/// can arise.
 pub struct SharedByPyObject<T: ?Sized> {
     owner: PyObject,
     state: &'static PySharedState,
@@ -498,11 +491,6 @@
     // because the underlying value is supposed to be mutated in Python
     // world, and the Rust library designer can't prevent it.
 
-    // try_borrow() and try_borrow_mut() are unsafe because self.data may
-    // have a function returning the inner &'static reference.
-    // If T is &'static U, its lifetime can be easily coerced to &'a U, but
-    // how could we do that for Whatever<'static> in general?
-
     /// Immutably borrows the wrapped value.
     ///
     /// Borrowing fails if the underlying reference has been invalidated.
@@ -511,6 +499,49 @@
     ///
     /// The lifetime of the innermost object is artificial. Do not obtain and
     /// copy it out of the borrow scope.
+    ///
+    /// The lifetime of the innermost object is artificial. Do not obtain and
+    /// copy it out of the borrow scope. More generally, the returned `&T`
+    /// may have a method returning an inner reference, which would typically
+    /// be `'static` and not safe without the `owner` Python object, so the
+    /// problem might be less obvious than in the example below.
+    ///
+    /// The following example does compile and illustrates the problem.
+    /// In this case, the data is a `Vec<String>` and the leaked reference
+    /// `&'static str`, which points to some element of the vector. This
+    /// illustrates that the leaks are not necessarily to the whole of the
+    /// shared data.
+    ///
+    /// ```no_run
+    /// # use pyo3::prelude::*;
+    /// # use pyo3_sharedref::PyShareable;
+    /// #[pyclass]
+    /// struct Owner {
+    ///     value: PyShareable<Vec<String>>
+    /// }
+    ///
+    /// #[pymethods]
+    /// impl Owner {
+    ///     #[new]
+    ///     fn new(s: &str) -> Self {
+    ///         let split: Vec<_> = s.split(' ').map(String::from).collect();
+    ///         Self { value: split.into() }
+    ///     }
+    /// }
+    ///
+    /// const EMPTY: &'static str = "";
+    ///
+    /// let mut outer = EMPTY;
+    /// Python::with_gil(|py| {
+    ///     let owner = Bound::new(py, Owner::new("hello")).unwrap();
+    ///     let shareable = &owner.borrow().value;
+    ///     let shared = unsafe { shareable.share(&owner) };
+    ///     {
+    ///         let inner = unsafe { shared.try_borrow(py) }.unwrap();
+    ///         outer = &inner[0]; // Bad, &'static str does outlive the scope
+    ///     }
+    /// });
+    /// ```
     pub unsafe fn try_borrow<'a>(
         &'a self,
         py: Python<'a>,
@@ -526,13 +557,61 @@
     ///
     /// Borrowing fails if the underlying reference has been invalidated.
     ///
-    /// Typically `T` is an iterator. If `T` is an immutable reference,
-    /// `get_mut()` is useless since the inner value can't be mutated.
+    /// Typically `T` would be an iterator obtained by the [`Self::map`]
+    /// method.
     ///
     /// # Safety
     ///
     /// The lifetime of the innermost object is artificial. Do not obtain and
-    /// copy it out of the borrow scope.
+    /// copy it out of the borrow scope. More generally, the returned `&T`
+    /// may have a method returning an inner reference, which would typically
+    /// be `'static` and not safe without the `owner` Python object, so the
+    /// problem might be less obvious than in the example below.
+    ///
+    /// The following example does compile and illustrates the problem.
+    /// It is very close to the example given in [`Self::try_borrow`] because
+    /// the problem does not arise from the mutability of the reference
+    /// returned by this function.
+    ///
+    /// In this case, the data is a `Vec<String>` and the leaked reference
+    /// `&'static str`, which points to some element of the vector. This
+    /// illustrates that the leaks are not necessarily to the whole of the
+    /// shared data.
+    ///
+    /// ```no_run
+    /// # use pyo3::prelude::*;
+    /// # use pyo3_sharedref::PyShareable;
+    /// #[pyclass]
+    /// struct Owner {
+    ///     value: PyShareable<Vec<String>>
+    /// }
+    ///
+    /// #[pymethods]
+    /// impl Owner {
+    ///     #[new]
+    ///     fn new(s: &str) -> Self {
+    ///         let split: Vec<_> = s.split(' ').map(String::from).collect();
+    ///         Self { value: split.into() }
+    ///     }
+    /// }
+    ///
+    /// const EMPTY: &'static str = "";
+    ///
+    /// let mut outer = EMPTY;
+    /// Python::with_gil(|py| {
+    ///     let owner = Bound::new(py, Owner::new("hello")).unwrap();
+    ///     let shareable = &owner.borrow().value;
+    ///     let shared = unsafe { shareable.share(&owner) };
+    ///     let mut shared_iter = unsafe { shared.map(py, |o| o.iter()) };
+    ///     {
+    ///         let mut iter = unsafe {
+    ///             shared_iter.try_borrow_mut(py)
+    ///         }.unwrap();
+    ///         let inner = iter.next().unwrap();  // Good, in borrow scope
+    ///         outer = inner; // Bad, &'static str does outlive the scope
+    ///     }
+    /// });
+    /// ```
     pub unsafe fn try_borrow_mut<'a>(
         &'a mut self,
         py: Python<'a>,