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

uniffi::custom_newtype!(Newtype, u16) gives Lift<UniFfiTag> is not implemented for Newtype #1988

Open
mgeisler opened this issue Feb 8, 2024 · 15 comments

Comments

@mgeisler
Copy link
Contributor

mgeisler commented Feb 8, 2024

Hello!

I've been poking around at UniFFI for a few days and I really like what I see: the ability to automatically generate high-level APIs is amazing.

However, I think I've found a small bug. I have two crates, a and b. The a crate defines a newtype which I hope to use from b:

a/src/lib.rs:

uniffi::setup_scaffolding!();

uniffi::custom_newtype!(Newtype, u16);

#[derive(Debug)]
pub struct Newtype(u16);

b/src/lib.rs:

uniffi::setup_scaffolding!();

#[uniffi::export]
pub fn use_newtype(newtype: a::Newtype) {
    println!("{newtype:?}");
}

This fails with

error[E0277]: the trait bound `Newtype: Lift<UniFfiTag>` is not satisfied
 --> src/lib.rs:3:1
  |
3 | #[uniffi::export]
  | ^^^^^^^^^^^^^^^^^ the trait `Lift<UniFfiTag>` is not implemented for `Newtype`
  |
  = help: the trait `Lift<a::UniFfiTag>` is implemented for `Newtype`
  = help: for that trait implementation, expected `a::UniFfiTag`, found `UniFfiTag`
  = note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `Newtype: Lift<UniFfiTag>` is not satisfied
 --> src/lib.rs:4:29
  |
4 | pub fn use_newtype(newtype: a::Newtype) {
  |                             ^^^^^^^^^^ the trait `Lift<UniFfiTag>` is not implemented for `Newtype`
  |
  = help: the trait `Lift<a::UniFfiTag>` is implemented for `Newtype`
  = help: for that trait implementation, expected `a::UniFfiTag`, found `UniFfiTag`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `b` (lib) due to 2 previous errors

It took me a while to spot the difference! 😄 The Newtype implements Lift<a::UniFfiTag> but not Lift<b::UniFfiTag>.

I'm not understanding the details of how the UniFfiTag struct is supposed to work, but I hope the above is more clear to someone here.

The Cargo.toml files are nearly identical, but the one for b has a path reference to a:

[package]
name = "b"
version = "0.1.0"
edition = "2021"

[lib]
crate-type = ["lib", "cdylib"]

[dependencies]
a = { path = "../a" }
uniffi = "0.26.1"
@mgeisler mgeisler changed the title uniffi::custom_newtype! in crate a gives Lift<UniFfiTag> is not implemented for Newtype in crate b uniffi::custom_newtype!(Newtype, u16) gives Lift<UniFfiTag> is not implemented for Newtype Feb 8, 2024
@mhammond
Copy link
Member

mhammond commented Feb 8, 2024

This is almost what's described at https://mozilla.github.io/uniffi-rs/proc_macro/index.html#types-from-dependent-crates, although I don't think this is supported for custom types. So I think the only path forward is to duplicate (or move) the uniffi::custom_newtype!(Newtype, u16);. We are still thinking about this, but I don't think we want custom type definitions to ever be global (at least by default) - our thinking is that for something like a custom type wrapping a url::Url (so the primitive type is a string), we don't want one crate to dictate how every crate uses a url - some might prefer to use an actual string. This argument doesn't make as much sense for a newtype like this, but it's still the same shaped problem.

@mgeisler
Copy link
Contributor Author

mgeisler commented Feb 9, 2024

So I think the only path forward is to duplicate (or move) the uniffi::custom_newtype!(Newtype, u16);

Oh, are you suggesting that I do the macro invocation from the b crate that uses the newtype? I had not thought of that 😄

We are still thinking about this, but I don't think we want custom type definitions to ever be global (at least by default) - our thinking is that for something like a custom type wrapping a url::Url (so the primitive type is a string), we don't want one crate to dictate how every crate uses a url - some might prefer to use an actual string. This argument doesn't make as much sense for a newtype like this, but it's still the same shaped problem.

Yeah, I think I agree with the non-global part. But I don't understand why this macro would do anything global? When I cargo expand the a crate, I see an implementation of ::uniffi::FfiConverter<crate::UniFfiTag>:

#[automatically_derived]
unsafe impl ::uniffi::FfiConverter<crate::UniFfiTag> for Newtype {
    type FfiType = <u16 as ::uniffi::Lower<crate::UniFfiTag>>::FfiType;
    fn lower(obj: Newtype) -> Self::FfiType {
        <u16 as ::uniffi::Lower<
            crate::UniFfiTag,
        >>::lower(<Newtype as crate::UniffiCustomTypeConverter>::from_custom(obj))
    }
    // etc...

That seems nice and local to me?

Would this work better if I use UDL files instead? I've been hoping that I could do everything with proc macros, but if not, then I should abandon this for now.

@mgeisler
Copy link
Contributor Author

mgeisler commented Feb 9, 2024

So I think the only path forward is to duplicate (or move) the uniffi::custom_newtype!(Newtype, u16);

Oh, are you suggesting that I do the macro invocation from the b crate that uses the newtype? I had not thought of that 😄

I probably misunderstood since I get an error when I try something like custom_newtype!(a::Newtype, u16). The error is

error: expected `,`

pointing to the place where the :: is.

@mgeisler
Copy link
Contributor Author

mgeisler commented Feb 9, 2024

Aha, I got it working! Using

uniffi::ffi_converter_forward!(a::Newtype, a::UniFfiTag, crate::UniFfiTag);

in b/src/lib.rs seems to do the right thing!

I first tried with uniffi::use_udl_record!(a, Newtype), but this failed with errors about Newtype not being found (and a::Newtype) did not work either. But use_udl_record! delegates to ffi_converter_forward! via a small step, and so I found that I could put the necessary types here directly.

I've only checked that this compiles with Rust — I don't know if it does something sensible in Kotlin or other languages 🙂

@mgeisler
Copy link
Contributor Author

mgeisler commented Feb 9, 2024

As a follow-up, Types from dependent crates gave me the impression that I don't need to do anything special when I depend on a proc-macro annotated type from a different crate.

I got this impression from this sentence:

When using proc-macros, you can use types from dependent crates in your exported library, as long as the dependent crate annotates the type with one of the UniFFI derives.

@mgeisler
Copy link
Contributor Author

mgeisler commented Feb 9, 2024

I've experimented some more and found that uniffi::ffi_converter_forward! doesn't do the trick for a type with #[derive(uniffi::Object)].

I now have a/src/lib.rs with:

uniffi::setup_scaffolding!();

#[derive(Debug, uniffi::Object)]
pub struct Wrapper {
    pub inner: String,
}

and b/src/lib.rs with:

uniffi::setup_scaffolding!();

#[uniffi::export]
pub fn use_wrapper(wrapper: a::Wrapper) {
    println!("{wrapper:?}");
}

Building b fails with

error[E0277]: the trait bound `a::Wrapper: Lift<UniFfiTag>` is not satisfied
  --> src/lib.rs:12:1
   |
12 | #[uniffi::export]
   | ^^^^^^^^^^^^^^^^^ the trait `Lift<UniFfiTag>` is not implemented for `a::Wrapper`

Adding

uniffi::ffi_converter_forward!(a::Wrapper, a::UniFfiTag, crate::UniFfiTag);

to b/src/lib.rs changes the error to

error[E0119]: conflicting implementations of trait `LowerReturn<UniFfiTag>` for type `a::Wrapper`
 --> src/lib.rs:5:1
  |
5 | uniffi::ffi_converter_forward!(a::Wrapper, a::UniFfiTag, crate::UniFfiTag);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: conflicting implementation in crate `a`:
          - impl<T> LowerReturn<T> for a::Wrapper;

error[E0119]: conflicting implementations of trait `LiftRef<UniFfiTag>` for type `a::Wrapper`
 --> src/lib.rs:5:1
  |
5 | uniffi::ffi_converter_forward!(a::Wrapper, a::UniFfiTag, crate::UniFfiTag);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: conflicting implementation in crate `a`:
          - impl<T> LiftRef<T> for a::Wrapper;

I found ffi_converter_arc_forward! as well, which fails slightly differently:

error[E0119]: conflicting implementations of trait `FfiConverterArc<UniFfiTag>` for type `a::Wrapper`
 --> src/lib.rs:5:1
  |
5 | uniffi::ffi_converter_arc_forward!(a::Wrapper, a::UniFfiTag, crate::UniFfiTag);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: conflicting implementation in crate `a`:
          - impl<T> FfiConverterArc<T> for a::Wrapper;

I've been skimming different issues here and found #1865 where you discussed removing the UniffiTag with @bendk and @jplatte. So I take it that I've stepped into a larger and deeper discussion about how this should work? 🙂

@mgeisler
Copy link
Contributor Author

mgeisler commented Feb 9, 2024

Ah, just as I wrote this, I remembered that I saw something about using Arc in the manual... so this actually works:

#[uniffi::export]
pub fn use_wrapper(wrapper: std::sync::Arc<a::Wrapper>) {
    println!("{wrapper:?}");
}

@bendk
Copy link
Contributor

bendk commented Feb 9, 2024

#[automatically_derived]
unsafe impl ::uniffi::FfiConverter<crate::UniFfiTag> for Newtype {
    type FfiType = <u16 as ::uniffi::Lower<crate::UniFfiTag>>::FfiType;
    fn lower(obj: Newtype) -> Self::FfiType {
        <u16 as ::uniffi::Lower<
            crate::UniFfiTag,
        >>::lower(<Newtype as crate::UniffiCustomTypeConverter>::from_custom(obj))
    }
    // etc...

That seems nice and local to me?

Yes this is local, which is why you're running into the issue. It's why Lift<a::UniFfiTag> is implemented for the type but not Lift<b::UniFfiTag>.

When using proc-macros, you can use types from dependent crates in your exported library, as long as the dependent crate annotates the type with one of the UniFFI derives.

This is true for everything except custom types. The whole system is currently a bit of a mess, sorry that you're running into it.

When using proc-macros, you can use types from dependent crates in your exported library, as long as the dependent crate annotates the type with one of the UniFFI derives.

Yup. I think the best solution for now is uniffi::ffi_converter_forward!(a::Newtype, a::UniFfiTag, crate::UniFfiTag); -- I think you mentioned that worked. Hopefully things will get better in 0.27.x or 0.28.x

@mgeisler
Copy link
Contributor Author

Hi @bendk, thanks for explaining!

This is true for everything except custom types. The whole system is currently a bit of a mess, sorry that you're running into it.

It's alright — this still beats writing FFI by hand 😄

I'm so far having better luck with structs that have explicit fields (struct Foo { inner: Bar }) instead of tuple structs that just wrap fields. In particular, #[derive(uniffi::Object)] works fine for structs with fields, but I'm running into other errors with the newtype macro.

Yup. I think the best solution for now is uniffi::ffi_converter_forward!(a::Newtype, a::UniFfiTag, crate::UniFfiTag); -- I think you mentioned that worked.

It worked for the simple case I had here. I tried today with a newtype that wraps a Vec<T> where T itself is a type which derives uniffi::Object. That did not work until I added an explicit field name. I did not try to reproduce it in my little a and b example yet.

@LBeernaertProton
Copy link

LBeernaertProton commented Mar 27, 2024

On the latest release (0.27) this no longer works.

I now get this error when using uniffi::ffi_convert_forward for the external type

the trait `FfiConverter<crate::UniFfiTag>` is not implemented for `External type`

Edit: I'm also getting the original error for record types.

Edit2: I'll submit an example MR tomorrow morning with the issue in question.

@LBeernaertProton
Copy link

My apologies for the noise but it appears the issue was related to mixing to different versions of uniffi. One of the crates was not inheriting the workspace definitions.

@mgeisler
Copy link
Contributor Author

Hi @LBeernaertProton, just in case it helps you further: I've stopped trying to export types across crates like this. Instead, I'm now writing a thing wrapper API with UniFFI-compatible types.

@nain-F49FF806
Copy link

@mgeisler If you have time, could you give an example of this thing wrapper API?

It would be useful for many who get stuck when trying to export types from dependency crates.
This also sounds like a good topic for a blog post. If you are so inclined. :)

@mgeisler
Copy link
Contributor Author

mgeisler commented Apr 1, 2024

@mgeisler If you have time, could you give an example of this thing wrapper API?

Sure! The wrapper I've been created lives here: https://github.com/awslabs/mls-rs/blob/main/mls-rs-uniffi/src/lib.rs

It's a separate crate for a two main reasons:

  1. Because UniFFI generates idiomatic APIs in the target languages, it requires the Rust API to use a lot of Arc<T> argument types. This is different from the original API, which often takes ownership of the values passed in.
  2. It allows me to simplify the underlying API in a few places. Instead of exposing the full API of mls-rs, we will start by exposing the parts which we believe are the most important for messaging apps.

@LBeernaertProton
Copy link

@mgeisler I have some use cases where I need to share Uniffi records from other crates. This would save me an extra step of converting a Vec of types into a Vec of uniffi types

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

No branches or pull requests

5 participants