Allow conversion from Rust errors to JS throws

[RReverser]

I decided not to go the separate JsError type route because it makes integration more complicated. Namely, JS imports are still returning generic JsValue which would need to be somehow validated and converted to JsError or live alongside it to preserve ability of rethrowing. In both cases, it adds complications to the implementation with unclear benefits.

Instead, as mentioned in #1017 (comment), I'm adding a support for transparent conversion from arbitrary std::error::Error to proper JS error throwing on the other side.

This allows user to use any Result in exported functions with either an explicit Error type they already use in the codebase or any generic implementation, e.g. Box<dyn Error> or failure::Error.

Fixes #1017.

[alexcrichton]

Thanks for this! This is actually quite clean and I think pretty reasonable. I'd personally be in favor of merging!

@fitzgen how's this look to you?

[alexcrichton]

Would it be possible to include more than .to_string() here? Sometimes an Error could be a stack of errors (following .cause()) which makes the most sense when presented all at once. Using just Error can we do some iteration here or something like that to build a larger message?

[RReverser]

We could, but I'm not sure how well that would interact with JS-generated stacktrace... Don't you think it might get a bit confusing to see two different but overlapping stacktraces on one error?

[alexcrichton]

Oh right I don't think it'd interact with the JS stack trace, only that we'd use the Rust stack trace to generate a more descriptive message

[RReverser]

Yeah but what I mean, when you throw it on the JS side, you'll get Rust stack trace in the message followed by JS stack trace right below (as that's what JS engines normally print right after the message). Hence my concern that it might be somewhat confusing to see both following each other when in fact parts of them overlap in terms of functions... Not sure how to address this nicely.

[alexcrichton]

It's true yeah it could get a bit garbled, but I actually think that's sort of what we want exactly, a Rust stack trace followed by a JS one!

We could perhaps experiment with various formattings though to see which works out best for the Rust message?

[RReverser]

I'm still not sure how I feel about multiline error message on JS side (it is rather unusual) and also about potential duplication of information within it (since Display implementations often tend to forward printing to nested errors, so adding .source() chain would produce duplicates of same messages already included in the first line).

While I totally agree that experimentation with different formats is useful, as well as with API itself, I still feel that it's not as simple as it seems at first glance and would prefer to leave such experiments to separate PRs.

[alexcrichton]

I would prefer to not land this unless it conveys the backtrace information for the Rust error. That often contains critical information about where errors come from needed for debugging, and as a built-in wrapper we in this crate would want to be sure to convey that information.

[RReverser]

Sorry, as mentioned, I'm not sure how this should look like (especially as I never used / looked into the cause chains in Rust land, usually relying just on stack backtraces) and on the other hand never seen multiline error messages in JS land. Taking these two experiences into account, I'm not really confident with adding this part myself.

Feel free to close the PR, I'm happy for someone else to take over and come up with a better solution.

[alexcrichton]

@fitzgen I'm curious, do you have thoughts on this?

I tried rebasing this today and while we can use Error::cause (or Error::source) to get more of a backtrace and put that in the error message, it doesn't work with failure by default. I'm a little wary to have an "official" solution that doesn't work with failure as that seems like it's one of the more common error-handling crates today...

[fitzgen]

Hm, so I guess an ideal version of this in my mind would be something like:

• We allow any E: Into<JsValue> for Result<T, E> and have wasm-bindgen generate the if-error-then-convert-and-throw glue

• We either implement

  • From<failure::Error> for JsValue that does the Right Thing, or
  • From<E: std::error::Error> for JsValue

---

Alternatively, we do basically the same thing but define our own JsError type and allow Result<T, E: Into<JsError>>.

This is maybe slightly better in that it doesn't involve seemingly out-of-left-field blanket conversion impls for JsValue...

However it is a bit funky that JsError is not the same type as js_sys::Error (even if they end up being the same thing on the JS side). We could maybe move js_sys::Error into wasm_bindgen itself and re-export it in js_sys, but this is starting to feel a bit hacky.

---

Both of these solutions should work with failure errors, I think.

[fitzgen]

And we should probably send a PR upstream to failure to use js_sys::Error::stack to get backtraces on wasm32.

[fitzgen]

* `From<failure::Error>` for `JsValue` that does the Right Thing, or

* `From<E: std::error::Error>` for `JsValue`

I think actually we would have to implement both, because failure::Error doesn't implement std::error::Error.

[alexcrichton]

Unfortunately though those two impls conflict :(

I think we have to either pick one or the other, I'm not actually sure how we'd support both 'natively'

[RReverser]

I really don't think we should allow arbitrary types that can get converted to JsValue for reasons mentioned both in original issue and in the description - it'd make way too easy to accidentally throw a primitive or something without any stacktrace.

[alexcrichton]

I'm going to close this since it's quite old at this point, but we're still interested in improving the error story with wasm-bindgen, so a rebase and/or a new PR would always be welcome!