Error Propagation for Better Usability and Debugging

[cainthebest]

In the current implementation, error propagation is handled by the GDError enum and a GDResult<T> type. While this structure provides the basic functionality for error propagation, it's worth considering several improvements to enhance both the user experience and the debugging process:

1. Detailed Error Messages: The existing Display implementation for GDError offers a basic representation of the error - the enum variant's name. This approach might not always be sufficient or clear for end-users or developers. For example, in the case of BadGame(String), we could append the game's name to the error message for a more explicit explanation of the error.

2. Including Error Sources: The current GDError lacks the source method implementation which can provide more context about an error. Implementing this feature would allow developers to trace errors propagated from deeper layers of the software, which could significantly simplify the debugging process.

3. Inclusion of Error Backtrace: Incorporating the backtrace of an error can provide valuable insight during debugging. It helps to find the sequence of function calls leading up to the error's occurrence. This would be particularly useful when dealing with nested or chained errors.

4. Richer Error Types: Currently, some errors like BadGame(String) carry extra information (the game's name). We might want to expand this concept to other error types, encapsulating more context within the error itself, improving the ability to handle errors more accurately at runtime.

5. Consistent Error Kind Enumeration: The current error enum contains a mix of high-level and low-level errors (like PacketSend, SocketConnect, JsonParse). We might want to separate these into different error kinds, which can be nested, providing a more granular and consistent error categorization.

I welcome your thoughts, suggestions, and proposed solutions to refine the error-handling approach.

[CosminPerRam]

By the great work put in by @Douile, we now have the points 1, 2, 3, 4 but as mentioned by him, that PR didnt had the scope of solving the 5th point mentioned here.

As the time of writing this comment, we have the following error kinds:

    /// The received packet was bigger than the buffer size.
    PacketOverflow,
    /// The received packet was shorter than the expected one.
    PacketUnderflow,
    /// The received packet is badly formatted.
    PacketBad,
    /// Couldn't send the packet.
    PacketSend,
    /// Couldn't send the receive.
    PacketReceive,
    /// Couldn't decompress data.
    Decompress,
    /// Couldn't create a socket connection.
    SocketConnect,
    /// Couldn't bind a socket.
    SocketBind,
    /// Invalid input.
    InvalidInput,
    /// The server queried is not the queried game server.
    BadGame,
    /// Couldn't automatically query.
    AutoQuery,
    /// A protocol-defined expected format was not met.
    ProtocolFormat,
    /// Couldn't cast a value to an enum.
    UnknownEnumCast,
    /// Couldn't parse a json string.
    JsonParse,
    /// Couldn't parse a value.
    TypeParse,

So, the 5th points want to suggest to group these in subgroups or to have less of them and more generalized ones?

Subgrouping them would look like this (very crude example):

pub enum PacketError {
    /// The received packet was bigger than the buffer size.
    Overflow,
    /// The received packet was shorter than the expected one.
    Underflow,
    /// The received packet is badly formatted.
    tBad,
    /// Couldn't send the packet.
    Send,
    /// Couldn't send the receive.
    Receive,
}

pub enum SocketError {
    /// Couldn't create a socket connection.
    Connect,
    /// Couldn't bind a socket.
    Bind,
}

pub enum ParseError {
    /// Couldn't decompress data.
    Decompress,
    /// Couldn't cast a value to an enum.
    UnknownEnumCast,
    /// Couldn't parse a json string.
    JsonParse,
    /// Couldn't parse a value.
    TypeParse,
    /// The server queried is not the queried game server.
    BadGame,
    /// A protocol-defined expected format was not met.
    ProtocolFormat,
}

enum InputError {
    /// Invalid input.
    InvalidInput,
    /// Couldn't automatically query.
    AutoQuery,
}

/// GameDig Error.
pub enum GDErrorKind {
    Packet(PacketError),
    Socket(SocketError),
    Parse(ParseError),
    Input(InputError),
}

Which then would make the usage a bit weird (or is it?) to specify the group and then the error (Err(GDErrorKind::Packet(PacketError::Overflow)). This made me think that having generalized errors (lets say only the ones from the GDErrorKind mentioned about), but that too doesn't look very good to me as we could be missing on some finer details.

Any opinions on how to do to properly?

[Douile]

I'm not sure if 4 was covered either, we now support inclusion of extra context in the form of anything that implements std::error::Error, but that is mainly error messages for errors entirely from the library e.g. buffer. It might be nice to have the information stored in a machine accessible way for example the buffer errors could include extra info about the buffers state. The nightly provides API might be useful for this specific point when it hits.

[Douile]

Which then would make the usage a bit weird (or is it?) to specify the group and then the error (Err(GDErrorKind::Packet(PacketError::Overflow)).

By implementing From on each of the subgroups for ErrorKind we could just return the inner most generic (when there is an implicit into)

Could also have something like this for context

impl<T: Into<GDErrorKind>> WithContext for T {
  fn context<E: ...>(self, source: E) -> GDError {
    self.into().context(source)
  }
}

So that context could be used on the innermost error type that can be converted to an error kind.

---

A completely opposite solution would be to keep the errors kinds in one big enum like we have now but statically assign each one a category that can be accessed. Which would give us the richer information but maintain the ease of accessing error kinds.

[CosminPerRam]

I forgot for a second that From and Into exists, with them it would look great to me.

A completely opposite solution would be to keep the errors kinds in one big enum like we have now but statically assign each one a category that can be accessed. Which would give us the richer information but maintain the ease of accessing error kinds.

Can you bring up a crude example for this please? I don't fully understand the usage.

@cainthebest any opinions about grouping them together or doing what @Douile suggested?

[Douile]

Can you bring up a crude example for this please

pub enum GDErrorCategory {
  Packet,
  Socket,
  // ...
}

impl GDErrorKind {
  fn category(&self) -> GDErrrorCategory {
    match self {
      PacketBad | PacketOverflow => GDErrorCategory::Packet,
      _ => todo!(),
    }
  }
}

[Douile]

Example of the enum groups with generics, each type just has to implement Into<GDErrorKind>: https://github.com/Douile/rust-gamedig/commit/873f5f3b771aa34ded0d52945663fa5a2854d70c#diff-6ff19a1ff611f3835b5a025663cf4dec901ed914626f78847414836439bd1a79

[CosminPerRam]

Oh, that actually looks really clean, guess it's going to get done that way. Although, the grouping of errors (and maybe renames? wouldn't PacketError::Bad be better than PacketError::PacketBad?) still have to be decided before starting rewriting stuff.

[cainthebest]

I've had a look through the current discussion about refining the error-handling approach and heres are my thoughts:

Subgrouping the Errors: The subgrouping idea proposed does bring clarity by logically grouping related error kinds. As @Douile pointed out, leveraging the From and Into traits can streamline error propagation and usage, making it more ergonomic. The approach Err(GDErrorKind::Packet(PacketError::Overflow)) is more verbose but offers clearer categorization of errors, which can aid in debugging and error handling at higher layers.

Assigning Static Categories: The alternative suggestion by @Douile to statically assign categories to each error kind while retaining the single GDErrorKind enum is intriguing. This would offer a middle ground, preserving ease of access while adding a layer of categorization. The snippet provided to get the category for a given error kind is a neat solution, giving users the ability to understand the broader category of an error if required.

Rich Context for Errors: As pointed out, while the library will support inclusion of additional context via anything implementing std::error::Error, having machine-accessible information might be beneficial. For instance, buffer errors could provide buffer state details, which could prove invaluable in debugging complex scenarios. If nightly provides an API that aligns with this goal, considering its adoption when it becomes stable could be beneficial.

Optimal Solution: Both solutions have merits. If we favor clarity and structured organization, subgrouping is the way to go. However, if we want to maintain the current simplicity while adding some form of categorization, the static assignment approach might be more apt.

[Douile]

I agree with you on the verbose errors, while streamlining means typing less it takes away a lot of the contextual information.

While the static categories would be good at runtime i think they fall short when writing code. For example if I'm writing code that returns errors my editor will complete based on the type info. So having grouped enums gives more fine-grained control of what will be auto completed. The static groups are invisible to someone while they are editing code.

I do quite like the simplicity of just prefixing the group name on the error as we have at the moment because:

• All the error kinds are in one place
• I can get editor completions based on either the group prefix (e.g. Packet) or the name
• It is very simple, adding a new group doesn't require writing more boilerplate

In terms of richer context, we can probably add that just as struct elements. The demands nightly api seems only really useful for those downcasting the struct to an Error but still wanting to access the data points.

The sub-groups could be useful in organising the extra context data, each group could have its own struct (if needed) that stores the extra contextual info. E.g.

enum GDErrorKind {
  Buffer(GDErrorBuffer),
  Packet(GDErrorPacket),
  // ...
}

struct GDErrorBuffer {
  kind: GDErrorBufferKind,
  offset: usize,
  buffer: Vec<u8>,
}

enum GDErrorPacket {
  Bad,
  Underflow,
  Overflow,
}

Although doing that makes it less of a "kind" type and more additional info.