Serializable errors proof-of-concept #328
Conversation
Implement a custom proc-macro to derive the flat/C-style enum variants of errors automatically, to reduce maintenance.
Derive From for conveting internal errors to JsValue with Into::into.
cycraig
force-pushed
the
feat/serde-errors-poc
branch
from
becff1c to
5a25bd8
Compare
|
I think this approach---deriving Advantages:
Disadvantages:
An alternative approach would be to define the C-style enum errors in the wasm bindings to avoid polluting the Edit: this isn't particularly useful for serializable errors for the actor. I would still go with manually-defined serializable errors that wrap the errors in Rust in that case. |
|
Very nice PoC! I am primarily wondering whether JS developers would actually match on the code property. I'm very unfamiliar with JS error handling, but at least according to this SO answer, the accepted default is both messy and inaccurate, i.e. catching all or nothing. So, essentially, is the trouble with the It would be interesting to have a JS developer's view. @abdulmth do you think having {"code":"DecodeBitmap","description":"Failed to decode roaring bitmap: something went wrong!"}thrown instead of just "Failed to decode roaring bitmap: something went wrong!"would be a worthwhile improvement? Would JS devs actually match on the @cycraig as you mentioned in #321, the C-style enums aren't actually a fundamental binding requirement, contrary to my initial belief. If we end up going with the current approach anyway, because it has value for the JS devs, I think I would still prefer regular Rust enums (option 2) for the actor, since that would be more idiomatic for matching. Do you think we could allow deriving both C-style enums for bindings, and Rust enums for the actor? Depending on whether we go forward with the current JS error approach with |
|
Exceptions are usually done by throwing an |
Indeed, the intermediate C-style are basically useless for the wasm-bindings because the enum variants can't be matched on in JavaScript except as a string and the Rust code exported to Wasm can just use the rich error enums directly. In my opinion the actor should use Rust enums that wrap any errors it needs as a string, unless it is a useful subvariant e.g. Then implement If we need to send things like backtrace information (which seems to be a somewhat complex subject), then we could use an intermediate struct (that is still serializable) to hold it, e.g. The above examples are just for demonstration. One could derive those
This changes things a bit. The current implementation only returns arbitrary objects/strings, never a proper JavaScript This will always print "not an Error..." We can convert the error we return into a Based on the points raised already, I'll work on another proof-of-concept. Essentially it will convert the Rust errors directly to some |
I'm not sure I've understood yet why we can't map the I've taken a quick look at aws-sdk-js to see their approach, and it seems they frequently use |
We absolutely can do that and that will likely be the final implementation. I'm just checking whether we can do any better, by e.g. adding more fields to the returned error apart from the description, such as stacktraces to aid in debugging. On a related note: I have to correct my previous statement that we can't use This enables us to generate something like the following: #[wasm_bindgen]
#[derive(Clone, Debug)]
pub struct WasmError {
code: WasmErrorCode,
description: String,
}
#[wasm_bindgen]
impl WasmError {
#[wasm_bindgen(constructor)]
pub fn new(code: WasmErrorCode, description: String) -> Self {
Self {
code,
description,
}
}
#[wasm_bindgen(getter)]
pub fn code(&self) -> WasmErrorCode {
self.code
}
#[wasm_bindgen(getter)]
pub fn description(&self) -> String {
self.description.clone()
}
}
#[wasm_bindgen]
#[derive(Clone, Copy, Debug)]
pub enum WasmErrorCode {
CryptoError,
...
}In Javascript, this would enable the user to check the type of code too: let messageId;
try {
messageId = await client.publishDocument(doc.toJSON());
} catch (e) {
if (e instanceof Error) {
console.log(`e is an Error! ${e}`) // WasmError is still NOT a Javascript Error
} else if (e instanceof WasmError) { // WasmError is exported by identity-wasm
if (e.code === WasmErrorCode.CryptoError) { // WasmErrorCode enum variants can be compared directly
console.log(`CryptoError: ${e}`)
} else {
console.log(`Unknown WasmError: ${e}`)
}
} else {
console.log(`not an Error: ${e}`)
}
}However, doing that in practice for all the different error enums identity.rs exports, the linker fails due to "duplicate symbols": We could try and make the derived struct names hygienic (it's really a problem with the code generated by wasm_bindgen that's unhygienic and conflicting in the same namespace though). The fix would be to give unique names to all
With the norm being generic error strings in Javascript, I think simply returning a Javascript Error with |
|
After some yak shaving, I've updated the code with proper usage of This means proper type comparisons may now be used as per the original proposal in the issue (instead of string comparisons). let messageId;
try {
messageId = await client.publishDocument(doc.toJSON());
} catch (e) {
if (e instanceof Error) { // exported errors are still NOT subclasses of Javascript Errors
console.log(`e is an Error! ${e}`)
} else if (e instanceof CoreError) { // CoreError is exported by identity-core
switch (e.code) { // CoreErrorCode enum variants can be compared directly
case CoreErrorCode.Crypto: {
console.log(`CryptoError: ${e}`); break;
}
case CoreErrorCode.InvalidUrl: {
console.log(`InvalidUrl: ${e}`); break;
}
default: {
console.log(`Unhandled CoreError: ${e}`)
}
}
} else {
console.log(`not an Error: ${e}`)
}
}Most of the original disadvantages of this approach have been addressed now (at the cost of re-naming the errors to avoid namespace conflicts and including I'll still push the alternative implementation based on |
|
Closed in favour of #344 |
Description of change
Exploring various implementations of serializable errors for actor handlers and bindings.
Introduces a
FlatEnumprocedural macro to derive serializable C-style enums for errors, which hopefully reduces maintenance required at the cost of obscuring the actual types in the source code. The generated code isno_stdcompatible, even though theflat-enumcrate itself requiresstd.Essentially this changes the
JsValueerrors emitted from the wasm bindings to include the enum variant code. E.g.before, JS received only a string description of the error:
"Failed to decode roaring bitmap: something went wrong!"after, JS can match on the type of error too (as a string):
{"code":"DecodeBitmap","description":"Failed to decode roaring bitmap: something went wrong!"}Edit: the above behaviour has changed, the code is now accessible as a field on the error exported with
#[wasm_bindgen], so it's no longer a string map/dictionary.Note that specifying macro attributes such asEdit: this is incorrect, see my comment below. We still can't use#[wasm_bindgen]in derive proc-macros is an unstable feature only available on nightly for now (tracking issue), so we still have to serialize errors to and fromJsValuemanually with this approach. The alternative is ditching theFlatEnumproc-macro and writing out all the flat error structs and enum codes manually.#[wasm_bindgen]right now but that's due to conflicting exported symbols, which can be circumvented if we think it's worth it.Without theEdit: this is inaccurate, returning an#[wasm_bindgen]attribute we cannot generate proper error types with Typescript. However, untilwasm-bindgenswitches to a more usefulJsErroralternative toJsValuefor handling errors inResult<T, JsValue>, we wouldn't be able to use those types anyway without manually casting the returnedJsValuein Typescript code (possibly). There are several tracking issues for better error handling inwasm-bindgen(rustwasm/wasm-bindgen#1004, rustwasm/wasm-bindgen#1017, rustwasm/wasm-bindgen#1742, rustwasm/wasm-bindgen#2463).Err(SomeError)whereSomeErroris annotated with#[wasm_bindgen]converts it to the correct type on the Javascript side when catching it.Another downside of the
proc_macroapproach to derivingFlatEnumis that it needs to be called in the libraries where those errors are defined, and also adds thewasm-bindgendependency to all the libraries which export an error. The dependency is optional and feature-gated but it's still not ideal. There may be an alternative, cleaner approach using a declarative macro which can be called in the bindings code instead, but iterating over the enum variants is more difficult with that approach and needs more work.Incomplete and just a proof-of-concept, do no merge.
Links to any relevant issues
Serializable errors #321
Type of change
Add an
xto the boxes that are relevant to your changes.How the change has been tested
Describe the tests that you ran to verify your changes.
Make sure to provide instructions for the maintainer as well as any relevant configurations.
Change checklist
Add an
xto the boxes that are relevant to your changes.