Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify PyMethodsInventoryDispatch and PyMethodsProtocol #889

Merged
merged 2 commits into from
May 2, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions guide/src/class.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ impl pyo3::IntoPy<PyObject> for MyClass {
}
}

pub struct MyClassGeneratedPyo3Inventory {
pub struct Pyo3MethodsInventoryForMyClass {
methods: &'static [pyo3::class::PyMethodDefType],
}

impl pyo3::class::methods::PyMethodsInventory for MyClassGeneratedPyo3Inventory {
impl pyo3::class::methods::PyMethodsInventory for Pyo3MethodsInventoryForMyClass {
fn new(methods: &'static [pyo3::class::PyMethodDefType]) -> Self {
Self { methods }
}
Expand All @@ -77,11 +77,11 @@ impl pyo3::class::methods::PyMethodsInventory for MyClassGeneratedPyo3Inventory
}
}

impl pyo3::class::methods::PyMethodsInventoryDispatch for MyClass {
type InventoryType = MyClassGeneratedPyo3Inventory;
impl pyo3::class::methods::PyMethodsImpl for MyClass {
type Methods = Pyo3MethodsInventoryForMyClass;
}

pyo3::inventory::collect!(MyClassGeneratedPyo3Inventory);
pyo3::inventory::collect!(Pyo3MethodsInventoryForMyClass);
# let gil = Python::acquire_gil();
# let py = gil.python();
# let cls = py.get_type::<MyClass>();
Expand Down
8 changes: 4 additions & 4 deletions pyo3-derive-backend/src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ fn parse_descriptors(item: &mut syn::Field) -> syn::Result<Vec<FnType>> {
fn impl_inventory(cls: &syn::Ident) -> TokenStream {
// Try to build a unique type that gives a hint about it's function when
// it comes up in error messages
let name = cls.to_string() + "GeneratedPyo3Inventory";
let name = format!("Pyo3MethodsInventoryFor{}", cls);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let name = format!("Pyo3MethodsInventoryFor{}", cls);
let name = format!("PyMethodsOf{}", cls);

Longer name is also fine, just wondered if I could find a shorter one which worked.

Copy link
Member Author

@kngwyu kngwyu May 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a generated struct, I think it's better if a user can understand that this is generated from the name if he or she uses cargo expand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. PyO3GeneratedMethodsFor{} ? Your choice of name is also fine.

let inventory_cls = syn::Ident::new(&name, Span::call_site());

quote! {
Expand All @@ -234,8 +234,8 @@ fn impl_inventory(cls: &syn::Ident) -> TokenStream {
}
}

impl pyo3::class::methods::PyMethodsInventoryDispatch for #cls {
type InventoryType = #inventory_cls;
impl pyo3::class::methods::PyMethodsImpl for #cls {
type Methods = #inventory_cls;
}

pyo3::inventory::collect!(#inventory_cls);
Expand Down Expand Up @@ -461,7 +461,7 @@ fn impl_descriptors(
Ok(quote! {
pyo3::inventory::submit! {
#![crate = pyo3] {
type ClsInventory = <#cls as pyo3::class::methods::PyMethodsInventoryDispatch>::InventoryType;
type ClsInventory = <#cls as pyo3::class::methods::PyMethodsImpl>::Methods;
<ClsInventory as pyo3::class::methods::PyMethodsInventory>::new(&[#(#py_methods),*])
}
}
Expand Down
2 changes: 1 addition & 1 deletion pyo3-derive-backend/src/pyimpl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub fn impl_methods(ty: &syn::Type, impls: &mut Vec<syn::ImplItem>) -> syn::Resu
Ok(quote! {
pyo3::inventory::submit! {
#![crate = pyo3] {
type TyInventory = <#ty as pyo3::class::methods::PyMethodsInventoryDispatch>::InventoryType;
type TyInventory = <#ty as pyo3::class::methods::PyMethodsImpl>::Methods;
<TyInventory as pyo3::class::methods::PyMethodsInventory>::new(&[#(
#(#cfg_attributes)*
#methods
Expand Down
29 changes: 8 additions & 21 deletions src/class/methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,7 @@ impl PySetterDef {
}
}

#[doc(hidden)] // Only to be used through the proc macros, use PyMethodsProtocol in custom code
/// This trait is implemented for all pyclass so to implement the [PyMethodsProtocol]
/// through inventory
pub trait PyMethodsInventoryDispatch {
/// This allows us to get the inventory type when only the pyclass is in scope
type InventoryType: PyMethodsInventory;
}

#[doc(hidden)] // Only to be used through the proc macros, use PyMethodsProtocol in custom code
#[doc(hidden)] // Only to be used through the proc macros
/// Allows arbitrary pymethod blocks to submit their methods, which are eventually collected by pyclass
pub trait PyMethodsInventory: inventory::Collect {
/// Create a new instance
Expand All @@ -133,20 +125,15 @@ pub trait PyMethodsInventory: inventory::Collect {
fn get_methods(&self) -> &'static [PyMethodDefType];
}

/// The implementation of this trait defines which methods a Python type has.
///
/// For pyclass derived structs this is implemented by collecting all impl blocks through inventory
pub trait PyMethodsProtocol {
/// Returns all methods that are defined for a class
fn py_methods() -> Vec<&'static PyMethodDefType>;
}
#[doc(hidden)] // Only to be used through the proc macros
/// For pyclass derived structs, this trait collects method from all impl blocks using inventory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I put attributes after doc? Maybe it doesn't work for #[doc(hidden)]

Suggested change
#[doc(hidden)] // Only to be used through the proc macros
/// For pyclass derived structs, this trait collects method from all impl blocks using inventory.
/// For pyclass derived structs, this trait collects method from all impl blocks using inventory.
#[doc(hidden)] // Only to be used through the proc macros

pub trait PyMethodsImpl {
davidhewitt marked this conversation as resolved.
Show resolved Hide resolved
/// Normal methods, mainly defined by `#[pymethod]`.
type Methods: PyMethodsInventory;

impl<T> PyMethodsProtocol for T
where
T: PyMethodsInventoryDispatch,
{
/// Returns all methods that are defined for a class
fn py_methods() -> Vec<&'static PyMethodDefType> {
inventory::iter::<T::InventoryType>
inventory::iter::<Self::Methods>
.into_iter()
.flat_map(PyMethodsInventory::get_methods)
.collect()
Expand Down
8 changes: 4 additions & 4 deletions src/pyclass.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! `PyClass` trait
use crate::class::methods::{PyMethodDefType, PyMethodsProtocol};
use crate::class::methods::{PyMethodDefType, PyMethodsImpl};
use crate::pyclass_slots::{PyClassDict, PyClassWeakRef};
use crate::type_object::{type_flags, PyLayout};
use crate::{class, ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python};
Expand Down Expand Up @@ -71,7 +71,7 @@ pub unsafe fn tp_free_fallback(obj: *mut ffi::PyObject) {
/// The `#[pyclass]` attribute automatically implements this trait for your Rust struct,
/// so you don't have to use this trait directly.
pub trait PyClass:
PyTypeInfo<Layout = PyCell<Self>> + Sized + PyClassAlloc + PyMethodsProtocol
PyTypeInfo<Layout = PyCell<Self>> + Sized + PyClassAlloc + PyMethodsImpl
{
/// Specify this class has `#[pyclass(dict)]` or not.
type Dict: PyClassDict;
Expand Down Expand Up @@ -215,7 +215,7 @@ fn py_class_flags<T: PyTypeInfo>(type_object: &mut ffi::PyTypeObject) {
}
}

fn py_class_method_defs<T: PyMethodsProtocol>() -> (
fn py_class_method_defs<T: PyMethodsImpl>() -> (
Option<ffi::newfunc>,
Option<ffi::PyCFunctionWithKeywords>,
Vec<ffi::PyMethodDef>,
Expand Down Expand Up @@ -274,7 +274,7 @@ fn py_class_async_methods<T>(defs: &mut Vec<ffi::PyMethodDef>) {
}
}

fn py_class_properties<T: PyMethodsProtocol>() -> Vec<ffi::PyGetSetDef> {
fn py_class_properties<T: PyMethodsImpl>() -> Vec<ffi::PyGetSetDef> {
let mut defs = std::collections::HashMap::new();

for def in T::py_methods() {
Expand Down