changeset 51255:24d3298189d7

rust-index: document safety invariants being upheld for every `unsafe` block

We've added a lot of `unsafe` code that shares Rust structs with Python.
While this is unfortunate, it is also unavoidable, so let's at least
systematically explain why each call to `unsafe` is sound.

If any of the unsafe code ends up being wrong (because everyone screws up
at some point), this change at least continues the unspoken rule of always
explaining the need for `unsafe`, so we at least get a chance to think.

--- a/rust/hg-cpython/src/ancestors.rs	Sun Oct 29 12:18:03 2023 +0100
+++ b/rust/hg-cpython/src/ancestors.rs	Thu Nov 23 03:41:58 2023 +0100
@@ -60,9 +60,9 @@
 // of the `map` method with a signature such as:
 //
 // ```
-//   unafe fn map_or_err(py: Python,
-//                       f: impl FnOnce(T) -> Result(U, E),
-//                       convert_err: impl FnOnce(Python, E) -> PyErr)
+//   unsafe fn map_or_err(py: Python,
+//                        f: impl FnOnce(T) -> Result(U, E),
+//                        convert_err: impl FnOnce(Python, E) -> PyErr)
 // ```
 //
 // This would spare users of the `cpython` crate the additional `unsafe` deref
@@ -74,9 +74,11 @@
     convert_err: impl FnOnce(Python, E) -> PyErr,
 ) -> PyResult<UnsafePyLeaked<T>> {
     // Result.inspect_err is unstable in Rust 1.61
+    // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
     if let Err(e) = *unsafe { leaked.try_borrow(py)? } {
         return Err(convert_err(py, e));
     }
+    // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
     Ok(unsafe {
         leaked.map(py, |res| {
             res.expect("Error case should have already be treated")
@@ -89,6 +91,7 @@
 
     def __next__(&self) -> PyResult<Option<PyRevision>> {
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let mut inner = unsafe { leaked.try_borrow_mut(py)? };
         match inner.next() {
             Some(Err(e)) => Err(GraphError::pynew_from_vcsgraph(py, e)),
@@ -99,6 +102,7 @@
 
     def __contains__(&self, rev: PyRevision) -> PyResult<bool> {
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let mut inner = unsafe { leaked.try_borrow_mut(py)? };
         inner.contains(rev.0)
             .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))
@@ -136,10 +140,12 @@
         inclusive: bool,
     ) -> PyResult<AncestorsIterator> {
         let index = py_rust_index_to_graph(py, index)?;
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let initvec: Vec<_> = {
             let borrowed_idx = unsafe { index.try_borrow(py)? };
             rev_pyiter_collect(py, &initrevs, &*borrowed_idx)?
         };
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let res_ait = unsafe {
             index.map(py, |idx| {
                 VCGAncestorsIterator::new(
@@ -167,6 +173,7 @@
 
     def __contains__(&self, rev: PyRevision) -> PyResult<bool> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner: &RefCell<VCGLazyAncestors<PySharedIndex>> =
             &*unsafe { leaked.try_borrow(py)? };
         let inner_mut: &mut VCGLazyAncestors<PySharedIndex> =
@@ -185,6 +192,7 @@
 
     def __bool__(&self) -> PyResult<bool> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner = unsafe { leaked.try_borrow(py)? };
         let empty = inner.borrow().is_empty();
         Ok(!empty)
@@ -200,10 +208,12 @@
         let cloned_index = index.clone_ref(py);
         let index = py_rust_index_to_graph(py, index)?;
         let initvec: Vec<_> = {
+            // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
             let borrowed_idx =  unsafe {index.try_borrow(py)?};
             rev_pyiter_collect(py, &initrevs, &*borrowed_idx)?
         };
 
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let res_lazy =
             unsafe { index.map(py, |idx| VCGLazyAncestors::new(
                 idx,
@@ -213,6 +223,7 @@
             ))};
         let lazy = pyleaked_or_map_err(py, res_lazy,
                                        GraphError::pynew_from_vcsgraph)?;
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let lazy_cell = unsafe { lazy.map(py, RefCell::new)};
         let res = Self::create_instance(
             py, RefCell::new(lazy_cell),
@@ -236,11 +247,13 @@
     -> PyResult<MissingAncestors> {
         let cloned_index = index.clone_ref(py);
         let inner_index = py_rust_index_to_graph(py, index)?;
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let bases_vec: Vec<_> = {
             let borrowed_idx = unsafe { inner_index.try_borrow(py)? };
             rev_pyiter_collect(py, &bases, &*borrowed_idx)?
         };
 
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner = unsafe {
             inner_index.map(py, |idx| CoreMissing::new(idx, bases_vec))
         };
@@ -253,6 +266,7 @@
 
     def hasbases(&self) -> PyResult<bool> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner: &CoreMissing<PySharedIndex> =
             &*unsafe { leaked.try_borrow(py)? };
         Ok(inner.has_bases())
@@ -262,11 +276,13 @@
         let bases_vec: Vec<_> = {
             let leaked = py_rust_index_to_graph(py,
                                                self.index(py).clone_ref(py))?;
+            // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
             let index = &*unsafe { leaked.try_borrow(py)? };
             rev_pyiter_collect(py, &bases, index)?
         };
 
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner: &mut CoreMissing<PySharedIndex> =
             &mut *unsafe { leaked.try_borrow_mut(py)? };
 
@@ -279,6 +295,7 @@
 
     def bases(&self) -> PyResult<HashSet<PyRevision>> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner: &CoreMissing<PySharedIndex> =
             &*unsafe { leaked.try_borrow(py)? };
         Ok(inner.get_bases()
@@ -290,6 +307,7 @@
 
     def basesheads(&self) -> PyResult<HashSet<PyRevision>> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner: &CoreMissing<PySharedIndex> =
             &*unsafe { leaked.try_borrow(py)? };
         Ok(
@@ -315,11 +333,13 @@
             //    implement it for a Python set rewrapped with the GIL marker
             let leaked = py_rust_index_to_graph(py,
                                                self.index(py).clone_ref(py))?;
+            // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
             let index = &*unsafe { leaked.try_borrow(py)? };
             rev_pyiter_collect(py, &revs, &*index)?
         };
 
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner: &mut CoreMissing<PySharedIndex> =
             &mut *unsafe { leaked.try_borrow_mut(py)? };
 
@@ -342,11 +362,13 @@
         let revs_vec: Vec<Revision> = {
             let leaked = py_rust_index_to_graph(py,
                                                self.index(py).clone_ref(py))?;
+            // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
             let index = &*unsafe { leaked.try_borrow(py)? };
             rev_pyiter_collect(py, &revs, index)?
         };
 
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner: &mut CoreMissing<PySharedIndex> =
             &mut *unsafe { leaked.try_borrow_mut(py)? };
 

--- a/rust/hg-cpython/src/dagops.rs	Sun Oct 29 12:18:03 2023 +0100
+++ b/rust/hg-cpython/src/dagops.rs	Thu Nov 23 03:41:58 2023 +0100
@@ -28,6 +28,7 @@
     revs: PyObject,
 ) -> PyResult<HashSet<PyRevision>> {
     let py_leaked = py_rust_index_to_graph(py, index)?;
+    // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
     let index = &*unsafe { py_leaked.try_borrow(py)? };
     let mut as_set: HashSet<Revision> = rev_pyiter_collect(py, &revs, index)?;
     dagops::retain_heads(index, &mut as_set)

--- a/rust/hg-cpython/src/discovery.rs	Sun Oct 29 12:18:03 2023 +0100
+++ b/rust/hg-cpython/src/discovery.rs	Thu Nov 23 03:41:58 2023 +0100
@@ -60,18 +60,21 @@
 
     def hasinfo(&self) -> PyResult<bool> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner = unsafe { leaked.try_borrow(py)? };
         Ok(inner.has_info())
     }
 
     def iscomplete(&self) -> PyResult<bool> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner = unsafe { leaked.try_borrow(py)? };
         Ok(inner.is_complete())
     }
 
     def stats(&self) -> PyResult<PyDict> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner = unsafe { leaked.try_borrow(py)? };
         let stats = inner.stats();
         let as_dict: PyDict = PyDict::new(py);
@@ -84,6 +87,7 @@
 
     def commonheads(&self) -> PyResult<HashSet<PyRevision>> {
         let leaked = self.inner(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let inner = unsafe { leaked.try_borrow(py)? };
         let res = inner.common_heads()
                     .map_err(|e| GraphError::pynew(py, e))?;
@@ -113,11 +117,12 @@
         let index = repo.getattr(py, "changelog")?.getattr(py, "index")?;
         let cloned_index = py_rust_index_to_graph(py, index.clone_ref(py))?;
         let index = py_rust_index_to_graph(py, index)?;
-
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let target_heads = {
             let borrowed_idx = unsafe { index.try_borrow(py)? };
             rev_pyiter_collect(py, &targetheads, &*borrowed_idx)?
         };
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let lazy_disco = unsafe {
             index.map(py, |idx| {
                 CorePartialDiscovery::new(
@@ -142,6 +147,7 @@
         iter: &PyObject,
     ) -> PyResult<Vec<Revision>> {
         let leaked = self.index(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let index = unsafe { leaked.try_borrow(py)? };
         rev_pyiter_collect(py, iter, &*index)
     }
@@ -168,6 +174,7 @@
             }
         }
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let mut inner = unsafe { leaked.try_borrow_mut(py)? };
         inner
             .add_common_revisions(common)
@@ -185,6 +192,7 @@
     ) -> PyResult<PyObject> {
         let commons_vec = self.pyiter_to_vec(py, &commons)?;
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let mut inner = unsafe { leaked.try_borrow_mut(py)? };
         inner
             .add_common_revisions(commons_vec)
@@ -199,6 +207,7 @@
     ) -> PyResult<PyObject> {
         let missings_vec = self.pyiter_to_vec(py, &missings)?;
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let mut inner = unsafe { leaked.try_borrow_mut(py)? };
         inner
             .add_missing_revisions(missings_vec)
@@ -213,6 +222,7 @@
         size: usize,
     ) -> PyResult<PyObject> {
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let mut inner = unsafe { leaked.try_borrow_mut(py)? };
         let sample = inner
             .take_full_sample(size)
@@ -232,6 +242,7 @@
     ) -> PyResult<PyObject> {
         let revsvec = self.pyiter_to_vec(py, &headrevs)?;
         let mut leaked = self.inner(py).borrow_mut();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let mut inner = unsafe { leaked.try_borrow_mut(py)? };
         let sample = inner
             .take_quick_sample(revsvec, size)

--- a/rust/hg-cpython/src/revlog.rs	Sun Oct 29 12:18:03 2023 +0100
+++ b/rust/hg-cpython/src/revlog.rs	Thu Nov 23 03:41:58 2023 +0100
@@ -42,6 +42,7 @@
 ) -> PyResult<UnsafePyLeaked<PySharedIndex>> {
     let midx = index.extract::<Index>(py)?;
     let leaked = midx.index(py).leak_immutable();
+    // Safety: we don't leak the "faked" reference out of the `UnsafePyLeaked`
     Ok(unsafe { leaked.map(py, |idx| PySharedIndex { inner: idx }) })
 }
 
@@ -975,6 +976,7 @@
     /// been meanwhile mutated.
     def is_invalidated(&self) -> PyResult<bool> {
         let leaked = self.index(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let result = unsafe { leaked.try_borrow(py) };
         // two cases for result to be an error:
         // - the index has previously been mutably borrowed
@@ -986,6 +988,7 @@
 
     def insert(&self, rev: PyRevision) -> PyResult<PyObject> {
         let leaked = self.index(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let index = &*unsafe { leaked.try_borrow(py)? };
 
         let rev = UncheckedRevision(rev.0);
@@ -1020,6 +1023,7 @@
 
         let nt = self.nt(py).borrow();
         let leaked = self.index(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let index = &*unsafe { leaked.try_borrow(py)? };
 
         Ok(nt.find_bin(index, prefix)
@@ -1031,6 +1035,7 @@
     def shortest(&self, node: PyBytes) -> PyResult<usize> {
         let nt = self.nt(py).borrow();
         let leaked = self.index(py).borrow();
+        // Safety: we don't leak the "faked" reference out of `UnsafePyLeaked`
         let idx = &*unsafe { leaked.try_borrow(py)? };
         match nt.unique_prefix_len_node(idx, &node_from_py_bytes(py, &node)?)
         {