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

Implement Error and DOMException. Add patterns for error handling. #89

Merged
merged 5 commits into from Feb 1, 2018

Conversation

Herschel
Copy link
Contributor

I've had this floating around on my hard drive for a bit -- there is room for improvement, feedback requested!

Addresses #28. Methods that throw an error were using an ad-hoc js! block to catch the error and transfer it back. However, this gets difficult when a method returns multiple error types. This PR adds macros to make error handling more consistent and easy.

  • Implement Error and DOMException types.
  • Implement a js_try! macro that works like js!, but also catches errors and convert them to a Result type.
    • This macro relies on TryFrom<Value> being implemented for both the value and error type.
    • Specifically for DOMException, these have the same type in JS, but different names. For the concrete exception types in Rust, FromReference and TryFrom is implemented to check that the name matches.
    • The implementation of js_try! isn't optimal, with lots of calls back and forth to JS. I expect this could be done much better, perhaps by adding serialization support for a Result type. I was more concerned with establishing the pattern of how the errors should be caught on the Rust side; the macro can be improved.
    • Unlike the js! macro, js_try! does a try_into internally.
  • For calls that may return several different errors, an enum can be used, and the error_enum_boilerplate! macro is used to implement TryFrom<Value> for the enum.
    • This will try to convert the value to each variant in sequence.
    • My macro-fu is a little weak, so maybe the syntax for this macro should be different. Even better, I think this could be done as a custom derive if we're okay with that.
  • As an example, the js_try! macro is used to return errors from these APIs:
    • Node::replace_child, Node::insert_before
    • WebSocket::new and WebSocket::close_with_status (thanks @Diggsey!)

Things left for a future PR:

  • Backtraces (could be placed in the IError trait?).
  • Using a crate such as failure.
    • It didn't seem like there was a consensus on this yet.
    • Use error-chain #21 specified that error-chain was too heavy, but failure might be more appropriate. It would allow us to upcast into a unifying Error type. Maybe it's more ergonomic for failing methods to return Result<_, failure::Error> instead of defining an enum-per-throwing-function. But I think it's nice to know explicitly what errors a function might throw.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 24, 2018

This looks great!

A few comments:

  • I wonder if js_try! should include the unwrap - given the way it is used, it's unlikely the caller will ever not want to unwrap the conversion, and if the macro performs it, then it can give nicer behaviour in case the unwrap fails.

  • DOMExceptions should expose their type as an enum rather than a string.

  • DOMExceptions should be constructible by type.

Copy link
Owner

@koute koute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far this is looking really solid!

Thanks a lot!

Err(CreationError::SyntaxError(_)) => (),
v => panic!("expected SyntaxError, got {:?}", v),
}
}
Copy link
Owner

@koute koute Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two tests should be web_tests. (That is - they should be in a mod protected by #[cfg(all(test, feature = "web_test"))] so that the tests don't fail when they're being run under nodejs.)


// To safely cast from a reference to a specific DomException, we must check that the object is an
// instance of DOMException, and that the error name matches.
impl<T: ConcreteException + ::webcore::value::FromReferenceUnchecked> ::webcore::value::FromReference for T {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor stylistic nitpick: use-ing these two would probably be nicer.

($name:ident) => {
impl ::std::fmt::Display for $name {
fn fmt(&self, formatter: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
write!(formatter, "{}: {}", stringify!($name), self.message())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of stringify!($name) wouldn't it be better to print out self.name()? (That way errors upcast into a base type would still show the actual type.)

assert_eq!(error.message(), "foo");

let mut text = String::new();
write!(&mut text, "{}", error).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just do let text = format!("{}", error); here? (:

@koute koute mentioned this pull request Jan 26, 2018
12 tasks
@koute
Copy link
Owner

koute commented Feb 1, 2018

Since the comments I've made are pretty minor and this in general looks pretty solid (and I want to release a new stdweb with this soon) I'm going to merge it in for now. We can further tweak it as we go.

Thanks a lot @Herschel!

@koute koute merged commit 322afa8 into koute:master Feb 1, 2018
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.

None yet

3 participants