Error Management: switch to `Failure` ? (Disposition: No)

[m4b]

So I've been holding off on error libraries, but Failure genuinely looks exciting and cool.

I am ok with dynamic allocation, since the parser allocates already, and parsing binaries generally won't be in a hot loop, and if it is, it'll likely be dwarfed by io reads anyway.

This might also be an opportunity to provide better error messages because errors in goblin aren't so great. but this is mostly because of scroll error messages sucking pretty hard (but that's because it supports no-std).

[luser]

I've used failure in a few projects now and it's pretty great. It feels similar to error-chain but with a bit less magic. You could probably start using it now without changing much--just add #[derive(Fail)] to your Error enum as a start, and then you ought to be able to remove the std::error::Error impl, and the From impls. You might want to switch scroll over first so that you get implicit conversion from its errors as well.

You can see a simple example of using failure with a custom error type in this code I wrote:
https://github.com/luser/rust-disasm/blob/0758b8ac2820cd73137284423b7dfaf04c2701bd/src/lib.rs#L20

There are a few things there that are silly because the gimli crates are using error-chain, and interop between failure and error-chain is a little wonky. (boats has a PR open on error-chain with a fix, but it's a breaking change.)

In the meantime, the recommended workaround is to define a new trait that converts error-chain errors, which isn't the best but it works. Here's an example of that:
https://github.com/luser/query-crates-index/blob/26a825fc11ac8d29b32bdf35a92748e3e31dedf4/src/main.rs#L30

You wind up writing whatever().sync()? for things that return error-chain errors, which is workable.

[fitzgen]

I think we wouldn't want to use failure::Error in this crate, but instead #[derive(Failure)] on a custom error enum.

[fitzgen]

silly because the gimli crates are using error-chain

Would love to get PRs to move to failure, btw.

[luser]

I think we wouldn't want to use failure::Error in this crate, but instead #[derive(Failure)] on a custom error enum.

Yeah, that's what I was suggesting, just deriving Fail on the existing Error enum. That should immediately give you some niceties like auto-conversion from other error types, as well as easy error message formatting. (That being said, I'm not sure what the practical benefits of using your own error type vs. failure::Error are, really. Just the ability to pattern-match specific errors, I guess?)

[m4b]

So with the new alloc feature I think just adding derive annotation will work as expected and won't cause any problems in no_std environments, would you agree @philipc ?

[philipc]

Yes, my understanding is that failure::Error can't be used with no_std, but derive is fine.

gimli crates are using error-chain

This was only in addr2line afaik, and that usage has been removed. Adding failure support to gimli would be good though.

[m4b]

Ok I think we should get the error story figured out. I'm leaning towards sticking with current error system, not pulling on another dep, as it's compatible with downstream users using failure.