Simple API should return finer typed error kind

[DarkFenX]

This is a follow-up from discussion from #168, and issue #169.

I suggest to redo simple API error type from boxed dyn error to an enum with a few variants:

• reader error
• deserializer error
• <any other struson-specific error variants>
• misc/custom (with boxed dyn error)

If #169 and this is implemented, initial code I posted to #169 could become super short:

#[derive(thiserror::Error, Debug)]
pub(in crate::phb) enum PhbParseError {
    #[error("reading failed: {0}")]
    ReadFailed(String),
    #[error("parsing failed: {0}")]
    ParseFailed(String),
}
impl From<struson::reader::HybridReaderError> for PhbParseError {
    fn from(error: struson::reader::HybridReaderError) -> Self {
        match error.is_io() {
            true => Self::ReadFailed(error.to_string()),
            false => Self::ParseFailed(error.to_string()),
        }
    }
}

[Marcono1234]

Thanks for the suggestion!

I tried to prototype this a bit, but wasn't very successful so far. It seems there are two main issues. Let's assume the new error type looks similar to this:

pub enum SimpleReaderError {
    ReaderError(ReaderError),
    DeserializerError(DeserializerError),
    Custom(Box<dyn Error>),
}

• Cannot impl Error and From<Error> at the same time
  Ideally this error type would impl Error, so that it is actually usable as error like users would expect, and From<Error>, so that users can easily use ? to convert an arbitrary error to SimpleReaderError.
  However these two implementations conflict:

  note: conflicting implementation in crate `core`:
    - impl<T> From<T> for T;`
  

  It seems there are ways to work around this, similar to how anyhow does it. But I am not sure if that can be implemented in safe Rust only (anyhow has a few unsafe blocks for this).
  Or maybe it can be implemented in safe Rust, but it would then have to delegate to some 'inner' value which implements Error. But because the error enum and its variants are public, the only choice might be to directly delegate to the wrapped errors (e.g. ReaderError). Not sure if that is a good approach.

• Converting error to most specific SimpleReaderError variant
  Ideally ReaderError would be wrapped into SimpleReaderError::ReaderError, and only custom / unknown errors would become SimpleReaderError::Custom. However, without Rust's 'specialization' being stable yet it seems this is not directly possible? When there is From<Error>, there cannot be From<ReaderError>.
  Maybe this can be worked around with a From<Error> doing a downcast to check if the error type is actually ReaderError. Not sure though if there are any drawbacks with this.
  Another alternative would be to just not have From<ReaderError> and instead manually wrap ReaderError in SimpleReaderError::ReaderError. But then all simple reader methods would have to return SimpleReaderError, even those which currently return ReaderError (such as read_bool), so that the user cannot encounter ReaderError directly and accidentally wrap it into Custom when using ?. That approach is not really ideal either, then users have to deal with error variants which cannot actually occur for that specific method.

---

To make it easier for users to obtain their custom errors, I also tried to do enum SimpleReaderError<E> { Custom(E) } (possibly defaulting / hinting E = Box<dyn Error>), but wasn't really successful with that either so far. For example it had issues inferring E when no custom error was actually returned.
Though such a SimpleReaderError<E> would only be nice to have, the enum SimpleReaderError { Custom(Box<dyn Error>) } would also be fine for now.

[Marcono1234]

To clarify, I still like the suggestion here, but it seems to be difficult to implement.
However, maybe I am just missing knowledge in that area of Rust.

If you have any ideas (or even proof of concepts) please let me know! Though no obligations.

For now the workaround is probably to use the 'advanced API' of Struson, as you mentioned in one of your other issues. That makes handling errors easier.

[DarkFenX]

Not gonna lie, at this point I do not care much about simple API. I switched to low-level one just not to deal with custom error kind. Resulted in simpler code (higher complexity when dealing with errors went to lower complexity when dealing with reader). So, no ideas from my side, consider this as a note I left when giving simple API a try.

[Marcono1234]

Fair point, and thanks for the honest feedback!

The Simple API was mostly created because I got feedback that the panics on incorrect usage were unexpected. So the Simple API prevents (nearly) all these panics, but unfortunately that makes some aspects of it more cumbersome to use.

I hope once users are a bit familiar with how Struson's Advanced API works, its usage becomes intuitive and risk of incorrect usage and panics becomes lower.

But your feedback regarding the Simple API is still useful, so I will keep this issue here open.