Hiding error source information from handlers

[arthurp]

(This relates back to #504 How to create a custom snafu error type with an optional source field?, but explores the place such a field fills even without any "stringly" typing of errors.)

Snafu is generally focused on adding information and context to an error which allows better handling of those errors in a caller. However, another important operation on errors is hiding information that would be meaningless for a caller while still maintaining that information for logging and debugging purposes. This means that an error may or may not have a source depending on what caused it internally. For example, a parser which uses from_str to parse numbers, but performs other parsing manually. We want to report ParseErrors to the caller simply, but also include any underlying from_str error for later debugging.

I have a solution, but I want input since I'm sure others have encountered related issues.

/// The parse failed with information as to where
#[derive(Snafu, Debug)]
struct ParseError {
    loc: usize,
    source: OptionalSource,
}

/// An error type which is really just `Option<Box<dyn Error + ...>>`
#[derive(Snafu, Debug)]
enum OptionalSource {
    WithSource {
        source: Box<dyn Error + Sync + Send + 'static>,
    },
    WithoutSource,
}

/// Parse "&" followed by a number.
fn parse_it(s: &str) -> Result<usize, ParseError> {
    if s.chars().nth(0) != Some('&') {
        return WithoutSourceSnafu
            .fail()
            .context(ParseSnafu { loc: 0usize });
    }
    s[1..]
        .parse()
        .boxed()
        .context(WithSourceSnafu)
        .context(ParseSnafu { loc: 1usize })
}

This should work and the code can be improved with a few extension methods to make parse_it more readable. But the use of a fake Option type and introducing another layer of Error::source just to attach another error which may be needed for debugging seems messy to me.

The questions are:

• Is there a more idiomatic way to do this?
• Is there a refactor anyone would recommend?

Thanks.

PS: This issue has hit me particularly hard because I want to gradually update an existing system to use Snafu, so I need to avoid major surgery on the error types to limit the scope of the refactor. But even after the full transition, we will still want to have errors that hide information, to make debugging easier when errors in one component pass through another before actually being handled.

Extension methods for ergonomics for the interested

trait ResultExt2<T, E> {
    fn as_source(self) -> Result<T, OptionalSource>;
}

impl<T, E: Error + Sync + Send + 'static> ResultExt2<T, E> for Result<T, E> {
    fn as_source(self) -> Result<T, OptionalSource> {
        self.boxed().context(WithSourceSnafu)
    }
}

impl<T0> ParseSnafu<T0>
where
    ParseError: ::snafu::Error + ::snafu::ErrorCompat,
    T0: ::core::convert::Into<usize>,
{
    fn fail<T>(self) -> Result<T, ParseError> {
        WithoutSourceSnafu.fail().context(self)
    }
}

/// Parse "&" followed by a number.
fn parse_it_2(s: &str) -> Result<usize, ParseError> {
    if s.chars().nth(0) != Some('&') {
        return ParseSnafu { loc: 0usize }.fail();
    }
    s[1..]
        .parse()
        .as_source()
        .context(ParseSnafu { loc: 1usize })
}

[shepmaster]

This is the way I'd write that code:

use snafu::prelude::*;

#[derive(Snafu, Debug)]
enum ParseError {
    MissingAmpersand,

    Invalid { source: std::num::ParseIntError },
}

impl ParseError {
    fn location(&self) -> usize {
        match self {
            ParseError::MissingAmpersand => 0,
            ParseError::Invalid { .. } => 1,
        }
    }
}

/// Parse "&" followed by a number.
fn parse_it(s: &str) -> Result<usize, ParseError> {
    let v = s.strip_prefix("&").context(MissingAmpersandSnafu)?;
    v.parse().context(InvalidSnafu)
}

fn main() {
    let _ = parse_it("&42").unwrap();

    let e = parse_it("42").unwrap_err();
    assert!(matches!(e, ParseError::MissingAmpersand));
    assert_eq!(0, e.location());

    let e = parse_it("&amp;").unwrap_err();
    assert!(matches!(e, ParseError::Invalid { .. }));
    assert_eq!(1, e.location());
}

A few possible changes I could see happening:

1. If parse() returned an error that indicated the error offset, I'd chain off to that in location.
2. Mark Invalid as either #[snafu(context(false))] or #[snafu(transparent)].
3. Introduce an opaque wrapper around the error, especially if it's crossing module / crate boundaries. The inherent methods on the inner type could be moved to the outer type or a second layer of methods could delegate to the inner implementation.

Here's the code with 2 and 3 applied:

#[derive(Snafu, Debug)]
struct ParseError(ParseErrorInner);

impl ParseError {
    fn location(&self) -> usize {
        match self.0 {
            ParseErrorInner::MissingAmpersand => 0,
            ParseErrorInner::Invalid { .. } => 1,
        }
    }
}

#[derive(Snafu, Debug)]
enum ParseErrorInner {
    MissingAmpersand,

    #[snafu(transparent)]
    Invalid { source: std::num::ParseIntError },
}

/// Parse "&" followed by a number.
fn parse_it(s: &str) -> Result<usize, ParseError> {
    let v = s.strip_prefix("&").context(MissingAmpersandSnafu)?;
    Ok(v.parse()?)
}