Make UniffiTag consistent for UDL and procmacros

[mhammond]

Currently procmacros all have a blanket implementation of FfiConverter whereas UDL uses a local "tagged" implementation (ie, FfiConverter<crate::UniFfiTag>. This inconsistency causes problems with "external" types - in some cases it is impossible to know whether a given type name is "tagged" or not, and thus impossible to generate working code for these types (eg, #1854) without significant and undesirable work.

I see 3 solutions

Remove UniFfiTag completely.

@bendk and I strongly prefer this option as it would remove a ton of code and complexity. Only problem is that we can't work out how to make this work. #1863 is our attempt, which gets tantalizingly close, but fails with "custom types" - whether implemented via UDL or via procmacros.

In that scenario we can't implement FfiConverter<Url> because neither the trait nor the type are defined in our crate. We've tried a few things (eg, trying to use a newtype wrapper etc) but none of them play out.

Use UniFfiTag for procmacro implementations

This is fairly easy. It leaves the complications and duplication in the code but does work. Custom types work fine because the FfiConverter implementations are tagged with a local type so don't fall foul of Rust's rules.

The biggest downside here is that it means all external types from procmacros must be explicitly/magically imported before they can work (in the exact same way UDL types must today). Eg, when using a type from another crate it will be necessary to:

uniffi::use_udl_record!(uniffi_sublib, SubLibType);

Before the type can be used. (Obviously we'd change the name of the macro so it's not UDL specific, and we can probably even make it do the actual use if we want, but the spelling here isn't the point, it's the fact it's needed at all)

This is unfortunate (and another reason we'd prefer to kill the tag entirely), is a breaking change for external types from procmacros, but has the killer advantage of actually working! Plus it is consistent (ie, the requirements are the same regardless of how the type was implemented)

You can see this in #1864

Lean into the difference

Making things work when some types are tagged and some are not would probably mean adding a new bool to all metadata to indicate whether the type is tagged, and means that many things would need 2 ways of doing things. However, this is still an odd and confusing distinction for users - they'd need to learn about the difference where details like this should be hidden.

IMO this option is significantly worse than either of the above.

Help?

We have a preference, but can't make that preference work. So unless we can work out how to make that preference work we will probably be forced to accept the 2nd best option. We'd love feedback and help! cc @jplatte in particular!

[bendk]

Thanks for writing this up! We should definitely figure this one out.

One other option would be to make the blanket impl (impl<UT> FfiConverter<UT> for MyType) the default for UDL as well as proc-macros. This is sort-of the reverse of Use UniFfiTag for procmacro implementations.

The downside here is that remote types like Url would need some sort of special handling in the UDL.

[bendk]

I'm currently leaning toward: Use UniFfiTag for procmacro implementations.

The only downside of this one is that it requires users to explicitly annotate which external types they're using -- either with some sort of uniffi::use_type! macro or by wrapping the use statement in an attribute.

First off, that doesn't seem like that big of a downside to me.

Secondly, I just filed #1872, where one crate want to import an external type but doesn't use it in an exported function. This seems like pretty legitimate use-case to me. If we want to support it in proc-macros, then users would need to need to use some sort of syntax like that.

I guess we could make it so that you only needed to wrap the use statement if that type wasn't used in a function, but that doesn't feel right to me.

[jplatte]

One other option would be to make the blanket impl (impl<UT> FfiConverter<UT> for MyType) the default for UDL as well as proc-macros. This is sort-of the reverse of Use UniFfiTag for procmacro implementations.

The downside here is that remote types like Url would need some sort of special handling in the UDL.

I much prefer this over the other options presented, but I'm actually a little bit unclear on the problem being solved here. Sure, consistency is nice but are there actual problems with the current approach? I might have missed it when initially reading the issue description, I'm currently rather busy but that should change soon.

[mhammond]

The actual problem here is trying to use "transitive" external types. Eg, crate1 defines Type1, crate2 defines a Dict1 dictionary which holds a Type1, then crate3 uses Dict1 as an external type - crate3 doesn't know if Type1 is tagged or not.

One way we try to work around this is that Type::External has an is_tagged bool, and this starts to fall apart in the above scenario. That scenario is what forces this test code to be commented out

The more theoretical concern here is that needing to know whether an external type is tagged or not just seems very smelly - the types behave differently in ways that aren't intuitive - UDL needs different techniques to refer to them, crates need different techniques again, and the fact that Type::External needs that bool just seems wrong and inconsistency for inconsistency sake.

Or to ask the question in another way - what is the justification for not having them be the same?

[bendk]

I think they should certainly be the same, the only question in my mind is if we should make UDL match the proc-macro approach or the proc-macros match the UDL approach. I tried to frame the question in the above ADR, maybe we could discuss further there.

[bendk]

I've been hacking away at this for a week or so and I think I've figured a couple things out.

First off, I think UniFfiTag is a good solution to the complexity we face. There are ways to eliminate it, but they lead to more overall complexity. For example, we could define a macro that handles converting remote types to something that works with the rest of the traits and then remove the tag from all the traits. That removes some boilerplate from the implementations for simple types like u8 and String, but the simplification is minimal. Container types like Record/Enum aren't that bad once you do something like #2083, but they're not any easier either. Then you get to generic types (Option, Vec, and especially Result) and the complexity increases compared to the status quo because you have to figure out how to call the macro correctly for the inner type. Then you have to consider what if one of the inner types is defined in another crate and my brain starts to melt. I tried several solutions and they all ended up worse.

Secondly, remote types are useful and we should support them. The fact that #1593 was opened indicates to me that teams are currently using them in the wild. Furthermore, the team at Mozilla is trying to figure out how to use anyhow::Error in our type signatures, which is best expressed as extending remote types to also support interfaces.

It's possible to reduce remote types to a type conversion, like how serde does it, but I don't think it's a good idea. Note that serde solution gets a bit clunky because it needs to explain which type gets used as the struct field. This issue would be much worse for UniFFI since the types are used in many more places, including in the foreign code. I really don't like the idea of having to define a type in your crate root, but then tell users to never use that type even though it exactly mirrors the correct type.

At this point, I think the best solution is to make UDL work like the proc-macros and blanket implement the ffi traits for all tags by default (AKA Jonas's solution). It's the most convenient and if you have multiple crates share a bunch of types there could be a significant difference. My main worry about this was how to explain to users that you need to do something special because UniFFI only implemented the ffi traits for the local tag. However, if remote types become more of a first-class thing, then there's a fairly straightforward and intuitive answer: you need to do the special thing if you want to share remote types between multiple crates.