Switch to thiserror or snafu

[Stebalien]

libipld currently uses anyhow. Unfortunately:

• According to the author, thiserror is a better fit for libraries. Anyhow makes it difficult to handle error cases.
• Anyhow doesn't have great nostd support as ? will no longer convert errors into anyhow::Error.
• Anyhow requires allocation, adding an allocation to all error paths. While libipld itself requires allocation at the moment, it would be nice to eventually drop this requirement (neither the Encode nor Decode trait require allocations).

[Stebalien]

Note: I believe @samuelburnham would be able/willing to implement this if approved.

[dvc94ch]

we already use thiserror https://github.com/ipfs-rust/libipld/blob/master/Cargo.toml#L23

an allocation on the error path is actually good for performance as it does less cache thrashing

For applications, I recommend using a trait object, rather than an enum, to manage your many kinds of errors. I believe that usually trying to enumerate all of the errors every library you use can throw is more trouble than its worth, and I still assert that for most applications this will lead to higher performance by keeping the happy path less pessimized. I recommend anyhow::Error as a strictly better trait object than Box<dyn Error + Send + Sync + 'static>, not to mention one that is much easier to deal with.

https://without.boats/blog/failure-to-fehler/

Anyhow makes it difficult to handle error cases.

I disagree, have you seen downcast_ref? works very well actually, also allows providing custom context.

quoting myself as people constantly want to argue with me about this :)

That is actually a myth in most cases. The way thiserror is usually abused people put all the possible errors that any code path in a crate could have in an enum and then return it from everywhere. The main argument for thiserror is handling all possible errors and have the compiler check that all cases are handled. Which is a bad argument for one because not all errors are possible in every code path and the wrapping and rewrapping of errors (each library has at least an Io variant) actually makes it harder, and most ways of handling errors involve one of retrying/logging/aborting. And there are a whole host of other reasons why that "rule of thumb" brainlessly applied yields worse results than just using anyhow. You need to capture your backtraces manually which absolutely no one does, it's slower, etc.

[dvc94ch]

errors that need to be handled or recovered from shouldn't really be errors. returning a custom enum or an option is much better in my opinion.

[samuelburnham]

Regarding no-std support, anyhow seems to work fine but requires using .map_err(anyhow::Error::msg)? instead of just ?, for example:

impl Encode<RawCodec> for [u8] {
  fn encode<W: Write>(&self, _: RawCodec, w: &mut W) -> Result<()> {
    #[cfg(feature = "std")] {
      Ok(w.write_all(self)?)
    }
    #[cfg(not(feature = "std"))] {
      Ok(w.write_all(self).map_err(anyhow::Error::msg)?)
    }
  }
}

@dvc94ch do you think this is would be too verbose or okay to add?

[Stebalien]

an allocation on the error path is actually good for performance as it does less cache thrashing

That's for applications with many kinds of errors. And I'm really skeptical about that performance assertion (unless you're dealing with large error types).

I disagree, have you seen downcast_ref? works very well actually, also allows providing custom context.

Sure, but it makes the error types undiscoverable. That's reasonable in application code, but painful in library code.

quoting myself as people constantly want to argue with me about this :)

I agree but that's not an argument against using it correctly. Importantly, "thiserror" allows building error hierarchies. Usually, one doesn't care about the specific error, but the general kind of error:

• Not found/missing. (although I guess this can be handled with a Result<Option<T>, Error>.
• Encoding/decoding:
  • Codec error: the block is bad.
  • Decoding error: the IPLD block has the wrong shape for the target type, but is otherwise correct.
• IO/internal error: there's a problem with your datastore/node.
• Name error:
  • Invalid CID
  • Insecure/forbidden CID

With anyhow, determining the general error kind requires downcasting to all possible underlying errors. This becomes a game of whack-a-mole as new error cases pop up over time.

[Stebalien]

Backing up a bit, I think my primary motivations are really:

1. Not imposing anyhow on downstream crates.
2. Better nostd support.

I don't have any concrete use-cases with respect to the rest of my objections.

[dvc94ch]

@samuelburnham

do you think this is would be too verbose or okay to add?

I guess if you put it in a function or macro it's not that bad

@Stebalien

Not imposing anyhow on downstream crates

well, if you return a Stream you impose the futures crate. I think anyhow like futures is used by so many crates it's fine to impose as a dependency.

[Stebalien]

Well, futures are slowly being upstreamed into the standard library. I'd have a similar stance if this library depended on a specific event loop.

In the case of error handling, we have (and have had):

• API Intrusive:
  • Failure
  • Anyhow
• Non Intrusive:
  • snafu
  • error-chain
  • thiserror

At this point, I don't expect anyhow to be the end-all-be-all of error handling. At a minimum, it can't work without allocations, which excludes a class of applications.

[dvc94ch]

well, with the global_allocator attribute every target can support heap allocations. and environments that require hard real time and can't deal with non determinism of heap allocations are probably very few. ideally rust would support generating an error enum per function. that would help the rewrapping issue and the unreachable variants. actually thinking about it, it would look quite similar to the Fehler api, really should give it a try at some point.

[vmx]

@Stebalien writes:

• Not imposing anyhow on downstream crates.

I'd like to talk about this from a usability (for maintainers of this library, as well as for users), rather then performance. There doesn't seem to be agreement about performance, but perhaps we can find it in regards to usability.

Currently anyhow is used as a wrapper around the existing errors. I don't see any usage of adding context and only a single ensure! in current code base. If my observation is correct, what would then be the difference of having a enum that contains all existing errors? How would it be different from an anyhow error?

So users of the library would then match instead of downcast. Library authors would add a new enum member in case there's a new error class. Would that be all the impact?

• Better nostd support.

@Stebalien: looking at @samuelburnham's branch at https://github.com/yatima-inc/libipld/tree/sb/core-io it looks like no-std support should work even if anyhow is used. Which I guess is the biggest blocker.

[dvc94ch]

One big difference is that no backtrace is captured. The rust ecosystem hasn't really noticed that there is such a thing because it only works on nightly. This is very unfortunate, but once the rust team manages to get it's act together and stabilize this feature all the error handling in 99% of crates will need to be fixed. It's extremely annoying when debugging someone elses code and I compile with nightly to get a backtrace but they used thiserror. So then I need to go printlning and modifying the code until I find where the error came from.

[Stebalien]

@Stebalien: looking at @samuelburnham's branch at https://github.com/yatima-inc/libipld/tree/sb/core-io it looks like no-std support should work even if anyhow is used. Which I guess is the biggest blocker.

It does, it's just a bit annoying (see all the cfgs).

---

One big difference is that no backtrace is captured.

That is a good point, but note that both snafu and thiserror support backtraces as well. But its definitely a more work and less magical.

---

Honestly, I don't feel super strongly about this at the moment. I'm happy to table the discussion till I have concrete data one way or the other.