-
Notifications
You must be signed in to change notification settings - Fork 729
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
Use PyMethodsImpl instead of *ProtocolImpl::methods #917
Conversation
6738955
to
b4f247f
Compare
@@ -17,76 +17,7 @@ struct MyClass { | |||
The above example generates implementations for [`PyTypeInfo`], [`PyTypeObject`], | |||
and [`PyClass`] for `MyClass`. | |||
|
|||
Specifically, the following implementation is generated: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this code block to the last of the page.
It's too long for a user who doesn't have a strong interest in the implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, good to see ~400 lines reduction total!
One step closer to stable Rust 🎉
pyo3-derive-backend/src/pyclass.rs
Outdated
/// To allow multiple #[pymethods] block, we define inventory types. | ||
fn impl_methods_inventory(cls: &syn::Ident) -> TokenStream { | ||
// Try to build a unique type for better error messages | ||
let name = format!("Pyo3MethodsInventoryFor{}", cls); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the comment "The orphan rule disallows using a generic inventory struct..."
It might be possible now to achieve this, as Rust 1.41.0
relaxed orphan rules slightly: https://blog.rust-lang.org/2020/01/30/Rust-1.41.0.html#relaxed-restrictions-when-implementing-traits
So maybe we can have Pyo3MethodsInventory<MyClass>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the above change made possible is impl<T> ForeignTrait<LocalType> for ForeignType<T>
.
impl inventory::Collect for PyO3MethodsInventory<MyClass>
is still impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep you're correct 👍
@@ -57,53 +57,52 @@ fn impl_proto_impl( | |||
ty: &syn::Type, | |||
impls: &mut Vec<syn::ImplItem>, | |||
proto: &defs::Proto, | |||
) -> TokenStream { | |||
let mut tokens = TokenStream::new(); | |||
) -> syn::Result<TokenStream> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
ml_doc: "" | ||
}) | ||
} | ||
// TODO(kngwyu): doc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
src/class/methods.rs
Outdated
} | ||
|
||
/// Implementation detail. Only to be used through the proc macros. | ||
/// Similar to `PyProtoMethodsImpl`, but for `#[pyproto]`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Similar to" looks wrong as it's listing the same type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/pyclass.rs
Outdated
@@ -73,7 +75,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 + PyMethodsImpl | |||
PyTypeInfo<Layout = PyCell<Self>> + Sized + PyClassAlloc + PyMethodsImpl + PyProtoMethodsImpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand - why is it necessary to make a new PyProtoMethodsImpl
rather than using PyMethodsImpl
for all of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically we can reuse PyMethodsImpl
but I felt it easier to understand to have two inventory types corresponding to #[pymethods]
and #[pyproto]
.
Not so string reason, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this isn't a user-facing API, I guess the only people who need to understand it are us. I probably have a preference for just one inventory type so that there's less code overall to understand, but happy if you prefer to keep it as two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed PyProtoMethodsImpl
.
b4f247f
to
efbb414
Compare
efbb414
to
4f8bc0d
Compare
4f8bc0d
to
69dba08
Compare
To reduce specialization.
We have two kinds of protocol methods.
mp_length
to enablelen(obj)
.PyMethods
in our proc-macro implementation.For the later one, we don't need to use specialization. We can use the same trick as
#[pymethods]
.#897 should be merged before this PR.
I'll add CHANGELOG later to make rebasing easier.