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

Experiment: Use Rust code directly as the interface definition. #416

Closed
wants to merge 1 commit into from

Conversation

rfk
Copy link
Collaborator

@rfk rfk commented Mar 8, 2021

This is a fun little something I've been messing around with,
partly just to learn more about Rust macros and syn, and partly
to see if they could make a good replacement for our current use
of WebIDL for an interface definition language.

Key ideas:

  • Declare the UniFFI component interface using a restricted subset
    of Rust syntax, directly as part of the code that implements the
    Rust side of the component. There is no separate .udl file.

  • Use a procedural macro to insert the Rust scaffolding directly
    at crate build time, rather than requiring a separate build.rs
    setup to generate and include it.

  • When generating the foreign-language bindings, have uniffi-bindgen
    parse the Rust source directly in order to find the component
    interface.

This seems to work surprisingly well so far. If we do decide to
go this route, the code here will need a lot more polish before
it's ready to land...but it works! And it works in a way that's
not too conceptually different from what we're currently doing
with a separate .udl file.


[EDIT] I'm going top use the PR summary here to keep a list of the key points we'll need
to consider and open questions to be resolved:

  • How can we support splitting the Rust implementation code out into a manageable
    set of files, while still allowing UniFFI to see the whole interface at once?
  • How can we support features that are not native to Rust but common in other languages,
    such as default argument values?
  • How can we support good error reporting, ideally providing context-appropriate
    hints on the user's Rust source code?

@rfk rfk force-pushed the for-i-have-synned branch from 628655b to 3a8209d Compare March 8, 2021 03:28
# The Interface Definition

The public interface of your Rust crate should be defined as an inline module
decorated with the `#[uniffi_macros::declare_interface]` macro. Code inside
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a clarifying-my-thoughts exercise, I tried to update the manual to describe how to use the new macro-based approach. This file is probably the best place to start to get a high-level view of what I'm up to in the PR - it describes the interface definition in high-level terms and gives a worked example.


```toml
[dependencies]
uniffi = "0.5"
uniffi = "0.7"
uniffi_macros = "0.7"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Procedural macros have to live in their own special crate, hence uniffi_macros. However, I recall there being a way for uniffi to import and re-export macros from another crate, so maybe we don't need to have two separate dependencies visible to the consumer here..?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think serde does this - IIRC it used to be necessary to explicitly use serde_derive alot more than it is today, and grepping in a-s seems to support that (eg, the only reference to serde_derive in autofill is in Cargo.toml, but older crates use it everywhere.)

But that probably implies they will need to know about it in the [dependencies] list but you might be able to change the decorations to, say, #[uniffi::declare_interface], which is a subtly different thing than you are saying here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the consumer write #[uniffi::declare_interface] seems reasonable to me, with the separate "macros" crate being an implementation detail.

include!(concat!(env!("OUT_DIR"), "/geometry.uniffi.rs"));
pub fn intersection(ln1: Line, ln2: Line) -> Option<Point> {
// TODO: yuck, should be able to take &Line as argument here
// and have rust figure it out with a bunch of annotations...
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried it, but I expect that we can get rid of this and similar "yuck" around borrowed vs owned arguments, because we can see the actual Rust code that the user is writing. The whole [ByRef] thing in the UDL can go away.

@@ -1,5 +1,5 @@
uniffi_macros::build_foreign_language_testcases!(
"src/geometry.udl",
"src/lib.rs",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere, we're passing a reference to a Rust file rather than a reference to a separate UDL file.

I think it might be better if we passed a reference to the crate itself rather than to a specific file, to reinforce a particular mental model where the crate boundary is the unit of abstraction, both for the rust code ("a uniffi-ed crate") and for the foreign-language bindings ("consuming a Rust crate"). But I need to play around with it some more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better if we passed a reference to the crate itself rather than to a specific file,

Months later, I still have a pretty strong gut feeling that treating the crate as the unit of UniFFIcation is the right way to go.

@@ -0,0 +1,69 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, so, this is a litle odd, but I also added a UDL-generating bindings backend. That is: you can use this to parse a ComponentInterface from a Rust source file and then generate a corresponding .udl file. This was entirely in support of mozilla/application-services#3876, because I didn't want to keep the interface and its doc in sync between two different files. Consider this a way to experiment with the parsing-rust-code stuff without necessarily committing to it right away - I was able to write my crate as though we had all this fun macro stuff up and running, then automagically generate a corresponding .udl file so I could use it with the current release version of UniFFI.

I expect we would not want to actually land this or maintain it long term.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While that's very cool, I agree we probably don't want it - I was very confused above when I saw a .udl back-end :)

pub struct Enum {
pub(super) name: String,
pub(super) variants: Vec<Variant>,
// "Flat" enums do not have, and will never have, variants with associated data.
pub(super) flat: bool,
pub(super) docs: Vec<String>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One neat thing that we get pretty much for free from parsing Rust code, is excellent support for inline docs on all items. Here, I've given each ComponentInterface item a simple docs attribute to let it hold any docs parsed from the Rust code, and have spat the docs back out again when converting it into a .udl. file. You can imagine a future where we carry the docs through to the generated Kotlin, Swift etc files as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact the ffi is bound much tighter to the actual impl really is the key feature here, and doc generation is just the most obvious manifestation of that.

name: self.ident.to_string(),
variants,
flat,
docs: attrs.docs,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I've taken the same APIConverter pattern that we've been using for weedle parse nodes and have implemented it for syn parse nodes as well. It more-or-less Just Worked. Like what we currently do for weedle, the trickiest part is looking for all the extra bits of syntax that we don't want to support and erroring out if we see any.

For example, this takes a parsed Rust enum definition and converts it into a definition for a UniFFI component enum.

// that my own lifetime is too short to worry about figuring out; unwrap and move on.
let file_src = syn::parse_file(rs).expect("Failed to parse Rust code");
// First, let's see what sort of style of file we're dealing with.
// It might be `include_scaffolding!` or it might be `#[declare_interface]`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently a bit weird. I'm trying to support parsing both the new #[declare_interface] syntax proposed in this PR, and also the existing include_scaffolding!() syntax that is offered by the current release version of UniFFI. That's probably not worthwhile except to support short-term experiments with the parsing logic here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed over some of the implementation, but I think this has real value and we should push on it.

// It can use things from stdlib, submodules, or other Rust crates.
use std::io::prelude::*;

// But its public interface should be defined inside the `declare_interface` macro.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it's clear what you mean, "defined inside the macro" doesn't quite match my mental model of macros. Even just s/inside/using/ is a bit better?

#[uniffi_macros::declare_interface]
mod sprites {

// The interface can define "records" as structs with public fields.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is as clear as it could be either - it says "records" but doesn't state what records are. Saying "they do not have any public method impls" contradicts below where one does.

Maybe you were trying to say something like "A simple struct without an public method impls can be considered a kind of simple 'record'" or similar?


```toml
[dependencies]
uniffi = "0.5"
uniffi = "0.7"
uniffi_macros = "0.7"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think serde does this - IIRC it used to be necessary to explicitly use serde_derive alot more than it is today, and grepping in a-s seems to support that (eg, the only reference to serde_derive in autofill is in Cargo.toml, but older crates use it everywhere.)

But that probably implies they will need to know about it in the [dependencies] list but you might be able to change the decorations to, say, #[uniffi::declare_interface], which is a subtly different thing than you are saying here?

cargo uniffi generate --language kotlin
```

The advantage of this UI would be that you don't have to specify the path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the word 'UI' isn't valuable here.

@@ -0,0 +1,69 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While that's very cool, I agree we probably don't want it - I was very confused above when I saw a .udl back-end :)

pub struct Enum {
pub(super) name: String,
pub(super) variants: Vec<Variant>,
// "Flat" enums do not have, and will never have, variants with associated data.
pub(super) flat: bool,
pub(super) docs: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact the ffi is bound much tighter to the actual impl really is the key feature here, and doc generation is just the most obvious manifestation of that.

// that my own lifetime is too short to worry about figuring out; unwrap and move on.
let file_src = syn::parse_file(rs).expect("Failed to parse Rust code");
// First, let's see what sort of style of file we're dealing with.
// It might be `include_scaffolding!` or it might be `#[declare_interface]`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!

It fits in the practice of consolidating business logic in a single Rust library while targeting multiple platforms, making it simpler to develop and maintain a cross-platform codebase.
Note that this tool will not help you ship a Rust library to these platforms, but simply not have to write bindings code by hand [[0]](https://i.kym-cdn.com/photos/images/newsfeed/000/572/078/d6d.jpg).

## Design

uniffi requires to write an Interface Definition Language (based on [WebIDL](https://heycam.github.io/webidl/)) file describing the methods and data structures available to the targeted languages.
This .udl (Uniffi Definition Language) file, whose definitions must match with the exposed Rust code, is then used to generate Rust *scaffolding* code and foreign-languages *bindings*. This process can take place either during the build process or be manually initiated by the developer.
UniFFI requires to declare the interface you want to expose to other languages using a restricted
Copy link
Contributor

@lougeniaC64 lougeniaC64 Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: requires you to declare

@@ -16,4 +23,4 @@ This .udl (Uniffi Definition Language) file, whose definitions must match with t
- Kotlin
- Swift
- Python
- [Gecko](https://en.wikipedia.org/wiki/Gecko_(software)) C++
- [Gecko](https://en.wikipedia.org/wiki/Gecko_(software)) JavaScript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarifying question: Is this a necessary part of this change or tangentially related?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely unnecessary tangent

its native error-handling mechanism. For example:

* In Kotlin, there would be an `ArithmeticErrorException` class with an inner class
for each ariant, and the `add` function would throw it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: variant

@rfk rfk force-pushed the for-i-have-synned branch 7 times, most recently from dbd1b47 to 9613f96 Compare August 4, 2021 05:08
@rfk
Copy link
Collaborator Author

rfk commented Aug 4, 2021

I've rebased this atop latest main and simplified it a bit in the hope of making it more approachable. In particular:

  • I've removed the UDL-generating backend, which was a fun hack but isn't something we'd want to actually land.
  • I've removed support for parsing a Rust file with the include_scaffolding! syntax; we now only support the uniffi::declare_interface macro.

I think a great next step here for interested folks, could be to try porting more of the examples and/or tests over to use the new macro syntax. Probably this won't work for any but the simplest examples, because of missing features! But it will be an interesting exercise in trying to use the new approach.

@rfk
Copy link
Collaborator Author

rfk commented Aug 4, 2021

Architecturally, I think the parsing-related logic in the interface module is getting a bit out of hand here. If we decide we want to keep both the UDL parsing for b/w compat and the new syn-based parsing, I think we should:

  • Keep the base interface module as just defining the structs and traits, without any parsing logic.
  • Move the weedle-based parsing impls into a submodule like interface/parse/udl.
  • Move the syn-based parsing impls into a submodule like interface/parse/syn.

This would help ensure that you can look at just one piece of the parsing logic and a time, and also make it easier to eventually delete the deprecated UDL syntax.

uniffi_bindgen/src/interface/enum_.rs Outdated Show resolved Hide resolved
}
}

impl APIBuilder for &syn::ItemMod {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if there's a reason these are implemented for references and not the bare type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is almost certainly "the compiler was complaining at me and I randomly added/removed ampersands until it stopped"


```rust,no_run
pub struct TodoEntry {
#[uniffi(default=false)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I like this idea, and it tackles @badboy's thought about making sure that default values are supported

```

The corresponding Rust struct would need to look like this:
Fields can be made optional using Rust's builtin `Option` type:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to the work here but a bit of terminology confusion on my end, when we say Optional here, we mean nullable - in the context of functions, when we say Optional we mean "has a default value". Don't think we should fix it here, but something for us to keep in mind

println!("value: {}", v.value);
}

// Error: UniFFI doesn't understand the `std::vec::` path; just use `Vec<T>`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting! makes sense, since we check the types by name, For the future: since we're now operating almost completely in Rust we could have something that resolves paths to types (might even help with the type aliases) but I am thinking too far ahead 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's tricky. The Rust language has so much complexity/power that I think it's hard to inhabit a middle ground where you parse the Rust code independently of the compiler and correctly infer its semantics. In order to be safe, you need to either:

  • Let the Rust compiler actually compile the code and output the information you want as a side-effect. This is how wasm-bindgen works for example, it makes Rust actually build the code and then arranges for the resulting library to have an extra data section with details about the types it encountered.
  • Parse a subset of Rust that is so restricted that you can't possibly misinterpret what it means. That's what I was going for here. In fact, I think if we want to do this for real, we might even want to forcibly inject e.g. a use std::vec::Vec into the code so that we know that when we see a type named Vec, it is 100% definitely the Vec from the stdlib and some smart-alec hasn't done use std::option::Option as Vec at the top of the file for a laugh.

```kotlin
interface TodoListInterface {
// Copy the items from another TodoList into this one.
// TODO: hrm, actually we need to pass a concrete `TodoList` instance here,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about this comment. I assumed that in the swift example above, it is recommended that the protocol (TodoListProtocol) be passed to a function instead of the concretion (TodoList) because it allows for increased flexibility so I'm not sure why it is a bad idea here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I really want here is a Self type parameter. If you have some Swift-level mock implementation of the TodoListInterface then you absolutely cannot pass that in to the Rust code and expect it to work out well. The Rust code will only accept instances of the concrete TodoList type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to think that this "shared references" section could move to a more "advanced" section - this also confused me, and the Arc<> discussion still made me stop and think, so someone new to UniFFI might be best served by not encountering this here.

(OTOH, the following "concurrent access" section did feel OK in this doc, mainly because it's something you really can't ignore - whereas this section can be ignored until you actually need it?

Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK finally was able to do a full go-through of this PR, I'm very happy with how we were able to almost plug-in into the existing patterns that we used for the udl file 💯 Excited to see where this goes

docs/manual/src/interface/errors.md Outdated Show resolved Hide resolved
uniffi_macros/src/lib.rs Outdated Show resolved Hide resolved
.collect::<Vec<_>>();
proc_macro::TokenStream::from(quote! {
#module
#(#imports)*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own understanding, any reason we're exposing those functions at the top level? Is it for other rust consumers, in which case wouldn't it be better to just keep them inside the module where consumers can see them in the code file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my head, I really want the UniFFI component interface to also be the public interface exposed by the crate, which is why I'm lifting them to the top-level here. But I agree that's probably weird/confusing for Rust consumers since the names magically appear in a scope other than where they're defined.

uniffi_bindgen/src/interface/function.rs Outdated Show resolved Hide resolved

/// Parse a `ComponentInterface` from a string containing a pre-parsed Rust module,
/// of the sort you might encounter if you're a macro.
pub fn from_rust_module(rsmod: &syn::ItemMod) -> Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I am very pleasantly surprised how elegant this function ended up being 🎉

let component = parse_udl(udl_file)?;
let config = get_config(&component, udl_file, config_file_override)?;
let component = if matches!(interface_file.extension(), Some(ext) if ext == "rs") {
println!("FROM RUST");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

@@ -9,4 +9,4 @@ fn add(a: u32, b: u32) -> u32 {
```

And top brass would like you to expose this *business-critical* operation to Kotlin and Swift.
**Don't panic!** We will show you how do that using UniFFI.
**Don't panic!** We will show you how to do that using uniffi.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the case change of UniFFI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no 😱

This is probably a rebase artifact, my original macros experiment predates my attempts to force everyone to capitalize UniFFI a certain way

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only got to the docs so far, but took some notes on the way. Feel free to reject any suggestions - I thought I'd leave even the dubious ones as they accurately reflect my first reading of those docs, but I understand it might not be at all clear how to resolve.

| `std::time::SystemTime` | |
| `std::time::Duration` | |

And of course you can use your own types, which is covered in the following sections.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc nit, but "use your own types" might give the wrong expectation - maybe something like "And of course you can use types you define via UniFFI..." or similar?

a key-value store exposed by the host operating system (e.g. the system keychain).

```rust
trait Keychain: Send {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing pub here, which contradicts the para above (or maybe it should be removed above?)

```rust
trait Keychain: Send {
pub fn get(key: String) -> Option<String>
pub fn put(key: String, value: String)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but pub on the functions is explicitly not allowed IIRC

* In Swift, enums are exposed using the native `enum` syntax.

Like [records](./records.md), enums are only used to communicate data
and do not have any associated behaviours.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add something like: "It's fine if your rust code declares an impl block for enums, but that will be ignored by UniFFI` (assuming that's actually true :)

}
```

Note that you cannot currently use a typedef for the `Result` type, UniFFI only supports
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit - s/typedef/type alias/?

In the foreign-language bindings they would typically correspond to a class.

In the interface definition, objects may have public methods but *must not have public fields*,
because they are intended to be opaque to the foreign-language code. Like this:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially wrote "I don't understand the justification for this limitation", but down in record.md I see that the public fields would make uniffi treat this as a record - which makes far more sense to me. Maybe update this para to reflect that?

```rust
#[uniffi::declare_interface]
mod example {
struct TodoList {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might make the docs more unwieldy than you like, but to help with the mental model, maybe consider:

  • Having the struct definition outside of the module, which a comment explaining it need not be inside.
  • Another impl TodoList block outside the module, explaining that these can be thought of as "uniffi-private"

(edit: now that I get down to record.md I'm not actually sure about this)

```kotlin
interface TodoListInterface {
// Copy the items from another TodoList into this one.
// TODO: hrm, actually we need to pass a concrete `TodoList` instance here,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to think that this "shared references" section could move to a more "advanced" section - this also confused me, and the Arc<> discussion still made me stop and think, so someone new to UniFFI might be best served by not encountering this here.

(OTOH, the following "concurrent access" section did feel OK in this doc, mainly because it's something you really can't ignore - whereas this section can be ignored until you actually need it?

struct TodoEntry {
pub done: bool,
pub due_date: u64,
pub text: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens to non-pub fields?

@@ -32,8 +32,8 @@ this is so.
## Lifetimes

In order to allow for instances to be used as flexibly as possible from foreign-language code,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates some of the docs above - I'd suggest maybe merging the above with this - ie, this can be the "advanced" section I referred to.

@badboy
Copy link
Member

badboy commented Aug 5, 2021

Today I took this branch for a test run against my minimal Glean+UniFFI fork to see how far I get.
The Glean UDL is here: https://github.com/badboy/glean/blob/13e765da0fccdfb376c995888b4fdc123419f6c1/glean-core/uniffi/src/glean_core.udl

One big issue with the current implmementation is a bug, see this commit: 9a23722
tl;dr: It doesn't handle objects yet properly and generates broken code.

I have some more comments:

I don't think the auto-insertion of pub use $namespace::$thing is a good idea. See this commit: 87448da

In the current implementation a lot of errors are hidden. That includes some compile errors, import errors, etc., because the macro happens before the Rust compiler sees the code and can compile it.
I had to remove the uniffi::declare_interface a couple of times to fix compile errors first, then add it back and fix the remaining uniffi errors.

Cramming everything of the exposed API into a single module is ... not good.
Glean's API is huge (General API + 15+ metric types, each with 3-5 methods).
It's unwieldy to keep all these structs, functions and the full function implementations in a single file.
If it's kept like that we might resort to hacks like concatenating files ahead of the build to keep proper code organization possible.


Initially I thought the macro approach can work really well and with this PR up I thought we might be close to a working implementation we could ship.
After spending this morning on it I'm not so convinced anymore.
Some of this boils down to needing an improved implementation (bug fixes + better error reporting). And that's just engineering effort.

But IMO right now the requirement of having it all in one module is a showstopper for me and I don't have a good idea how we can work around that.
Annotate each enum, struct and impl with #[uniffi::interface]?
cargo expand the whole codebase, then parse that to find the right things to uniffi-export?
(I now understand why wasm-bindgen does it that way)

@rfk
Copy link
Collaborator Author

rfk commented Aug 5, 2021

But IMO right now the requirement of having it all in one module is a showstopper for me
and I don't have a good idea how we can work around that.

That's fair. One other data point on this front is the fxa-client crate, which is the crate for which I was using this little experiment in order to autogenerate the UDL from the Rust code. It's a bit different syntax but I was actually parsing the top-level lib.rs to determine the interface:

In that case, most of the functions/methods are just little stub wrappers around implementations that live elsewhere in the crate. This helped keep the code organization under control while still exposing all the function signatures in the one Rust file, albeit at the cost of some duplication of the function signatures.

It's probably not a pattern we'd want to force on all consumers, though.

This is a fun little something I've been messing around with,
partly just to learn more about Rust macros and `syn`, and partly
to see if they could make a good replacement for our current use
of WebIDL for an interface definition language.

Key ideas:

* Declare the UniFFI component interface using a restricted subset
  of Rust syntax, directly as part of the code that implements the
  Rust side of the component. There is no separate .udl file.

* Use a procedural macro to insert the Rust scaffolding directly
  at crate build time, rather than requiring a separate `build.rs`
  setup to generate and include it.

* When generating the foreign-language bindings, have `uniffi-bindgen`
  parse the Rust source directly in order to find the component
  interface.

This seems to work surprisingly well so far. If we do decide to
go this route, the code here will need a lot more polish before
it's ready to land...but it works! And it works in a way that's
not too conceptually different from what we're currently doing
with a separate `.udl` file.
@badboy
Copy link
Member

badboy commented Jun 14, 2023

This is now fully superseded by the existing macro approach.

@badboy badboy closed this Jun 14, 2023
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.

5 participants