Renew #[pyproto] implementation for abi3 branch #1254

kngwyu · 2020-10-20T16:24:11Z

So what's the problem?
Currently, on the master, #[pyproto] is implemented via 'table initialization'.
I.e., #[pyproto] generates basically such a function:

#[ctor] 
fn __table_initialization() {
    let mut methods = PyNumberMethods::default();
    methods.set_add::<MyClass>();
    <MyClass as HasProtoRegistory>::add_number_methods(methods);  // I don't remember the function name correctly...
}

I designed this since many protocols required such kinds of tables before.
However, when we are to use ABI3, most protocols are initialized via PyType_Slot and tables are unnecessary.
So I decided to renew the whole design, using Vec<PyType_Slot> instead of tables.
I believe the new implementation is simpler but has one disadvantage: weak typing.
PyType_Slot casts function pointers to *mut c_void so the function type is eliminated, which can lead to bugs. We should carefully check the slot and pfunc of each PyType_Slot are correct.

Now #[pyproto] generates (roughly):

pyo3::inventory::submit!( {
    let mut proto_methods = vec![];
    proto_methods.push(<MyClass as PyNumberSlots>::get_add());
    proto_methods.into()
});

alex · 2020-10-20T16:31:41Z

I only skimmed it, but the design makes sense to me.

davidhewitt · 2020-10-21T08:07:21Z

👍 looks like a good idea! I've started reviewing but need more time to finish reading. I have a very busy day today so tomorrow is more likely I'm afraid.

davidhewitt

Sorry for the delay in reviewing. Looks good to me. I have a couple of suggestions.

I believe the new implementation is simpler but has one disadvantage: weak typing.
PyType_Slot casts function pointers to *mut c_void so the function type is eliminated, which can lead to bugs. We should carefully check the slot and pfunc of each PyType_Slot are correct.

How about adding an enum so that when we put the proto methods in the inventory we still have strong types? See my other comment.

Also, I think we should be able to remove the special case for the buffer methods and just use slots for them too.

davidhewitt · 2020-10-21T08:03:50Z

pyo3-derive-backend/src/pyproto.rs

+        tokens.push(quote! { let mut proto_methods = vec![]; });
+        for getter in proto.slot_getters(method_names) {
+            let get = syn::Ident::new(getter, Span::call_site());
+            tokens.push(quote! { proto_methods.push(<#ty as #ext_trait>::#get()); });
+        }


How about this form, which is easier to read imo:

Suggested change

tokens.push(quote! { let mut proto_methods = vec![]; });

for getter in proto.slot_getters(method_names) {

let get = syn::Ident::new(getter, Span::call_site());

tokens.push(quote! { proto_methods.push(<#ty as #ext_trait>::#get()); });

}

let getters = proto.slot_getters(method_names).map(|getter| {

let get = syn::Ident::new(getter, Span::call_site());

quote!(<#ty as #ext_trait>::#get())

});

tokens.push(quote! { let proto_methods = vec![ $(#getters),* ]; });

(and similar for the buffer method tokens just above, but I think that we can probably remove those completely.)

davidhewitt · 2020-10-21T12:13:56Z

src/class/proto_methods.rs

-    }
-    fn basic_methods() -> Option<NonNull<PyObjectMethods>> {
-        None
+    fn get_type_slots() -> Vec<ffi::PyType_Slot> {


Instead of storing ffi::PyType_Slot in this inventory, what about storing an enum:

pub enum PyProtoMethodDef { #[cfg(not(Py_LIMITED_API))], GetBuffer(ffi::getbufferproc), #[cfg(not(Py_LIMITED_API))], SetBuffer(ffi::setbufferproc), SetItem(ffi::ssizeobjargproc), DelItem(ffi::ssizeobjargproc), }

This would allow us to keep typing inside the inventory, and avoid the special case for the buffer methods in the macros.

We could do the conversion to ffi::PyType_Slot inside get_type_slots().

Also this would work very nicely with the buffer, where slot definitions do exist as long as it is not limited API. https://github.com/python/cpython/blob/cb115e36e1aba04b90b0ecac6f043e60064ac65b/Include/typeslots.h#L7-L8

Also this would work very nicely with the buffer, where slot definitions do exist as long as it is not limited API. https://github.com/python/cpython/blob/cb115e36e1aba04b90b0ecac6f043e60064ac65b/Include/typeslots.h#L7-L8

Nice catch, thanks!

davidhewitt · 2020-10-21T12:15:19Z

src/pyclass.rs

+    if let Some(buffer) = T::get_buffer() {
        unsafe {
-            (*(*type_object).tp_as_buffer).bf_getbuffer = buffer.as_ref().bf_getbuffer;
-            (*(*type_object).tp_as_buffer).bf_releasebuffer = buffer.as_ref().bf_releasebuffer;
+            (*(*type_object).tp_as_buffer).bf_getbuffer = buffer.bf_getbuffer;
+            (*(*type_object).tp_as_buffer).bf_releasebuffer = buffer.bf_releasebuffer;
        }
    }


I think we can remove this special case now, because buffer methods do have slot definitions (when not limited abi). See my other comment.

kngwyu · 2020-10-25T09:24:00Z

Thanks for the review!

How about adding an enum so that when we put the proto methods in the inventory we still have strong types?

This sounds the same as the current approach, and if so, it does not address the code complexity issue.
I think what we need is adding some assertion before we cast a function pointer to *mut c_void. I'd rather try to implement this using a macro and ask your review again.

kngwyu · 2020-10-25T09:42:10Z

Oh, I just recalled that type slots does not work for buffer... (#1132 (comment))

davidhewitt

Oh, I just recalled that type slots does not work for buffer... (#1132 (comment))

👍 let's leave it as-is for this PR. I think it might work for 3.9 - I'll experiment in a follow-up PR sometime.

This sounds the same as the current approach, and if so, it does not address the code complexity issue.
I think what we need is adding some assertion before we cast a function pointer to *mut c_void. I'd rather try to implement this using a macro and ask your review again.

Fair point. See my new suggestion in comments below, which I think can reduce LOC quite a bit.

davidhewitt · 2020-10-25T10:30:40Z

pyo3-derive-backend/src/pyproto.rs

+        tokens.push(quote! { let mut proto_methods = vec![]; });
+        for getter in proto.slot_getters(method_names) {
+            let get = syn::Ident::new(getter, Span::call_site());
+            tokens.push(quote! { proto_methods.push(<#ty as #ext_trait>::#get()); });


Maybe we can do the creation of PyType_Slot objects and conversion to *mut void here?

Then the slot getters can return well-typed function pointers.

Suggested change

tokens.push(quote! { proto_methods.push(<#ty as #ext_trait>::#get()); });

tokens.push(quote! { proto_methods.push(pyo3::ffi::PyType_Slot {

slot: #slot,

pfunc: <#ty as #ext_trait>::#get() as _

}); });

We would need to move the #slot definitions into the proc macro. I think that could be a good idea anyway to help keep the definitions in one place.

Sounds nice 👍 !

davidhewitt · 2020-10-25T10:33:18Z

src/class/basic.rs

-    // Set functions used by `#[pyproto]`.
-    pub fn set_str<T>(&mut self)
+pub trait PyBasicSlots {
+    fn get_str() -> ffi::PyType_Slot


If you take my suggestion above then this should just return the typed function pointer:

Suggested change

fn get_str() -> ffi::PyType_Slot

fn get_str() -> ffi::reprfunc {

py_unary_func!(PyObjectStrProtocol, Self::__str__)

}

And same for all other methods. I think this'll reduce the LOC quite a lot!

kngwyu · 2020-10-26T15:46:37Z

Chaned to use struct TypedSlot<T: Sized>(pub std::os::raw::c_int, pub T) as internal representation of ffi::PyType_Slot to resolve the weak typing problem.

I also tried to slot: #slot that @davidhewitt mentioned, but found myself hard to write "ffi::Py_*"s without code completion.

davidhewitt

Nice, LGTM!

kngwyu added 2 commits October 21, 2020 01:13

Renew PyProtoMethods for new ABI3-based type construction

eb8ff15

Renew proc-macros for new #[pyproto] backend

6627658

kngwyu force-pushed the abi3-new-proto branch from e3af6ca to 6627658 Compare October 20, 2020 16:29

alex approved these changes Oct 24, 2020

View reviewed changes

davidhewitt reviewed Oct 24, 2020

View reviewed changes

davidhewitt reviewed Oct 25, 2020

View reviewed changes

Use TypedSlot as internal representation of ffi::PyType_Slot

16ad3bf

davidhewitt approved these changes Oct 26, 2020

View reviewed changes

kngwyu merged commit 95bec25 into abi3 Oct 27, 2020

kngwyu deleted the abi3-new-proto branch October 27, 2020 03:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renew #[pyproto] implementation for abi3 branch #1254

Renew #[pyproto] implementation for abi3 branch #1254

kngwyu commented Oct 20, 2020 •

edited

Loading

alex commented Oct 20, 2020

davidhewitt commented Oct 21, 2020

davidhewitt left a comment

davidhewitt Oct 21, 2020

davidhewitt Oct 24, 2020 •

edited

Loading

davidhewitt Oct 21, 2020

kngwyu Oct 25, 2020

davidhewitt Oct 21, 2020

kngwyu commented Oct 25, 2020

kngwyu commented Oct 25, 2020

davidhewitt left a comment

davidhewitt Oct 25, 2020 •

edited

Loading

kngwyu Oct 26, 2020

davidhewitt Oct 25, 2020

kngwyu commented Oct 26, 2020 •

edited

Loading

davidhewitt left a comment

-            tokens.push(quote! { proto_methods.push(<#ty as #ext_trait>::#get()); });
+            tokens.push(quote! { proto_methods.push(pyo3::ffi::PyType_Slot {
+                slot: #slot,
+                pfunc: <#ty as #ext_trait>::#get() as _
+            }); });

-    fn get_str() -> ffi::PyType_Slot
+    fn get_str() -> ffi::reprfunc {
+        py_unary_func!(PyObjectStrProtocol, Self::__str__)
+    }

Renew #[pyproto] implementation for abi3 branch #1254

Renew #[pyproto] implementation for abi3 branch #1254

Conversation

kngwyu commented Oct 20, 2020 • edited Loading

alex commented Oct 20, 2020

davidhewitt commented Oct 21, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Oct 21, 2020

Choose a reason for hiding this comment

davidhewitt Oct 24, 2020 • edited Loading

Choose a reason for hiding this comment

davidhewitt Oct 21, 2020

Choose a reason for hiding this comment

kngwyu Oct 25, 2020

Choose a reason for hiding this comment

davidhewitt Oct 21, 2020

Choose a reason for hiding this comment

kngwyu commented Oct 25, 2020

kngwyu commented Oct 25, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Oct 25, 2020 • edited Loading

Choose a reason for hiding this comment

kngwyu Oct 26, 2020

Choose a reason for hiding this comment

davidhewitt Oct 25, 2020

Choose a reason for hiding this comment

kngwyu commented Oct 26, 2020 • edited Loading

davidhewitt left a comment

Choose a reason for hiding this comment

kngwyu commented Oct 20, 2020 •

edited

Loading

davidhewitt Oct 24, 2020 •

edited

Loading

davidhewitt Oct 25, 2020 •

edited

Loading

kngwyu commented Oct 26, 2020 •

edited

Loading