Use proper error types

[casey]

Currently, we use anyhow for all errors. I like anyhow, but it makes it too easy to bubble up an error without context, which leads to a less-than-useful error message. For example, I/O errors don't include the offending file, making them quite inscrutable. We should probably switch to using something like snafu or thiserror.

[cryptoni9n]

Hi @casey & @raphjaph - I've started working on converting the code base over to thiserror and wanted to check-in with you to make sure that this is still desired and won't all be for nought.

I've created an errors.rs under /src and am stuffing most of the custom errors into it.

I started out converting the files underneath /src/subcommands/wallet and it just kind of snowballed from there. I've been working on the src/settings.rs and that one is requiring many, many changes.

I know that this is a ton of changes, but I hope to get through it all eventually. I'm not sure exactly how I should submit the changes once I can get everything ready.

Any thoughts/feedback?

[casey]

Thanks for tackling this! I think it will make error messages much more useful, and I wish we did it from the beginning.

I actually think that prefer snafu over thiserror. The context selector is a very nice way to select an error type. For example:

#[derive(Snafu)]
enum Error {
  #[snafu(display("I/O error at `{}`", path.display()))]
  Io { path: PathBuf, source: io::Error}
}

Then, from another module:

do_something(&path).context(error::Io { path: &path })?;

Is there any way to do this piecemeal? It will be an absolutely massive PR, and will conflict with basically every other PR.

Like converting the top-level error type to:

#[derive(Snafu)]
enum Error {
  #[snafu(display("{0}"))]
  Any(Box<dyn std::Error::Error),
  #[snafu(display("I/O error at `{}`", path.display()))]
  Io { path: PathBuf, source: io::Error}
}

And then adding error cases one by one.

[cryptoni9n]

Thanks for the quick response, Casey. If I understand you correctly (and I may not), the context selector that you've mentioned seems very similar to what I've been doing with thiserror. The errors.rs creates the MyError enum:

use std::env::VarError;
use std::io;
use std::num::ParseIntError;
use std::num::TryFromIntError;
use thiserror::Error;
use redb::TransactionError;

#[derive(Error, Debug)]

pub enum MyError {
  #[error("Invalid environment variable `{var}`: `{value}`")]
  InvalidEnvVar { var: String, value: String },
  },

and the errors are called as shown in this arguments.rs:

use {
  super::*,
  clap::builder::styling::{AnsiColor, Effects, Styles},
};

#[derive(Debug, Parser)]
#[command(
  version,
  styles = Styles::styled()
    .header(AnsiColor::Green.on_default() | Effects::BOLD)
    .usage(AnsiColor::Green.on_default() | Effects::BOLD)
    .literal(AnsiColor::Blue.on_default() | Effects::BOLD)
    .placeholder(AnsiColor::Cyan.on_default()))
]
pub(crate) struct Arguments {
  #[command(flatten)]
  pub(crate) options: Options,
  #[command(subcommand)]
  pub(crate) subcommand: Subcommand,
}

impl Arguments {
  pub(crate) fn run(self) -> Result<Option<Box<dyn subcommand::Output>>, MyError> {
    let mut env: BTreeMap<String, String> = BTreeMap::new();

    for (var, value) in env::vars_os() {
      let Some(var) = var.to_str() else {
        continue;
      };

      let Some(key) = var.strip_prefix("ORD_") else {
        continue;
      };

      env.insert(
        key.into(),
        value
          .into_string()
          .map_err(|value| MyError::InvalidEnvVar {
            var: var.to_string(),
            value: value.to_string_lossy().to_string(),
          })?,
      );
    }

    self.subcommand.run(Settings::load(self.options)?)
  }
}

I'm not certain if it can be done piecemeal or not yet, but I have a hunch that it can be. I was thinking I could leave anyhow in and just convert the files currently throwing compile errors on my build. I imagine that I could get it to start compiling without fully converting to 'thiserror' and could push those changes as piece A (with blessing) before finishing off the back half (or however much) and then permanently removing anyhow after it's fully replaced. I could be wrong though and it could make more sense to bang it all out in one go. (edit: or rather still, it may be required to be done all at once)

I've done a good amount of conversion already, but I really have no idea if it is 1% or 10% - only been working on it for a couple of days. Happy to switch horses though - just say the word.

[casey]

Check out #3832, which can be merged as a start and more errors can be done in a piecemeal fashion. I think doing it piecemeal is the way to go. The diff would be absolutely huge otherwise and hard to review, and the merge conflicts would be brutal. I think doing it file or module at a time are the way to go.

[casey]

Whoops, didn't mean to close.

[casey]

@cryptoni9n I merged #3832, which uses snafu to make it easier to use typed errors. Check out that PR, which adds a couple new error variants. It introduces a SnafuError and .snafu_context and .with_snafu_context methods, which can be removed once the conversion is complete, and SnafuError renamed to just Error.

[cryptoni9n]

@cryptoni9n I merged #3832, which uses snafu to make it easier to use typed errors. Check out that PR, which adds a couple new error variants. It introduces a SnafuError and .snafu_context and .with_snafu_context methods, which can be removed once the conversion is complete, and SnafuError renamed to just Error.

Will do - thanks for getting this started!

[casey]

You bet! Feel free to ask for help, snafu, and in particular the context selectors, are tricky.

[cryptoni9n]

You bet! Feel free to ask for help, snafu, and in particular the context selectors, are tricky.

Yeah, I was banging my head against it for a little bit earlier today. I'm just starting my vacation now though, so I won't really be able to dive in until I get back in about 10 days.

[casey]

No worries, have fun on vacation!