Use anyhow for error handling

[dbrgn]

Use anyhow instead of custom error types and strings for error handling. This should give us "caused by" style traces when an error occurs.

Before:

$ tldr --update
Error: Could not update cache: Could not remove cache directory (/home/danilo/.cache/tealdeer/tldr-pages): Permission denied (os error 13)

And after:

$ cargo run -- --update
Error: Could not update cache

Caused by:
    0: Could not clear the cache directory
    1: Could not remove the cache directory at /home/danilo/.cache/tealdeer/tldr-pages
    2: Permission denied (os error 13)

PR summary:

116 additions and 195 deletions

We're doing something right 😉

[dbrgn]

Note: For debugging of errors, if messages aren't entirely obvious, we could also provide a debug build of tealdeer with features=["backtrace"] enabled for anyhow. Then you get an error backtrace if RUST_BACKTRACE or RUST_LIB_BACKTRACE is enabled.

$ RUST_BACKTRACE=1 cargo run -- --update
Error: Could not update cache

Caused by:
    0: Could not clear the cache directory
    1: Could not remove the cache directory at /home/danilo/.cache/tealdeer/tldr-pages
    2: Permission denied (os error 13)

Stack backtrace:
   0: anyhow::context::<impl anyhow::Context<T,E> for core::result::Result<T,E>>::with_context::{{closure}}
             at /home/danilo/.cargo/registry/src/github.com-1ecc6299db9ec823/anyhow-1.0.52/src/context.rs:58:30
   1: core::result::Result<T,E>::map_err
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/result.rs:852:27
   2: anyhow::context::<impl anyhow::Context<T,E> for core::result::Result<T,E>>::with_context
             at /home/danilo/.cargo/registry/src/github.com-1ecc6299db9ec823/anyhow-1.0.52/src/context.rs:58:9
   3: tldr::cache::Cache::clear
             at ./src/cache.rs:359:21
   4: tldr::cache::Cache::update
             at ./src/cache.rs:188:9
   5: tldr::update_cache
             at ./src/main.rs:222:5
   6: tldr::main
             at ./src/main.rs:454:9
   7: core::ops::function::FnOnce::call_once
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/ops/function.rs:227:5
   8: std::sys_common::backtrace::__rust_begin_short_backtrace
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/sys_common/backtrace.rs:123:18
   9: std::rt::lang_start::{{closure}}
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/rt.rs:146:18
  10: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/ops/function.rs:259:13
      std::panicking::try::do_call
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panicking.rs:403:40
      std::panicking::try
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panicking.rs:367:19
      std::panic::catch_unwind
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panic.rs:133:14
      std::rt::lang_start_internal::{{closure}}
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/rt.rs:128:48
      std::panicking::try::do_call
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panicking.rs:403:40
      std::panicking::try
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panicking.rs:367:19
      std::panic::catch_unwind
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panic.rs:133:14
      std::rt::lang_start_internal
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/rt.rs:128:20
  11: std::rt::lang_start
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/rt.rs:145:17
  12: main
  13: __libc_start_main
  14: _start

[niklasmohrin]

Nice! Some things I found:

[niklasmohrin]

I found a couple of small talking points :)

Honestly, it feels so much better to read the word context when, well, adding context to an error instead of having this ugly map_err. I really hope benchmarks don't bear any surprises :^)

[niklasmohrin]

I found a couple of small talking points :)

Honestly, it feels so much better to read the word context when, well, adding context to an error instead of having this ugly map_err. I really hope benchmarks don't bear any surprises :^)

[niklasmohrin]

It might also be worth it to inspect all uses of unwrap etc. and check whether it is now more reasonable to propagate as an anyhow Erorr instead (maybe in another issue / PR though)

[dbrgn]

It might also be worth it to inspect all uses of unwrap etc. and check whether it is now more reasonable to propagate as an anyhow Erorr instead (maybe in another issue / PR though)

I didn't really find problematic unwraps outside of tests, except for that unzip issue that popped up in that other issue. However, let's fix that separately.

[niklasmohrin]

everything looks good to me, I am building the benchmark docker right now!

[dbrgn]

I am building the benchmark docker right now!

Let me know if it makes a difference! The only place that could make a difference in my eyes is the replacement of the unwrap in the inner printing loop with a .context(...)?. However, we're not exactly dealing with millions of loop iterations, so I don't think it'll make a significant difference (especially considering that we're dealing with I/O).

[niklasmohrin]

Okay so this took longer than I hoped it would. In the docker image, everything is within measurement error. On my machine, the anyhow version is anywhere from 0.2 to 0.7 ms slower than main (around 8 to 9 ms usually) on a cold disk cache. Without clearing caches, both are too fast to measure / too fast to factor out system noise. I am willing to take the performance "hit", I really like anyhow, lgtm