Can't use procmacro derived types as external types

[mhammond]

I started looking at this but need to put it down for a while - so throwing this up in case anyone can offer clues.

With the patch at the end, I get:

% cargo test -p uniffi-fixture-ext-types
error[E0119]: conflicting implementations of trait `uniffi::FfiConverter<UniFfiTag>` for type `uniffi_one::UniffiOneProcMacroEnum`
   --> /Users/skip/src/moz/uniffi-rs/target/debug/build/uniffi-fixture-ext-types-23c75fdeb14707b4/out/ext-types-lib.uniffi.rs:446:1
    |
446 | ::uniffi::ffi_converter_forward!(r#UniffiOneProcMacroEnum, ::r#uniffi_one::UniFfiTag, crate::UniFfiTag);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `uniffi_one`:
            - impl<T> FfiConverter<T> for UniffiOneProcMacroEnum;
    = note: this error originates in the macro `::uniffi::ffi_converter_forward` (in Nightly builds, run with -Z macro-backtrace for more info)

I doubt this is limited to enums, but this is as far as I got.

diff --git a/fixtures/ext-types/lib/src/ext-types-lib.udl b/fixtures/ext-types/lib/src/ext-types-lib.udl
index 78d7c81b..6fe169bd 100644
--- a/fixtures/ext-types/lib/src/ext-types-lib.udl
+++ b/fixtures/ext-types/lib/src/ext-types-lib.udl
@@ -26,6 +26,9 @@ typedef extern UniffiOneType;
 // An enum in the same crate
 [External="uniffi-one"]
 typedef extern UniffiOneEnum;
+// An enum derived via a procmacro in that same crate
+[External="uniffi-one"]
+typedef extern UniffiOneProcMacroEnum;
 // An interface in the same crate
 [ExternalInterface="uniffi-one"]
 typedef extern UniffiOneInterface;
diff --git a/fixtures/ext-types/lib/src/lib.rs b/fixtures/ext-types/lib/src/lib.rs
index c8de7b72..f9f57e1e 100644
--- a/fixtures/ext-types/lib/src/lib.rs
+++ b/fixtures/ext-types/lib/src/lib.rs
@@ -1,7 +1,7 @@
 use custom_types::Handle;
 use ext_types_guid::Guid;
 use std::sync::Arc;
-use uniffi_one::{UniffiOneEnum, UniffiOneInterface, UniffiOneType};
+use uniffi_one::{UniffiOneEnum, UniffiOneProcMacroEnum, UniffiOneInterface, UniffiOneType};
 use url::Url;
 
 pub struct CombinedType {
diff --git a/fixtures/ext-types/uniffi-one/src/lib.rs b/fixtures/ext-types/uniffi-one/src/lib.rs
index e069df75..8bc7822c 100644
--- a/fixtures/ext-types/uniffi-one/src/lib.rs
+++ b/fixtures/ext-types/uniffi-one/src/lib.rs
@@ -9,6 +9,12 @@ pub enum UniffiOneEnum {
     Two,
 }
 
+#[derive(uniffi::Enum)]
+pub enum UniffiOneProcMacroEnum {
+    Proc1,
+    Proc2,
+}
+
 #[derive(Default)]
 pub struct UniffiOneInterface {
     current: AtomicI32,

┆Issue is synchronized with this Jira Task
┆Issue Number: UNIFFI-257

[bendk]

The issue here is that we're generating this in uniffi-one: impl<T> FfiConverter<T> for UniffiOneProcMacroEnum. i.e. it's implementing FfiConverter for all UniFfiTags. This means there's a conflict when the ffi_converter_forward macro tries to implement it for the local crate's tag.

One solution is to change the generated code to only implement it for the local crate (impl FfiConverter<crate::UniFfiTag> for UniffiOneProcMacroEnum). I'm pretty sure this would fix the build error.

The drawback to is that we now need to also generate FfiConverter impls for any crates that use the external type. With UDL, I don't think this is a drawback. You need to declare your external types in the UDL file, so we can always generate the ffi_converter_forward! line. However, proc-macros could potentially improve this and allow you to use external types without needing to any sort of special declaration.

One complication is external types from a remote crate. In this case, we must use a local type in the impl line, and therefore can't define a generic implementation.

My feeling is that crate mode could solve a lot of these issues and provide a very nice experience. I'm not 100% confident that's true though. In the meantime, If there's a need to mix external types, UDL, and proc-macros, then I think we should go with the quick solution I mentioned at the top.

[mhammond]

@jplatte, for this issue, I naively thought that the issue was simply that the recent work Ben did with crate::UniFFITag hadn't been migrated to the procmacro work.
However, it seems that the procmacros have taken a different approach, which I'd like to better understand. Instead of unconditionally using the crate tag, it seems to (a) support arbitrary tags and (b) default to no tag at all.

Re (a), I don't understand what other tags make any sense here - ie, why isn't a bool better here?

Re (b), IIUC, this means that there's a "global" FfiConverter available for a given type - a type can be used via UniFFI in crates other than which they were defined.

However, the downside of this is that it's effectively impossible to expose types from crates which are not UniFFI aware - eg, it would be impossible to expose, say, Log::Level - and this is the motivation which caused ben to come up with the UniFfiTag magic.

IIUC, in the current world, it means the consumer would need to "opt in" to this via something like:

#[derive(uniffi::Enum)]
#[uniffi(tag = crate::UniFfiTag)]
pub enum MyEnum ...

but this needs to be done where the enum is defined - someone who wants to consume it might not control the original crate, so would be unable to specify this.

However, also IIUC, if procmacros unconditionally used UniFfiTag like UDL, then it would need some way to tell UniFFI that a particular "external" type is used by a crate - something like:

#[uniffi::external]
use some_crate::SomeType;

which is done by the consumer - no change needs to be made to the original enum to enable its consumption by other crates.

Doing this seems like it might work and would close the gap between procmacros and UDL and seems easier to understand. I suspect I'm missing something, but is there a good reason to not do this?

[jplatte]

Re (a), I don't understand what other tags make any sense here - ie, why isn't a bool better here?

A bool for whether the given impl is global? I don't see how that would be possible.

Re (b), IIUC, this means that there's a "global" FfiConverter available for a given type - a type can be used via UniFFI in crates other than which they were defined.

Yes, that's exactly why the default is to use a generic impl where the tag can be anything.

Unfortunately I don't really understand the rest of your comment. I'll add some of my own comments to hopefully move this forward..

First, I don't think #[uniffi(tag = ...)] is all that useful for the derives, though it looks like we support it. For the attribute macros that UDL codegen spits out nowadays it makes sense because they might define an impl for a type from an external crate, for which a generic FfiConverter impl would be forbidden. I think you came to the same conclusion, pretty much?

Something I've also had it in the back of my mind for a long time but that isn't implemented yet, is that the proc-macros ought to work with conditional compilation for crates that want to conditionally support UniFFI in the same way many crates conditionally support serde. Luckily, #[cfg_attr] largely makes this possible already, i.e. you can have #[cfg_attr(feature = "uniffi", uniffi::export)] on an impl block today and it works, it just gets a little hairy when extra attributes like #[uniffi::constructor] enter the mix (but it's possible to support #[cfg_attr(feature = "uniffi", uniffi::constructor)], or a pseudo-cfg which is a bit magic but I personally quite like because it's much shorter: #[cfg_attr(uniffi, constructor)])

Finally, for truly non-cooperative external crates, #[uniffi::external] on a use statement won't work no matter what, since the codegen has to know what the type definition looks like. We could just document the attribute macros ffi_converter_record, ffi_converter_enum and so on for this use case, I don't really expect things to become much nicer. However, we could go more sophisticated and add support for types with private fields as records and stuff like that. See Derive for remote crate for how serde solves pretty much the same problem.

[bendk]

Finally, for truly non-cooperative external crates, #[uniffi::external] on a use statement won't work no matter what, since the codegen has to know what the type definition looks like. We could just document the attribute macros ffi_converter_record, ffi_converter_enum and so on for this use case, I don't really expect things to become much nicer. However, we could go more sophisticated and add support for types with private fields as records and stuff like that. See Derive for remote crate for how serde solves pretty much the same problem.

I don't think #[uniffi::external] would prevent the need for doing something like #[serde(remote)] and spelling out the remote type definition. However, it would be needed if then needed if you had a multi-crate setup and wanted to use that type in another crate.

The current existing enum/record proc-macros generate an impl<T> FfiConverter<T> for Type. This allows other crates to use them without needing any special scaffolding generation.

However, with a non-cooperative external type, we wouldn't be able to generate impl<T> FfiConverter<T> for Type because of the orphan rules. This means that if another crate wanted to use that type it, it would need to generate some code to implement FfiConverter for it's local UniFfiTag (probably with uniffi:ffi_converter_forward!()). The #[uniffi::external] attribute would be a way to generate this.

In either case I think we need bindings code to handle the external type. I think that provides some more motivation for something like #[uniffi::external], although I haven't thought this all though.

So, maybe we should use a system like this:

• Always implement FfiConverter for the local UniFfiTag only (i.e. remove the tag parameter and always assume tag=crate::UniFfiTag).
• If consumers want to use a UniFFIed type from another create, they need to wrap it with #[uniffi::external].

What do you think about this? I think you may have an existing multi-crate setup, how disruptive would this change be?

[jplatte]

Yes, #[uniffi::external] could be a solution for importing a foreign FfiConverter impl from another crate (not something I care about, but I understand that it can be useful sometimes). How does that relate to removing support for the generic impls?

[bendk]

If #[uniffi::external] generates a ffi_converter_forward!(), then it would be incompatible with the generic impl. This adds complexity for the user: for some types they would use #[uniffi::external] for other types they wouldn't.

One option to handle this would be to remove support for the generic impl. I'm not sure if this is a good idea or not though, I'm hoping to understand how that would impact other UniFFI users.

[jplatte]

Right.. Well I don't foresee needing to import external FfiConverter impls, so I would much prefer to keep the current solution where the uses of external types with "first-party" FfiConverter impls don't require annotation.

[bendk]

You've convinced me (maybe again?, sorry about going back and forth on this one so much).

We should generate the generic impl in all cases, there shouldn't even be a flag. This keeps things simple and consistent.

The concern about remote/external types can still be handled:

• We implement some system like serde(remote) where you copy the struct/enum definition to define FfiConverters. This is the one time when we generate the local version (impl FfiConverter<crate::UniFfiTag>).
• To use the type in a second crate you need to call some sort of macro. Maybe it would just be a top-level call rather than wrapping the use statement (uniffi::remote_tag_from!(my_other_crate, RemoteType);).

[stan-irl]

@bendk I tried using your new library mode but the procmacro externals still fail to generate with ComponentInterface consistency error. the CLI is hitting this error.

Does this sound like user error on my part or is that expected?

[stan-irl]

I put together a minimal repro here: https://github.com/stan-irl/external-procmacro-repro

[bendk]

I don't think library mode solves all of these problems, so it's still expected. But thank you for putting together that example. I hope to fix this soon and I'll make sure I check against that.

[stan-irl]

Is correct at this point to say that there is no way to return an external UDL defined dict/object from a procmacro defined function?

[bendk]

Is correct at this point to say that there is no way to return an external UDL defined dict/object from a procmacro defined function?

Unfortunately, that's the current state.

[stan-irl]

no worries. i just wanted to clarify because i've been playing around with these for a couple of days

[bendk]

#1600 should fix this.