Add rich error type #80
Conversation
|
Looks really good, although, would we want to have backtracing as default in the library? Maybe have this as a feature? |
src/errors.rs
Outdated
| // Helpers for creating specific kinds of Rich Errors | ||
| pub fn packet_underflow(source: Option<ErrorSource>) -> Self { Self::new(GDError::PacketUnderflow, source) } | ||
| pub fn packet_bad(source: Option<ErrorSource>) -> Self { Self::new(GDError::PacketBad, source) } | ||
| pub fn protocol_format(source: Option<ErrorSource>) -> Self { Self::new(GDError::ProtocolFormat, source) } | ||
| pub fn unknown_enum_cast(source: Option<ErrorSource>) -> Self { Self::new(GDError::UnknownEnumCast, source) } | ||
| pub fn invalid_input(source: Option<ErrorSource>) -> Self { Self::new(GDError::InvalidInput, source) } | ||
| pub fn decompress(source: Option<ErrorSource>) -> Self { Self::new(GDError::Decompress, source) } | ||
| pub fn type_parse(source: Option<ErrorSource>) -> Self { Self::new(GDError::TypeParse, source) } | ||
|
|
||
| // Helpers for converting source types, these were added as needed feel free to | ||
| // add your own | ||
| pub fn packet_underflow_from_into<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> Self { | ||
| Self::packet_underflow(Some(source.into())) | ||
| } | ||
| pub fn packet_bad_from_into<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> Self { | ||
| Self::packet_bad(Some(source.into())) | ||
| } | ||
| pub fn protocol_format_from_into<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> Self { | ||
| Self::protocol_format(Some(source.into())) | ||
| } | ||
| pub fn unknown_enum_cast_from_into<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> Self { | ||
| Self::unknown_enum_cast(Some(source.into())) | ||
| } | ||
| pub fn invalid_input_from_into<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> Self { | ||
| Self::invalid_input(Some(source.into())) | ||
| } | ||
| pub fn decompress_from_into<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> Self { | ||
| Self::decompress(Some(source.into())) | ||
| } | ||
| pub fn type_parse_from_into<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> Self { | ||
| Self::type_parse(Some(source.into())) | ||
| } |
There was a problem hiding this comment.
I dont like creaing a function for every error we have, couldn't we shorten this using macros/traits? Not really a must do thing but I'll look into it.
There was a problem hiding this comment.
A macro would definitely be possible, I'm not sure about a generic function as I don't think rust supports a generic implementation for each variant of an enum.
There was a problem hiding this comment.
Yeah, I can't really see a solution for this now.
|
A macro could look like this: macro_rules! error_kind_impl {
($kind: expr, $name: ident, $from_name: ident) => {
pub fn $name(source: Option<ErrorSource>) -> GDRichError {
GDRichError::new($kind, source)
}
pub fn $from_name<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> GDRichError {
GDRichError::$name(Some(source.into()))
}
}
}
impl GDRichError {
// ...
error_kind_impl!(GDError::PacketUnderflow, packet_underflow, packet_underflow_from_into);
// ...
}Unfortunately without proc macros both functions names would need to be specified (no way to use macros in place of idents: rust-lang/rust#29599) and each variant needs to be listed. |
|
I cannot believe how much I was over-complicating things. Implementing a single method on the error kind enum is way simpler and works better... Lines 51 to 61 in ebacab3 rust-gamedig/src/protocols/valve/protocol.rs Lines 113 to 117 in ebacab3 Things I would have to do for this PR to be usable (assuming everyone is happy):
|
|
Hey, regarding this point:
I think this should be discussed in #65, as this is about giving extra information, and this might be included in the enum by default, we can omit this in this PR. |
|
Also, please rebase! |
Adds a rich error type that will take a backtrace and allow capturing
the source of the error. The best way to use this is with the included
helpers that will automatically capture the backtrace and convert the
source error:
```
GDRichError::packet_bad_from_into("Reason packet was bad")
```
This is required for backtraces in rich errors.
This overhaul replaces the exhaustive impls of each variant as multiple methods on the rich error type with a singular .rich() method on the kind enum. This consumes the variant and converts it to a rich error with a source (.into() can be used if a source is not needed). I also took the liberty of replacing all usages with the this new method as I saw fit (adding various error messages) and converting a few PacketBad errors to TypeParse errors when they are the result of parse failing.
CosminPerRam
added
enhancement
A first draft of implementing a better error type that propagates source errors and messages.
Requires bumping MSRV to 1.65.0 as it uses backtraces.
I didn't rename
GDErroras that would require a lot more changes throughout the codebase, however I changedGDResultto use the newGDRichError(which usesGDErroras its kind part) and added a From implementation soGDErrors that are not updated are implicitly converted to the newGDRichError(with an added backtrace).Other than that its a pretty basic type but has a custom debug formatter so an unwrapped error output looks like this:
I need to look into whether implicitly capturing backtraces whenever an error is created (errors are created whenever they are used in or_err etc.) affects performance, although that may be a non-issue as backtraces are only captured if the
RUST_BACKTRACEenvironment variable is set.Related #65
This error type only allows either an error message or an error source as the error message would be stored as a source by converting
&strtoBox<dyn std::error::Error + 'static>.This doesn't address #65 point 5.