Use error-chain

[sphinxc0re]

I recognized that a most parts of the api either panic! or return an Option<T>. Option<T> should only be used when there legitimately might be nothing to return. In any other case I would suggest to use the Result<T> that the error-chain crate offers.

I wouldn't use the actual chaining feature of the crate in tight loops as this can lead to severe performance issues.

I would like to implement this myself if it is a wanted feature. The error-chain crate also is very well maintained and already found a good adoption rate in the Rust community.

[koute]

Could you perhaps give a few examples of places where we return an Option<T> when we should return a Result<T, E> or there's a panic! that shouldn't be there? (There are a few stray panics, but they're mostly there because the error handling in that particular place hasn't been properly implemented yet.)

[sphinxc0re]

stdweb/src/webapi/document.rs

Line 72 in 5926b66

js!( return @{self}.createElement( @{tag} ); ).into_reference_unchecked().unwrap()

stdweb/src/webapi/document.rs

Line 81 in 5926b66

js!( return @{self}.createTextNode( @{text} ); ).into_reference_unchecked().unwrap()

These are lines where you'd just call unwrap() on the result. This is usually considered "bad style".

stdweb/src/webapi/document.rs

Lines 37 to 41 in 5926b66

	// TODO: This can throw an exception in case of an invalid selector;
	// convert the return type to a Result.
	unsafe {
	js!( return @{self}.querySelector( @{selector} ); ).into_reference_unchecked()
	}

You even wrote a comment for that one. My suggestion is to simply wrap every call that could probably fail with an error message to produce a Result<T>

[koute]

The first two examples you've given should call unwrap. The createElement function (according to the documentation) will always return a valid reference to an element. Ditto for the createTextNode.

You need to remember that unwrap is effectively the same as an assert!, telling the compiler that "yes, we'll always have a value here, trust me"; using unwrap is bad style, but only when you're unwrapping possible errors - here there are no possible errors, so it would actually be a bad style to return an Result<T, E> even though you know that it will never contain E.

In the case of the third example - yes, it should return a Result.

[CryZe]

The documentation is wrong then. createElement can throw exceptions if the tag name is not valid.

[koute]

The MDN explicitly says that:

In an HTML document, the Document.createElement() method creates the HTML element specified by tagName, or an HTMLUnknownElement if tagName isn't recognized.

So it either returns a concrete element, or HTMLUnknownElement if it isn't recognized.

If the docs are wrong (and they might) and the function actually can result in an exception then yes, it should return a Result.

[edit]
Looking at the standard it seems you're right, it can result in an exception. Well then, I retract my statement that it should not return a Result. Good catch! I should probably look at the standards a bit more instead of relying on the MDN docs all the time.
[/edit]

[sphinxc0re]

Yeah I'm usually really tempted to look at the MDN as well, so 🤷‍♂️ 😁
I'm generally in favor of the Rust coding standards. So returning a result which can be handled by the using application is the way to go IMHO. error-chain just makes it easier to produce and handle errors.

[koute]

Well, if you feel like it then I would quite welcome any changes which fix places which currently don't return a Result and yet they should.

As for the error-chain dependency I'm not entirely convinced that it should be a required feature, however I wouldn't have anything against making it an optional feature (just like, for example, I've made serde support an optional feature).

[CryZe]

error-chain is super heavy weight. I'd go for quick-error, derive-error or something simpler like that. error-chain brings a ton of non-portable garbage with it by default (e.g. the backtrace dependency)

[koute]

Yep. Which is if we'd have support for it I would absolutely want it to be optional.

[sphinxc0re]

I admit, error-chain is not the lightest crate. But I think we should use some kind of error handling crate

[ibotty]

What about failure?

[beefsack]

What about failure?

It seems the community trend is moving from error-chain towards failure at the moment. It's owned by a Mozillian and appears intended to replace things like error-chain.