Proc macro callbacks #1573
Merged
Proc macro callbacks #1573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bendk
force-pushed
the
proc-macro-callbacks
branch
from
e0ad5da to
7c2f05e
Compare
bendk
commented
Jun 2, 2023
jplatte
reviewed
Jun 5, 2023
uniffi_bindgen/src/interface/mod.rs
Outdated
| @@ -218,6 +218,42 @@ impl ComponentInterface { | |||
| self.errors.get(name) | |||
| } | |||
|
|
|||
| /// Get an Enum definition by name or panic | |||
| pub fn get_enum_definition_unchecked(&self, name: &str) -> &Enum { | |||
There was a problem hiding this comment.
"unchecked" is generally used for unsafe functions that have undefined behavior in the unexpected case. But maybe it's fine to use the same naming here.
uniffi_macros/src/export/item.rs
Outdated
Comment on lines
86
to
90
| let sig = FnSignature::new_constructor(self_ident.clone(), impl_fn.sig)?; | ||
| if sig.is_async { | ||
| return sig.syn_err("Async constructors are not supported"); | ||
| } | ||
| ImplItem::Constructor(sig) |
There was a problem hiding this comment.
Why is the is_async check not part of new_constructor? Also, having a syn_err method seems kind of weird to me.
CHANGELOG.md
Outdated
| - Methods implemented by standard Rust traits, such as `Debug`, `Display`, `Eq` and `Hash` can now be exposed over the FFI and bindings may implement special methods for them. | ||
| See [the documentation](https://mozilla.github.io/uniffi-rs/udl/interfaces.html#exposing-methods-from-standard-rust-traits). | ||
| - Implemented proc-macro callback inteface support |
There was a problem hiding this comment.
typo: inteface -> interface
docs/manual/src/proc_macro/index.md
Outdated
| @@ -228,6 +228,56 @@ fn do_http_request() -> Result<(), MyApiError> { | |||
| } | |||
| ``` | |||
|
|
|||
| ## The `#[uniffi::callback_interface]` attribute | |||
|
|
|||
| `#[uniffi::callback_interface]` can be used to export a [callback interface](../udl/callback_interfaces.html) definition | |||
There was a problem hiding this comment.
Missing period at the end of the sentence.
docs/manual/src/proc_macro/index.md
Outdated
Comment on lines
237
to
238
| #[uniffi::callback_interface] | ||
| pub trait Person { |
There was a problem hiding this comment.
Hm, why a separate macro? I think #[uniffi::export] or (#[uniffi::export(callback_interface)] if trait declarations can have a different meaning to UniFFI) would be more "natural", but of course that's very subjective.
There was a problem hiding this comment.
This was a very incisive comment and made me rethink a lot of how the code was organized. I updated the code so not only does it share the same macro name, but the underlying uniffi macro code works more similarly. In particular, callback interface traits and non-callback interface traits work very similarly which I think is a big win.
bendk
force-pushed
the
proc-macro-callbacks
branch
from
7c2f05e to
3893479
Compare
jplatte
reviewed
Jun 14, 2023
docs/manual/src/proc_macro/index.md
Outdated
| // string name(); | ||
| // u32 age(); | ||
| // } | ||
| } |
There was a problem hiding this comment.
Extraneous closing brace.
This will easier to share between the template and macro code.
Added a method to `FfiConverter` for this. The default implementation panics. If we see the `uniffi::callback_error` attribute, then we generate a function that uses the `From<UnexpectedUniFFICallbackError>` impl. That `uniffi::callback_error` attribute is set by the template code if it sees that an error is used in a callback interface. This is needed for callback interface support for proc-macros.
This allows for nicer error messages when the lookup fails.
Moved the `FnSignature` type into its own module and added more
functionality to it. I'm hoping to reuse this for callback interface
methods.
Cleaned up and simplified `export::scaffolding`. I've been hoping to do
this for a while and now seemed like a good time.
This is mostly just a refactor, but a few things changed:
- Scaffolding functions now use the argument names rather than `argN`.
I found it easier to code this way and I don't think we can ever
really support nameless arguments since we need to use something on
the foreign bindings side.
- The error handling now uses `Error::new` rather than
`Error::new_spanned`. I don't think this will hurt the diagnostics
much, but if we wanted to we could go back to `new_spanned()`, we
would just need to clone the `syn::FnArg`.
I used a separate module in the `proc-macro` fixture, which worked really well with `cargo expand` to see how the code was being generated. The trickiest part here is that the order of the methods matter, since it's index is the methods list is part of the API contract. To deal with that I updated the `uniffi_meta` code to order the metadata items and ensured that the methods are processed in the correct order.
- Move the async check to the `FnSignature::new_constructor()` function - Removed the syn_err function - Moved the `get_*_definitions` functions to a shared filters module. This allows the templates to use them without the unwrap() but avoids the awkward `_unchecked` names. - Documentation fixes
This brings the concepts closer together and should make it easier to allow trait interfaces to be created from foreign objects (mozilla#1578). - Refactored `get_or_insert_object()` to just be `get_object()`. Ordered the metadata enum so that object definitions always come before methods. - Removed the `Result` return from `add_known_type()`, since it couldn't fail anyways.
This makes `export` the one way to export an interface definition, which seems good. It also brings the callback inteface code closer to the trait interafce code, which is good because we want to allow creating trait objects from callback interface objects.
bendk
force-pushed
the
proc-macro-callbacks
branch
from
3893479 to
7d5c4f8
Compare
Co-authored-by: Jonas Platte <[email protected]>
jplatte
approved these changes
Jun 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This took a lot longer than I expected, but I think it's working now. I ended up refactoring a lot of related code, hopefully for the better but tell me if I made anything worse.