Custom Types using bytes lead to duplicate declarations in bindings

[OtaK]

Hi!

We just came across an issue where if you have a Custom type that is typealiased to the new bytes AND it appears in function parameters / other structs, UniFFI will default to an equivalent to sequence<u8> and emit a double declaration of the custom type in the generated bindings.

This affects all binding target languages (I tested Swift, Kotlin and Python, all are affected).

I setup a minimal repro repository over here: https://github.com/OtaK/uniffi-custom-bug

Cheers,

[badboy]

I added your example as a regression test here: e9193ca

I'm getting a different error though:

thread 'uniffi_foreign_language_testcase_test_kts' panicked at 'assertion failed: `(left == right)`
  left: `External { module_path: "UniffiCustomBug", name: "CustomType", kind: DataClass }`,
 right: `Custom { module_path: "UniffiCustomBug", name: "CustomType", builtin: Bytes }`', uniffi_bindgen/src/interface/universe.rs:42:17

So something else already changed, not necessarily for the better though

[OtaK]

Ah yes, I forgot to mention that I also tried the git version and I was getting such an error, preventing me from producing any kind of meaningful result beyond a panic trace :/

[badboy]

That assertion was part of 044b85d

[mhammond]

I think some of the behaviour change is from #1600 and specifically this code doing a conversion, and 044b85d probably made it worse because that code is now run for UDL whereas before it was not . I'll see what I can do.

[mhammond]

There are (at least) 2 things going wrong here:

• First is as reported against the released version: The core problem is that bytes is really just an alias for Vec<u8>, so when the procmacro reports the type of the param, it has lost all context about whether it was originally bytes or the Vec<>. I suspect this is fixable, but in the meantime I suspect sticking with the Vec<> should be a workaround

• The second is due to some confusion with the various names used here, and in particular, because the type is defined in UDL but referenced via a procmacro. The short version of this is that stuff defined in a procmacro uses the crate name¹ as the "module path" - so in this example, the procmacro effectively assumes it is in uniffi_regression_test_duplicated_custom_type (because that's the lib name being built) whereas the UDL thinks it is in the namespace defined in the UDL (ie, UniffiCustomBug) Because these names do not match some other code assumes it must be externally defined so tries to fix one of them up, and things fall apart. This can be worked around in the example by changing the UDL to specify the namespace as uniffi_regression_test_duplicated_custom_type - once that's done we then are left with "just" the first problem above - the behaviour has changed from the initial report:

thread 'uniffi_foreign_language_testcase_test_kts' panicked at 'assertion failed: `(left == right)`
  left: `Custom { module_path: "uniffi_regression_test_duplicated_custom_type", name: "CustomType", builtin: Sequence { inner_type: UInt8 } }`,
 right: `Custom { module_path: "uniffi_regression_test_duplicated_custom_type", name: "CustomType", builtin: Bytes }`', uniffi_bindgen/src/interface/universe.rs:42:17

(ie, it's now an error rather than generating the dupes) but it's the same underlying problem.

I'm really not sure how to solve that second problem - I think it's clear that the procmacro code and udl code in the same crate need to agree on what various names are, but I'm not sure how exactly. When build.rs is being run as don't actually know what the crate name actually is. I think it might be necessary to introduce a new generate_scaffolding function which takes that final name, but we might also choose some other option (eg, all the procmacro code calls a certain attribute module_path but it's not a module path at all - except on Nightly - which seems like a future foot-gun we should take care of sooner rather than later.

¹ It's not even clear to me it's the crate name - there are other examples which can demonstrate it must match the lib name rather than the package name, which also confuses me - but in this particular example those 2 names do indeed match, so that's not directly relevant here.

[mhammond]

The core problem is that bytes is really just an alias for Vec, ... I suspect this is fixable

I'm actually not sure how to proceed here. procmacros simply have no concept of bytes and I'm not sure how it should learn about them.

Eg, it's impossible to export a fn which takes bytes as a param or return value - while the Rust side doesn't care (ie, using Vec<u8> works fine on the Rust side of the world) it's important that the bindings see the distinction because they are different types there.

[OtaK]

I think #1638 might be the way forward. I'm not very versed in proc_macros in general but it might be possible through forcing the type in proc macros to force disambiguation.

Because yeah, I see the issue. Either there are drastic changes in how UniFFI computes types (i.e. struct declarations are processed before functions/methods and cannot be replaced from refs in functions) or we could let the user sort out the conflicts themselves by steering the proc macros in the right direction.

Also, very honestly, I don't see any use case where sequence<u8> would be resolved to an array of u8 on the target platform. You'd always want it to be effectively bytes.