rust-cpython: make PyLeakedRef operations relatively safe

This patch encapsulates the access to the leaked reference to make most
leaked-ref operations safe. The only exception is leaked_ref.map(). I
couldn't figure out how to allow arbitrary map operation safely over an
unsafe static reference. See the docstring and inline comment for details.

Now leak_immutable() can be safely implemented as the PyLeakedRef owns
its inner data.

rust/hg-cpython/src/dirstate/copymap.rs

@@ -1,120 +1,117 b''
 // copymap.rs
 //
 // Copyright 2019 Raphaël Gomès <[e-mail]>
 //
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
 
 //! Bindings for `hg::dirstate::dirstate_map::CopyMap` provided by the
 //! `hg-core` package.
 
 use cpython::{PyBytes, PyClone, PyDict, PyObject, PyResult, Python};
 use std::cell::RefCell;
 
 use crate::dirstate::dirstate_map::DirstateMap;
 use crate::ref_sharing::PyLeakedRef;
-use hg::DirstateMap as RustDirstateMap;
 use hg::{utils::hg_path::HgPathBuf, CopyMapIter};
 
 py_class!(pub class CopyMap |py| {
     data dirstate_map: DirstateMap;
 
     def __getitem__(&self, key: PyObject) -> PyResult<PyBytes> {
         (*self.dirstate_map(py)).copymapgetitem(py, key)
     }
 
     def __len__(&self) -> PyResult<usize> {
         self.dirstate_map(py).copymaplen(py)
     }
 
     def __contains__(&self, key: PyObject) -> PyResult<bool> {
         self.dirstate_map(py).copymapcontains(py, key)
     }
 
     def get(
         &self,
         key: PyObject,
         default: Option<PyObject> = None
     ) -> PyResult<Option<PyObject>> {
         self.dirstate_map(py).copymapget(py, key, default)
     }
 
     def pop(
         &self,
         key: PyObject,
         default: Option<PyObject> = None
     ) -> PyResult<Option<PyObject>> {
         self.dirstate_map(py).copymappop(py, key, default)
     }
 
     def __iter__(&self) -> PyResult<CopyMapKeysIterator> {
         self.dirstate_map(py).copymapiter(py)
     }
 
     // Python's `dict()` builtin works with either a subclass of dict
     // or an abstract mapping. Said mapping needs to implement `__getitem__`
     // and `keys`.
     def keys(&self) -> PyResult<CopyMapKeysIterator> {
         self.dirstate_map(py).copymapiter(py)
     }
 
     def items(&self) -> PyResult<CopyMapItemsIterator> {
         self.dirstate_map(py).copymapitemsiter(py)
     }
 
     def iteritems(&self) -> PyResult<CopyMapItemsIterator> {
         self.dirstate_map(py).copymapitemsiter(py)
     }
 
     def __setitem__(
         &self,
         key: PyObject,
         item: PyObject
     ) -> PyResult<()> {
         self.dirstate_map(py).copymapsetitem(py, key, item)?;
         Ok(())
     }
 
     def copy(&self) -> PyResult<PyDict> {
         self.dirstate_map(py).copymapcopy(py)
     }
 
 });
 
 impl CopyMap {
     pub fn from_inner(py: Python, dm: DirstateMap) -> PyResult<Self> {
         Self::create_instance(py, dm)
     }
     fn translate_key(
         py: Python,
         res: (&HgPathBuf, &HgPathBuf),
     ) -> PyResult<Option<PyBytes>> {
         Ok(Some(PyBytes::new(py, res.0.as_ref())))
     }
     fn translate_key_value(
         py: Python,
         res: (&HgPathBuf, &HgPathBuf),
     ) -> PyResult<Option<(PyBytes, PyBytes)>> {
         let (k, v) = res;
         Ok(Some((
             PyBytes::new(py, k.as_ref()),
             PyBytes::new(py, v.as_ref()),
         )))
     }
 }
 
 py_shared_iterator!(
     CopyMapKeysIterator,
-    PyLeakedRef<&'static RustDirstateMap>,
-    CopyMapIter<'static>,
+    PyLeakedRef<CopyMapIter<'static>>,
     CopyMap::translate_key,
     Option<PyBytes>
 );
 
 py_shared_iterator!(
     CopyMapItemsIterator,
-    PyLeakedRef<&'static RustDirstateMap>,
-    CopyMapIter<'static>,
+    PyLeakedRef<CopyMapIter<'static>>,
     CopyMap::translate_key_value,
     Option<(PyBytes, PyBytes)>
 );

rust/hg-cpython/src/dirstate/dirs_multiset.rs

@@ -1,133 +1,129 b''
 // dirs_multiset.rs
 //
 // Copyright 2019 Raphaël Gomès <[e-mail]>
 //
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
 
 //! Bindings for the `hg::dirstate::dirs_multiset` file provided by the
 //! `hg-core` package.
 
 use std::cell::RefCell;
 use std::convert::TryInto;
 
 use cpython::{
     exc, ObjectProtocol, PyBytes, PyClone, PyDict, PyErr, PyObject, PyResult,
     Python,
 };
 
 use crate::dirstate::extract_dirstate;
 use crate::ref_sharing::{PyLeakedRef, PySharedRefCell};
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
     DirsMultiset, DirsMultisetIter, DirstateMapError, DirstateParseError,
     EntryState,
 };
 
 py_class!(pub class Dirs |py| {
     data inner: PySharedRefCell<DirsMultiset>;
 
     // `map` is either a `dict` or a flat iterator (usually a `set`, sometimes
     // a `list`)
     def __new__(
         _cls,
         map: PyObject,
         skip: Option<PyObject> = None
     ) -> PyResult<Self> {
         let mut skip_state: Option<EntryState> = None;
         if let Some(skip) = skip {
             skip_state = Some(
                 skip.extract::<PyBytes>(py)?.data(py)[0]
                     .try_into()
                     .map_err(|e: DirstateParseError| {
                         PyErr::new::<exc::ValueError, _>(py, e.to_string())
                     })?,
             );
         }
         let inner = if let Ok(map) = map.cast_as::<PyDict>(py) {
             let dirstate = extract_dirstate(py, &map)?;
             DirsMultiset::from_dirstate(&dirstate, skip_state)
         } else {
             let map: Result<Vec<HgPathBuf>, PyErr> = map
                 .iter(py)?
                 .map(|o| {
                     Ok(HgPathBuf::from_bytes(
                         o?.extract::<PyBytes>(py)?.data(py),
                     ))
                 })
                 .collect();
             DirsMultiset::from_manifest(&map?)
         };
 
         Self::create_instance(
             py,
             PySharedRefCell::new(inner),
         )
     }
 
     def addpath(&self, path: PyObject) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?.add_path(
             HgPath::new(path.extract::<PyBytes>(py)?.data(py)),
         );
         Ok(py.None())
     }
 
     def delpath(&self, path: PyObject) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?.delete_path(
             HgPath::new(path.extract::<PyBytes>(py)?.data(py)),
         )
             .and(Ok(py.None()))
             .or_else(|e| {
                 match e {
                     DirstateMapError::PathNotFound(_p) => {
                         Err(PyErr::new::<exc::ValueError, _>(
                             py,
                             "expected a value, found none".to_string(),
                         ))
                     }
                     DirstateMapError::EmptyPath => {
                         Ok(py.None())
                     }
                 }
             })
     }
     def __iter__(&self) -> PyResult<DirsMultisetKeysIterator> {
-        let mut leak_handle =
-            unsafe { self.inner_shared(py).leak_immutable()? };
-        let leaked_ref = leak_handle.data.take().unwrap();
+        let leaked_ref = self.inner_shared(py).leak_immutable()?;
         DirsMultisetKeysIterator::from_inner(
             py,
-            leak_handle,
-            leaked_ref.iter(),
+            unsafe { leaked_ref.map(py, |o| o.iter()) },
         )
     }
 
     def __contains__(&self, item: PyObject) -> PyResult<bool> {
         Ok(self.inner(py).borrow().contains(HgPath::new(
             item.extract::<PyBytes>(py)?.data(py).as_ref(),
         )))
     }
 });
 
 py_shared_ref!(Dirs, DirsMultiset, inner, inner_shared);
 
 impl Dirs {
     pub fn from_inner(py: Python, d: DirsMultiset) -> PyResult<Self> {
         Self::create_instance(py, PySharedRefCell::new(d))
     }
 
     fn translate_key(
         py: Python,
         res: &HgPathBuf,
     ) -> PyResult<Option<PyBytes>> {
         Ok(Some(PyBytes::new(py, res.as_ref())))
     }
 }
 
 py_shared_iterator!(
     DirsMultisetKeysIterator,
-    PyLeakedRef<&'static DirsMultiset>,
-    DirsMultisetIter<'static>,
+    PyLeakedRef<DirsMultisetIter<'static>>,
     Dirs::translate_key,
     Option<PyBytes>
 );

rust/hg-cpython/src/dirstate/dirstate_map.rs

@@ -1,521 +1,504 b''
 // dirstate_map.rs
 //
 // Copyright 2019 Raphaël Gomès <[e-mail]>
 //
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
 
 //! Bindings for the `hg::dirstate::dirstate_map` file provided by the
 //! `hg-core` package.
 
 use std::cell::{Ref, RefCell};
 use std::convert::TryInto;
 use std::time::Duration;
 
 use cpython::{
     exc, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyObject,
     PyResult, PyTuple, Python, PythonObject, ToPyObject,
 };
 
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
     dirstate::{dirs_multiset::Dirs, make_dirstate_tuple},
     ref_sharing::{PyLeakedRef, PySharedRefCell},
 };
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
     DirsMultiset, DirstateEntry, DirstateMap as RustDirstateMap,
     DirstateParents, DirstateParseError, EntryState, StateMapIter,
     PARENT_SIZE,
 };
 
 // TODO
 //     This object needs to share references to multiple members of its Rust
 //     inner struct, namely `copy_map`, `dirs` and `all_dirs`.
 //     Right now `CopyMap` is done, but it needs to have an explicit reference
 //     to `RustDirstateMap` which itself needs to have an encapsulation for
 //     every method in `CopyMap` (copymapcopy, etc.).
 //     This is ugly and hard to maintain.
 //     The same logic applies to `dirs` and `all_dirs`, however the `Dirs`
 //     `py_class!` is already implemented and does not mention
 //     `RustDirstateMap`, rightfully so.
 //     All attributes also have to have a separate refcount data attribute for
 //     leaks, with all methods that go along for reference sharing.
 py_class!(pub class DirstateMap |py| {
     data inner: PySharedRefCell<RustDirstateMap>;
 
     def __new__(_cls, _root: PyObject) -> PyResult<Self> {
         let inner = RustDirstateMap::default();
         Self::create_instance(
             py,
             PySharedRefCell::new(inner),
         )
     }
 
     def clear(&self) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?.clear();
         Ok(py.None())
     }
 
     def get(
         &self,
         key: PyObject,
         default: Option<PyObject> = None
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
         match self.inner(py).borrow().get(HgPath::new(key.data(py))) {
             Some(entry) => {
                 Ok(Some(make_dirstate_tuple(py, entry)?))
             },
             None => Ok(default)
         }
     }
 
     def addfile(
         &self,
         f: PyObject,
         oldstate: PyObject,
         state: PyObject,
         mode: PyObject,
         size: PyObject,
         mtime: PyObject
     ) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?.add_file(
             HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
             oldstate.extract::<PyBytes>(py)?.data(py)[0]
                 .try_into()
                 .map_err(|e: DirstateParseError| {
                     PyErr::new::<exc::ValueError, _>(py, e.to_string())
                 })?,
             DirstateEntry {
                 state: state.extract::<PyBytes>(py)?.data(py)[0]
                     .try_into()
                     .map_err(|e: DirstateParseError| {
                         PyErr::new::<exc::ValueError, _>(py, e.to_string())
                     })?,
                 mode: mode.extract(py)?,
                 size: size.extract(py)?,
                 mtime: mtime.extract(py)?,
             },
         );
         Ok(py.None())
     }
 
     def removefile(
         &self,
         f: PyObject,
         oldstate: PyObject,
         size: PyObject
     ) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?
             .remove_file(
                 HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
                 oldstate.extract::<PyBytes>(py)?.data(py)[0]
                     .try_into()
                     .map_err(|e: DirstateParseError| {
                         PyErr::new::<exc::ValueError, _>(py, e.to_string())
                     })?,
                 size.extract(py)?,
             )
             .or_else(|_| {
                 Err(PyErr::new::<exc::OSError, _>(
                     py,
                     "Dirstate error".to_string(),
                 ))
             })?;
         Ok(py.None())
     }
 
     def dropfile(
         &self,
         f: PyObject,
         oldstate: PyObject
     ) -> PyResult<PyBool> {
         self.inner_shared(py).borrow_mut()?
             .drop_file(
                 HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
                 oldstate.extract::<PyBytes>(py)?.data(py)[0]
                     .try_into()
                     .map_err(|e: DirstateParseError| {
                         PyErr::new::<exc::ValueError, _>(py, e.to_string())
                     })?,
             )
             .and_then(|b| Ok(b.to_py_object(py)))
             .or_else(|_| {
                 Err(PyErr::new::<exc::OSError, _>(
                     py,
                     "Dirstate error".to_string(),
                 ))
             })
     }
 
     def clearambiguoustimes(
         &self,
         files: PyObject,
         now: PyObject
     ) -> PyResult<PyObject> {
         let files: PyResult<Vec<HgPathBuf>> = files
             .iter(py)?
             .map(|filename| {
                 Ok(HgPathBuf::from_bytes(
                     filename?.extract::<PyBytes>(py)?.data(py),
                 ))
             })
             .collect();
         self.inner_shared(py).borrow_mut()?
             .clear_ambiguous_times(files?, now.extract(py)?);
         Ok(py.None())
     }
 
     // TODO share the reference
     def nonnormalentries(&self) -> PyResult<PyObject> {
         let (non_normal, other_parent) =
             self.inner(py).borrow().non_normal_other_parent_entries();
 
         let locals = PyDict::new(py);
         locals.set_item(
             py,
             "non_normal",
             non_normal
                 .iter()
                 .map(|v| PyBytes::new(py, v.as_ref()))
                 .collect::<Vec<PyBytes>>()
                 .to_py_object(py),
         )?;
         locals.set_item(
             py,
             "other_parent",
             other_parent
                 .iter()
                 .map(|v| PyBytes::new(py, v.as_ref()))
                 .collect::<Vec<PyBytes>>()
                 .to_py_object(py),
         )?;
 
         py.eval("set(non_normal), set(other_parent)", None, Some(&locals))
     }
 
     def hastrackeddir(&self, d: PyObject) -> PyResult<PyBool> {
         let d = d.extract::<PyBytes>(py)?;
         Ok(self.inner_shared(py).borrow_mut()?
             .has_tracked_dir(HgPath::new(d.data(py)))
             .to_py_object(py))
     }
 
     def hasdir(&self, d: PyObject) -> PyResult<PyBool> {
         let d = d.extract::<PyBytes>(py)?;
         Ok(self.inner_shared(py).borrow_mut()?
             .has_dir(HgPath::new(d.data(py)))
             .to_py_object(py))
     }
 
     def parents(&self, st: PyObject) -> PyResult<PyTuple> {
         self.inner_shared(py).borrow_mut()?
             .parents(st.extract::<PyBytes>(py)?.data(py))
             .and_then(|d| {
                 Ok((PyBytes::new(py, &d.p1), PyBytes::new(py, &d.p2))
                     .to_py_object(py))
             })
             .or_else(|_| {
                 Err(PyErr::new::<exc::OSError, _>(
                     py,
                     "Dirstate error".to_string(),
                 ))
             })
     }
 
     def setparents(&self, p1: PyObject, p2: PyObject) -> PyResult<PyObject> {
         let p1 = extract_node_id(py, &p1)?;
         let p2 = extract_node_id(py, &p2)?;
 
         self.inner_shared(py).borrow_mut()?
             .set_parents(&DirstateParents { p1, p2 });
         Ok(py.None())
     }
 
     def read(&self, st: PyObject) -> PyResult<Option<PyObject>> {
         match self.inner_shared(py).borrow_mut()?
             .read(st.extract::<PyBytes>(py)?.data(py))
         {
             Ok(Some(parents)) => Ok(Some(
                 (PyBytes::new(py, &parents.p1), PyBytes::new(py, &parents.p2))
                     .to_py_object(py)
                     .into_object(),
             )),
             Ok(None) => Ok(Some(py.None())),
             Err(_) => Err(PyErr::new::<exc::OSError, _>(
                 py,
                 "Dirstate error".to_string(),
             )),
         }
     }
     def write(
         &self,
         p1: PyObject,
         p2: PyObject,
         now: PyObject
     ) -> PyResult<PyBytes> {
         let now = Duration::new(now.extract(py)?, 0);
         let parents = DirstateParents {
             p1: extract_node_id(py, &p1)?,
             p2: extract_node_id(py, &p2)?,
         };
 
         match self.inner_shared(py).borrow_mut()?.pack(parents, now) {
             Ok(packed) => Ok(PyBytes::new(py, &packed)),
             Err(_) => Err(PyErr::new::<exc::OSError, _>(
                 py,
                 "Dirstate error".to_string(),
             )),
         }
     }
 
     def filefoldmapasdict(&self) -> PyResult<PyDict> {
         let dict = PyDict::new(py);
         for (key, value) in
             self.inner_shared(py).borrow_mut()?.build_file_fold_map().iter()
         {
             dict.set_item(py, key.as_ref().to_vec(), value.as_ref().to_vec())?;
         }
         Ok(dict)
     }
 
     def __len__(&self) -> PyResult<usize> {
         Ok(self.inner(py).borrow().len())
     }
 
     def __contains__(&self, key: PyObject) -> PyResult<bool> {
         let key = key.extract::<PyBytes>(py)?;
         Ok(self.inner(py).borrow().contains_key(HgPath::new(key.data(py))))
     }
 
     def __getitem__(&self, key: PyObject) -> PyResult<PyObject> {
         let key = key.extract::<PyBytes>(py)?;
         let key = HgPath::new(key.data(py));
         match self.inner(py).borrow().get(key) {
             Some(entry) => {
                 Ok(make_dirstate_tuple(py, entry)?)
             },
             None => Err(PyErr::new::<exc::KeyError, _>(
                 py,
                 String::from_utf8_lossy(key.as_bytes()),
             )),
         }
     }
 
     def keys(&self) -> PyResult<DirstateMapKeysIterator> {
-        let mut leak_handle =
-            unsafe { self.inner_shared(py).leak_immutable()? };
-        let leaked_ref = leak_handle.data.take().unwrap();
+        let leaked_ref = self.inner_shared(py).leak_immutable()?;
         DirstateMapKeysIterator::from_inner(
             py,
-            leak_handle,
-            leaked_ref.iter(),
+            unsafe { leaked_ref.map(py, |o| o.iter()) },
         )
     }
 
     def items(&self) -> PyResult<DirstateMapItemsIterator> {
-        let mut leak_handle =
-            unsafe { self.inner_shared(py).leak_immutable()? };
-        let leaked_ref = leak_handle.data.take().unwrap();
+        let leaked_ref = self.inner_shared(py).leak_immutable()?;
         DirstateMapItemsIterator::from_inner(
             py,
-            leak_handle,
-            leaked_ref.iter(),
+            unsafe { leaked_ref.map(py, |o| o.iter()) },
         )
     }
 
     def __iter__(&self) -> PyResult<DirstateMapKeysIterator> {
-        let mut leak_handle =
-            unsafe { self.inner_shared(py).leak_immutable()? };
-        let leaked_ref = leak_handle.data.take().unwrap();
+        let leaked_ref = self.inner_shared(py).leak_immutable()?;
         DirstateMapKeysIterator::from_inner(
             py,
-            leak_handle,
-            leaked_ref.iter(),
+            unsafe { leaked_ref.map(py, |o| o.iter()) },
         )
     }
 
     def getdirs(&self) -> PyResult<Dirs> {
         // TODO don't copy, share the reference
         self.inner_shared(py).borrow_mut()?.set_dirs();
         Dirs::from_inner(
             py,
             DirsMultiset::from_dirstate(
                 &self.inner(py).borrow(),
                 Some(EntryState::Removed),
             ),
         )
     }
     def getalldirs(&self) -> PyResult<Dirs> {
         // TODO don't copy, share the reference
         self.inner_shared(py).borrow_mut()?.set_all_dirs();
         Dirs::from_inner(
             py,
             DirsMultiset::from_dirstate(
                 &self.inner(py).borrow(),
                 None,
             ),
         )
     }
 
     // TODO all copymap* methods, see docstring above
     def copymapcopy(&self) -> PyResult<PyDict> {
         let dict = PyDict::new(py);
         for (key, value) in self.inner(py).borrow().copy_map.iter() {
             dict.set_item(
                 py,
                 PyBytes::new(py, key.as_ref()),
                 PyBytes::new(py, value.as_ref()),
             )?;
         }
         Ok(dict)
     }
 
     def copymapgetitem(&self, key: PyObject) -> PyResult<PyBytes> {
         let key = key.extract::<PyBytes>(py)?;
         match self.inner(py).borrow().copy_map.get(HgPath::new(key.data(py))) {
             Some(copy) => Ok(PyBytes::new(py, copy.as_ref())),
             None => Err(PyErr::new::<exc::KeyError, _>(
                 py,
                 String::from_utf8_lossy(key.data(py)),
             )),
         }
     }
     def copymap(&self) -> PyResult<CopyMap> {
         CopyMap::from_inner(py, self.clone_ref(py))
     }
 
     def copymaplen(&self) -> PyResult<usize> {
         Ok(self.inner(py).borrow().copy_map.len())
     }
     def copymapcontains(&self, key: PyObject) -> PyResult<bool> {
         let key = key.extract::<PyBytes>(py)?;
         Ok(self
             .inner(py)
             .borrow()
             .copy_map
             .contains_key(HgPath::new(key.data(py))))
     }
     def copymapget(
         &self,
         key: PyObject,
         default: Option<PyObject>
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
         match self
             .inner(py)
             .borrow()
             .copy_map
             .get(HgPath::new(key.data(py)))
         {
             Some(copy) => Ok(Some(
                 PyBytes::new(py, copy.as_ref()).into_object(),
             )),
             None => Ok(default),
         }
     }
     def copymapsetitem(
         &self,
         key: PyObject,
         value: PyObject
     ) -> PyResult<PyObject> {
         let key = key.extract::<PyBytes>(py)?;
         let value = value.extract::<PyBytes>(py)?;
         self.inner_shared(py).borrow_mut()?.copy_map.insert(
             HgPathBuf::from_bytes(key.data(py)),
             HgPathBuf::from_bytes(value.data(py)),
         );
         Ok(py.None())
     }
     def copymappop(
         &self,
         key: PyObject,
         default: Option<PyObject>
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
         match self
             .inner_shared(py)
             .borrow_mut()?
             .copy_map
             .remove(HgPath::new(key.data(py)))
         {
             Some(_) => Ok(None),
             None => Ok(default),
         }
     }
 
     def copymapiter(&self) -> PyResult<CopyMapKeysIterator> {
-        let mut leak_handle =
-            unsafe { self.inner_shared(py).leak_immutable()? };
-        let leaked_ref = leak_handle.data.take().unwrap();
+        let leaked_ref = self.inner_shared(py).leak_immutable()?;
         CopyMapKeysIterator::from_inner(
             py,
-            leak_handle,
-            leaked_ref.copy_map.iter(),
+            unsafe { leaked_ref.map(py, |o| o.copy_map.iter()) },
         )
     }
 
     def copymapitemsiter(&self) -> PyResult<CopyMapItemsIterator> {
-        let mut leak_handle =
-            unsafe { self.inner_shared(py).leak_immutable()? };
-        let leaked_ref = leak_handle.data.take().unwrap();
+        let leaked_ref = self.inner_shared(py).leak_immutable()?;
         CopyMapItemsIterator::from_inner(
             py,
-            leak_handle,
-            leaked_ref.copy_map.iter(),
+            unsafe { leaked_ref.map(py, |o| o.copy_map.iter()) },
         )
     }
 
 });
 
 impl DirstateMap {
     pub fn get_inner<'a>(
         &'a self,
         py: Python<'a>,
     ) -> Ref<'a, RustDirstateMap> {
         self.inner_shared(py).borrow()
     }
     fn translate_key(
         py: Python,
         res: (&HgPathBuf, &DirstateEntry),
     ) -> PyResult<Option<PyBytes>> {
         Ok(Some(PyBytes::new(py, res.0.as_ref())))
     }
     fn translate_key_value(
         py: Python,
         res: (&HgPathBuf, &DirstateEntry),
     ) -> PyResult<Option<(PyBytes, PyObject)>> {
         let (f, entry) = res;
         Ok(Some((
             PyBytes::new(py, f.as_ref()),
             make_dirstate_tuple(py, entry)?,
         )))
     }
 }
 
 py_shared_ref!(DirstateMap, RustDirstateMap, inner, inner_shared);
 
 py_shared_iterator!(
     DirstateMapKeysIterator,
-    PyLeakedRef<&'static RustDirstateMap>,
-    StateMapIter<'static>,
+    PyLeakedRef<StateMapIter<'static>>,
     DirstateMap::translate_key,
     Option<PyBytes>
 );
 
 py_shared_iterator!(
     DirstateMapItemsIterator,
-    PyLeakedRef<&'static RustDirstateMap>,
-    StateMapIter<'static>,
+    PyLeakedRef<StateMapIter<'static>>,
     DirstateMap::translate_key_value,
     Option<(PyBytes, PyObject)>
 );
 
 fn extract_node_id(py: Python, obj: &PyObject) -> PyResult<[u8; PARENT_SIZE]> {
     let bytes = obj.extract::<PyBytes>(py)?;
     match bytes.data(py).try_into() {
         Ok(s) => Ok(s),
         Err(e) => Err(PyErr::new::<exc::ValueError, _>(py, e.to_string())),
     }
 }

rust/hg-cpython/src/ref_sharing.rs

@@ -1,469 +1,500 b''
 // ref_sharing.rs
 //
 // Copyright 2019 Raphaël Gomès <[e-mail]>
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to
 // deal in the Software without restriction, including without limitation the
 // rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
 // sell copies of the Software, and to permit persons to whom the Software is
 // furnished to do so, subject to the following conditions:
 //
 // The above copyright notice and this permission notice shall be included in
 // all copies or substantial portions of the Software.
 //
 // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
 // IN THE SOFTWARE.
 
 //! Macros for use in the `hg-cpython` bridge library.
 
 use crate::exceptions::AlreadyBorrowed;
 use cpython::{PyClone, PyObject, PyResult, Python};
 use std::cell::{Cell, Ref, RefCell, RefMut};
 
 /// Manages the shared state between Python and Rust
 #[derive(Debug, Default)]
 pub struct PySharedState {
     leak_count: Cell<usize>,
     mutably_borrowed: Cell<bool>,
 }
 
 // &PySharedState can be Send because any access to inner cells is
 // synchronized by the GIL.
 unsafe impl Sync for PySharedState {}
 
 impl PySharedState {
     pub fn borrow_mut<'a, T>(
         &'a self,
         py: Python<'a>,
         pyrefmut: RefMut<'a, T>,
     ) -> PyResult<PyRefMut<'a, T>> {
         if self.mutably_borrowed.get() {
             return Err(AlreadyBorrowed::new(
                 py,
                 "Cannot borrow mutably while there exists another \
                  mutable reference in a Python object",
             ));
         }
         match self.leak_count.get() {
             0 => {
                 self.mutably_borrowed.replace(true);
                 Ok(PyRefMut::new(py, pyrefmut, self))
             }
             // TODO
             // For now, this works differently than Python references
             // in the case of iterators.
             // Python does not complain when the data an iterator
             // points to is modified if the iterator is never used
             // afterwards.
             // Here, we are stricter than this by refusing to give a
             // mutable reference if it is already borrowed.
             // While the additional safety might be argued for, it
             // breaks valid programming patterns in Python and we need
             // to fix this issue down the line.
             _ => Err(AlreadyBorrowed::new(
                 py,
                 "Cannot borrow mutably while there are \
                  immutable references in Python objects",
             )),
         }
     }
 
     /// Return a reference to the wrapped data and its state with an
     /// artificial static lifetime.
     /// We need to be protected by the GIL for thread-safety.
     ///
     /// # Safety
     ///
     /// This is highly unsafe since the lifetime of the given data can be
     /// extended. Do not call this function directly.
     pub unsafe fn leak_immutable<T>(
         &self,
         py: Python,
         data: &PySharedRefCell<T>,
     ) -> PyResult<(&'static T, &'static PySharedState)> {
         if self.mutably_borrowed.get() {
             return Err(AlreadyBorrowed::new(
                 py,
                 "Cannot borrow immutably while there is a \
                  mutable reference in Python objects",
             ));
         }
         // TODO: it's weird that self is data.py_shared_state. Maybe we
         // can move stuff to PySharedRefCell?
         let ptr = data.as_ptr();
         let state_ptr: *const PySharedState = &data.py_shared_state;
         self.leak_count.replace(self.leak_count.get() + 1);
         Ok((&*ptr, &*state_ptr))
     }
 
     /// # Safety
     ///
     /// It's unsafe to update the reference count without knowing the
     /// reference is deleted. Do not call this function directly.
     pub unsafe fn decrease_leak_count(&self, _py: Python, mutable: bool) {
         if mutable {
             assert_eq!(self.leak_count.get(), 0);
             assert!(self.mutably_borrowed.get());
             self.mutably_borrowed.replace(false);
         } else {
             let count = self.leak_count.get();
             assert!(count > 0);
             self.leak_count.replace(count - 1);
         }
     }
 }
 
 /// `RefCell` wrapper to be safely used in conjunction with `PySharedState`.
 ///
 /// Only immutable operation is allowed through this interface.
 #[derive(Debug)]
 pub struct PySharedRefCell<T> {
     inner: RefCell<T>,
     py_shared_state: PySharedState,
 }
 
 impl<T> PySharedRefCell<T> {
     pub fn new(value: T) -> PySharedRefCell<T> {
         Self {
             inner: RefCell::new(value),
             py_shared_state: PySharedState::default(),
         }
     }
 
     pub fn borrow(&self) -> Ref<T> {
         // py_shared_state isn't involved since
         // - inner.borrow() would fail if self is mutably borrowed,
         // - and inner.borrow_mut() would fail while self is borrowed.
         self.inner.borrow()
     }
 
     pub fn as_ptr(&self) -> *mut T {
         self.inner.as_ptr()
     }
 
     // TODO: maybe this should be named as try_borrow_mut(), and use
     // inner.try_borrow_mut(). The current implementation panics if
     // self.inner has been borrowed, but returns error if py_shared_state
     // refuses to borrow.
     pub fn borrow_mut<'a>(
         &'a self,
         py: Python<'a>,
     ) -> PyResult<PyRefMut<'a, T>> {
         self.py_shared_state.borrow_mut(py, self.inner.borrow_mut())
     }
 }
 
 /// Sharable data member of type `T` borrowed from the `PyObject`.
 pub struct PySharedRef<'a, T> {
     py: Python<'a>,
     owner: &'a PyObject,
     data: &'a PySharedRefCell<T>,
 }
 
 impl<'a, T> PySharedRef<'a, T> {
     /// # Safety
     ///
     /// The `data` must be owned by the `owner`. Otherwise, the leak count
     /// would get wrong.
     pub unsafe fn new(
         py: Python<'a>,
         owner: &'a PyObject,
         data: &'a PySharedRefCell<T>,
     ) -> Self {
         Self { py, owner, data }
     }
 
     pub fn borrow(&self) -> Ref<'a, T> {
         self.data.borrow()
     }
 
     pub fn borrow_mut(&self) -> PyResult<PyRefMut<'a, T>> {
         self.data.borrow_mut(self.py)
     }
 
-    /// Returns a leaked reference temporarily held by its management object.
-    ///
-    /// # Safety
-    ///
-    /// It's up to you to make sure that the management object lives
-    /// longer than the leaked reference. Otherwise, you'll get a
-    /// dangling reference.
-    pub unsafe fn leak_immutable(&self) -> PyResult<PyLeakedRef<&'static T>> {
-        let (static_ref, static_state_ref) = self
-            .data
-            .py_shared_state
-            .leak_immutable(self.py, self.data)?;
+    /// Returns a leaked reference.
+    pub fn leak_immutable(&self) -> PyResult<PyLeakedRef<&'static T>> {
+        let state = &self.data.py_shared_state;
+        unsafe {
+            let (static_ref, static_state_ref) =
+                state.leak_immutable(self.py, self.data)?;
         Ok(PyLeakedRef::new(
             self.py,
             self.owner,
             static_ref,
             static_state_ref,
         ))
     }
 }
+}
 
 /// Holds a mutable reference to data shared between Python and Rust.
 pub struct PyRefMut<'a, T> {
     inner: RefMut<'a, T>,
     py_shared_state: &'a PySharedState,
 }
 
 impl<'a, T> PyRefMut<'a, T> {
     // Must be constructed by PySharedState after checking its leak_count.
     // Otherwise, drop() would incorrectly update the state.
     fn new(
         _py: Python<'a>,
         inner: RefMut<'a, T>,
         py_shared_state: &'a PySharedState,
     ) -> Self {
         Self {
             inner,
             py_shared_state,
         }
     }
 }
 
 impl<'a, T> std::ops::Deref for PyRefMut<'a, T> {
     type Target = RefMut<'a, T>;
 
     fn deref(&self) -> &Self::Target {
         &self.inner
     }
 }
 impl<'a, T> std::ops::DerefMut for PyRefMut<'a, T> {
     fn deref_mut(&mut self) -> &mut Self::Target {
         &mut self.inner
     }
 }
 
 impl<'a, T> Drop for PyRefMut<'a, T> {
     fn drop(&mut self) {
         let gil = Python::acquire_gil();
         let py = gil.python();
         unsafe {
             self.py_shared_state.decrease_leak_count(py, true);
         }
     }
 }
 
 /// Allows a `py_class!` generated struct to share references to one of its
 /// data members with Python.
 ///
 /// # Warning
 ///
 /// TODO allow Python container types: for now, integration with the garbage
 ///     collector does not extend to Rust structs holding references to Python
 ///     objects. Should the need surface, `__traverse__` and `__clear__` will
 ///     need to be written as per the `rust-cpython` docs on GC integration.
 ///
 /// # Parameters
 ///
 /// * `$name` is the same identifier used in for `py_class!` macro call.
 /// * `$inner_struct` is the identifier of the underlying Rust struct
 /// * `$data_member` is the identifier of the data member of `$inner_struct`
 /// that will be shared.
 /// * `$shared_accessor` is the function name to be generated, which allows
 /// safe access to the data member.
 ///
 /// # Safety
 ///
 /// `$data_member` must persist while the `$name` object is alive. In other
 /// words, it must be an accessor to a data field of the Python object.
 ///
 /// # Example
 ///
 /// ```
 /// struct MyStruct {
 ///     inner: Vec<u32>;
 /// }
 ///
 /// py_class!(pub class MyType |py| {
 ///     data inner: PySharedRefCell<MyStruct>;
 /// });
 ///
 /// py_shared_ref!(MyType, MyStruct, inner, inner_shared);
 /// ```
 macro_rules! py_shared_ref {
     (
         $name: ident,
         $inner_struct: ident,
         $data_member: ident,
         $shared_accessor: ident
     ) => {
         impl $name {
             /// Returns a safe reference to the shared `$data_member`.
             ///
             /// This function guarantees that `PySharedRef` is created with
             /// the valid `self` and `self.$data_member(py)` pair.
             fn $shared_accessor<'a>(
                 &'a self,
                 py: Python<'a>,
             ) -> $crate::ref_sharing::PySharedRef<'a, $inner_struct> {
                 use cpython::PythonObject;
                 use $crate::ref_sharing::PySharedRef;
                 let owner = self.as_object();
                 let data = self.$data_member(py);
                 unsafe { PySharedRef::new(py, owner, data) }
             }
         }
     };
 }
 
 /// Manage immutable references to `PyObject` leaked into Python iterators.
-///
-/// In truth, this does not represent leaked references themselves;
-/// it is instead useful alongside them to manage them.
 pub struct PyLeakedRef<T> {
-    _inner: PyObject,
-    pub data: Option<T>, // TODO: remove pub
+    inner: PyObject,
+    data: Option<T>,
     py_shared_state: &'static PySharedState,
 }
 
+// DO NOT implement Deref for PyLeakedRef<T>! Dereferencing PyLeakedRef
+// without taking Python GIL wouldn't be safe.
+
 impl<T> PyLeakedRef<T> {
     /// # Safety
     ///
     /// The `py_shared_state` must be owned by the `inner` Python object.
     // Marked as unsafe so client code wouldn't construct PyLeakedRef
     // struct by mistake. Its drop() is unsafe.
     pub unsafe fn new(
         py: Python,
         inner: &PyObject,
         data: T,
         py_shared_state: &'static PySharedState,
     ) -> Self {
         Self {
-            _inner: inner.clone_ref(py),
+            inner: inner.clone_ref(py),
             data: Some(data),
             py_shared_state,
         }
     }
+
+    /// Returns an immutable reference to the inner value.
+    pub fn get_ref<'a>(&'a self, _py: Python<'a>) -> &'a T {
+        self.data.as_ref().unwrap()
+    }
+
+    /// Returns a mutable reference to the inner value.
+    ///
+    /// Typically `T` is an iterator. If `T` is an immutable reference,
+    /// `get_mut()` is useless since the inner value can't be mutated.
+    pub fn get_mut<'a>(&'a mut self, _py: Python<'a>) -> &'a mut T {
+        self.data.as_mut().unwrap()
+    }
+
+    /// Converts the inner value by the given function.
+    ///
+    /// Typically `T` is a static reference to a container, and `U` is an
+    /// iterator of that container.
+    ///
+    /// # Safety
+    ///
+    /// The lifetime of the object passed in to the function `f` is cheated.
+    /// It's typically a static reference, but is valid only while the
+    /// corresponding `PyLeakedRef` is alive. Do not copy it out of the
+    /// function call.
+    pub unsafe fn map<U>(
+        mut self,
+        py: Python,
+        f: impl FnOnce(T) -> U,
+    ) -> PyLeakedRef<U> {
+        // f() could make the self.data outlive. That's why map() is unsafe.
+        // In order to make this function safe, maybe we'll need a way to
+        // temporarily restrict the lifetime of self.data and translate the
+        // returned object back to Something<'static>.
+        let new_data = f(self.data.take().unwrap());
+        PyLeakedRef {
+            inner: self.inner.clone_ref(py),
+            data: Some(new_data),
+            py_shared_state: self.py_shared_state,
+        }
+    }
 }
 
 impl<T> Drop for PyLeakedRef<T> {
     fn drop(&mut self) {
         // py_shared_state should be alive since we do have
         // a Python reference to the owner object. Taking GIL makes
         // sure that the state is only accessed by this thread.
         let gil = Python::acquire_gil();
         let py = gil.python();
+        if self.data.is_none() {
+            return; // moved to another PyLeakedRef
+        }
         unsafe {
             self.py_shared_state.decrease_leak_count(py, false);
         }
     }
 }
 
 /// Defines a `py_class!` that acts as a Python iterator over a Rust iterator.
 ///
 /// TODO: this is a bit awkward to use, and a better (more complicated)
 ///     procedural macro would simplify the interface a lot.
 ///
 /// # Parameters
 ///
 /// * `$name` is the identifier to give to the resulting Rust struct.
 /// * `$leaked` corresponds to `$leaked` in the matching `py_shared_ref!` call.
 /// * `$iterator_type` is the type of the Rust iterator.
 /// * `$success_func` is a function for processing the Rust `(key, value)`
 /// tuple on iteration success, turning it into something Python understands.
 /// * `$success_func` is the return type of `$success_func`
 ///
 /// # Example
 ///
 /// ```
 /// struct MyStruct {
 ///     inner: HashMap<Vec<u8>, Vec<u8>>;
 /// }
 ///
 /// py_class!(pub class MyType |py| {
 ///     data inner: PySharedRefCell<MyStruct>;
 ///
 ///     def __iter__(&self) -> PyResult<MyTypeItemsIterator> {
-///         let mut leak_handle =
-///             unsafe { self.inner_shared(py).leak_immutable()? };
-///         let leaked_ref = leak_handle.data.take().unwrap();
+///         let leaked_ref = self.inner_shared(py).leak_immutable()?;
 ///         MyTypeItemsIterator::from_inner(
 ///             py,
-///             leak_handle,
-///             leaked_ref.iter(),
+///             unsafe { leaked_ref.map(py, |o| o.iter()) },
 ///         )
 ///     }
 /// });
 ///
 /// impl MyType {
 ///     fn translate_key_value(
 ///         py: Python,
 ///         res: (&Vec<u8>, &Vec<u8>),
 ///     ) -> PyResult<Option<(PyBytes, PyBytes)>> {
 ///         let (f, entry) = res;
 ///         Ok(Some((
 ///             PyBytes::new(py, f),
 ///             PyBytes::new(py, entry),
 ///         )))
 ///     }
 /// }
 ///
 /// py_shared_ref!(MyType, MyStruct, inner, MyTypeLeakedRef);
 ///
 /// py_shared_iterator!(
 ///     MyTypeItemsIterator,
-///     PyLeakedRef<&'static MyStruct>,
-///     HashMap<'static, Vec<u8>, Vec<u8>>,
+///     PyLeakedRef<HashMap<'static, Vec<u8>, Vec<u8>>>,
 ///     MyType::translate_key_value,
 ///     Option<(PyBytes, PyBytes)>
 /// );
 /// ```
 macro_rules! py_shared_iterator {
     (
         $name: ident,
         $leaked: ty,
-        $iterator_type: ty,
         $success_func: expr,
         $success_type: ty
     ) => {
         py_class!(pub class $name |py| {
             data inner: RefCell<Option<$leaked>>;
-            data it: RefCell<$iterator_type>;
 
             def __next__(&self) -> PyResult<$success_type> {
                 let mut inner_opt = self.inner(py).borrow_mut();
-                if inner_opt.is_some() {
-                    match self.it(py).borrow_mut().next() {
+                if let Some(leaked) = inner_opt.as_mut() {
+                    match leaked.get_mut(py).next() {
                         None => {
                             // replace Some(inner) by None, drop $leaked
                             inner_opt.take();
                             Ok(None)
                         }
                         Some(res) => {
                             $success_func(py, res)
                         }
                     }
                 } else {
                     Ok(None)
                 }
             }
 
             def __iter__(&self) -> PyResult<Self> {
                 Ok(self.clone_ref(py))
             }
         });
 
         impl $name {
             pub fn from_inner(
                 py: Python,
                 leaked: $leaked,
-                it: $iterator_type
             ) -> PyResult<Self> {
                 Self::create_instance(
                     py,
                     RefCell::new(Some(leaked)),
-                    RefCell::new(it)
                 )
             }
         }
     };
 }