-
Notifications
You must be signed in to change notification settings - Fork 9
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
Bind to uniffi using proc macros #29
base: master
Are you sure you want to change the base?
Conversation
799459f
to
8424d3f
Compare
I was unnecessarily wrapping some types where a uniffi custom type would do, so I fixed that along with the There's also a complexity where an enum apparently needs to be defined both in Rust and UDL that's wasn't addressed in the initial commit but is now. I think that's the only place where moving to proc macros creates more rather than less complexity:
|
I'm realizing now I also didn't do a direct translation of OutPoint. It became an Object instead of a Record, a breaking change. I've converted to draft until that is resolved |
I am also unsure if the proc macros support doc strings yet. Whether or not |
Proc macros let rust-analyzer check that types are compatible at rust compile time, and thus let downstream projects more easily depend on bitcoin-ffi. The `Network` enum is defined in the UDL file, but we need to define it in rust with `uniffi::Enum` in order to check UniFFI bindings at rust compile time. "It is permitted to use this [`uniffi::Enum`] macro on a type that is also defined in the UDL file as long as the two definitions are equal in the names and ordering of variants and variant fields" see: https://mozilla.github.io/uniffi-rs/latest/proc_macro/index.html#the-uniffienum-derive
8424d3f
to
24a53bc
Compare
Separate `uniffi::custom_type` and `impl_string_custom_typedef` calls were made to define uniffi String types. This combines them into one macro.
I think OutPoint as a Record (aka dictionary) is addressed now. Does UDL support docstrings? |
Yes, but let's discuss if that warrants the UDL in an issue because my assumption is the primary consumers of this library will be other Rust projects where docs exist |
Regarding the source of the concern about whether or not proc macros can export docstringsWhere does this concern come from? I don't' see docstrings being used in the replaced UDL. #14's concern about documentation is very different from mine if that's where this concern comes from.
I don't think this has anything to do with docstring production and is rather telling implementers to read the UDL documentation and not only the proc macro documentation to understand how to apply proc macros. Regarding the actual capability
Here's the PR that did it referenced in the UniFFI v0.26 changelog: mozilla/uniffi-rs#1862 To be fair, I haven't tested the capability using proc macros but in addition to being documented, and code being available e to export docstrings with the proc macros as-is, the UDL file this replaces does not appear to contain any docstrings |
@@ -53,8 +50,8 @@ impl Address { | |||
|
|||
pub fn is_valid_for_network(&self, network: Network) -> bool { |
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.
Did we miss a macro here?
#[derive(Clone, Debug)] | ||
pub struct FeeRate(pub BitcoinFeeRate); | ||
#[derive(Clone, Debug, uniffi::Object)] | ||
pub struct FeeRate(pub bitcoin::FeeRate); |
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.
While I like the look of using bitcoin::
more, maybe this could be done in a follow up?
I don't know where I got the notion that macros simply do not support docstrings then. Obviously this is much better as the UDL has no LSP support and is a huge pain to format and navigate. Are there any known limitations of the macros at the moment? I think we should just use them if not |
|
||
#[derive(Clone, Default, uniffi::Enum)] | ||
#[non_exhaustive] | ||
pub enum Network { |
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.
If we have to copy and paste enums from rust-bitcoin
into here just to call uniffi::Enum
I can see that getting annoying. Is it possible to use the UDL for direct exports of rust-bitcoin
and nothing else?
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.
See my comment in the first commit
"It is permitted to use this [
uniffi::Enum
] macro on a type that is
also defined in the UDL file as long as the two definitions are equal in
the names and ordering of variants and variant fields"
see: https://mozilla.github.io/uniffi-rs/latest/proc_macro/index.html#the-uniffienum-derive
I think it's possible to only use the UDL, but then it needs to manually be imported using uniffi::use_udl_enum!(dependent_crate, EnumType); but repeating the pure enums (not the errors, for some reason) seems to be a better trade off. It's detailed in this issue @thunderbiscuit made in uniffi-rs
This is the only macro limitation that frustrates me. I prefer ProcMacros in every other circumstance and dealing with this trade off is worth it to me.
UniFFI Procedural Macros "allows you to define your function signatures and type definitions directly in your Rust code, avoiding the need to duplicate them in a UDL file and so avoiding the possibility for the two to get out of sync." They also let rust-analyzer check that types are compatible
at rust compile time, and thus let downstream projects more easily depend on bitcoin-ffi to produce their own uniffi.
A couple types needed explicit wrappers in order to use macros.
Seems like they require less code overall, as well.
I also took the opportunity to get rid of the requirement for specific aliasing of types in
impl_from_*
declarative macros (e.g.use bitcoin::Address as BitcoinAddress
) in the second commit.Depending on
bitcoin-ffi
this way seems to work with payjoin-ffi downstream. Please let me know if I've interrupted the build process at all. I couldn't figure out exactly what needed to be checked other than my instinct to usecargo build
from the README