Patchwork [3,of,7,RESEND] rust-cpython: add panicking version of borrow_mut() and use it

login
register
mail settings
Submitter Yuya Nishihara
Date Jan. 28, 2020, 11:50 p.m.
Message ID <cfa21e54a563abc69ff3.1580255429@mimosa>
Download mbox | patch
Permalink /patch/44724/
State Accepted
Headers show

Comments

Yuya Nishihara - Jan. 28, 2020, 11:50 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1570890845 -32400
#      Sat Oct 12 23:34:05 2019 +0900
# Node ID cfa21e54a563abc69ff3989e331db75b175b26a0
# Parent  a1c692f0bbfb6b07180c5caf6bc59cea30e6f050
rust-cpython: add panicking version of borrow_mut() and use it

The original borrow_mut() is renamed to try_borrow_mut().

Since leak_immutable() no longer incref the borrow count, the caller should
know if the underlying value is borrowed or not. No Python world is involved.
That's why we can simply use the panicking borrow_mut().

Patch

diff --git a/rust/hg-cpython/src/dirstate/dirs_multiset.rs b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
@@ -72,7 +72,7 @@  py_class!(pub class Dirs |py| {
     }
 
     def addpath(&self, path: PyObject) -> PyResult<PyObject> {
-        self.inner_shared(py).borrow_mut()?.add_path(
+        self.inner_shared(py).borrow_mut().add_path(
             HgPath::new(path.extract::<PyBytes>(py)?.data(py)),
         ).and(Ok(py.None())).or_else(|e| {
             match e {
@@ -90,7 +90,7 @@  py_class!(pub class Dirs |py| {
     }
 
     def delpath(&self, path: PyObject) -> PyResult<PyObject> {
-        self.inner_shared(py).borrow_mut()?.delete_path(
+        self.inner_shared(py).borrow_mut().delete_path(
             HgPath::new(path.extract::<PyBytes>(py)?.data(py)),
         )
             .and(Ok(py.None()))
diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs
@@ -53,7 +53,7 @@  py_class!(pub class DirstateMap |py| {
     }
 
     def clear(&self) -> PyResult<PyObject> {
-        self.inner_shared(py).borrow_mut()?.clear();
+        self.inner_shared(py).borrow_mut().clear();
         Ok(py.None())
     }
 
@@ -80,7 +80,7 @@  py_class!(pub class DirstateMap |py| {
         size: PyObject,
         mtime: PyObject
     ) -> PyResult<PyObject> {
-        self.inner_shared(py).borrow_mut()?.add_file(
+        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()
@@ -108,7 +108,7 @@  py_class!(pub class DirstateMap |py| {
         oldstate: PyObject,
         size: PyObject
     ) -> PyResult<PyObject> {
-        self.inner_shared(py).borrow_mut()?
+        self.inner_shared(py).borrow_mut()
             .remove_file(
                 HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
                 oldstate.extract::<PyBytes>(py)?.data(py)[0]
@@ -132,7 +132,7 @@  py_class!(pub class DirstateMap |py| {
         f: PyObject,
         oldstate: PyObject
     ) -> PyResult<PyBool> {
-        self.inner_shared(py).borrow_mut()?
+        self.inner_shared(py).borrow_mut()
             .drop_file(
                 HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
                 oldstate.extract::<PyBytes>(py)?.data(py)[0]
@@ -163,7 +163,7 @@  py_class!(pub class DirstateMap |py| {
                 ))
             })
             .collect();
-        self.inner_shared(py).borrow_mut()?
+        self.inner_shared(py).borrow_mut()
             .clear_ambiguous_times(files?, now.extract(py)?);
         Ok(py.None())
     }
@@ -198,7 +198,7 @@  py_class!(pub class DirstateMap |py| {
 
     def hastrackeddir(&self, d: PyObject) -> PyResult<PyBool> {
         let d = d.extract::<PyBytes>(py)?;
-        Ok(self.inner_shared(py).borrow_mut()?
+        Ok(self.inner_shared(py).borrow_mut()
             .has_tracked_dir(HgPath::new(d.data(py)))
             .map_err(|e| {
                 PyErr::new::<exc::ValueError, _>(py, e.to_string())
@@ -208,7 +208,7 @@  py_class!(pub class DirstateMap |py| {
 
     def hasdir(&self, d: PyObject) -> PyResult<PyBool> {
         let d = d.extract::<PyBytes>(py)?;
-        Ok(self.inner_shared(py).borrow_mut()?
+        Ok(self.inner_shared(py).borrow_mut()
             .has_dir(HgPath::new(d.data(py)))
             .map_err(|e| {
                 PyErr::new::<exc::ValueError, _>(py, e.to_string())
@@ -217,7 +217,7 @@  py_class!(pub class DirstateMap |py| {
     }
 
     def parents(&self, st: PyObject) -> PyResult<PyTuple> {
-        self.inner_shared(py).borrow_mut()?
+        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))
@@ -235,13 +235,13 @@  py_class!(pub class DirstateMap |py| {
         let p1 = extract_node_id(py, &p1)?;
         let p2 = extract_node_id(py, &p2)?;
 
-        self.inner_shared(py).borrow_mut()?
+        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()?
+        match self.inner_shared(py).borrow_mut()
             .read(st.extract::<PyBytes>(py)?.data(py))
         {
             Ok(Some(parents)) => Ok(Some(
@@ -268,7 +268,7 @@  py_class!(pub class DirstateMap |py| {
             p2: extract_node_id(py, &p2)?,
         };
 
-        match self.inner_shared(py).borrow_mut()?.pack(parents, now) {
+        match self.inner_shared(py).borrow_mut().pack(parents, now) {
             Ok(packed) => Ok(PyBytes::new(py, &packed)),
             Err(_) => Err(PyErr::new::<exc::OSError, _>(
                 py,
@@ -280,7 +280,7 @@  py_class!(pub class DirstateMap |py| {
     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()
+            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())?;
         }
@@ -336,7 +336,7 @@  py_class!(pub class DirstateMap |py| {
 
     def getdirs(&self) -> PyResult<Dirs> {
         // TODO don't copy, share the reference
-        self.inner_shared(py).borrow_mut()?.set_dirs()
+        self.inner_shared(py).borrow_mut().set_dirs()
             .map_err(|e| {
                 PyErr::new::<exc::ValueError, _>(py, e.to_string())
             })?;
@@ -353,7 +353,7 @@  py_class!(pub class DirstateMap |py| {
     }
     def getalldirs(&self) -> PyResult<Dirs> {
         // TODO don't copy, share the reference
-        self.inner_shared(py).borrow_mut()?.set_all_dirs()
+        self.inner_shared(py).borrow_mut().set_all_dirs()
             .map_err(|e| {
                 PyErr::new::<exc::ValueError, _>(py, e.to_string())
             })?;
@@ -431,7 +431,7 @@  py_class!(pub class DirstateMap |py| {
     ) -> PyResult<PyObject> {
         let key = key.extract::<PyBytes>(py)?;
         let value = value.extract::<PyBytes>(py)?;
-        self.inner_shared(py).borrow_mut()?.copy_map.insert(
+        self.inner_shared(py).borrow_mut().copy_map.insert(
             HgPathBuf::from_bytes(key.data(py)),
             HgPathBuf::from_bytes(value.data(py)),
         );
@@ -445,7 +445,7 @@  py_class!(pub class DirstateMap |py| {
         let key = key.extract::<PyBytes>(py)?;
         match self
             .inner_shared(py)
-            .borrow_mut()?
+            .borrow_mut()
             .copy_map
             .remove(HgPath::new(key.data(py)))
         {
diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
--- a/rust/hg-cpython/src/ref_sharing.rs
+++ b/rust/hg-cpython/src/ref_sharing.rs
@@ -202,7 +202,19 @@  impl<'a, T> PySharedRef<'a, T> {
         self.data.borrow(self.py)
     }
 
-    pub fn borrow_mut(&self) -> PyResult<RefMut<'a, T>> {
+    /// Mutably borrows the wrapped value.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the value is currently borrowed through `PySharedRef`
+    /// or `PyLeaked`.
+    pub fn borrow_mut(&self) -> RefMut<'a, T> {
+        self.try_borrow_mut().expect("already borrowed")
+    }
+
+    /// Mutably borrows the wrapped value, returning an error if the value
+    /// is currently borrowed.
+    pub fn try_borrow_mut(&self) -> PyResult<RefMut<'a, T>> {
         self.data.try_borrow_mut(self.py)
     }
 
@@ -572,7 +584,7 @@  mod test {
         let (gil, owner) = prepare_env();
         let py = gil.python();
         let leaked = owner.string_shared(py).leak_immutable();
-        owner.string_shared(py).borrow_mut().unwrap().clear();
+        owner.string_shared(py).borrow_mut().clear();
         assert!(leaked.try_borrow(py).is_err());
     }
 
@@ -582,7 +594,7 @@  mod test {
         let py = gil.python();
         let leaked = owner.string_shared(py).leak_immutable();
         let mut leaked_iter = unsafe { leaked.map(py, |s| s.chars()) };
-        owner.string_shared(py).borrow_mut().unwrap().clear();
+        owner.string_shared(py).borrow_mut().clear();
         assert!(leaked_iter.try_borrow_mut(py).is_err());
     }
 
@@ -592,40 +604,40 @@  mod test {
         let (gil, owner) = prepare_env();
         let py = gil.python();
         let leaked = owner.string_shared(py).leak_immutable();
-        owner.string_shared(py).borrow_mut().unwrap().clear();
+        owner.string_shared(py).borrow_mut().clear();
         let _leaked_iter = unsafe { leaked.map(py, |s| s.chars()) };
     }
 
     #[test]
-    fn test_borrow_mut_while_leaked_ref() {
+    fn test_try_borrow_mut_while_leaked_ref() {
         let (gil, owner) = prepare_env();
         let py = gil.python();
-        assert!(owner.string_shared(py).borrow_mut().is_ok());
+        assert!(owner.string_shared(py).try_borrow_mut().is_ok());
         let leaked = owner.string_shared(py).leak_immutable();
         {
             let _leaked_ref = leaked.try_borrow(py).unwrap();
-            assert!(owner.string_shared(py).borrow_mut().is_err());
+            assert!(owner.string_shared(py).try_borrow_mut().is_err());
             {
                 let _leaked_ref2 = leaked.try_borrow(py).unwrap();
-                assert!(owner.string_shared(py).borrow_mut().is_err());
+                assert!(owner.string_shared(py).try_borrow_mut().is_err());
             }
-            assert!(owner.string_shared(py).borrow_mut().is_err());
+            assert!(owner.string_shared(py).try_borrow_mut().is_err());
         }
-        assert!(owner.string_shared(py).borrow_mut().is_ok());
+        assert!(owner.string_shared(py).try_borrow_mut().is_ok());
     }
 
     #[test]
-    fn test_borrow_mut_while_leaked_ref_mut() {
+    fn test_try_borrow_mut_while_leaked_ref_mut() {
         let (gil, owner) = prepare_env();
         let py = gil.python();
-        assert!(owner.string_shared(py).borrow_mut().is_ok());
+        assert!(owner.string_shared(py).try_borrow_mut().is_ok());
         let leaked = owner.string_shared(py).leak_immutable();
         let mut leaked_iter = unsafe { leaked.map(py, |s| s.chars()) };
         {
             let _leaked_ref = leaked_iter.try_borrow_mut(py).unwrap();
-            assert!(owner.string_shared(py).borrow_mut().is_err());
+            assert!(owner.string_shared(py).try_borrow_mut().is_err());
         }
-        assert!(owner.string_shared(py).borrow_mut().is_ok());
+        assert!(owner.string_shared(py).try_borrow_mut().is_ok());
     }
 
     #[test]
@@ -638,10 +650,19 @@  mod test {
     }
 
     #[test]
+    fn test_try_borrow_mut_while_borrow() {
+        let (gil, owner) = prepare_env();
+        let py = gil.python();
+        let _ref = owner.string_shared(py).borrow();
+        assert!(owner.string_shared(py).try_borrow_mut().is_err());
+    }
+
+    #[test]
+    #[should_panic(expected = "already borrowed")]
     fn test_borrow_mut_while_borrow() {
         let (gil, owner) = prepare_env();
         let py = gil.python();
         let _ref = owner.string_shared(py).borrow();
-        assert!(owner.string_shared(py).borrow_mut().is_err());
+        owner.string_shared(py).borrow_mut();
     }
 }