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

Add support for returning and handling errors #7

Closed
linabutler opened this issue Jun 24, 2020 · 5 comments · Fixed by #31
Closed

Add support for returning and handling errors #7

linabutler opened this issue Jun 24, 2020 · 5 comments · Fixed by #31
Assignees
Labels
blocks-nimbus Blocks Nimbus SDK on mobile get involved Good opportunity for deeper-dive onboarding

Comments

@linabutler
Copy link
Member

linabutler commented Jun 24, 2020

We have lots of TODOs for adding error handling. A few things we’ll want to think about:

  • How do we represent errors in WebIDL? As Result return values? Following how Firefox does it, using a Throws annotation on fallible methods? Do we want to have structured errors that can be inspected, or are error strings good enough?
  • In the FFI, we’ll need to have fallible methods take an error out parameter.
  • Swift and Kotlin specify fallible functions differently—Swift has the throws keyword to indicate a function is fallible, and Kotlin and Python functions can throw exceptions. We will need to teach our code generation layer to reflect error results as exceptions.

┆Issue is synchronized with this Jira Task

@linabutler linabutler added the get involved Good opportunity for deeper-dive onboarding label Jun 24, 2020
@rfk
Copy link
Collaborator

rfk commented Jun 24, 2020

Following how Firefox does it, using a Throws annotation on fallible methods?

This was my first instinct as well.

But I also have to be honest, I've been spending my evenings exploring what it would look like to use our own custom variant of an IDL rather than just relying on what's available in WebIDL...which might come to nothing or might change how we want to proceed here. We should not block on that though :-)

Do we want to have structured errors that can be inspected, or are error strings good enough?

I think we need some structure, e.g. in the fxa-client the kotlin code needs to be able to distinguish between a network error and a hard failure.

Strawman: [Throws(EnumName)] would annotate a function/method that can throw, and the resulting error would have both a string and an integer errcode that maps to the specified enum. This is basically how our existing ExternError class works in the hand-written bindings IIUC, except you have to know the possible range of error codes via out-of-band means.

@tarikeshaq
Copy link
Member

tarikeshaq commented Jul 24, 2020

I'm thinking of taking a look at this, but before I go in over my head, I wanted to dump my thought process in case I have something off (or maybe there are blockers I haven't thought about or something)

The WebIDL would look something like this:

enum Error {
   "NETWORK_ERROR", "TYPE_ERROR"
};

interface InterfaceName {
   constructor();
   [Throws(Error)]
   string func();
};

This should give us all the information needed to form the scaffolding and the bindings (right?)

Edit: Guess we'd need to add the same Option for Function and (maybe) Constructor

From the scaffolding side

Bindings

Each binding will be different but the concept is the same

  • Detect errors from the ffi call
  • appropriately propagate the error

Haven't really looked at the details here and it might be painful, but if the details of the error are in the Method struct, then I think we have all the info needed

Thoughts? Anything large I miss? Or is there a different path you'd rather we go with? Any feedback is welcome!

@linabutler
Copy link
Member Author

That sounds perfect! 💯

On the Rust side, we can use the Error enum variants as names for the error_codes module, and leave it up to the consumer to implement From<MyErrorType> for ExternError—or are you thinking of autogenerating the Rust-side errors, too, and having the crate use the autogenerated errors directly?

Swift has fallible functions, methods and constructors; plumbing the throws Option<T> through to them seems like it would work perfectly, plus generating the Swift-side Error enum. Kotlin doesn't need to specially annotate fallible functions at all; we can transform the RustError into a Kotlin error and throw directly.

@tarikeshaq
Copy link
Member

Thanks @linacambridge!!

For

On the Rust side, we can use the Error enum variants as names for the error_codes module, and leave it up to the consumer to implement From<MyErrorType> for ExternError

I was playing around and one thing I tried is to let the consumer define their own Rust Error enum. The Rust enum exactly matches the IDL one and has to be named Error. Then, we generate the From<Error> for ExternError. The upside is that the user doesn't have to know about ExternError, but there are a few downsides, a couple off the top of my head:

  • We have to find a way to mark errors when we parse the IDL, right now I just assume that there will be exactly one Error enum and its name is Error (similar to how we have only one in fxa_client and such), but we can use attributes or something to allow more flexibility there I guess.
  • The consumer has to abide by the limitations of WebIDL enums, (biggest one I think is that they are forced to only have flat non-struct variants with the same names from the IDL).

I guess it's a matter of flexibility vs if we want the consumer to know about ExternError

Anywhoo, I'll put up my WIP in a PR, but I might end up switching to what you said 👀

@tarikeshaq tarikeshaq mentioned this issue Jul 24, 2020
12 tasks
@rfk
Copy link
Collaborator

rfk commented Jul 25, 2020

I was playing around and one thing I tried is to let the consumer define their own Rust Error enum.
The Rust enum exactly matches the IDL one and has to be named Error.

FWIW, in general I like the principal that you should be able to look at the rust code and understand in isolation as plain old rust code, without having to understand the extra things that will get magicked into existence by the scaffolding. I think that aligns broadly with what you describe above - the user defines an Error in rust and uses it for idiomatic rust error-handling stuff, they declare a matching Error in the IDL and we generate all the boilerplate to convert it into an ExternError and pass it over the FFI.

(I haven't looked at your PR yet, that's purely an idea-level reaction based on what you describe above).

We have to find a way to mark errors when we parse the IDL,

We could use an attribute, or we could infer which enums are errors by walking the set of all functions/methods and mark anything that gets thrown as being an error enum. (If we do use an attribute, we'll have to do the opposite check and ensure that anything things marked with the attribute are thrown).

The consumer has to abide by the limitations of WebIDL enums

Yep, which IIUC is similar to the current restrictions on non-error enums between Rust and the IDL.

One of the advantages of this philosophy I think, is that it keeps open the open of moving from a separate IDL to some inline definitions based on macros.

@tarikeshaq tarikeshaq self-assigned this Jul 28, 2020
@eoger eoger added the blocks-nimbus Blocks Nimbus SDK on mobile label Jul 29, 2020
@data-sync-user data-sync-user changed the title Add support for returning and handling errors SYNC-1566 ⁃ Add support for returning and handling errors Jul 30, 2020
@eoger eoger reopened this Jul 30, 2020
@data-sync-user data-sync-user changed the title SYNC-1566 ⁃ Add support for returning and handling errors Add support for returning and handling errors Jul 31, 2020
arg0d referenced this issue in NordSecurity/uniffi-rs Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocks-nimbus Blocks Nimbus SDK on mobile get involved Good opportunity for deeper-dive onboarding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants