FFI callbacks and vtables #1818

bendk · 2023-10-30T14:36:35Z

This PR updates the callback interface code by adding new FfiType variants:

FfiType::Callback is a callback function.
FfiType::Struct is a repr(C) struct of FfiType fields.
FfiType::Reference is a pointer/reference to another FfiType.

FfiType::Callback and FfiType::Struct store a name that corresponds to a callback/struct definition that the foreign bindings render. IOW, we can now define callbacks/structs in the ComponentInterface, then refer to them by name as ffi types.

These types are used to replace the callback interface code. Instead of having the foreign code register a FfiType::ForeignCallback, it now registers a reference to a vtable: a struct where each field is a callback that implements one of the callback interface methods, plus a field for the uniffi_free method.

The primary motivation for this is to prepare async callback methods. I think those methods should input an extra argument that's a callback to call when the async method completes. However, that presents a couple challenges to the current system:

That callback would be generic on the FfiType, which is a new thing that could get very messy. This code will allow us to handle the monomorphization in the ComponentInterface code and keep the bindings generators simple.
Currently all arguments are passed in a RustBuffer, but I don't want to read a function pointer out of that. For one, it feels like too much transmuting to me and I don't like making too many assumptions about function pointers. Secondly, it seems like it's mixing abstractions. The read operation should be used for Type values, not for FfiType. VTables give us the flexibility to avoid this since we can just add a new FfiType parameter for async methods only.

I think this change will also improve our lives in general. It's currently pretty tedious to add a new callback type or refactor an existing one. I think this system will make that easier with a lot less fiddling around to get the signatures and FFI types correct for each language. Also, the concept of vtables opens up some interesting possibilities and if we had that before the initial async work it could have simplified some things.

mhammond

This is a large patch, but it makes a lof of sense and makes callbacks more first-class instead of bolted on. I think you need a CHANGELOG entry for the maintainers of other bindings, and we should avoid landing this until 0.26 is (nearly?) out the door, but it looks great, thanks!

mhammond · 2023-12-31T04:49:21Z

uniffi_macros/src/export/callback_interface.rs

-        static #internals_ident: ::uniffi::ForeignCallbackInternals = ::uniffi::ForeignCallbackInternals::new();
+        struct #vtable_type {
+            #(#vtable_fields)*
+            uniffi_dec_ref: extern "C" fn(handle: u64),


maybe uniffi_drop here too?

dec_ref takes the place of drop now.

Oops, I thought you were suggesting to add a uniffi_drop field, but now I realize you were probably just saying that the name doesn't make sense which is true. I'll rename this to uniffi_drop.

less "doesn't make sense" and more "would be more consistent and clearer" but yeah, it was all about the name.

I went with uniffi_free, which matches the name in the foreign code and uniffi_core.

uniffi_bindgen/src/interface/ffi.rs

uniffi_bindgen/src/bindings/python/gen_python/mod.rs

mhammond · 2023-12-31T05:52:42Z

uniffi_bindgen/src/bindings/ruby/gen_ruby/mod.rs

+            FfiType::Callback(_) => unimplemented!("FFI Callbacks not implemented"),
+            // Note: references are not really used by the Ruby bindings yet, maybe this should
+            // change to a different type when they are actually implemented.
+            FfiType::Reference(_) => ":pointer".to_string(),


just unimplemented like the above?

Unfortunately, the tests fail if it's unimplemented and this is the easiest way to make them pass. I updated the docstring to explain the situation better.

uniffi_bindgen/src/bindings/python/templates/Helpers.py

uniffi_bindgen/src/interface/ffi.rs

fixtures/keywords/kotlin/src/lib.rs

This type can be used whenever we need to define a callback functions. Rather than adding the type by hand, which was quite annoying when I was doing the async work, we now can just add an item in `ffi_callback_definitions()`. This also can help avoid FFI type mismatches, like how the Kotlin signature used to an i16 instead of an i8 by accident. Use the new type for the future continuation callback. I have another plan for callback interface callbacks. I didn't touch the foreign executor callback, I hope to work on that very soon.

Instead of registering a single method to implement a callback interface, foreign code now registers a VTable. VTable sounds fancy, but it's just a repr(C) struct where each field is a callback function. This gives us some more flexibility with the method signatures. Before, all arguments were passed using a RustBuffer, but not all FFI types can be written to a RustBuffer. In particular, I want to be able to pass callback function pointers. This also makes the callback interface FFI closer to the Rust one. I wanted to make it match exactly, but it didn't work out. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (mozilla#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. Removed the reexport-scaffolding-macro fixture. I don't think this one is giving us a lot of value anymore and I don't want to keep updating it when the FFI changes.

bendk · 2024-01-25T15:40:50Z

I think this one is ready now, officially re-requesting review.

mhammond

🚢

mhammond · 2024-01-25T18:32:20Z

uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt

@@ -304,8 +304,8 @@ open class {{ impl_class_name }}: Disposable, AutoCloseable, {{ interface_name }
 }

 {%- if obj.has_callback_interface() %}
-{%- let callback_handler_class = format!("UniffiCallbackInterface{}", name) %}
-{%- let callback_handler_obj = format!("uniffiCallbackInterface{}", name) %}
+{%- let vtable = obj.vtable().expect("trait interface should have a vtable") %}


this appears unused in this file, but is used in CallbackInterfaceTempl - maybe move the let there?

It depends on obj though, so I don't think I can move it. I think of this as ObjectTemplate.kt passing arguments to CallbalkInterfaceImplTemplate.kt

bendk requested a review from a team as a code owner October 30, 2023 14:36

bendk requested review from jhugman and mhammond and removed request for a team October 30, 2023 14:36

bendk mentioned this pull request Oct 30, 2023

Adding FfiType::Callback #1814

Closed

bendk force-pushed the ffi-vtables branch 5 times, most recently from 27ff2f8 to cfdb66b Compare November 1, 2023 18:01

bendk force-pushed the ffi-vtables branch from cfdb66b to bdd4017 Compare November 9, 2023 17:37

bendk force-pushed the ffi-vtables branch 2 times, most recently from 3b63929 to da19f25 Compare November 22, 2023 22:03

bendk force-pushed the ffi-vtables branch 4 times, most recently from 2803505 to 06801af Compare December 11, 2023 17:30

bendk mentioned this pull request Dec 11, 2023

Always use uniffi_macros to generate callback interface scaffolding #1894

Merged

bendk force-pushed the ffi-vtables branch 4 times, most recently from 4be2eed to 5308aa0 Compare December 13, 2023 01:00

bendk mentioned this pull request Dec 13, 2023

Async blocking task queue #1837

Open

bendk force-pushed the ffi-vtables branch 2 times, most recently from 6610dfc to 9f3a7d5 Compare December 18, 2023 16:07

bendk mentioned this pull request Dec 21, 2023

0.26.0 Release #1911

Closed

2 tasks

mhammond approved these changes Jan 1, 2024

View reviewed changes

bendk force-pushed the ffi-vtables branch 3 times, most recently from 835efe0 to 880cbcf Compare January 24, 2024 23:04

bendk force-pushed the ffi-vtables branch 6 times, most recently from a9054a1 to f9b3be4 Compare January 25, 2024 14:53

bendk added 2 commits January 25, 2024 10:32

bendk force-pushed the ffi-vtables branch from f9b3be4 to af04dd0 Compare January 25, 2024 15:40

bendk requested a review from mhammond January 25, 2024 15:40

mhammond approved these changes Jan 25, 2024

View reviewed changes

bendk merged commit 26a2fef into mozilla:main Jan 25, 2024
5 checks passed

mgeisler mentioned this pull request Feb 26, 2024

[uniffi] Add support for custom GroupStateStorage interface awslabs/mls-rs#86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FFI callbacks and vtables #1818

FFI callbacks and vtables #1818

bendk commented Oct 30, 2023 •

edited

Loading

mhammond left a comment

mhammond Dec 31, 2023

bendk Jan 24, 2024

bendk Jan 25, 2024

mhammond Jan 25, 2024

bendk Jan 25, 2024

mhammond Dec 31, 2023

bendk Jan 24, 2024

bendk commented Jan 25, 2024

mhammond left a comment

mhammond Jan 25, 2024

bendk Jan 25, 2024

FFI callbacks and vtables #1818

FFI callbacks and vtables #1818

Conversation

bendk commented Oct 30, 2023 • edited Loading

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk commented Jan 25, 2024

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk commented Oct 30, 2023 •

edited

Loading