Move from `error-chain` to `thiserror` for error management

[althonos]

Hi!

error-chain is not maintained anymore, so it would be a good idea to move to failure, which has all the features needed already, as well as backtrace support (i.e. the underlying pest error could be accessed from a ParserError using the cause() method).

[pchampin]

Thank you for this suggestion. I need more input before I make a decision.

You argue that error-chain is not maintained anymore, however the conclusion of the thread you cite is not as clear-cut. Furthermore, it seems that error-chain is still being maintained after all.

That being said, after a quick glance at failure, I must say it does look more elegant and lightweight than error-chain (I'm not a big fan of the Error/ErrorKind dichotomy... it feels like coding in Java 😈).

Still, I'm a bit uncomfortable with investing in a crate whose ambition is to deprecate a trait from the standard lib. Especially when some of the complains about the standard trait have been fixed in the meantime. In that respect, the Evolution section in their README is not really clear, but it makes me nervous.

So, could you please elaborate more why you think failure is a safer bet for the future than error-chain?

[althonos]

As you said, failure has the advantage of removing the Error/ErrorKind duplication. Furthermore, it can create a proper backtrace, without ambiguity, from any error, and not just ChainedError implementors: the backtrace is reached through the cause method, which returns the value of one of the variant's fields. It is unclear how to do that with error-chain, since I'm not sure you can both expose an error as an attribute and access it through the backtrace.

The other advantage is that the conversion traits make it easier to use ?, whereas error-chain sometimes require you to wrap/convert the error yourself (see the PR for the XML parser, where implementing another error category makes it more verbose to return a top-level error from the lower-level one).

[pchampin]

the backtrace is reached through the cause method, which returns the value of one of the variant's fields

But the cause method in failure seems to be redundant with the new source method in std::error::Error. This is the kind of divergence that I am concerned about.

the conversion traits make it easier to use ?, whereas error-chain sometimes require you to wrap/convert the error yourself (see the PR for the XML parser...)

It seems that you solved this problem in your latest commit f6eb5fe. Or is it something else?

As you can see, I'm still reluctant to make the leap. But I'm also still open to discussion.

[althonos]

The difference is that to do that with error-chain, you need to call Error::with_chain every time you want to chain something, and ultimately you'll write your own impl From<InnerError> for OuterError whereas failure lets you derive it with an attribute.

Furthermore, as seen in my PR, there is ambiguity between chaining and wrapping the error: in failure, you can both wrap the error as a field of your variant (hence accessing it without downcasting) and have it returned by the source method.

Ultimately, both libraries are doing the same thing, but failure is removing a lot of boilerplate code, as well as the Error/ErrorKind dichotomy.

[pchampin]

The difference is that to do that with error-chain, you need to call Error::with_chain every time you want to chain something,

actually, you would use Result:chain_err, which is not that verbose.

and ultimately you'll write your own impl From<InnerError> for OuterError whereas failure lets you derive it with an attribute.

I'm not sure about that. Explicit is better than implicit, and chaining an error is not the same as converting it...

as seen in my PR, there is ambiguity between chaining and wrapping the error

Granted. But in the case of ParseError, I wouldn't have wanted to fix in advance the type of error that can be its cause (because different parsers will have different underlying errors, your XML parser is a perfect example). So if I had had to define a member for the chained error, its type would have been Box<std::error::Error>, and it would still have required downcasting.

Ultimately, both libraries are doing the same thing

Yes, but failure is doing it its own specific way, while error-chain is playing ball with the standard Error trait...

I agree that error-chain is generally more verbose, but it still looks like a safer choice. Sorry.

[althonos]

I just discovered err-derive that provides a derive macro equivalent to failure, but that implements std::fmt::Display and std::error::Error. Leaving it there just in case 😉

[pchampin]

Now you're talking :-D
It looks very interesting indeed. Let's rename this issue and leave it open til I get some time to seriously consider the migration.

[MattesWhite]

When talking about alternative error handling crates, I would also consider snafu. It uses the std::error::Error trait and provides nicer derive-display syntax then failure, i.e. you can call methods on a variants elements. In addition, it has some kind of failure's context mechanics which are very nice to use.

[pchampin]

Thanks @MattesWhite, I'll also have a look.

[MattesWhite]

When talking about error-handling, what about the coercible_error.

I can definetly see its use-case and the performance it could bring and I can understand that you want to use it as it was probably your first crate in Rust. But, it feels very unergonomic and produces a lot of noise. In addition, I think its performance boost is not as much as one might guess.

A correctly designed error-enum should have not more than 24 bytes (two pointers and an extra 64 bit field on a 64-bit-machine). This shouldn't affect the overall memory requirement of an RDF-application.

In order to handle errors in an public API, it is totally okay to go with Box<dyn Error + Send + Sync> in my opinion. The improvements of the std::error::Error-trait allow downcasting it. In addition, a Box has only a size of 8 bytes.

An alternative would be using an own error-enum like:

use thiserror:Error as ThisError;

#[derive(ThisError, Debug)]
enum Error {
    ... ,
    #[error(External Error: {0})]
    FromExtern(String),
}

impl Error {
    from_external<E: std::error::Error>(e: E) -> Self {
        Error::FromExternal(e.to_string())
    }
}

This way every implementor of a custom graph, parser, etc. could impl From<ExternalError> for Error easily.

[pchampin]

Actually, coercible-error was not my first crate. It was something developed for Sophia that I thought could be useful to others.

it feels very unergonomic

Does it? It pains we that you would think that, I spent quite some time to try and make it ergonomic...

and produces a lot of noise

I cant deny that :-/ But most of the time, users would not be required to directly use it (the most verbose part are in trait methods with a default implementation).

I agree that Box<dyn Error + Send + Sync> would be a good default in many places, and that the overhead would be minimal. But I find a little sad to renounce the "Zero Cost Abstraction" motto, when a solution can be made with (in my opinion) no too much added complexity.