Skip to content
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

Potential approach to support external errors #2265

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Hinton
Copy link
Contributor

@Hinton Hinton commented Oct 11, 2024

This is a bit of a stab in the dark attempt at supporting external errors in uniffi.

It currently seems to behave well for swift but before I continue diving into the uniffi internals I would appreciate some guidance if this is the appropriate way to address this as the internals of uniffi is a large mystery to me.

Remaining tasks:

  • Support kotlin
  • Fix any broken tests.
  • Write new tests

@mhammond
Copy link
Member

I'm mildly surprised we don't support this, but it looks like we do not! A very quick look implies this seems sane for Swift as it's what we did for other uses for external types - however, we'd obviously want tests here. I'd suggest maybe reusing the ext-types fixture - eg, there's a GuidError in the guid crate which you might be able to have a top-level function throw.

@Hinton
Copy link
Contributor Author

Hinton commented Oct 14, 2024

I wrote up a test case for the proc-macro which works as expected. Unfortunately I ran into a few issues with udl. With the latest commit I get the following error message:

error[E0277]: the trait bound `GuidError: LowerError<UniFfiTag>` is not satisfied
   --> /target/debug/build/uniffi-fixture-ext-types-dac13a3e9f364522/out/ext-types-lib.uniffi.rs:253:1
    |
253 | #[::uniffi::export_for_udl]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    | |
    | the trait `LowerError<UniFfiTag>` is not implemented for `GuidError`, which is required by `Result<(), GuidError>: LowerReturn<UniFfiTag>`
    | in this procedural macro expansion
    |
   ::: /uniffi_macros/src/lib.rs:203:1
    |
203 | pub fn export_for_udl(attrs: TokenStream, input: TokenStream) -> TokenStream {
    | ---------------------------------------------------------------------------- in this expansion of `#[::uniffi::export_for_udl]`
    |
    = help: the trait `LowerError<ext_types_custom::UniFfiTag>` is implemented for `GuidError`
    = help: for that trait implementation, expected `ext_types_custom::UniFfiTag`, found `UniFfiTag`
    = note: required for `Result<(), GuidError>` to implement `LowerReturn<UniFfiTag>

My suspicion from debugging the expanded udl file is that it's missing a ffi_converter_forward for GuidError. Unfortunately I'm a bit outside my knowledge depth, but I suspect we need to wire up some logic in uniffi_udl/src/attributes.rs to ensure external errors are appropriately imported?


I also experimented slightly with Kotlin which in some ways seems to also be quite straightforward as we should only need to add an import to the Exception. Unfortunately my initial attempt of adding a add_import function call inside uniffi_bindgen/src/bindings/kotlin/templates/macros.kt failed with no method named `add_import` found for reference `&KotlinWrapper<'a>` in the current scope.

Curious if you have any quick thoughts on a better approach.

@mhammond
Copy link
Member

My suspicion from debugging the expanded udl file is that it's missing a ffi_converter_forward for GuidError

Yeah - I suspect you need ::uniffi::ffi_converter_forward!(GuidError, ext_types_custom::UniFfiTag, crate::UniFfiTag); where it's imported.

uniffi_bindgen/src/bindings/kotlin/templates/macros.kt failed with no method named `add_import` found for reference `&KotlinWrapper<'a>` in the current scope.

The templating system is tricky, but that should work if you move the call out of macros.kt

A quick look though shows you seem to be on the right track!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants