Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanGould
Copy link

@DanGould DanGould commented Nov 13, 2024

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 use cargo build from the README

@DanGould
Copy link
Author

I was unnecessarily wrapping some types where a uniffi custom type would do, so I fixed that along with the cargo fmt bug

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:

"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

@DanGould DanGould marked this pull request as draft November 15, 2024 20:14
@DanGould
Copy link
Author

DanGould commented Nov 15, 2024

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

@rustaceanrob
Copy link
Contributor

I am also unsure if the proc macros support doc strings yet. Whether or not bitcoin-ffi should have doc strings might want to be opened as an issue.

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
Separate `uniffi::custom_type` and `impl_string_custom_typedef`
calls were made to define uniffi String types. This combines them
into one macro.
@DanGould DanGould marked this pull request as ready for review November 15, 2024 20:32
@DanGould
Copy link
Author

I think OutPoint as a Record (aka dictionary) is addressed now. Does UDL support docstrings?

@rustaceanrob
Copy link
Contributor

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

@DanGould
Copy link
Author

Regarding the source of the concern about whether or not proc macros can export docstrings

Where 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.

Further, using this capability probably means you still need to refer to the UDL documentation, because at this time, that documentation tends to conflate the UniFFI type model and the description of how foreign bindings use that type model. For example, the documentation for a UDL interface describes both how it is defined in UDL and how Swift and Kotlin might use that interface. The latter is relevant even if you define the interface using proc-macros instead of in UDL.

-- https://mozilla.github.io/uniffi-rs/0.27/proc_macro/index.html

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

#[uniffi::export] does appear to call down to this extract_docstring function that extracts them from Rust code and includes them in generated bindings.

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 {
Copy link
Contributor

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);
Copy link
Contributor

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?

@rustaceanrob
Copy link
Contributor

#[uniffi::export] does appear to call down to this extract_docstring function that extracts them from Rust code and includes them in generated bindings.

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 {
Copy link
Contributor

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?

Copy link
Author

@DanGould DanGould Nov 16, 2024

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.

@thunderbiscuit
Copy link
Member

Hey @DanGould! Just wanted to say the migration to proc_macros is very interesting, and I do want to look into it and potentially migrate things if all functionality is there. I have not been ignoring this unintentionally.

At the moment it's a little too much to aim for that for the 1.0 release, so I'm putting the migration to proc_macros on the sidelines a little until that's out (we're pushing to release the final 1.0 before the end of 2024 btw).

Some questions I want to explore:

  • I see that the docstrings are indeed apparently ported. There was talk from Mozilla of it not working for some time (they merged the UDL docstrings first) so that's where you got that idea @rustaceanrob, but I can see they even have a test-fixture for it here so I expect it to work. Question: will the docstrings from bitcoin-ffi types appear in our API docs for bdk-ffi?
  • Can we use/mix types exported via proc_macro in bitcoin-ffi in the bdk-ffi library (if the bdk-ffi is still on UDL), or do we need to migrate everything at once?
  • Does the migration involve slightly different types' structure or opens up new ways of dealing with the shortcomings of uniffi? Errors come to mind.
  • Are there still things you can't do with proc_macros? It's the reason we held off on the migration when the proc_macro stuff was announced a while back, but I don't know if they've patched up all the holes yet.

@DanGould
Copy link
Author

DanGould commented Nov 27, 2024

we're pushing to release the final 1.0 before the end of 2024

Awesome news 😎, meaning that for bitcoin-ffi 1.0, bdk-ffi 1.0, or both? Or BDK 1.0? Also curious what these numbers mean compared to rust-bitcoin and bdk's version numbers.

I see that the docstrings are indeed apparently ported. There was talk from Mozilla of it not working for some time (they merged the UDL docstrings first) so that's where you got that idea @rustaceanrob, but I can see they even have a test-fixture for it here so I expect it to work. Question: will the docstrings from bitcoin-ffi types appear in our API docs for bdk-ffi?

I have checked that building payjoin-ffi using bitcoin-ffi as a dependency does indeed show docstrings at the higher level crate that exposes them. Since docstrings are compiled into the crate metadata they should behave the same and appear in API docs for bdk-ffi if it depends on the bitcoin-ffi which has them compiled in.

Can we use/mix types exported via proc_macro in bitcoin-ffi in the bdk-ffi library (if the bdk-ffi is still on UDL), or do we need to migrate everything at once?

I don't believe we'd need to migrate everything at once and do believe mixing types is possible. See Declaring external procmacro types.

Does the migration involve slightly different types' structure or opens up new ways of dealing with the shortcomings of uniffi? Errors come to mind.

The only issue I ran into was requiring me to define Enums in both UDL and by procmacro. I'm not sure I explored the limits of procmacro and there well may be a way to define the enum only in procmacro, since that's what the testfixture does

Are there still things you can't do with proc_macros? It's the reason we held off on the migration when the proc_macro stuff was announced a while back, but I don't know if they've patched up all the holes yet.

I have not encountered anything beyond the limitations in the documentation, specifically conditional compilation, which could be worked around by conditionally compiling modules instead of using cfg_attr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants