Patchwork [7,of,9] rust-cpython: drop manual management of mutably_borrowed

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 19, 2019, 10:07 a.m.
Message ID <80179a0d5152dd7ca902.1571479644@mimosa>
Download mbox | patch
Permalink /patch/42500/
State New
Headers show

Comments

Yuya Nishihara - Oct. 19, 2019, 10:07 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1570280349 14400
#      Sat Oct 05 08:59:09 2019 -0400
# Node ID 80179a0d5152dd7ca90227ba5218c55795f1b647
# Parent  6f77f433dd27ae979bcebdd356b611c413c26195
rust-cpython: drop manual management of mutably_borrowed

RefCell::borrow() should guarantee there's no mutable borrow.

As a follow up, maybe PySharedState can be a pure data structure + function.
Most ref-sharing business has already been moved to PySharedRef* and PyLeaked*.

Patch

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
@@ -24,7 +24,7 @@ 
 
 use crate::exceptions::AlreadyBorrowed;
 use cpython::{exc, PyClone, PyErr, PyObject, PyResult, Python};
-use std::cell::{Cell, Ref, RefCell, RefMut};
+use std::cell::{Ref, RefCell, RefMut};
 use std::ops::{Deref, DerefMut};
 use std::sync::atomic::{AtomicUsize, Ordering};
 
@@ -48,7 +48,6 @@  use std::sync::atomic::{AtomicUsize, Ord
 ///   `generation` at the time of `leak_immutable()`.
 #[derive(Debug, Default)]
 struct PySharedState {
-    mutably_borrowed: Cell<bool>,
     // The counter variable could be Cell<usize> since any operation on
     // PySharedState is synchronized by the GIL, but being "atomic" makes
     // PySharedState inherently Sync. The ordering requirement doesn't
@@ -57,26 +56,14 @@  struct PySharedState {
     generation: AtomicUsize,
 }
 
-// &PySharedState can be Send because any access to inner cells is
-// synchronized by the GIL.
-unsafe impl Sync for PySharedState {}
-
 impl PySharedState {
     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.current_borrow_count(py) {
             0 => {
-                self.mutably_borrowed.replace(true);
                 // Note that this wraps around to the same value if mutably
                 // borrowed more than usize::MAX times, which wouldn't happen
                 // in practice.
@@ -100,16 +87,9 @@  impl PySharedState {
     /// extended. Do not call this function directly.
     unsafe fn leak_immutable<T>(
         &self,
-        py: Python,
+        _py: Python,
         data: Ref<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",
-            ));
-        }
         let ptr: *const T = &*data;
         let state_ptr: *const PySharedState = self;
         Ok((&*ptr, &*state_ptr))
@@ -130,20 +110,6 @@  impl PySharedState {
         assert!(prev_count > 0);
     }
 
-    /// # Safety
-    ///
-    /// It's up to you to make sure the reference is about to be deleted
-    /// when updating the leak count.
-    fn decrease_leak_count(&self, py: Python, mutable: bool) {
-        if mutable {
-            assert_eq!(self.current_borrow_count(py), 0);
-            assert!(self.mutably_borrowed.get());
-            self.mutably_borrowed.replace(false);
-        } else {
-            unimplemented!();
-        }
-    }
-
     fn current_generation(&self, _py: Python) -> usize {
         self.generation.load(Ordering::Relaxed)
     }
@@ -262,23 +228,19 @@  impl<'a, T> PySharedRef<'a, T> {
 
 /// Holds a mutable reference to data shared between Python and Rust.
 pub struct PyRefMut<'a, T> {
-    py: Python<'a>,
     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>,
+        _py: Python<'a>,
         inner: RefMut<'a, T>,
-        py_shared_state: &'a PySharedState,
+        _py_shared_state: &'a PySharedState,
     ) -> Self {
         Self {
-            py,
             inner,
-            py_shared_state,
         }
     }
 }
@@ -296,12 +258,6 @@  impl<'a, T> std::ops::DerefMut for PyRef
     }
 }
 
-impl<'a, T> Drop for PyRefMut<'a, T> {
-    fn drop(&mut self) {
-        self.py_shared_state.decrease_leak_count(self.py, true);
-    }
-}
-
 /// Allows a `py_class!` generated struct to share references to one of its
 /// data members with Python.
 ///