cpython-ext: add ExtractInner trait

Summary:
A common pattern in Mercurial's data storage layer Python bindings is to have a Python object that wraps a Rust object. These Python objects are often passed across the FFI boundary to Rust code, which then may need to access the underlying Rust value.

Previously, the objects that used this pattern did so in an ad-hoc manner, typically by providing an `into_inner` or `to_inner` inherent method. This diff introduces a new `ExtractInner` trait that standardizes this pattern into a single interface, which in turn allows this pattern to be used with generics.

Reviewed By: quark-zju

Differential Revision: D22429347

fbshipit-source-id: cab4c24b8b98c6ef8307f72a9b4726aabdc829cc

eden/scm/edenscmnative/bindings/modules/pyrevisionstore/src/lib.rs

@@ -20,7 +20,7 @@ use anyhow::{format_err, Error};
 use cpython::*;
 use parking_lot::RwLock;
 
-use cpython_ext::{PyErr, PyNone, PyPath, PyPathBuf, ResultPyErrExt, Str};
+use cpython_ext::{ExtractInner, PyErr, PyNone, PyPath, PyPathBuf, ResultPyErrExt, Str};
 use pyconfigparser::config;
 use revisionstore::{
     repack, ContentStore, ContentStoreBuilder, CorruptionPolicy, DataPack, DataPackStore,
@@ -93,7 +93,8 @@ fn repack_py(
     shared: bool,
     config: config,
 ) -> PyResult<PyNone> {
-    let stores = stores.map(|(content, metadata)| (content.to_inner(py), metadata.to_inner(py)));
+    let stores =
+        stores.map(|(content, metadata)| (content.extract_inner(py), metadata.extract_inner(py)));
 
     let kind = if full {
         RepackKind::Full
@@ -481,6 +482,14 @@ py_class!(pub class mutabledeltastore |py| {
     }
 });
 
+impl ExtractInner for mutabledeltastore {
+    type Inner = Arc<dyn HgIdMutableDeltaStore>;
+
+    fn extract_inner(&self, py: Python) -> Self::Inner {
+        self.store(py).clone()
+    }
+}
+
 impl HgIdDataStore for mutabledeltastore {
     fn get(&self, key: &Key) -> Result<Option<Vec<u8>>> {
         let gil = Python::acquire_gil();
@@ -566,6 +575,14 @@ py_class!(pub class mutablehistorystore |py| {
     }
 });
 
+impl ExtractInner for mutablehistorystore {
+    type Inner = Arc<dyn HgIdMutableHistoryStore>;
+
+    fn extract_inner(&self, py: Python) -> Self::Inner {
+        self.store(py).clone()
+    }
+}
+
 impl HgIdHistoryStore for mutablehistorystore {
     fn get_node_info(&self, key: &Key) -> Result<Option<NodeInfo>> {
         let gil = Python::acquire_gil();
@@ -763,8 +780,10 @@ py_class!(pub class pyremotestore |py| {
     }
 });
 
-impl pyremotestore {
-    fn into_inner(&self, py: Python) -> Arc<PyHgIdRemoteStore> {
+impl ExtractInner for pyremotestore {
+    type Inner = Arc<PyHgIdRemoteStore>;
+
+    fn extract_inner(&self, py: Python) -> Self::Inner {
         self.remote(py).clone()
     }
 }
@@ -773,7 +792,7 @@ py_class!(pub class contentstore |py| {
     data store: Arc<ContentStore>;
 
     def __new__(_cls, path: Option<PyPathBuf>, config: config, remote: pyremotestore, memcache: Option<memcachestore>) -> PyResult<contentstore> {
-        let remotestore = remote.into_inner(py);
+        let remotestore = remote.extract_inner(py);
         let config = config.get_cfg(py);
 
         let mut builder = ContentStoreBuilder::new(&config).remotestore(remotestore);
@@ -785,7 +804,7 @@ py_class!(pub class contentstore |py| {
         };
 
         builder = if let Some(memcache) = memcache {
-            builder.memcachestore(memcache.into_inner(py))
+            builder.memcachestore(memcache.extract_inner(py))
         } else {
             builder
         };
@@ -850,8 +869,10 @@ py_class!(pub class contentstore |py| {
     }
 });
 
-impl contentstore {
-    pub fn to_inner(&self, py: Python) -> Arc<ContentStore> {
+impl ExtractInner for contentstore {
+    type Inner = Arc<ContentStore>;
+
+    fn extract_inner(&self, py: Python) -> Self::Inner {
         self.store(py).clone()
     }
 }
@@ -860,7 +881,7 @@ py_class!(class metadatastore |py| {
     data store: Arc<MetadataStore>;
 
     def __new__(_cls, path: Option<PyPathBuf>, config: config, remote: pyremotestore, memcache: Option<memcachestore>) -> PyResult<metadatastore> {
-        let remotestore = remote.into_inner(py);
+        let remotestore = remote.extract_inner(py);
         let config = config.get_cfg(py);
 
         let mut builder = MetadataStoreBuilder::new(&config).remotestore(remotestore);
@@ -872,7 +893,7 @@ py_class!(class metadatastore |py| {
         };
 
         builder = if let Some(memcache) = memcache {
-            builder.memcachestore(memcache.into_inner(py))
+            builder.memcachestore(memcache.extract_inner(py))
         } else {
             builder
         };
@@ -905,8 +926,10 @@ py_class!(class metadatastore |py| {
     }
 });
 
-impl metadatastore {
-    pub fn to_inner(&self, py: Python) -> Arc<MetadataStore> {
+impl ExtractInner for metadatastore {
+    type Inner = Arc<MetadataStore>;
+
+    fn extract_inner(&self, py: Python) -> Self::Inner {
         self.store(py).clone()
     }
 }
@@ -921,8 +944,10 @@ py_class!(pub class memcachestore |py| {
     }
 });
 
-impl memcachestore {
-    fn into_inner(&self, py: Python) -> Arc<MemcacheStore> {
+impl ExtractInner for memcachestore {
+    type Inner = Arc<MemcacheStore>;
+
+    fn extract_inner(&self, py: Python) -> Self::Inner {
         self.memcache(py).clone()
     }
 }

eden/scm/edenscmnative/bindings/modules/pyworker/src/lib.rs

@@ -21,7 +21,7 @@ use bytes::Bytes;
 use cpython::*;
 use crossbeam::channel::{bounded, Receiver, Sender};
 
-use cpython_ext::{PyNone, PyPath, PyPathBuf, ResultPyErrExt, Str};
+use cpython_ext::{ExtractInner, PyNone, PyPath, PyPathBuf, ResultPyErrExt, Str};
 use pyrevisionstore::contentstore;
 use revisionstore::{ContentStore, HgIdDataStore};
 use types::{HgId, Key, RepoPath, RepoPathBuf};
@@ -228,7 +228,7 @@ py_class!(class writerworker |py| {
     data inner: RefCell<Option<Worker<(usize, Vec<(RepoPathBuf, Option<UpdateFlag>)>), (RepoPathBuf, HgId, Option<UpdateFlag>)>>>;
 
     def __new__(_cls, contentstore: contentstore, root: &PyPath, numthreads: usize) -> PyResult<writerworker> {
-        let store = contentstore.to_inner(py);
+        let store = contentstore.extract_inner(py);
         let root = root.to_path_buf();
         let writer_state = WriterState::new(root, store).map_pyerr(py)?;
 

eden/scm/lib/cpython-ext/src/extract.rs

@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This software may be used and distributed according to the terms of the
+ * GNU General Public License version 2.
+ */
+
+use cpython::Python;
+
+/// Trait for extracting a Rust object from a Python wrapper class.
+///
+/// A common pattern when writing bindings for Rust objects is to define
+/// a Python wrapper class using the `py_class!` macro, with the underlying
+/// Rust object stored as a data field within the Python object.
+///
+/// When Rust code interacts with a Python wrapper, it may want to work
+/// with the underlying Rust object directly. This trait provides a means
+/// to do so. Note that the `extract_inner` methods takes `&self`, meaning
+/// the inner Rust value cannot be moved out of the wrapper. As a result,
+/// the inner value will typically be wrapped in something like an `Arc`.
+pub trait ExtractInner {
+    type Inner;
+
+    fn extract_inner(&self, py: Python) -> Self::Inner;
+}

eden/scm/lib/cpython-ext/src/lib.rs

@@ -9,6 +9,7 @@ mod bytearrayobject;
 mod bytes;
 mod bytesobject;
 pub mod error;
+mod extract;
 mod io;
 mod none;
 mod path;
@@ -20,6 +21,7 @@ mod str;
 pub use crate::bytearrayobject::{boxed_slice_to_pyobj, vec_to_pyobj};
 pub use crate::bytesobject::allocate_pybytes;
 pub use crate::error::{format_py_error, AnyhowResultExt, PyErr, ResultPyErrExt};
+pub use crate::extract::ExtractInner;
 pub use crate::io::{wrap_pyio, wrap_rust_write, PyRustWrite, WrappedIO};
 pub use crate::none::PyNone;
 pub use crate::path::{Error, PyPath, PyPathBuf};