Expose PER errors

[jkalez]

ber, cer, and der modules all expose their error types publicly via easy-to-use enums. However aper and uper hide their error implementations within the (pub(crate)-only) hidden per module. Ideally, aper and uper would expose their error types publicly as well so that users can take different actions based off of the encountered error.

A common use-case is to read bytes from some Read into an intermediate buffer and attempt to decode. If the decode fails with Incomplete error (e.g. rasn::ber::de::Error::Incomplete), the user can read more bytes off and try again.

Since the errors in aper and uper are non-public, the user cannot make any determination as to what the error was, and cannot take this type of approach.

[XAMPPRocky]

Thank you for your issue! This isn't intentional and they should be exposed. There's an ongoing refactor of errors at the moment, but we can also make a small improvement to expose them in the meantime

[Nicceboy]

I am currently refactoring errors, and new implementation will include full backtrace of all the errors in every codec and at least the Error type and Message from display e.g. #[snafu(display("Expected {:?} tag, actual tag: {:?}", expected, actual))].

The new approach is following how aper and uper works, there is separate struct which wraps errors as EncodeError or DecodeError including Kind (similar Enum from ber et al., ). Struct wrapper makes it cleaner to include additional meta data. Error and ErrorKind will be public and you can match the error types, but the functionality will be breaking from current one.

Codecs will all use the same Error module (Same structs, same enums), and my main problem is currently to decide whether we want to still expand errors with .context from snafu. e.g. if per gets decoding error of UTCTime from ber, do we need to known explicitly with the help of .contex (Snafu trait) that error is coming from ber, since it can be already deduced from the backtrace.

This has impact for how the error module API could be used or designed, so any comments or requirements would be helpful on that.

[XAMPPRocky]

@Nicceboy I was thinking about that. I think (ideally) we should not use string errors where possible, to allow users to easily match on codec specific issues.

For example, matching against tags is only relevant to DER, BER, CER, etc. So it shouldn't be possible to match against errors that will never happen.

I think either we should use a generic type parameter or a dyn trait that can be downcasted to a specific codec error type.

Right now I think I lean towards a type parameter over a dyn trait, as that's easier to use for callers, but it does make it slightly annoying if you are handling multiple codecs, so I'm open to being persuaded one way or the other.

[Nicceboy]

For example, matching against tags is only relevant to DER, BER, CER, etc. So it shouldn't be possible to match against errors that will never happen.

Hmm, that might make this much harder to implement and refactoring less useful. I was hoping that it would be easy to reuse all common errors without multiple definitions, e.g. all codecs can share single IntegerOverflow error.

We could group errors for common and codec specific, but then again 2 codecs can share error what the rest do not share, and then eventually the current implementation could be the most useful. It also can be very time consuming to identify which errors can be shared among all codecs.

I was thinking, for example, the following, if matching an error which cannot happen can be allowed.

#[derive(Snafu)]
#[snafu(visibility(pub(crate)))]
#[derive(Debug)]
#[snafu(display("Error Kind: {}\nCodec:\n{}\nBacktrace:\n{}", kind, backtrace, codec))]
pub struct DecodeError {
    kind: Kind,
    codec: crate::Codec,
    backtrace: Backtrace,
}

impl DecodeError {
    fn new(kind: Kind, codec: crate::Codec) -> Self {
        Self {
            kind,
            codec,
            backtrace: Backtrace::generate(),
        }
    }
    #[must_use]
    pub fn integer_overflow(max_width: u32, codec: codec::Codec) -> Self {
        Self::new(Kind::IntegerOverflow { max_width }, codec)
    }

#[derive(Snafu)]
#[snafu(visibility(pub(crate)))]
#[derive(Debug)]
pub enum Kind {
   #[snafu(display("Actual integer larger than expected {} bits", max_width))]
    IntegerOverflow {
        /// The maximum integer width.
        max_width: u32,
    },
}

Then error could be created as DecodeError::integer_overflow(u32::MAX, <yourcodec>) for every codec.

Right now I think I lean towards a type parameter over a dyn trait, as that's easier to use for callers, but it does make it slightly annoying if you are handling multiple codecs, so I'm open to being persuaded one way or the other.

I might a need a bit more details to get the idea. I assume this is a bit different than I have been currently doing. When compared to previous, DecodeError would be trait and grouped codec-specific error structs just implement it?

[XAMPPRocky]

I might a need a bit more details to get the idea.

It's essentially the same, except there's an extra variant in Kind for codec specific errors.

trait CodecError: std::fmt::Display {}

// Static version, allows for `Kind<BerError>`, `Kind<DerError>`, etc.
#[derive(Snafu)]
#[snafu(visibility(pub(crate)))]
#[derive(Debug)]
pub enum Kind<T: CodecError> {
   #[snafu(display("Actual integer larger than expected {} bits", max_width))]
    IntegerOverflow {
        /// The maximum integer width.
        max_width: u32,
    },
   #[snafu(display("codec error {} bits", max_width))]
    CodecError {
        inner: T,
    },
}
// dyn version, allows for one type.
#[derive(Snafu)]
#[snafu(visibility(pub(crate)))]
#[derive(Debug)]
pub enum Kind {
   #[snafu(display("Actual integer larger than expected {} bits", max_width))]
    IntegerOverflow {
        /// The maximum integer width.
        max_width: u32,
    },
   #[snafu(display("codec error {} bits", max_width))]
    CodecError {
        inner: Box<dyn CodecError>,
    },
}

impl Kind {
    fn get_inner<C: CodecError>(&self) -> C {
        // downcast to `C`
    }
}

[jkalez]

Any updates on this? Even before getting the whole error refactor in, it would be nice to have the Kind exposed so we can match off of it

[Nicceboy]

I have time to finish the refactor this week if it is soon enough, based on the suggested static version. Then you can already move to using the new API directly.

[Nicceboy]

I used quite many hours on trying to use the generic version, and it seemed to be rather difficult.

E.g. when using

pub enum Kind<T: CodecError> {
   #[snafu(display("Actual integer larger than expected {} bits", max_width))]
    IntegerOverflow {
        /// The maximum integer width.
        max_width: u32,
    },
   #[snafu(display("codec error {} bits", max_width))]
    CodecError {
        inner: T,
    },
}

It means that also the above DecodeError struct needs to define the generic variable, and that means all functions in the codec needs to return same Codec specific CodecError, which might not be always true.

So instead of generic, using another enum there seems to work so, that there can be common errors and codec specific.

#[derive(Debug)]
pub enum CodecDecodeError {
    Ber(BerDecodeError),
    Cer(CerDecodeError),
    Der(DerDecodeError), 
    Uper(UperDecodeError),
    Aper(AperDecodeError)
}

And then, for example

pub enum Kind {
   #[snafu(display("Actual integer larger than expected {} bits", max_width))]
    IntegerOverflow {
        /// The maximum integer width.
        max_width: u32,
    },
   #[snafu(display("codec error {} bits", max_width))]
    CodecSpecific {
        inner: CodecDecodeError,
    },
}

On some cases it can be a bit verbose, but I guess it fulflls the use case where everything can be matched explicitly.

[XAMPPRocky]

An enum also fits the goal of allowing users to catch codec specific errors. I think that's an acceptable solution.

[Nicceboy]

As status update, it took a bit longer than it should. Everything, including standards seems to compile now and tests pass.

But I need to write some documentation/tests still, and some errors (probably many) might be incorrectly categorised, but the core functionality should be there.

All custom string-based errors have been removed and there is type for all kinds of errors, excluding some standards and macros.

[XAMPPRocky]

Great work! 🙏

[Nicceboy]

When writing some test code and examples, I also ended up moving custom *String errors under this module and improving them a bit, is this okay?

[XAMPPRocky]

Yes

[Nicceboy]

Errors should be public now. I forgot to reference this on PR and I guess it can be closed now?