shepmaster / snafu

Easily assign underlying errors into domain-specific errors while adding context
https://docs.rs/snafu/
Apache License 2.0
1.45k stars 61 forks source link

Implement Termination #335

Closed hellow554 closed 2 years ago

hellow554 commented 2 years ago

Since Rust 1.61.0 the Termination trait is stable.

I would like to see a new type, e.g. SnafuExit<T> where T is a Snafu derived error whichs report method looks something like this:

impl<T> Termination for SnafuExit<T> where T: snafu::Error {
    fn report(self) -> ExitCode {
        if let Some(e) = self.error {
            eprintln!("An error occurred: {}", e);
            if let Some(bt) = ErrorCompat::backtrace(&e) {
                eprintln!("{}", bt);
            }
            ExitCode::FAILURE
        } else {
            ExitCode::SUCCESS
        }
    }
}

do you like it? If not, what can be done better?

hellow554 commented 2 years ago

and maybe a From<()> and/or From<Result<(), SnafuExit<T>> or SnafuExit::SUCCESS, whatever :)

Enet4 commented 2 years ago

I would advise that the error reporting constructs are made available without having to depend on Termination. This could exist either as a reporting function (like this one) or as an encapsulating report error with suitable Debug/Display impls (kind of what eyre does).

hellow554 commented 2 years ago

The thing is, currently I'm feeling that I'm writing some old style rust code where I have:

fn main() {
  if let Err(e) = run_real_main() { ... }
}

and I don't like that. What I like is:

fn main() -> SnafuExit<MyErr> { ... }

and place all the content of run_real_main into main... That's why the Termination trait.

I agree, that you may have a method on SnafuExit which is named report or similar and does exactly the same thing without implementing Termination. I'm open for that.

Enet4 commented 2 years ago

It's a bit early to require 1.61.0 to all SNAFU users, so it would have to be feature gated anyway, whereas viable alternatives to error reporting could be available now.

There was also an idea to provide a procedural macro to be placed on top of the main function. This was before Termination was sought to be stabilized, I think.

#[snafu(main)]
fn main() -> Result<(), MyError> {
    Ok(())
}
hellow554 commented 2 years ago

Honestly, I don't think that a proc macro would be a good idea here. How would you combine that with e.g. #[tokio::main]?

What about the idead from https://github.com/shepmaster/snafu/issues/333#issuecomment-1136289313 to use the rustversion crate?

shepmaster commented 2 years ago

Generally, I'm amenable to addressing the stated goal. I think there's some details to be ironed out. For example, what exactly would an ideal main function look like? Here's some noodling I did, names should not be taken seriously but general shape of code is believable:

#![feature(try_trait_v2)]

use snafu::prelude::*;

fn main() {
    _ = main_nightly();
    _ = main_1_61();
    _ = main_older();
}

fn main_nightly() -> Exit<Error> {
    usage()?;
    Exit::ok()
}

fn main_1_61() -> Exit<Error> {
    Exit::try_to(|| {
        usage()?;
        Ok(())
    })
}

fn main_older() -> Result<(), Print<Error>> {
    usage()?;
    Ok(())
}

fn usage() -> Result<()> {
    Snafu { id: 42 }.fail()
}

#[derive(Debug, Snafu)]
#[snafu(display("Hey it broke {id}"))]
struct Error {
    id: i32,
}

type Result<T, E = Error> = std::result::Result<T, E>;

// --------------------
// Would be in library

use snafu::ErrorCompat;
use std::{
    convert::Infallible,
    ops::FromResidual,
    process::{ExitCode, Termination},
};

struct Exit<E>(std::result::Result<(), E>);

impl<E> Exit<E> {
    fn try_to(f: impl FnOnce() -> Result<(), E>) -> Self {
        Self(f())
    }

    fn ok() -> Self {
        Self(Ok(()))
    }
}

impl<E> FromResidual<std::result::Result<Infallible, E>> for Exit<E> {
    fn from_residual(residual: std::result::Result<Infallible, E>) -> Self {
        Self(residual.map(drop))
    }
}

// TODO: support ExitCode::from_u8 in some fashion
impl<E> Termination for Exit<E>
where
    E: 'static + snafu::Error + snafu::ErrorCompat,
{
    fn report(self) -> std::process::ExitCode {
        match self.0 {
            Ok(_) => ExitCode::SUCCESS,
            Err(e) => {
                eprintln!("{}", Print(e));
                ExitCode::FAILURE
            }
        }
    }
}

struct Print<E>(E);

impl<E> From<E> for Print<E> {
    fn from(other: E) -> Self {
        Self(other)
    }
}

impl<E> std::fmt::Display for Print<E>
where
    E: 'static + snafu::Error + snafu::ErrorCompat,
{
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        std::fmt::Debug::fmt(self, f)
    }
}

impl<E> std::fmt::Debug for Print<E>
where
    E: 'static + snafu::Error + snafu::ErrorCompat,
{
    // TODO: Review how this actually looks
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let e = &self.0;
        write!(f, "An error occurred: {e}")?;
        for e in ErrorCompat::iter_chain(e).skip(1) {
            write!(f, "{e}")?;
        }
        if let Some(bt) = ErrorCompat::backtrace(&e) {
            write!(f, "{}", bt)?;
        }
        Ok(())
    }
}

I would advise that the error reporting constructs are made available without having to depend on Termination

I agree, and I think that this will naturally occur due to the implementation and feature gating.

Honestly, I don't think that a proc macro would be a good idea here. How would you combine that with e.g. #[tokio::main]?

Seems like Tokio shouldn't have used a procedural macro either then 🙃

use the rustversion crate?

Ultimately that's an orthogonal question. That crate would move the decision of "do I require Rust 1.XX" from the end user making a conscious decision to the code automatically doing so. I'm generally not a huge fan of that, and I don't think is simplifies anything implementation-wise.

hellow554 commented 2 years ago

That's a lot of (good) code.

I think for now providing a "proper" Debug implementation would help and would work for a lot of older rust versions. I think this can and should be done now.

Maybe you don't need two distinct types and you can implement Debug for Exit as well?

shepmaster commented 2 years ago

Maybe you don't need two distinct types and you can implement Debug for Exit as well?

Please try it and let me know how it works. I wasn’t able to get things to line up in a pleasing way in the short hour I spent.

Stargateur commented 2 years ago

But isn't already what rust do what you return a Result from main impl<E: Debug> Termination for Result<!, E> ? I think having a way to construct a ExitCode from an Snafu enum could be a thing adding it to the macro for example, but, I also think the use of ExitCode is very specific and should maybe not be "too easy".

basically a user could want:

#[derive(Error, Debug)]
pub enum Error {
    #[error("IO error from stdin")]
    StdinError(#[from] io::Error),
    #[error("stdin is empty")]
    StdinEmpty,
    #[error("require at least 2 arg")]
    ArgMissing,
}

impl Termination for Error {
    fn report(self) -> ExitCode {
        ExitCode::from(match self {
            Error::StdinError(_) => 1,
            Error::StdinEmpty => 2,
            Error::ArgMissing => 3,
        })
    }
}

also, this mean the main need to return a ExitCode not a Result<T, Error> that mean snafu would need a macro to encapsulate the main to avoid:

fn main() -> ExitCode {
    if let Err(e) = main_aux() {
        eprintln!("{e}");
        e.report()
    } else {
        ExitCode::SUCCESS
    }
}

I think the use case is limited enough to let this be handle by user, specially with already existing macro like tokio::main I expect this feature to require a lot of work for something that is already handle by Result or require the user to explicitly map error variant to ExitCode. Maybe we can do something with FromResidual but I still have trouble with this concept and anyway it's very nightly for now.

Yes the example use thiserror cause playground don't have snafu :p This come from some work I done in https://discord.com/channels/442252698964721669/448238009733742612/981365988694040576.

shepmaster commented 2 years ago

This will likely be fixed by #355