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)?)
{