error-stack: Overhaul of the "related-errors" feature

[TimDiekmann]

error-stack has the concept of "related-errors". This means it's possible to have multiple frames living next to each other. The current approach, however, has some downsides:

• It's not possible to convert a Report into Error as there may be multiple sources of an error, which the Error trait does not support. This is especially annoying if libraries want to use error-stack but downstream crates don't (for whatever reason) as Report cannot implement Error.
• Report::current_context only returns the first context, not a list of contexts, if multiple contexts at the same level exist. (The current implementation contains a bug in addition insofar that if adding a context and an attachment with the same type the attachment may be returned instead)
• Report::frames and similar functions return a linearized iterator of all frames in the order in which they would be printed, however, there is no real distinction between a related error and a non-related error
• Generally, every function needs to take a potentially related context into account (thus we e.g. removed source in favor of sources). In most use cases, however, there are no related errors.
• Constructing "related-errors" is hard as we want to ensure, that at least one context is available, thus, FromIterator cannot be implemented on Report.

"related-errors" are a great feature and have great potential when processing inside of a loop happens, but in all other cases, it's more of an annoyance.

I was thinking a lot about how to fix the above errors. Generally, I came up with two potential solutions. Both ideas revert most of the API changes done in #747 but all above points should be resolved. However, as we really want to continue supporting "related-errors", I propose to

1. either build-in functionality to support Report<Vec<C>>. Methods, currently returning potentially multiple things (like sources) would only return T in Report<T> (either C: Context or Vec<C: Context>). Sadly, this is not as simple as it sounds. Currently, we rely on C: Context in Report<C>, with Report<Vec<C>> this is not the case anymore. We eventually want to remove the Context trait as soon as core::error::Error has been stabilized, so it's not possible to implement Context for Vec<C: Context>. As an alternative, a MultiContext struct could be provided with built-in support from error-stack.
2. or extend the FrameKind enumeration with FrameKind::MultiContext to hold not only one but multiple contexts.

In both cases, we could provide easy conversions from Vec<Report<C>> to build a multi-context.

Both proposals are breaking changes.

---

Thanks to @indietyp for maintaining a list or expected changes

[indietyp]

As a big proponent of related errors (after all, I was the one that first implemented them 😅), I'd like to give some context and my view/opinion on the matter.

I agree with @TimDiekmann that the potential for related errors is significant but may have untapped potential, but I am not in favor of any of the outlined proposals.

It's not possible to convert a Report into Error as there may be multiple sources of an error, which the Error trait does not support.

I think this is by far the greatest issue, then again we have solved this through the into_error functions.

Report::current_context only returns the first context, not a list of contexts, if multiple contexts at the same level exist. (The current implementation contains a bug in addition insofar that if adding a context and an attachment with the same type the attachment may be returned instead)

Yes, but I don't think that this is a problem of related errors, more like that the API is currently in between pre and post related errors.

Report::frames and similar functions return a linearized iterator of all frames in the order in which they would be printed, however, there is no real distinction between a related error and a non-related error

This is still from the 0.1 to 0.2 switch, I am painfully aware that this isn't the correct behavior, maybe the visitor pattern would help here instead. (or one would only iterate until multiple contexts are encountered)

Generally, every function needs to take a potentially related context into account

I think this is generally just a thing that happens with nested values, same applies to things like XML or JSON.

Constructing "related-errors" is hard as we want to ensure, that at least one context is available. Thus, FromIterator cannot be implemented on Report.

This is something I am painfully aware of, but something that I at least locally have several fixes for. By implementing fold_report() on iterators and tuples it is possible to eliminate most of these problems. Currently, quite a lot of glue code is required that would be eliminated by them, an example for tuples can be seen in #1875.

Both solutions outlined are lacking because, IMO they either break encapsulation or hinder functionality (a lot). They only allow us to attach multiple contexts, but not create a real tree of reports, with independent attachments, but in a real-world application whenever there are independent computations, we'd like to aggregate we take multiple reports and smoosh them together. This means that we have two completely independent reports that are added together. This can happen multiple times, building up a report meaning that these multiple contexts might be deep down in the tree. Now I need to know/test if there have been multiple reports instead of a single API call.

These would practically reduce the functionality to the one present in miette, the error-handling library I "escaped" from, because of these exact reasons D:

In my opinion, it would make more sense to go more in the tree direction, instead of trying to fit a tree-sized piece in a stack-shaped hole. I think that the biggest problems for related errors in error-stack are:

1. tooling, how to construct them
2. visibility, make sure people know how to use them
3. consistency, make sure the API is consistent and not in 0.1-0.2 nirvana
4. usability, make sure surrounding calls like sources() are easier to use.

Do I have a specific solution in mind right now? Partially.

I think for (1) we can already do a lot more with methods exposed for iterators and tuples (fold_report). I think for (2) we need to explore how to maybe write the lib.rs to make it stand out more and/or create dedicated examples. (3) current_contexts should return &[T] instead of &T, ideally NonEmptySlice<T>, same with sources() and co. (4) for things like the iterator maybe it would make sense to switch up the pattern used (Visitor pattern) or create an iterator that results in multiple other iterators upon a split.

[TimDiekmann]

Thanks, that generally looks good! I think we could use this possibility and clean-up the existing interface.

First of all, I like Report<[C]>. Probably not an issue, but this requires a ?Sized bound.
I have a few comments on the provided list:

• into_multiple: I think having this is good, but I don't really like the name. What about this:
  • impl<C: Context> From<Report<C>> for Report<[C]>
  • impl<C: Context> TryFrom<Report[C]> for Report<C> with Error = Report<[C]>
• I just noticed that we don't have source on Report, only on Frame. If we add source to Report directly we, however, need to make it optional.
• We have current_context on singular reports, introducing NonEmptySlice, which would come with a whole new iterator interface, is greatly out of scope of error-stack. Generally, this will make interfacing error-stack with related errors a lot harder. Returning ExactSizeIterator (and TrustedLen on nightly) will be the better option here.
• instead of adding Iterator::fold_results, Tuple::fold_results, and from_vec (which actually is not a Vec) I'd provide fn from_iter(impl IntoIterator<Item = Report<C>>) -> Option<Report<[C]>>
• with seems to be unneeded (it's misleading and can easily be achieved by into_multiple().extend_one(), Report is not supposed to be a builder)
• contexts should be len or num_contexts, but contexts alone is misleading

[TimDiekmann]

This, however, does not yet address the first issue

[TimDiekmann]

Let's try to stick with the most minimal but correct interface and build more convenient functions on top of that.

[indietyp]

You are entirely correct, I was thinking of returning a slice, but the contexts won't share the same memory, so it's nonsense. An iterator would be the correct way to do things.

I think Iterator::fold_results and Tuple::fold_results are still valuable (I can go into more detail as to why if wanted), but something we can add after the overhaul, big 👍 on fn from_iter().

Names are ofc temporary, am bad with names; maybe our local naming genius™ @Alfred-Mountfield, has a better idea.

We have current_frames instead of source; IMO, it would make sense to rename current_frames to source/sources respectively.

[TimDiekmann]

current_frame also returns the, well, current frame 😄, including attachments so it's different and this is basically the entry point to the internal error-stack API.

I agree that fold_results may be useful. I'm also thinking about abusing the report! macro here, currently, it's not really useful in any way.

[I] am bad with names

I doubt that and I know @nonparibus agrees with me here. 🙃

[indietyp]

Moving this up so that our discussion can stay focused

---

1. change nothing to how everything works (e.g. Box<[Frame]>)
2. split into a low-level interface (LL, Frames) and high-level interface (HL, Report), LL stays the same, you will only be able to query for multiple sources, etc, HL overhaul to separate single Context vs. multiple Context

Separation between Report<C> and Report<[C]>

Report<C> and Report<[C]> methods:

• attach_*(self) -> Self
• change_context(context: C) -> Report<C>

Report<C> methods:

• new() -> Report<C>
• current_context() -> &C
• source() -> Option<&Frame>
• into_multiple() -> Report<[C]>, name TBD
• impl TryFrom<Report<[C]>>

Report<[C]> methods

• into_single() -> Option<Report<C>> (potentially Result<Report<C>, Report<[C]>> to be able to recover, name TBD)
• current_contexts() -> impl ExactSizeIterator<C> + TrustedLen
• sources() ->&[Frame]
• from_iter(reports: impl Iterator<Item = Report<C>>) -> Option<Report<[C]>>
• num_contexts() -> NonZeroUsize (the current amount of "splits"), name TBD
• impl Extend<Report<C>>
• impl From<Report<C>>

Additional methods for constructing Report<[C]>

• Iterator::fold_results(), Tuple::fold_results() (after overhaul)
• Adjust macros

[indietyp]

I think you misunderstood me, sorry! fold_report would complement from_iter() and would only be implemented on Iterator<Item = Result<T, C: ?Sized>> the result would be: Result<impl FromIterator, [C]>.

This would allow us to easily merge the results of an iterator into a single report.

[TimDiekmann]

I see, thanks for the clarification. On what you would implement fold_results? On Report itself?
Do we really want C: ?Sized? this implicitly would flatten the reports, which might be unexpected. Should we allow implicit conversion here generally/should we use Iterator<Item = Result<T, impl Into<Report<C>>>>?

Generally, we should never write C: ?Sized but use another generic name (e.g. E or T) as a context is never unsized.

[indietyp]

You are entirely correct, C: ?Sized is a dumb idea, ?Sized should only be an implementation detail and would likely confuse the user.

I think we would solve this via an IteratorExt and TupleExt trait. That way, you're able to do: iter.map(...).fold(...).fold_reports()

This also means that in the case of TupleExt, we're able to retain all types, e.g.

// t1, t2, t3 are all results of fallible *independent* computations
let t1: Report<C> = f1();
let t2: Report<C> = f2();
let t3: Report<C>  = f3();

let (t1, t2, t3) = (t1, t2, t3).fold_reports().change_context(Error)?;

Which is very handy!

Should we allow implicit conversion here generally/should we use Iterator<Item = Result<T, impl Into<Report<C>>>>?

That is a tricky question to answer; my gut feeling is to implement this for Iterator<Item = Result<T, impl Into<Report<[C]>>>>, which would allow implicit flattening. I think this is fine because for that to take place, the context needs to be the same anyway; this also allows for cases where in the .map(...) or other calls, we might have other computations that return Report<[C]> because they have some independent computations taking place and are already aggregating reports.

While thinking about it, should we also add into_multiple, and into_single (name TBD) to ResultExt?

[TimDiekmann]

Iterator<Item = Result<T, impl Into<Report<[C]>>>>

I specifically asked about the singular variant to utilize the current From implementation. I'm unsure, if we should implement From<C> for Report<[C]> as well as with this change you cannot write let r: Result<_, Report<_>> = x.into() anymore (I guess?).

While thinking about it, should we also add into_multiple, and into_single (name TBD) to ResultExt?

Probably we shouldn't (yet). The traits are polluting the global Result and Future namespace already. To keep the option to add it later we however should consider sealing it.

[indietyp]

I was thinking about From<Report<C>> for Report<[C]>, so your iterator can both use Report<C> and Report<[C]>

[TimDiekmann]

Done in #5047