mozilla / uniffi-rs

a multi-language bindings generator for rust
https://mozilla.github.io/uniffi-rs/
Mozilla Public License 2.0
2.83k stars 231 forks source link

Interface/Protocol use in foreign generated code #2189

Closed dowski closed 1 month ago

dowski commented 3 months ago

Thanks for uniffi, I'm using v 0.28 and it's been working out well in my early explorations. I've hit a snag though.

If I create a UDL like:

namespace sample {};

interface Foo {
  Bar doBar();
};

interface Bar {
  void doNothing();
};

it seems like the concrete type is used in the generated Kotlin FooInterface code, something like:

public interface FooInterface {
  fun `doBar`() : Bar

Is there any way to tell uniffi to use BarInterface in place of Bar in the generated FooInterface?

mhammond commented 3 months ago

It looks like #1352 tried to do exactly this. I'm not sure if that regressed or was never implemented.

dowski commented 3 months ago

Interesting, thanks! I can poke at it a bit more. In my real world case, I think it's using async methods so maybe there's some slight difference there (or like you said, there was a regression at some point).

jmartinesp commented 3 months ago

It looks like #1352 tried to do exactly this. I'm not sure if that regressed or was never implemented.

Never implemented, what was implemented instead is that Bar is now open. My first implementation returned interfaces and it seemed to work, but it was way more complex than the final one and I think it could become a problem for some use case I can't remember now or find the comment that pointed it out. Maybe it was related to existing mocking strategies.

EDIT: it turns out I was thinking about this, which was initially part of the solution that was implemented, nothing actually related to this. So I think in the end it was just a matter of how complex the new code would be and how hard it would be to maintain it.

dowski commented 3 months ago

Thanks for the insight. I can work with the open class implementation. Feel free to close this if it's not feasible.

mhammond commented 1 month ago

Feel free to close this if it's not feasible.

I had a bit of a look and it is feasible. It's a bit tricky because we can only do it for structs, not callbacks/traits, so it gets a little messy. It will never be feasible for args. I wonder how much of a win just these return types would be?

The open-class/NoPointer story seems pretty good and side-steps this - so does this actually make sense to do?

jmartinesp commented 1 month ago

To be honest, I still have the feeling that it makes little sense to have both an interface that you can't return from any fake implementation of another of these interfaces and then an open class that can.

In my opinion, either we can use the interface with other interfaces, which is the most usual use case in our project (maybe not in others, but I don't really see how) or we should have just the open class because having both the interface an the open class is quite confusing. In fact, a colleague of mine tried to use the interfaces everywhere in our source code because it's 'the right/normal thing to do' in a java/kotlin context only to discover it wouldn't be possible to use them at all in the end because for a generated interface FooInterface another BarInterface would have a method like:

interface BarInterface {
    fun doSomething(): Foo // Can't cast FooInterface to Foo to be used here
}

As for passing the interfaces as parameters, I think my changes in the PR that was closed just made it fail on runtime, which may not be ideal but if you're passing some JVM implementation to Rust in most contexts you're doing something quite weird, and it's not really that different to the NoPointer implementation in that sense if I'm not mistaken, it's just less hidden.

dowski commented 1 month ago

Yeah, for clarity, having one or the other sounds good.

mhammond commented 1 month ago

only to discover it wouldn't be possible to use them at all in the end because for a generated interface FooInterface another BarInterface would have a method like ...

Yeah exactly - this is what I meant by "never be viable for args" and also why this can't be used even for return values for callbacks etc. I saw failures at build time rather than build time, but if I tried to dynamically downcast I'm sure I could defer failure to runtime - but see no way of actually avoiding failure.

Thanks for both comments - I'll close this out for now, but that doesn't mean we can't revisit something like this in the future.