-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove FfiType::canonical_name()
#1592
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except I think Swift and Kotlin should have their own canonical_name
function rather than using ffi_type_label()
.
I feel like there's some future where some of this could be shared, but it's not clear to me exactly what that is. As a first step, it makes sense to untangle this code.
@@ -61,7 +61,7 @@ typedef struct RustCallStatus { | |||
|
|||
// Callbacks for UniFFI Futures | |||
{%- for ffi_type in ci.iter_future_callback_params() %} | |||
typedef void (*UniFfiFutureCallback{{ ffi_type.canonical_name() }})(const void * _Nonnull, {{ ffi_type|header_ffi_type_name }}, RustCallStatus); | |||
typedef void (*UniFfiFutureCallback{{ ffi_type|ffi_type_name }})(const void * _Nonnull, {{ ffi_type|header_ffi_type_name }}, RustCallStatus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are certain types that this would fail for and I think they might occur in practice. For example ForeignCallback
would end up as UniFfiFutureCallbackForeignCallback _Nonnull
and that might break returning a callback interface in an async function.
I think there should be a separate Swift ffi_canonical_name()
function that ensured the end result was always a valid swift class name. I'm not totally sure though, I can't really think through all of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that might break returning a callback interface in an async function.
Hrm - it's complicated to know because IIRC, no functions can return callback interfaces anywhere, right?
I think in this implementation I could just have this arrange to call ffi_converter_raw()
which exists purely to kill these _Nonnull
annotations? Which I think highlights why this PR makes sense - this consideration is 100% swift specific.
I'll mull on this some more though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm - it's complicated to know because IIRC, no functions can return callback interfaces anywhere, right?
Rust can't return them, but I was thinking of a callback interface method that returned a callback handle. However, that's not currently an issue either since we don't support async callback interfaces.
Seems like this would be a good thing to fix to prepare for the future, but it's not really critical.
These exist in `uniffi_bindgen::interface` but is only used internally by kotlin and swift - they're not used by the rest of `uniffi_bingen` or any other bindings. It creates a complicated relationship between the bindings and the core, so it's better the complications live purely inside the bindings that need it.
0212d1d
to
42fed92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this one so we can get it in 0.24. I still feel like we'll need to revisit that issue of callback handle returns, but we can do that in the future and the fix will be local to swift.
I thought I fixed that by calling |
This exists in
uniffi_bindgen::interface
but is only used internally by kotlin and swift - it's not used by the rest ofuniffi_bingen
or any other bindings.It creates a complicated relationship between the bindings and the core, so it's better the complications live purely inside the bindings that need it.