mozilla / application-services

Firefox Application Services
https://mozilla.github.io/application-services/
Other
608 stars 224 forks source link

Improve situation around sharing types between crates. #4231

Closed data-sync-user closed 3 years ago

data-sync-user commented 3 years ago

It's currently impossible to expose a struct etc defined in another crate - uniffi tries to define uniffi::ViaFfi for the type and the rust compiler doesn't let that happen (impl doesn't use only types from inside the current crate).

Even if we came up with a way to get around that, we'd need to take some care as the next obvious step would be that multiple crates all expose the same type and would expect to be able to pass these types between each other - which is another can of worms if each crate generates its own version of the type.

For a concrete example, grep for the DeviceType enum in application-services - the "real" type should be owned by sync15-traits, but fxa-client, sync-manager and tabs have clones of the enum, and at least the fxa and sync15 versions already diverge in how they serialize to json.

This might be tricky, so the least we can do is document this.

┆Issue is synchronized with this Jira Task

data-sync-user commented 3 years ago

➤ Philipp commented:

I just ran into this issue while trying to setup uniffi for a multi-crate project. Is there a typical way to work around this? It's not possible to write a udl file for each crate and combine the results somehow, right? Thanks.

data-sync-user commented 3 years ago

➤ Ryan Kelly commented:

Is there a typical way to work around this?

We don't have a good way to deal with it yet, but we've started running into this issue in our production code so we're actively working on a solution. Are you able to describe your multi-crate project setup a little bit, so we can use it as another reference point when discussing solutions?

data-sync-user commented 3 years ago

➤ Philipp commented:

Sure. Although, I just got started with uniffi and considered it to implement our bindings. Among others, there are the identity-core crate, and the identity-iota crate. There is a identity_core::crypto::KeyPair struct and that is used in one of the methods of identity_iota::IotaDocument. We also have an identity crate that re-exports all these types for easier usage.

Writing two separate udl files for identity_core and identity_iota doesn't allow the scaffolding to succeed, as identity_iota's udl file doesn't contain the KeyPair, because it is not defined in that crate. Importing the KeyPair also fails, since we cannot implement ViaFfi for such a type, as mentioned above. I could see one solution involving an import mechanism in udl files, to combine multiple crates into one "ffi library", though I'm sure that comes with is own caveats.

Writing a single udl file for identity would be ideal, as we have already defined all the types we want to export there. Unfortunately, that is again blocked by the Rust compiler not allow implementing ViaFfi for types from other crates.

I'm wondering if there is a solution involving derive macros that implement ViaFfi for types that are annotated with it.

Thanks for your work!

data-sync-user commented 3 years ago

➤ Mark Hammond commented:

Ryan and I were chatting about this earlier today, so the timing is good 😀 In our code, we've been lucky so far in that the things that are "external" are fairly simple to work around - a simple enum where we just make a squinty face and clone it. That then means some gymnastics to "unclone" back into the expected type on the other side. If it's really just your KeyPair, this might be an OK workaround for you too. Some of the other stuff recently landed (eg, objects as params in #462) might allow other creative solutions 😀

data-sync-user commented 3 years ago

➤ Ryan Kelly commented:

PhilippGackstatter would you be wanting to distribute separate bindings for identity-core and identity-iota, e.g. as two separately-compiled shared libraries? Or, would consumers be conceptually using a single identity library that happens to be implemented with multiple Rust crates under the hood?

Writing a single udl file for identity would be ideal, as we have already defined all the types we want to export there.

This ^ sounds like consumers might see it as a single library.

data-sync-user commented 3 years ago

➤ Philipp commented:

Yes, exactly, conceptually, it is one library that happens to be implemented with multiple crates.

data-sync-user commented 3 years ago

➤ Ryan Kelly commented:

Thanks, that makes sense.

Over in #482 Mark has started experimenting with the ability to declare "external" types, which sounds like it might also help with this use-case. For example, the identity_core crate could define the KeyPair type in its .udl file, and the identity_iota crate could declare KeyPair as an "external type", basically assuming that the type already has a working ViaFfi implementation rather than trying to generate one for it. It's not ready for use yet, but, sounds like it could be a step in a good direction.

Writing a single udl file for identity would be ideal, as we have already defined all the types we want to export there.

FWIW, I agree this sounds like it would be the best consumer experience. I wonder if we could make it work by auto-generating newtype wrappers that impl ViaFfi, rather than implementing it directly on the imported types 🤔 ...

data-sync-user commented 3 years ago

➤ Philipp commented:

Over in #482 Mark has started experimenting with the ability to declare "external" types

That sounds excellent! I'll keep an eye on that.

I wonder if we could make it work by auto-generating newtype wrappers that impl ViaFfi

I was just trying this actually with the newtype pattern and implementing Deref. However, I'm not sure how to properly wrap an enum. A newtype wrapper with a single-field tuple struct doesn't give access to the enum's variants. Maybe it has to be copied :thinking:.

data-sync-user commented 3 years ago

➤ Mark Hammond commented:

Writing a single udl file for identity would be ideal,

Obligatory "but what about #416?" 😀

However, I'm not sure how to properly wrap an enum.

If it helps at all, the specific example I was talking about is here ( https://github.com/mozilla/application-services/pull/4192/files ) and specifically the DeviceType enum.

data-sync-user commented 3 years ago

➤ Mark Hammond commented:

implementing Deref

there probably aren't references involved, so it's likely From you are after.

data-sync-user commented 3 years ago

➤ Mark Hammond commented:

I removed the "Document or" prefix from this issue - I think the aim should be to do the improvement part rather than the "document the limitations" part 😀