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

RFC(don't merge): Use proc macro instead of the UDL file #14

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Davidson-Souza
Copy link

@Davidson-Souza Davidson-Souza commented Sep 10, 2024

As discussed in one of our last calls, this PR implements everything using uniffi's proc macros rather than a UDL file. It offers the advantage of requiring less code duplication and not requiring knowledge of the weird UDL syntax. Another advantage over using UDL is that you have better control over the scaffolding, so in unusual cases where uniffi can't implement things on its own, you can step in and do it.

I've implemented Transaction and all it subfields, and migrated existing stuff to proc macros. Since this is an exploratory thing, I've used a mix of Records and Objects. Records are cool in that they let consumers access the fields themselves, and without the indirections of an Arc, but can't have associated impl blocks.

I've found some limitations along the way: looks like docstrings from the Rust code aren't used to document the bindings, only UDL docstrings. As per uniffi's docs:

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.

There's also an odd problem with dictionaries (or Record in proc macros land). If I define one in bitcoin-ffi and make a dependent crate that uses that dict as a field for some other dict, the compiler errs, saying the original dict doesn't implement Lower and Lift. But if I use in the same crate, it works just fine. I've followed their docs and issues, and couldn't find any reason why this didn't work (it does work with UDL definitions).

Edit: Each commit implements one thing (or at least "only the nescessary to implement that thing"). However, if we decide to go this way, this PR obviously needs to be broken down.

Remove the UDL definition for Amount and `ParseAmountError`, replace it
with proc macros
This commit implements the last type with proc macros and removes the
UDL file.
@reez
Copy link
Collaborator

reez commented Sep 12, 2024

Thanks for the draft PR! I like the idea of using proc macros to avoid UDL duplication and more. I appreciate the one commit for one thing path you're taking now, makes it easier to review, and we can always just squash down at the end etc.

Thoughts on some of the points you brought up:

  • Losing Rust docstrings is significant; I wonder what we can explore to remediate it.
  • Dictionaries Issue is interesting, the problem with those Lower/Lift traits across crates and the procedural macros' code generation boundaries. I wonder what we an explore to remediate that too. I've never run into that issue either but yeah it seems to not handle visibility or type implementations well when the types are split across multiple crates. I dont know if the Lower and Lift traits needed for FFI conversion are somehow only implemented within the originating crate or something so dependent crates that try to use those types wouldn't have access to the necessary trait implementations unless they are explicitly re-exported or generated differently maybe, hmmm. Just kind of my top-of-mind thoughts but it's something I'll think about more too.

Pumped to see the work you've done on this!

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

Successfully merging this pull request may close these issues.

2 participants