iximeow / yaxpeax-arch

fundamental traits to describe an architecture in the yaxpeax project
BSD Zero Clause License
11 stars 2 forks source link

Impl `std::error::Error` for errors #1

Closed ranweiler closed 3 years ago

ranweiler commented 4 years ago

Motivation: I would like to be able to easily use yaxpeax with anyhow (for ad hoc application errors) and thiserror (for writing custom error types for libraries). If yaxpeax-arch had a std::error::Error bound on yaxpeax_arch::DecodeError (and all that implies), then the following examples would work:

anyhow:

use yaxpeax_x86::long_mode as x64;

fn foo() -> Result<(), x64::DecodeError> {
    todo!()
}

fn bar() -> anyhow::Result<()> {
    foo()?;
    Ok(())
}

Currently we get:

error[E0277]: the trait bound `yaxpeax_x86::long_mode::DecodeError: std::error::Error` is not satisfied
  --> src/lib.rs:16:10
   |
16 |     foo()?;
   |          ^ the trait `std::error::Error` is not implemented for `yaxpeax_x86::long_mode::DecodeError`
   |
   = note: required because of the requirements on the impl of `std::convert::From<yaxpeax_x86::long_mode::DecodeError>` for `anyhow::Error`
   = note: required by `std::convert::From::from`

thiserror:

use yaxpeax_x86::long_mode as x64;

#[derive(Debug, thiserror::Error,)]
pub enum Error {
    #[error("Disassembly error")]
    X64Decode(#[from] x64::DecodeError),
}

The error in this test case (omitted) is misleading, but the root cause is the same.

Is this something you'd be interested in supporting?

iximeow commented 4 years ago

Is this something you'd be interested in supporting?

generally, yes. the only complication here is yaxpeax-arch being #![no_std] and std::error::Error ... being std::. so Arch will need another permutation of flags, similar to the use-serde flag, to include std and an Error bound: https://github.com/iximeow/yaxpeax-arch/blob/master/src/lib.rs#L52-L68

i have no idea what the expectations of trying to ? in no_std code might be like, but i assume a lack of Error in core implies that no one would be missing it there. that leaves me comfortable that no one is going to trip over a missing Error bound, so i see no reason why not (given the above required tweaking)

ranweiler commented 3 years ago

:wave: oh dang, this was so long ago! I'd still love to see this. Would you like a PR for it?

iximeow commented 3 years ago

oh! i'd thought we talked about Error somewhere, totally lost this issue. there's a smattering of changes to yaxpeax-arch on the way - this should be simple enough to include but a PR right now will probably just end up with weird conflicts. i'll probably include this in the next few days anyway.

iximeow commented 3 years ago

okay! in std builds of yaxpeax-arch, DecodeError now requires std::error::Error. only yaxpeax-x86=1.0 conforms to this at the moment, so using yaxpeax-arch 0.2.x is currently a good way to disable all the other decoders, if you use any others. they'll probably be updated shortly though, and everything should be std::error::Error-compatible in a bit.

i've tried to guess at how users might expect anyhow errors to fit (based somewhat on your example) and written some tests that the crate does actually behave properly, so i think this covers your needs! i did have an issue at first setting up the bounds (see also the ill-fated yaxpeax-arch 0.1.0) so if you can +ack then i'll close this.

ranweiler commented 3 years ago

Yesss, thanks @iximeow! Everything works just like I wanted (for both ad hoc and custom error types).