error-stack support for multiple related errors

[indietyp]

Pull Request:

• Support related errors in error-stack #747

---

I have been checking out error-stack lately, and it seems very promising, great work! For my specific usecase I would want report multiple errors at once, while not natively supported by the std::error::Error trait, crates like miette have the concept of related errors.

From my investigation into the library error-stack doesn't currently support this feature, is this correct? Are there any plans for supporting such a thing in the future? or would you be open for a PR implementing something like this? I think something like this would be a great addition to the library!

What I'd love to have is a DAG like the diagram below:

flowchart BT
  err1{{std::io::Error}}
  err2{{std::io::Error}}
  err3{{crate1::Error}}

  ctx1[crate2::Context]
  ctx2[crate3::Context]
  ctx3[crate4::Context]
  ctx4[crate2::Context2]
  ctx5[crate3::Context2]
 
  err1 --> ctx1
  ctx1 --> ctx2
  err2 --> ctx2
  ctx2 --> ctx3
  err3 --> ctx4
  ctx4 --> ctx5
  ctx5 --> ctx3

Loading

[indietyp]

For the "source" of the error (with compatibility to std::err::Error) we could either only take the left hand side, or linearize the resulting graph through topological sorting.

[indietyp]

Just as a reference, the topological sorting using DFS of the graph shown above would result in the following:

flowchart BT;
   a{{std::io::Error}} --> b{{crate1::Error}}
   b --> c[crate2::Context2]
   c --> d[crate3::Context2]
   d --> e{{std::io::Error}}
   e --> f[crate2::Context]
   f --> g[crate3::Context]
   g --> h[crate4::Context]

Loading

[TimDiekmann]

Hi @indietyp and thanks for filing this!

Generally, Pull Request are highly appreciated!

This is a pretty good scenario and we haven't thought about this yet. However, it's possible to attach any data to a Report, even other reports like:

let report_a = Report::new(...);
let report_b = Report::new(...);
report_a.attach(report_b);

Later it's then possible to call report_a.request_ref() or even for each frame in report_a.frames() to call frame.request_ref() (Note that requesting currently requires a nightly compiler). I think this is almost the behavior you are looking for?

However, there is one issue: Report has a generic for the current Context, so you would need to specify which kind of Report you want to get back.
Do you have a specific solution in mind? Maybe it makes sense to talk about a potential idea before opening a PR.

[indietyp]

My suggestion would be to have a new trait: ChangeContext (or named differently), which is implemented for Report and all tuple sizes, where each element must be a Report, but their context doesn't need to be the same.

The thing that would need to be changed is that Frame would have multiple sources.

Traversing the "graph" via .frames() would be more challenging, and I don't have a good way of doing that just yet.
Displaying the error would also probably involve further indentation on a "cross-road," e.g., where two reports/frames are joined.

ChangeContext::change_context<T: Context>(self, context: T) -> Report<T>

if a: Report<C1>, b: Report<C2>, c: C3 then: (a, b).change_context(c) -> Report<C3>

[indietyp]

but to your earlier point, the provider API would kind of make sense, but I would need to know the context type in advance, then traverse through, which is doable, but a lot of pain

[indietyp]

I had a thought when traversing frames, we could, in theory, use BFS or DFS (or the topological sorting), but this would mean that we wouldn't be able to guarantee that every frame before a parent of the previous one.

[TimDiekmann]

I thought about this for a long time and I think I came up with a possible solution.
First of all, the ReportImpl abstraction should be removed, so Report refers to Frame directly. This can be done by not storing the backtrace/span trace on Report but as an attachment, if they were captured. I think this moves both to a less "special" spot and make the behavior for the user more intuitive. I think this should be done regardless of related errors.

Now, a Report is only a Frame anymore. We now have two options (method naming TBD):

• Add a Report::attach_report method (which can be merged with attach once specialization is a thing):
  Stores the Frames of the attached Report inline of self
• Add a Report::combine method:
  Stores the Frames of the attached Report next to the current frame, e.g. by making Frame::source a Vec<Frame>

I'd prefer the second option, especially as it's not special casing Report when attaching but "simply" add a feature. For printing, I have a few thoughts. The structure is now a tree (but we keep pushing onto the root of it), Backtrace and SpanTrace as attachments instead could print the number of frames: Printing could happen like this:

Current context
| attachment
| attachment
|\
| Another context
| | attachment
| |
| Other context
| | attachment
| | backtrace with 15 frames
|
Other context
| attachment
| backtrace with 12 frames
| span trace with 3 frames

What do you think about that?

[indietyp]

I like the 2nd approach of using combine a lot more.

This would mean that in my aforementioned example of (a, b).change_contex(c) would become a.change_context(c).combine(b) (naming tbd), right?

I feel like this could be pretty confusing for users because in the whole library, you "layer" things on top of each other - but you then break that methodology, which could confuse if that makes sense. (Just trying to think aloud)

I like the output you proposed. It looks apparent and concise.

In my own projects (where I'd really like to switch to error-context) I am currently making use of a modified version of the Ctxc from serde, I wonder if something like this might also be useful to include in this crate so that in theory you could do:

let group = ReportGroup::new(context);
group.push(err);
group.push(err2);

group.check()?;

What do you think?

[TimDiekmann]

Great that we are on the same page regarding the 2nd approach!

Yes, logically, this would be more like a.change_context(c).combine(a), thus combine should probably be called add_source or similar. I don't think this would necessarily lead to confusion, but I agree that the API is suboptimal.

I experimented around a little bit locally and I think I was able to resolve this. I'll use a bit of pseudo-code to express the approaches:

struct Report {
    // Each report can hold multiple frames
    frames: Vec<Frame>, // Could be changed to `SmallVec<[Frame; 1]>`
}
struct Frame {
    // When attaching or changing context the `report.frames` are used as the sources for the new frame
    // so we don't need to add 
    sources: Box<[Self]>,
    ...
}

// Combining reports with same context
impl<C> Extend<Report<C>> for Report<C> {
   ...
}

// Probably, this could also be extended by implementing `FromIterator` to use `collect` but this also 
// introduces weird edge cases like passing an empty collection

I like the output you proposed. It looks apparent and concise.

Thank you! Actually, it would be grand if we could have a similar output like miette 👀

example

I'd prefer not to add new structs to the crate. I think my idea would result in the same behavior?

My approach however has two downside:

1. Currently, the API is designed around a Report holding a single Frame that holds a single source. The interface would change quite a bit, thus this would introduce breaking changes. The next release will also have a breaking change (switching to core::any::Provider) and will also deprecate IntoReport::report() in favor of IntoReport::into_report(). The former will only affect nightly users. We try to avoid breaking changes. We are not in a stable version right now, but if we keeping the breaking changes at a minimum would be good.
2. Report (if not using smallvec) and Frame would be bigger. With SmallVec, the Report should be the same in size, Frame would use a fat pointer rather than a thin pointer.

[indietyp]

Thank you! Actually, it would be grand if we could have a similar output like miette 👀

Maybe using this - in another PR - might be helpful to implement multiple possible output formats (with a hook system of some sorts) - I can imagine output to JSON might be beneficial as well.

I'd prefer not to add new structs to the crate. I think my idea would result in the same behavior?

Yes, it does. I just found this approach a bit better, especially the panic on the drop, as sometimes you forget to return the error. After thinking about it, this isn't something that should be in the main crate error-stack. This functionality can be easily implemented by itself, or we/I could release a companion crate like error-stack-group or something similar which implements this.

I think the benefits for (1) outweigh the downsides. If the next version already introduces breaking changes, it might be good to have those in one swoop and then keep them to a minimum - if that makes sense.

Regarding (2), Report and Frame would be larger, but this might be negligible with smallvec and the union feature. (Experimentation would be needed to confirm)

[TimDiekmann]

Maybe using this - in another PR - might be helpful to implement multiple possible output formats (with a hook system of some sorts) - I can imagine output to JSON might be beneficial as well.

We already support that. It's possible to set a debug hook or a display hook. A JSON output example is also provided.

I just found this approach a bit better, especially the panic on the drop, as sometimes you forget to return the error.

Couldn't this be solved by marking #[must_use] Report? This would at least emit a warning. I agree that this shouldn't be in error-stack, feel free to create a companion crate! 🙂

I think the benefits for (1) outweigh the downsides...

I agree. Having breaking changes in only one release is far better than having dozens of breaking releases, even though it's technically allowed.

Regarding (2)...

This should mostly be fine. The fast path is most likely the happy path, not the error path, so allocating one more usize shouldn't be a big deal. Anyway, I like to feature-flag smallvec (default-enabled), so it's possible to use error-stack without any dependencies.

Do you want to open a PR for this feature? 🚀

[indietyp]

We already support that. [...]

O yea I totally forgot about that 🤦

Anyway, I like to feature-flag smallvec (default-enabled), so it's possible to use error-stack without any dependencies.

sounds good to me!

Yea, I think I am going to give implementing this functionality a try :D

[TimDiekmann]

Great! Please feel free to ping me when having any problems! 🙂

[TimDiekmann]

This feature was implemented in #747 and will be available with v0.2.0.