goblin
goblin copied to clipboard
Error Management: switch to `Failure` ? (Disposition: No)
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).
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.
I think we wouldn't want to use failure::Error
in this crate, but instead #[derive(Failure)]
on a custom error enum.
silly because the gimli crates are using error-chain
Would love to get PRs to move to failure, btw.
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?)
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 ?
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.
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.
Failure is unmaintained and deprecated now. https://github.com/rust-lang-nursery/failure