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

GEN-68: .into_report() doesn't include error source chains #2127

Closed
jquesada2016 opened this issue Feb 26, 2023 · 11 comments · Fixed by #4678
Closed

GEN-68: .into_report() doesn't include error source chains #2127

jquesada2016 opened this issue Feb 26, 2023 · 11 comments · Fixed by #4678
Assignees
Labels
area/libs > error-stack Affects the `error-stack` crate (library) category/enhancement New feature or request hint/help wanted Experience: Extra attention is needed lang/rust Pull requests that update Rust code priority/3 low Lower priority: nice-to-have

Comments

@jquesada2016
Copy link

Related Problem

The problem I have seen is that when calling .into_report() on errors, Report only includes the top-most error, and does not include any of the sources.

Proposed Solution

I propose that .into_report() go through the source chain and add each source entry as a printable attachment, (or context frame?) to the error.

Alternatives

No response

Additional context

I've run into this problem using the Rust AWS SDK, where SdkError results in generic "service error" messages, where in reality, if we were to follow the error source chain, we would get all the information we'd need to diagnose and fix the error.

@TimDiekmann
Copy link
Member

Hi @jquesada2016 and thanks for filing this issue!

I really like the idea, but sadly I don't see how this could be implemented. Error::source returns a reference to another error with the lifetime bound to the error itself. When calling .into_report(), the error is moved into the Report, but we cannot move out the errors returned from source() and store them alongside in the Report. That way, it's neither possible to add them as attachments, nor as context frames (I prefer the latter).

One way how this could be possible is to not store the source errors in the report but install a Debug hook for the errors and print the sources as well. This, however, has two downsides:

  1. It's neither rendered as an attachment nor as context, which would be preferred.
  2. The Error::provide method would not be called when printing

Anyway, this would require installing a debug hook for each error, which you are expecting, which is obviously unviable. Sadly, to my knowledge, it's not possible to use the Provider API to get all objects, which are implementing a certain trait (i.e. Error).
However, we could wrap the error, where .into_report() is called on, and provide a custom debug implementation, which supports this feature, or even add some special handling for this (as Error is a major trait we are supporting). I don't know if this is achievable easily, but it's definitely worth a try!

It's likely, that this method will require a nightly compiler to work.

What do you think?

@jquesada2016
Copy link
Author

The last option you suggested might be the best option out of the ones you proposed.

What if .into_report() wrapped the Box<dyn Error> in Rc, so we can have each source as it's own context frame? The way this might work may be:

  1. wrap the error in Rc to make it a Rc<dyn Error>.
  2. For each .source() which yields Some(_), we clone the Rc along with the depth it was at to get to it's source error, and add that as a context frame, something like:
struct IntoReportFrame {
   depth: usize,
   error: Rc<dyn Error>,
}

The advantages to this, is that providing and downcasting would work just fine! We'd just need to check for two types:

a) the original T type
b) the possible Rc type

The biggest downside to this approach would be performance on errors, since you would need to walk down the error source chain for each level.

@TimDiekmann
Copy link
Member

Your approach most likely would work, however, we would need to use Arc instead of Rc as otherwise, the Context would not be Send + Sync. Personally, I'd prefer to avoid this because, as you mentioned, this requires traversing the list of sources of the errors on error creation. I think achieving this behavior is possible by special-casing an ErrorFrame (name to be decided on, but it's hidden anyway) when traversing the Report but this, obviously, is harder to implement.

The biggest issue, currently, is to detect if the Report is created from an Error or from any other Context as T: Error has just a blanket implementation for Context. The ideal solution would be to use #![feature(specialization)], but as min_specialization is not enough in this case, this is not really an option. The other option is to implement a hidden fn __source(&self) -> Option<&impl Context> { None } on the Context trait, which is only filled by the blanket implementation. We are considering removing the Context trait entirely and replacing it with Error, but as Error is not available in core (without a nightly toolchain), we can't do this yet. I will look into this as soon as I can find the time.

@jquesada2016
Copy link
Author

If you like, I can give ErrorFrame a try.

If I understood you correctly, ErrorFrame would do what I suggested above, but lazily when the error is being debugged, requested, etc. i.e., it would only walk down the error source chain when needed, rather than on .into_report().

@TimDiekmann
Copy link
Member

TimDiekmann commented Mar 3, 2023

If you like, I can give ErrorFrame a try.

Sure, why not! 🎉
Feel free to ping me if you need any help. It's not a problem to open a half-baked or draft PR.

Generally, this consists of two parts:

  1. Detecting, if the frame to be added is an Error or, by faking it, a hidden method on Context, which is only implemented on the blanket Error implementation. If you, however, can come up with a nicer way to solve this, I'd love to see that! (Note, that we try to keep the output on stable and nightly the same. Even though we don't have SemVer-guarantees for the output it would be nice to have a unified format)
  2. (probably the easiest solution) modifying the traversal logic of the Report::frames() method to return the sources of the errors as well. I, however, don't know if we can/want to expose the additional traversal as frames_mut probably cannot be adjusted.

@vilkinsons vilkinsons added category/enhancement New feature or request and removed C-enhancement labels Jul 20, 2023
@vilkinsons
Copy link
Member

Hi @jquesada2016, I was curious about this, and wanted to check in to see if you'd been able to give it any kind of a look. Thanks!

@jquesada2016
Copy link
Author

@nonparibus I have not had a chance to give this a shot, but have not forgotten about it either.

@vilkinsons vilkinsons added the lang/rust Pull requests that update Rust code label Aug 18, 2023
@vilkinsons vilkinsons changed the title .into_report() doesn't include error source chains GEN-68: .into_report() doesn't include error source chains Aug 19, 2023
@vilkinsons vilkinsons added priority/3 low Lower priority: nice-to-have hint/help wanted Experience: Extra attention is needed labels Jan 8, 2024
@VorpalBlade
Copy link

Would it be possible to implement this for anyhow at least? Without it, it is very difficult to gradually transition a code base from anyhow to error-stack. You basically end up having to switch everything over at once (or loose backtraces, which is a non-starter).

@QAston
Copy link

QAston commented Apr 27, 2024

Here's the workaround I'm using (requires anyhow and anyhow feature enabled in error_stack)

use error_stack::{Context, IntoReportCompat, Report};

/// Converts `Result<Ok, Err>` into `Result<Ok, Report<Err>>`
/// Workaround for <https://github.com/hashintel/hash/issues/4355>
pub trait IntoReport {
    /// Type of the [`Ok`] value in the [`Result`]
    type Ok;
    /// Converts the [`Err`] variant of the [`Result`] to a [`Report`]
    fn into_report<C: Context>(self, c: C) -> Result<Self::Ok, Report<C>>;
}

impl<T, E: std::error::Error + Send + Sync + 'static> IntoReport for Result<T, E> {
    type Ok = T;

    #[track_caller]
    fn into_report<C: Context>(self, c: C) -> Result<<Result<T, E> as IntoReport>::Ok, Report<C>> {
        IntoReportCompat::into_report(self.map_err(|e| anyhow::anyhow!(e)))
            .map_err(|e| e.change_context(c))
    }
}

@TimDiekmann
Copy link
Member

I implemented capturing for sources in the linked PR above. For simplicity (to get the output in the first place) I added them as strings (rendered as contexts).

@TimDiekmann
Copy link
Member

Part of error-stack@0.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library) category/enhancement New feature or request hint/help wanted Experience: Extra attention is needed lang/rust Pull requests that update Rust code priority/3 low Lower priority: nice-to-have
Development

Successfully merging a pull request may close this issue.

6 participants