introduce errors with info

[drahnr]

As discussed with @bkchr we need debug info in errors and the rationale behind not including them (i.e. such as in ring) is not a rational decision given rust error handling and the scope of our implementation.

[coriolinus]

Any chance we could use snafu instead of thiserror? We don't want to just propagate errors; we want to add context most of the time, and ResultExt is very useful for that.

For example, in broadcast_signal, every subsystem will produce precisely the same error, so it would be very helpful to be able to annotate which subsystem's error actually got propagated.

[drahnr]

My idea was to use thiserror for the subsystem error definition, but then use anyhow for the overseer, which also provides a fn context(..).

I never worked with snafu, thiserror is just usually a safe bet, given it boils down to something you would implement manually as well.

[drahnr]

Any chance we could use snafu instead of thiserror? We don't want to just propagate errors; we want to add context most of the time, and ResultExt is very useful for that.

I think thiserror is not limited by that, it just boils down having to add another error variant with the same type of source error, the ergnomics are a bit different though.

For example, in broadcast_signal, every subsystem will produce precisely the same error, so it would be very helpful to be able to annotate which subsystem's error actually got propagated.

Since that would bubble up in the overseer, we would have annotations there. I just would prefer to very concretely and explcitily typed errors being passed around.

[coriolinus]

Yeah, I do also like thiserror; it just makes the question of adding the context a bit more tricky. We could, for example, add a variant to SubsystemError:

Contextualized(&'static str, Box<SubsystemError>),

and

impl SubsystemError {
  fn context(self, ctx: &'static str) -> Self {
    Self::Contextualized(ctx, Box::new(self))
  }
}

and then do .map_err(|err| err.into().context("whatever")) all over the place, but that's kind of awkward.

[ordian]

Another error library option is https://github.com/yaahc/color-eyre which is developed by one the Error Handling Project Group's shepherds, see this video for more details.

[drahnr]

Another error library option is https://github.com/yaahc/color-eyre which is developed by one the Error Handling Project Group's shepherds, see this video for more details.

Ah yes! I remember, there was a video from rust conf this year, but it's mostly an anyhow replacement rather than an thiserror replacement.

I think I have a rough idea and will go for thiserror with eyre for now, unless the pain becomes too large, but right now it looks fairly good.

[ordian]

@drahnr it seems after the CI update to latest nightly, you need to update Cargo.lock to include paritytech/substrate#7381 and maybe paritytech/substrate#7395

[ordian]

bot merge

[ghost]

Waiting for commit status.

[ghost]

Checks failed; merge aborted.