Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Add support for customizable error handlers based on eyre #96

Open
yaahc opened this issue Jun 6, 2020 · 1 comment
Open

RFC: Add support for customizable error handlers based on eyre #96

yaahc opened this issue Jun 6, 2020 · 1 comment

Comments

@yaahc
Copy link
Contributor

yaahc commented Jun 6, 2020

Proposal TLDR

Add a dynamic dispatch based version of eyre::Report's EyreHandler trait and parameter, with a global hook for constructing the boxed trait object that stores additional context and handles rendering error reports.

// Name is a placeholder
pub trait ReportHandler: core::any::Any {
    fn report(&self, error: &(dyn StdError + 'static)) -> core::fmt::Result;
}

pub fn set_hook(
    hook: Box<
        dyn Fn(&(dyn StdError + 'static)) -> Box<dyn ReportHandler + Send + Sync + 'static>
            + Sync
            + Send
            + 'static,
    >,
) {
    todo!()
}

eyre::EyreHandler::default is replaced with the hook fn, the default fn is removed from the ReportHandler trait, and the eyre::EyreHandler::display fn is renamed to report to better represent it's usage.

The proof of concept is currently implemented in #97

Background

eyre is a library based on anyhow that has experimented with adding customization support to anyhow::Error to add support to forms of context in error reports that shouldn't be inserted into the chain of error messages, such as the output of tracing_error::SpanTrace.

eyre has accomplished this by adding a new ErrorHandler parameter to the primary dyn Error handling type. This approach has some downsides:

  • it breaking type inference in certain situations where anyhow! just works. [[1]]
  • exporting eyre::Report in a library interface where the ErrorHandler type is still selected by the user requires invasive use of generic parameters and breaks type inference even worse. [[2]]
  • the libraries can export different handler types in their interface requiring burdensome conversion logic
  • generic parameters likely have negative impacts on compilation times

New Design

Most of these problems can be solved by using a Boxed trait object instead of a generic parameter. Type inference is no longer an issue and behaves exactly the same as in anyhow. The hook is global for your application so any library that exports an anyhow::Error will still capture the type of context specified in the global hook, removing the need for generics in interfaces or conversions between handler types throughout the application.

The downside of this design is the addition of a Box to the ErrorImpl type behind the thin pointer, adding an extra allocation to error construction. However, in the default case this hook should create a boxed dyn object from a unit type, which wont actually allocate anything and so should be essentially free (I hope).

handler construction hook

This function is used to create the handler to be stored alongside or instead of the Backtrace. This function is called when the anyhow::Error is first constructed, letting it capture context sensitive information such as backtrace::Backtrace or a tracing_error::SpanTrace. This function takes a reference to the inner error as an argument to allow it to do conditional capture of context such as a Backtrace upon inspection of the wrapped error type.

Any trait bound

The ReportHandler trait requires that types that implement it support downcasting. This makes it possible to write extension traits for mutating the inner context. This lets store information like status codes, custom error report sections for stderr/stdout, suggestions, etc. This is necessary to implement a ReportHandler based on color-eyre.

One potential downside of this approach compared to the generic parameter is that the internals of these extension traits must rely upon downcasting that can fail, and must choose a handling strategy for when the context created by the global hook is different from the context they're written to downcast too, such as panicking or logging an warning indicating that the context couldn't be modified.

fn report

This fn provides the Debug implementation for anyhow::Error. It takes the ReportHandler as it's &self parameter letting it access and render any context stored within the handler and it takes a reference to the dyn Error stored within anyhow::Error to let it control how the chain of errors is displayed and to access other context captured by source errors intended for the error report itself, such as Backtraces.

Problems

fn backtrace(&self) -> Backtrace

Right now anyhow::Error guarantees that it will capture a std::backtrace::Backtrace when it is available. While this interface is unstable this causes some significant issues. anyhow::Error cannot delegate capturing this Backtrace to the handler because the handler could choose to not capture one, which would require an unwrap in fn backtrace to maintain the same exterior interface.

Additionally, if anyhow::Error continues to handle capturing the backtrace the hook and report printing functions cannot include the Backtrace as one of their args, because this arg would not be available on stable or even older versions once std::backtrace::Backtrace has been stabilized. This means that anyhow::Error would need to handle rendering the Backtrace it captures, preventing the handler from customizing the backtrace rendering with crates like color-backtrace. I haven't come up with a good solution for this other than making a breaking change and removing the fn backtrace(&self) -> Backtrace or changing it to return an Option and adding a second function to trait ReportHandler for accessing the inner backtrace if it exists.

@yaahc
Copy link
Contributor Author

yaahc commented Jun 6, 2020

One possible solution for the fn Backtrace stuff would be to wrap the inner error in a newtype that transparently forwards Display and Debug to the inner error but yields the anyhow::Error's Backtrace. This type would only be added to the inner error if the inner error didn't contain a backtrace and anyhow::Error instead caught one.

elasticdog added a commit to EarthmanMuons/rustops-blueprint that referenced this issue May 13, 2023
Good for CLI applications, it provides some nicer ergonomics, features
to inject context if needed, as well as non-trivial optimizations
compared to using `Box<dyn Error>`.

For the xtasks use case, I'm going with the simpler `anyhow` over
something like `color-eyre`, which supports more complicated output
formatting (but has other trade-offs).

See:
- dtolnay/anyhow#96
elasticdog added a commit to EarthmanMuons/rustops-blueprint that referenced this issue May 13, 2023
Good for CLI applications, it provides some nicer ergonomics, features
to inject context if needed, as well as non-trivial optimizations
compared to using `Box<dyn Error>`.

For the xtasks use case, I'm going with the simpler `anyhow` over
something like `color-eyre`, which supports more complicated output
formatting (but has other trade-offs).

See:
- dtolnay/anyhow#96
github-merge-queue bot pushed a commit to EarthmanMuons/rustops-blueprint that referenced this issue May 13, 2023
Good for CLI applications, it provides some nicer ergonomics, features
to inject context if needed, as well as non-trivial optimizations
compared to using `Box<dyn Error>`.

For the xtasks use case, I'm going with the simpler `anyhow` over
something like `color-eyre`, which supports more complicated output
formatting (but has other trade-offs).

See:
- dtolnay/anyhow#96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant