Document all `callback interface` method arguments must be owned, fail if `[ByRef]` is specified for them in UDL.

[ijc]

The docs indicate that [ByRef] string should work for &str and #229 points in that direction too.

However it seems both of these are referring only to interface and not to callback interface.

If I add such a case to the fixtures:

Click for diff

```diff diff --git a/fixtures/callbacks/src/callbacks.udl b/fixtures/callbacks/src/callbacks.udl index 0a19c27f..8c02d763 100644 --- a/fixtures/callbacks/src/callbacks.udl +++ b/fixtures/callbacks/src/callbacks.udl @@ -54,6 +54,7 @@ interface RustGetters { //interfaces that don't support them. callback interface StoredForeignStringifier { string from_simple_type(i32 value); + string from_string_ref([ByRef] string value); // Test if types are collected from callback interfaces. // kotlinc compile time error if not. string from_complex_type(sequence? values); diff --git a/fixtures/callbacks/src/lib.rs b/fixtures/callbacks/src/lib.rs index 27402f9d..f1f1cce5 100644 --- a/fixtures/callbacks/src/lib.rs +++ b/fixtures/callbacks/src/lib.rs @@ -98,6 +98,7 @@ impl Default for RustGetters { #[allow(clippy::wrong_self_convention)] trait StoredForeignStringifier: Send + Sync + std::fmt::Debug { fn from_simple_type(&self, value: i32) -> String; + fn from_string_ref(&self, value: &str) -> String; fn from_complex_type(&self, values: Option>>) -> String; } ``` (note this just defines the method but doesn't fully wire it up for testing)

then I see the following:

$ cargo test -p uniffi-fixture-callbacks
   Compiling uniffi-fixture-callbacks v0.22.0 (uniffi-rs/fixtures/callbacks)
error[E0053]: method `from_string_ref` has an incompatible type for trait
   --> uniffi-rs/target/debug/build/uniffi-fixture-callbacks-277c0187d029528f/out/callbacks.uniffi.rs:585:22
    |
585 |             r#value: String) -> String{
    |                      ^^^^^^
    |                      |
    |                      expected `&str`, found struct `String`
    |                      help: change the parameter type to match the trait: `&str`
    |
note: type in trait
   --> fixtures/callbacks/src/lib.rs:101:38
    |
101 |     fn from_string_ref(&self, value: &str) -> String;
    |                                      ^^^^
    = note: expected fn pointer `fn(&UniFFICallbackHandlerStoredForeignStringifier, &str) -> String`
               found fn pointer `fn(&UniFFICallbackHandlerStoredForeignStringifier, String) -> String`

My guess is that the docs didn't consider the callback case (which is fair enough, it looks like that chapter predates them...).

I understand that to cross the FFI the values pretty much have to be owned[^0] and it's easy enough to workaround by making the trait use a String but that means that I need a load of to_string etc in my Rust code which starts to look a bit non-idiomatic especially when the argument is a &'static str.

Would it be possible to push the to_string down into the autogenerated shim somehow?

Perhaps even a more general ToString or ToOwned based scheme might be possible and cover more types than just String/str?

[^0]: Maybe not strictly true, but if not then I suspect the complexity which would be needed to do it safely effectively rule it out.

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

[ijc]

I subsequently realised that I'm also using async-trait which is also unsupported for callbacks and so I needed to implement a wrapper type anyway which can deal with the &str vs String thing at the same time.

I won't close straight away since I think the feature request is still valid in the general sense, but feel free to close.

It might still be good to clarify the docs about the bounds on the support for ByRef though?

[mhammond]

Callbacks cause uniffi to generate a struct which implements the trait, so the signature of that is probably what also makes that tricky. Plus any fix here wouldn't help procmacros - so I think this is probably best treated as a bug in the docs.

[ijc]

FWIW I'd be ok with adjusting the trait to use ToString or ToOwned or whatever.

But I agree that this is probably best thought of as a docs thing.

[araujo-luis]

Having the same error, @ijc can you share the wrapper type to handle &str vs String?

[ijc]

It's not some clever generic thing, it's literally a bespoke wrapper type with a to_string in it.

So, My project has a trait T with a method f(s: &str). For use with Uniffi I have a trait U which is identical to T but using String, so it has f(s: String). U is what is exposed via FFI not T.

Then I have a struct TShim(Box<dyn U) and impl T for TShim with:

fn f(s: &str) {
    self.0.f(s.to_string())
}

and as needed I stick an instance of U into a TShim before passing it on, the core code continues to work in terms of T.

[gpeacock]

Generally speaking it looks like [ByRef] is just ignored in callback parameters. If it isn't supported it should be documented as such and cause an error. [ByRef] string is treated as string, [ByRef] bytes is treated as Vec.

[gpeacock]

This is a real problem for me since I'm trying to have a Write operation as a callback, which is just trying to read from bytes, but this forces me copy the entire bytes array instead of referencing it.

[gpeacock]

I think it is good to document and report on the current state, but does that mean this feature will never be implemented? Forcing a copy here makes this very inefficient for streaming. I'll file another issue as a feature request so we can track that.

[mhammond]

IIUC, any such proposal would require using "borrowed" foreign memory after the foreign call returns. If that's actually the case, then I suspect it will never be implemented. If I'm misunderstanding your use-case, can you please elaborate?

[mhammond]

Oh, I think I have it back-to-front - you want to pass a &[u8] to the foreign code for it to write to disk or similar? It does raise some questions about the mutability of that data (eg, in this use-case we expect the foreign side to treat the memory as read-only, but other use-cases might want it to be treated as mutable), but the use-case does make some sense.