search

rusticata / asn1-rs

Parsers/Encoders for ASN.1 BER/DER data
Apache License 2.0
9 stars 14 forks source link

Implement Error for OidParseError #39

Open MattesWhite opened 4 months ago

MattesWhite commented 4 months ago

While using your excellent crate I noticed that OidParseError does not implement Error. I can see that in no_std environments this is not required but for many cases this is a low-hanging nice-to-have.

This PR implements the Error trait for std builds.

MattesWhite commented 4 months ago

So is there anything else I can do to finish/merge this PR?

chifflier commented 4 months ago

Hi, Thank you for your contribution. Before considering merge (PR looks nice in itself), I considered how other errors are derived in this crate, in file src/error.rs.

Previously, this crate was doing the same, but ended up choosing displaydoc to implement errors in https://github.com/rusticata/asn1-rs/commit/ba72bde7572b09a11a8b7b47289c94bd2bb7883d (see also https://github.com/dtolnay/thiserror/pull/64#issuecomment-735805334). It is a bit annoying, because thiserror brings more features than just Display, but displaydoc integrates nicely with documentation since it uses regular doc comments. It is still used for SerializeError.

At this point, I am unsure what solution is best. Any thoughts on this?

MattesWhite commented 4 months ago

The real value of thiserror shows when you start to have nested errors. Because thiserror does not only do the empty implementation of std::error::Error but with the #[from] or #[source] annotation it also implements a custom version of Error::source() which returns the inner error, allowing users to inspect the whole error chain. Admittedly, this is not used for OidParseError but you could in std build which would require a lot of conditional compilation shenanigans. This is also the reason why I didn't do it.

displaydoc is interesting as it's first example literally shows how it interacts with thiserror. It seems that thiserror doesn't require the #[error(...)] annotations when the underlying type already implements Display. I change the PR accordingly. So all in all I came to the following conclusion:

  1. For no-std error types displaydoc is totally fine
  2. #[cfg_attr(feature = "std", derive(thiserror::Error))] on those errors for std builds should result in good enough std::error::Error types