Patchwork [1,of,9] rust-cpython: rename PyLeakedRef to PyLeaked

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

Comments

Yuya Nishihara - Oct. 19, 2019, 10:07 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1570875051 -32400
#      Sat Oct 12 19:10:51 2019 +0900
# Node ID da6ae3bc571609faeb2bd6696efc7869b0420f6a
# Parent  cfe3e9159c6c4eb5b7775a7b813f7f22f55a1f88
rust-cpython: rename PyLeakedRef to PyLeaked

This series will make PyLeaked* behave more like a Python iterator, which
means mutation of the owner object will be allowed and the leaked reference
(i.e. the iterator) will be invalidated instead.

I'll add PyLeakedRef/PyLeakedRefMut structs which will represent a "borrowed"
state, and prevent the underlying value from being mutably borrowed while the
leaked reference is in use:

  let shared = self.inner_shared(py);
  let leaked = shared.leak_immutable();
  {
      let leaked_ref: PyLeakedRef<_> = leaked.borrow(py);
      shared.borrow_mut();  // panics since the underlying value is borrowed
  }
  shared.borrow_mut();  // allowed

The relation between PyLeaked* structs is quite similar to RefCell/Ref/RefMut,
but the implementation can't be reused because the borrowing state will have
to be shared across objects having no lifetime relation.

PyLeaked isn't named as PyLeakedCell since it isn't actually a cell in that
leaked.borrow_mut() will require &mut self.
Raphaël Gomès - Oct. 19, 2019, 8:28 p.m.
Thanks a lot for this series Yuya. This reference-sharing business gets 
a lot better every time.

We should maybe think about bringing Georges and Mark along for the ride 
for the (near) future upstreaming, don't you think?

On 10/19/19 12:07 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1570875051 -32400
> #      Sat Oct 12 19:10:51 2019 +0900
> # Node ID da6ae3bc571609faeb2bd6696efc7869b0420f6a
> # Parent  cfe3e9159c6c4eb5b7775a7b813f7f22f55a1f88
> rust-cpython: rename PyLeakedRef to PyLeaked
>
> This series will make PyLeaked* behave more like a Python iterator, which
> means mutation of the owner object will be allowed and the leaked reference
> (i.e. the iterator) will be invalidated instead.
>
> I'll add PyLeakedRef/PyLeakedRefMut structs which will represent a "borrowed"
> state, and prevent the underlying value from being mutably borrowed while the
> leaked reference is in use:
>
>    let shared = self.inner_shared(py);
>    let leaked = shared.leak_immutable();
>    {
>        let leaked_ref: PyLeakedRef<_> = leaked.borrow(py);
>        shared.borrow_mut();  // panics since the underlying value is borrowed
>    }
>    shared.borrow_mut();  // allowed
>
> The relation between PyLeaked* structs is quite similar to RefCell/Ref/RefMut,
> but the implementation can't be reused because the borrowing state will have
> to be shared across objects having no lifetime relation.
>
> PyLeaked isn't named as PyLeakedCell since it isn't actually a cell in that
> leaked.borrow_mut() will require &mut self.
>
> diff --git a/rust/hg-cpython/src/dirstate/copymap.rs b/rust/hg-cpython/src/dirstate/copymap.rs
> --- a/rust/hg-cpython/src/dirstate/copymap.rs
> +++ b/rust/hg-cpython/src/dirstate/copymap.rs
> @@ -12,7 +12,7 @@ use cpython::{PyBytes, PyClone, PyDict,
>   use std::cell::RefCell;
>   
>   use crate::dirstate::dirstate_map::DirstateMap;
> -use crate::ref_sharing::PyLeakedRef;
> +use crate::ref_sharing::PyLeaked;
>   use hg::{utils::hg_path::HgPathBuf, CopyMapIter};
>   
>   py_class!(pub class CopyMap |py| {
> @@ -104,14 +104,14 @@ impl CopyMap {
>   
>   py_shared_iterator!(
>       CopyMapKeysIterator,
> -    PyLeakedRef<CopyMapIter<'static>>,
> +    PyLeaked<CopyMapIter<'static>>,
>       CopyMap::translate_key,
>       Option<PyBytes>
>   );
>   
>   py_shared_iterator!(
>       CopyMapItemsIterator,
> -    PyLeakedRef<CopyMapIter<'static>>,
> +    PyLeaked<CopyMapIter<'static>>,
>       CopyMap::translate_key_value,
>       Option<(PyBytes, PyBytes)>
>   );
> 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
> @@ -17,7 +17,7 @@ use cpython::{
>   };
>   
>   use crate::dirstate::extract_dirstate;
> -use crate::ref_sharing::{PyLeakedRef, PySharedRefCell};
> +use crate::ref_sharing::{PyLeaked, PySharedRefCell};
>   use hg::{
>       utils::hg_path::{HgPath, HgPathBuf},
>       DirsMultiset, DirsMultisetIter, DirstateMapError, DirstateParseError,
> @@ -123,7 +123,7 @@ impl Dirs {
>   
>   py_shared_iterator!(
>       DirsMultisetKeysIterator,
> -    PyLeakedRef<DirsMultisetIter<'static>>,
> +    PyLeaked<DirsMultisetIter<'static>>,
>       Dirs::translate_key,
>       Option<PyBytes>
>   );
> 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
> @@ -20,7 +20,7 @@ use cpython::{
>   use crate::{
>       dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
>       dirstate::{dirs_multiset::Dirs, make_dirstate_tuple},
> -    ref_sharing::{PyLeakedRef, PySharedRefCell},
> +    ref_sharing::{PyLeaked, PySharedRefCell},
>   };
>   use hg::{
>       utils::hg_path::{HgPath, HgPathBuf},
> @@ -483,14 +483,14 @@ py_shared_ref!(DirstateMap, RustDirstate
>   
>   py_shared_iterator!(
>       DirstateMapKeysIterator,
> -    PyLeakedRef<StateMapIter<'static>>,
> +    PyLeaked<StateMapIter<'static>>,
>       DirstateMap::translate_key,
>       Option<PyBytes>
>   );
>   
>   py_shared_iterator!(
>       DirstateMapItemsIterator,
> -    PyLeakedRef<StateMapIter<'static>>,
> +    PyLeaked<StateMapIter<'static>>,
>       DirstateMap::translate_key_value,
>       Option<(PyBytes, PyObject)>
>   );
> 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
> @@ -186,12 +186,12 @@ impl<'a, T> PySharedRef<'a, T> {
>       }
>   
>       /// Returns a leaked reference.
> -    pub fn leak_immutable(&self) -> PyResult<PyLeakedRef<&'static T>> {
> +    pub fn leak_immutable(&self) -> PyResult<PyLeaked<&'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(
> +            Ok(PyLeaked::new(
>                   self.py,
>                   self.owner,
>                   static_ref,
> @@ -307,16 +307,16 @@ macro_rules! py_shared_ref {
>   }
>   
>   /// Manage immutable references to `PyObject` leaked into Python iterators.
> -pub struct PyLeakedRef<T> {
> +pub struct PyLeaked<T> {
>       inner: PyObject,
>       data: Option<T>,
>       py_shared_state: &'static PySharedState,
>   }
>   
> -// DO NOT implement Deref for PyLeakedRef<T>! Dereferencing PyLeakedRef
> +// DO NOT implement Deref for PyLeaked<T>! Dereferencing PyLeaked
>   // without taking Python GIL wouldn't be safe.
>   
> -impl<T> PyLeakedRef<T> {
> +impl<T> PyLeaked<T> {
>       /// # Safety
>       ///
>       /// The `py_shared_state` must be owned by the `inner` Python object.
> @@ -355,19 +355,19 @@ impl<T> PyLeakedRef<T> {
>       ///
>       /// 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
> +    /// corresponding `PyLeaked` 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> {
> +    ) -> PyLeaked<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 {
> +        PyLeaked {
>               inner: self.inner.clone_ref(py),
>               data: Some(new_data),
>               py_shared_state: self.py_shared_state,
> @@ -375,7 +375,7 @@ impl<T> PyLeakedRef<T> {
>       }
>   }
>   
> -impl<T> Drop for PyLeakedRef<T> {
> +impl<T> Drop for PyLeaked<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
> @@ -383,7 +383,7 @@ impl<T> Drop for PyLeakedRef<T> {
>           let gil = Python::acquire_gil();
>           let py = gil.python();
>           if self.data.is_none() {
> -            return; // moved to another PyLeakedRef
> +            return; // moved to another PyLeaked
>           }
>           self.py_shared_state.decrease_leak_count(py, false);
>       }
> @@ -439,7 +439,7 @@ impl<T> Drop for PyLeakedRef<T> {
>   ///
>   /// py_shared_iterator!(
>   ///     MyTypeItemsIterator,
> -///     PyLeakedRef<HashMap<'static, Vec<u8>, Vec<u8>>>,
> +///     PyLeaked<HashMap<'static, Vec<u8>, Vec<u8>>>,
>   ///     MyType::translate_key_value,
>   ///     Option<(PyBytes, PyBytes)>
>   /// );
Yuya Nishihara - Oct. 20, 2019, 2:54 a.m.
On Sat, 19 Oct 2019 22:28:55 +0200, Raphaël Gomès wrote:
> We should maybe think about bringing Georges and Mark along for the ride
> for the (near) future upstreaming, don't you think?

The next batch will fix a couple of nits in our ref_sharing code. And then,
I'll probably start upstreaming it.

Patch

diff --git a/rust/hg-cpython/src/dirstate/copymap.rs b/rust/hg-cpython/src/dirstate/copymap.rs
--- a/rust/hg-cpython/src/dirstate/copymap.rs
+++ b/rust/hg-cpython/src/dirstate/copymap.rs
@@ -12,7 +12,7 @@  use cpython::{PyBytes, PyClone, PyDict, 
 use std::cell::RefCell;
 
 use crate::dirstate::dirstate_map::DirstateMap;
-use crate::ref_sharing::PyLeakedRef;
+use crate::ref_sharing::PyLeaked;
 use hg::{utils::hg_path::HgPathBuf, CopyMapIter};
 
 py_class!(pub class CopyMap |py| {
@@ -104,14 +104,14 @@  impl CopyMap {
 
 py_shared_iterator!(
     CopyMapKeysIterator,
-    PyLeakedRef<CopyMapIter<'static>>,
+    PyLeaked<CopyMapIter<'static>>,
     CopyMap::translate_key,
     Option<PyBytes>
 );
 
 py_shared_iterator!(
     CopyMapItemsIterator,
-    PyLeakedRef<CopyMapIter<'static>>,
+    PyLeaked<CopyMapIter<'static>>,
     CopyMap::translate_key_value,
     Option<(PyBytes, PyBytes)>
 );
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
@@ -17,7 +17,7 @@  use cpython::{
 };
 
 use crate::dirstate::extract_dirstate;
-use crate::ref_sharing::{PyLeakedRef, PySharedRefCell};
+use crate::ref_sharing::{PyLeaked, PySharedRefCell};
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
     DirsMultiset, DirsMultisetIter, DirstateMapError, DirstateParseError,
@@ -123,7 +123,7 @@  impl Dirs {
 
 py_shared_iterator!(
     DirsMultisetKeysIterator,
-    PyLeakedRef<DirsMultisetIter<'static>>,
+    PyLeaked<DirsMultisetIter<'static>>,
     Dirs::translate_key,
     Option<PyBytes>
 );
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
@@ -20,7 +20,7 @@  use cpython::{
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
     dirstate::{dirs_multiset::Dirs, make_dirstate_tuple},
-    ref_sharing::{PyLeakedRef, PySharedRefCell},
+    ref_sharing::{PyLeaked, PySharedRefCell},
 };
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
@@ -483,14 +483,14 @@  py_shared_ref!(DirstateMap, RustDirstate
 
 py_shared_iterator!(
     DirstateMapKeysIterator,
-    PyLeakedRef<StateMapIter<'static>>,
+    PyLeaked<StateMapIter<'static>>,
     DirstateMap::translate_key,
     Option<PyBytes>
 );
 
 py_shared_iterator!(
     DirstateMapItemsIterator,
-    PyLeakedRef<StateMapIter<'static>>,
+    PyLeaked<StateMapIter<'static>>,
     DirstateMap::translate_key_value,
     Option<(PyBytes, PyObject)>
 );
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
@@ -186,12 +186,12 @@  impl<'a, T> PySharedRef<'a, T> {
     }
 
     /// Returns a leaked reference.
-    pub fn leak_immutable(&self) -> PyResult<PyLeakedRef<&'static T>> {
+    pub fn leak_immutable(&self) -> PyResult<PyLeaked<&'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(
+            Ok(PyLeaked::new(
                 self.py,
                 self.owner,
                 static_ref,
@@ -307,16 +307,16 @@  macro_rules! py_shared_ref {
 }
 
 /// Manage immutable references to `PyObject` leaked into Python iterators.
-pub struct PyLeakedRef<T> {
+pub struct PyLeaked<T> {
     inner: PyObject,
     data: Option<T>,
     py_shared_state: &'static PySharedState,
 }
 
-// DO NOT implement Deref for PyLeakedRef<T>! Dereferencing PyLeakedRef
+// DO NOT implement Deref for PyLeaked<T>! Dereferencing PyLeaked
 // without taking Python GIL wouldn't be safe.
 
-impl<T> PyLeakedRef<T> {
+impl<T> PyLeaked<T> {
     /// # Safety
     ///
     /// The `py_shared_state` must be owned by the `inner` Python object.
@@ -355,19 +355,19 @@  impl<T> PyLeakedRef<T> {
     ///
     /// 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
+    /// corresponding `PyLeaked` 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> {
+    ) -> PyLeaked<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 {
+        PyLeaked {
             inner: self.inner.clone_ref(py),
             data: Some(new_data),
             py_shared_state: self.py_shared_state,
@@ -375,7 +375,7 @@  impl<T> PyLeakedRef<T> {
     }
 }
 
-impl<T> Drop for PyLeakedRef<T> {
+impl<T> Drop for PyLeaked<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
@@ -383,7 +383,7 @@  impl<T> Drop for PyLeakedRef<T> {
         let gil = Python::acquire_gil();
         let py = gil.python();
         if self.data.is_none() {
-            return; // moved to another PyLeakedRef
+            return; // moved to another PyLeaked
         }
         self.py_shared_state.decrease_leak_count(py, false);
     }
@@ -439,7 +439,7 @@  impl<T> Drop for PyLeakedRef<T> {
 ///
 /// py_shared_iterator!(
 ///     MyTypeItemsIterator,
-///     PyLeakedRef<HashMap<'static, Vec<u8>, Vec<u8>>>,
+///     PyLeaked<HashMap<'static, Vec<u8>, Vec<u8>>>,
 ///     MyType::translate_key_value,
 ///     Option<(PyBytes, PyBytes)>
 /// );