Ensure panic safety for all FFI methods

[linabutler]

This commit changes all FFI functions to take an out_err param, so
that they're called using call_with_result. Any functions that don't
have a [Throws] annotation in the IDL will throw a generic
InternalError instead.

Closes #32.

[tarikeshaq]

I like it a lot!!

A couple of things we can address here, or get this merged and address separately:

• We could add tests for panics
• We'll need to also create the mapping for panic codes to explicit errors (for all the errors defined with the [Error] attribute). Right now they map only to codes 1..numOfVariants

Also with this merged, we would have completely lost support for Python in all our examples, which might make testing for #215 and #214 a little annoying, but this is a higher priority 😄

[linabutler]

I think Python is broken now because methods without [Throws] are now expecting a RustError argument as well, but that'll be fixed with #215.

Edit: Hahaha, I wrote that literally at the same time as your review! 😅 I'll go ahead and add a test for panics now. For mapping these built-in codes to explicit errors...we could either have Panic and InvalidHandle variants for each generated error, or have them return the InternalError variant. Generating individual variants is quicker, so I'll just do that!

[rfk]

Will there be any conflicts or problems if #215 lands ahead of this PR, or should we hold on of that one until this one is landed?

[rfk]

We'll need to also create the mapping for panic codes to explicit errors (for all the errors
defined with the [Error] attribute). Right now they map only to codes 1..numOfVariants

Naively, as a consumer I think I would expect a panicking [Throws(E)] function to throw an InternalError.Panic, rather than throwing e.g. some sort of E.InternalError.Panic variant. Does that mesh with what's being proposed here?

[rfk]

Possibly contrary opinion: I'm not convinced it's valuable to define an elaborate series of cases for each individual type of internal error that might be thrown. If anyone ever finds themselves matching on these different cases and doing anything other than freaking out and filing a bug against uniffi, something has gone terribly wrong 😅.

In that spirit, the most important thing is that these errors surface with a useful human-readable message that helps us find and fix the bug. If enumerating all the cases helps us achieve that, awesome. But if they're just adding layers of indirection to a fixed string value, then I worry they might set incorrect expectations about how these internal errors should be handled (which is: they shouldn't be handled).

I'm not going to r- for that though, just my 2c.

[linabutler]

Hmm, thinking some more about this, I'm coming around to what you and Tarik suggested—assert for any kind of internal error instead of bubbling it up as an exception. In real-world code, [Throws] is for defining error cases that are meaningfully recoverable—a network error or duplicate experiment name, for example. A Rust-side panic might be, but...how do you fix that? If the app were fully in Rust, it would crash. The only reason we catch panics is to avoid stack unwinding into the foreign language, not as a graceful error recovery mechanism.

[tarikeshaq]

@rfk I think we can land #215. And after this merges, we can spin off another issue to tackle panics in Python based on what we come with here! Thoughts?

Naively, as a consumer I think I would expect a panicking [Throws(E)] function to throw an InternalError.Panic, rather than throwing e.g. some sort of E.InternalError.Panic variant. Does that mesh with what's being proposed here?

Hmm ya this makes more sense... Although I can't instantly think of a clean way to do this (since we pass the error type as a part of the rustCall).

This makes me wonder if we should just panic on the side of the binding (fatalError in swift and maybe a RuntimeException in Kotlin) if we receive an internal error (as opposed to raising a special exception)

I defer to @linacambridge's judgement though 😅

[rfk]

And after this merges, we can spin off another issue to tackle panics in Python based on what we come with here! Thoughts?

Yep. I'll probably try to tackle that as part of my big make-python-work refactor over in #214 (and I'd hate to derail any critical-path nimbus stuff in the service of the python bindings).

[linabutler]

Yes, let's land #215 first, and then this. I'll go ahead and make the changes to fatally assert on an internal error, instead of throwing.

[linabutler]

OK, I simplified the error hierarchy here so that we have a catch-all "internal error" instead of getting fancy with reporting panic and invalid handle codes. We might want to report these specially to Sentry, for example, but I totally agree consumers shouldn't be matching on them. WDYT?

[linabutler]

I went ahead and did a cleanup pass and added a quick test—would you mind taking another look please, @tarikeshaq? ☺️

[rfk]

Looks good to me :-)

[tarikeshaq]

LGTM!

We can tackle standarizing this behavior with fallible functions in a follow-up!