Mercurial > public > mercurial-scm > hg-stable
diff rust/hg-cpython/src/ancestors.rs @ 51276: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.
| author | Rapha?l Gom?s <rgomes@octobus.net> |
|---|---|
| date | Thu, 23 Nov 2023 03:41:58 +0100 |
| parents | 59d81768ad6d |
| children | c4cbb515b006 |
line wrap: on
line diff
--- 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)? };