Support references in foreign traits somehow

[mhammond]

Discussed a little with @linabutler and @bendk about this papercut. It exists for functions too, but seems more of a PITA for traits.

Eg, you can't use this as a foreign trait

55 | #[uniffi::export(with_foreign)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | |
   | expected `&str`, found `String`
   | help: change the parameter type to match the trait: `&str`

which seems unfortunate, even though it's tricky. I can't see an issue for this, so here's one!

[linabutler]

I promised I'd file an issue about this...and didn't, oops! 😬 Thanks for starting the discussion!

References in traits (and functions) feel tricky: they're definitely more ergonomic for Rust callers, but they hide a "surprise copy" for foreign callers. I think most folks would expect that an &T argument would avoid a copy, but the scaffolding actually creates an owned T, passes &T as an argument, and then destroys T.

(If your function or trait method wanted to conditionally take ownership of the &T, it would need to do another copy, leading to a weird incentive for such a function to take an owned T instead of &T).

I wonder if we should think about:

1. Disallowing &T arguments in UniFFI-exported functions and traits, but...
2. Allowing Cow<'_, T> arguments. (I also really liked @bendk's suggestion to allow impl Into<Cow<'_, T>>).

That way, the scaffolding could always pass a Cow::Owned(T), and a Rust caller could pass a Cow::Borrowed(T) or a Cow::Owned(T). I think this would work for foreign traits, too: a trait method can take a Cow<'_, T>, and the scaffolding code that calls into the actual foreign implementation can take ownership of the Cow via into_owned().

Cow doesn't have a blanket From<&'a T> implementation, but it does implement From for owned and borrowed strings, paths, and slices, so a Rust caller would be able to write, say, func("foo".into()) instead of func(Cow::Borrowed("foo")).

I'm not sure that we can fix this "for real", and make cross-language references work properly 😔 Foreign objects—especially in the languages we're targeting—can't uphold Rust's "aliasing XOR mutability" guarantee, in either direction (C / C++ could, maybe, if we were very careful?)...and if we're passing a dictionary or a list, we need to serialize it, anyway.

WDYT?

[mhammond]

References in traits (and functions) feel tricky: they're definitely more ergonomic for Rust callers, but they hide a "surprise copy" for foreign callers. I think most folks would expect that an &T argument would avoid a copy, but the scaffolding actually creates an owned T, passes &T as an argument, and then destroys T.

I see your point, but I'm not sure I entirely agree with the severity of the status quo.

Firstly, in this specific scenario, I don't really think it necessarily adds any new surprises, at least for foreign callers of these traits. You could potentially argue that foreign implementations of these traits might expect a reference there too, but I'm not particularly convinced that's a bigger surprise than they already have - ie, I don't think that trivial patch I noted above introduces more surprises than the non-foreign trait example has. Therefore, while we continue to support that capability today for non-foreign traits, I can't see a good argument for also not supporting it for foreign traits (unless, of course, we agree to remove that existing capability in the short term, which I'm not really sure we are in a position to do)

Secondly, in the general case, I'm also not particularly convinced we are surprising many users. I haven't really seen evidence of many users believing that a rust function taking an argument by reference will somehow allow foreign code to just pass references. And even if some are, I don't think it's so confusing that better documentation can't help there. While I'd agree that not supporting it would make sense if there was no concrete value in the support, I think supporting these args do have value because you can use more natural (and possibly your existing) Rust code (eg, our existing BridgedEngine trait, which I'm still hoping to use as a foreign trait, does use byref args). In some ways I see this not too dissimilar to being able to use &self. In other words, I'm asserting that the small surprise to some users is justified by the added convenience for the majority.

[mhammond]

But we can have both, right? We should be able to sniff out a Cow and carry a flag in the metadata to the bindings?

[bendk]

I feel very on the fence about this one, but I think I'm leaning towards "it's okay". Like Mark says, it seems fine to say that passing things across the FFI will likely require copying, even if you have a reference. You could even say the same thing happens with owned values. If I pass a Record value, UniFFI is going to make 2 copies that wouldn't happen with a normal Rust call -- one to serialize and one to deserialize.

[linabutler]

Thank you both! 😊

I'm not sure I entirely agree with the severity of the status quo.

Oh, definitely; I don't think it's a big problem now! I've seen tickets like #2245, #2149, #2014, and #1974 fly by, plus the fact that an &T argument to a foreign trait method will always be copied, got me thinking...what would it look like if we went the other way, and made the ownership more explicit?

In some ways I see this not too dissimilar to being able to use &self. In other words, I'm asserting that the small surprise to some users is justified by the added convenience for the majority.

I like your analogy to &self, but that feels a little different to me: an &self method called from a foreign language will increment the Rust-side Arc's strong reference count; it won't clone the object, call the &self method on it, and destroy it.

I think a closer analogy would be whether we allow &self (vs. self) methods on dictionaries and enums, since those are always copied when passed over the FFI (#1470, https://github.com/mozilla/uniffi-rs/issues/1935#issuecomment-1917736569), if we ever support them at all.

But we can have both, right?

I think so; that sounds super useful! 😊 That way:

• Mixed-language (Rust and foreign) callers who know they won't need to take ownership, or who are OK with an extra .clone(), can write &T to have an idiomatic interface for Rust.
• Callers who want to unconditionally take ownership of an argument can write T.
• Callers who want to conditionally take ownership, and want to avoid that extra .clone(), can write Cow<'_, T>.

[oriongonza]

Firstly, in this specific scenario, I don't really think it necessarily adds any new surprises

Reporting in as one of the surprised users. The reasoning was as follows: If uniffi supports non 'static data that means that it has to efficiently support them by some mechanism. If not, it would force us to use owned data as it's the most ergonomic option in the long term.

It was not obvious until much later that uniffi cloned everything, and I think that it's bad to allow "fake" references in the first place.

I think so; that sounds super useful! 😊 That way:

Mixed-language (Rust and foreign) callers who know they won't need to take ownership, or who are OK with an extra .clone(), can write &T to have an idiomatic interface for Rust. Callers who want to unconditionally take ownership of an argument can write T. Callers who want to conditionally take ownership, and want to avoid that extra .clone(), can write Cow<'_, T>.

I'm not sure if this is the way to go. As a user I solve this by having a uniffi version and a Rust version, each taking an owned, Cow or reference as needed. This is often the way to go anyways since I need to start all async calls in a tokio task anyways to unhook myself from the client's runtime.

I would try to avoid adding even more magic to uniffi!

[mhammond]

It was not obvious until much later that uniffi cloned everything, and I think that it's bad to allow "fake" references in the first place.

To be clear, you had, eg:

#[uniffi::export]
fn take_record(r: &Record) { ... }

and were surprised that (say) Kotlin wasn't passing a reference to a Rust Record struct?

I would try to avoid adding even more magic to uniffi!

I agree with that. I meant to come back to this issue, but my original proposal here is really just extending "surprises" we added in the past to foreign traits - eg, that take_record function can be used today everywhere other than foreign traits, so I'm still mildly in favour of allowing it there too for consistency. I believe the Cow proposal goes way beyond that and might be a path to actually passing references around, but I don't think it's worth pursuing at this time.

[oriongonza]

and were surprised that (say) Kotlin wasn't passing a reference to a Rust Record struct?

Yes, I had expected that it was something like a &'GC Record

[mhammond]

Yes, I had expected that it was something like a &'GC Record

that's interesting - it implies your mental model is that there really is a Rust struct in Kotlin rather than a Kotlin representation of the struct? I wonder if there's an opportunity for our docs to clear this up.

[skeet70]

It may be nice to call this out in the documentation somehow, I'm not sure the best place. The compiler message in the issue is nice, but the one I got wasn't as direct:

error[E0195]: lifetime parameters or bounds on method `post_json` do not match the trait declaration
  --> src/tenant_security_client/request.rs:40:1
   |
40 |   #[uniffi::export(with_foreign)]
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |   |
   |   lifetimes do not match method in trait
   |   in this procedural macro expansion
...
46 |       async fn post_json(
   |  ______________-
47 | |         &self,
48 | |         url: String,
49 | |         json_body: String,
50 | |         headers: &AlloyHttpClientHeaders,
51 | |     ) -> Result<AlloyHttpClientResponse, AlloyError>;
   | |_____- lifetimes in impl do not match this method in trait