Something like ContextCompat but for options only

[imbolc]

I like the idea of differentiating between options from errors WrapErr intent to bring. But I still like simplicity of Option.context(..)?. So I'd prefer to use .wrap_err on errors and .context on options. Actually after reading the docs I thought ContextCompat works exactly this way, implementing .context for options only. But it happens to bring full compatibility with anyhow.Context, so the compiler won't guide me through changing .context to .wrap_err for errors. Would you consider adding something like OptionContext which would add .context for options only?

To summarize:

• WrapErr brings .wrap_err for errors only, but no .context
• OptionContext brings .context for options only

What do you think?

[imbolc]

I've just noticed ContextCompat lets me use even .wrap_err on options instead of .context, why?

[yaahc]

Would you consider adding something like OptionContext which would add .context for options only?

I think it should be possible to write this trait impl outside of color-eyre, so I lean towards not adding it since it seems like a style preference rather than something I'd want to encourage generally. My preference is to use ok_or for converting options to results, and I feel adding an OptionContext would encourage ppl away from that common style.

I've just noticed ContextCompat lets me use even .wrap_err on options instead of .context, why?

I don't think there was a reason, if I had thought about it at the time leaving out wrap_err seems like it would have been a good choice, but now it's a breaking change :(

[aldanor]

Noticed the same thing as well.. +1 for .context consistency for options.

/* as for style preference, I wish bool also had .context / .with_context implemented, so you can write concise checks without macros or nesting, just relying on ? for control flow... */

[yaahc]

/* as for style preference, I wish bool also had .context / .with_context implemented, so you can write concise checks without macros or nesting, just relying on ? for control flow... */

I think the libs team is actively planning on adding methods for converting bools into Options though this would require a few extra function calls so it may not be what you want.

Here:

use std::fmt;

trait Context: Sized {
    type Return;

    fn context<D>(self, msg: D) -> Self::Return
    where
        D: fmt::Display + fmt::Debug + Send + Sync + 'static,
    {
        self.with_context(|| msg)
    }

    fn with_context<F, D>(self, op: F) -> Self::Return
    where
        D: fmt::Display + fmt::Debug + Send + Sync + 'static,
        F: FnOnce() -> D;
}

impl Context for bool {
    type Return = Result<(), eyre::Report>;

    fn with_context<F, D>(self, op: F) -> Self::Return
    where
        D: fmt::Display + fmt::Debug + Send + Sync + 'static,
        F: FnOnce() -> D,
    {
        if self {
            Ok(())
        } else {
            Err(eyre::eyre!(op()))
        }
    }
}

impl<T> Context for Option<T> {
    type Return = Result<T, eyre::Report>;

    fn with_context<F, D>(self, op: F) -> Self::Return
    where
        D: fmt::Display + fmt::Debug + Send + Sync + 'static,
        F: FnOnce() -> D,
    {
        self.ok_or_else(|| eyre::eyre!(op()))
    }
}

fn main() {
    let res: Result<(), eyre::Report> = false.context("hi");
    let report = res.unwrap_err();
    eprintln!("Error: {:?}", report);

    let res: Result<(), eyre::Report> = None.context("hi");
    let report = res.unwrap_err();
    eprintln!("Error: {:?}", report);
}

[aldanor]

I think the libs team is actively planning on adding methods for converting bools into Options though this would require a few extra function calls so it may not be what you want.

Yes, there's currently an ongoing bikeshed in rust repo, looks like it'll take another year... and it will be another call.

The example you've provided is pretty much what I suggested, it's just that bool and Option are such common at generating errors, especially in application-like code, that it might be nice if a library supported converting them to errors out of the box without you having to write the same generic boilerplate in the downstream... I would even go as far as saying that most Results are generated either (1) manually, i.e. constructing them directly, (2) propagated from somewhere else, with errors possibly converted, (3) from options being None, (4) from some conditions failing to be true.

I really enjoyed the fact that anyhow provided context on Option since it allows you to do things like

let x = y.checked_div(z).context("division error")?;

Why? Because there's no macros and no extra function calls. The resulting syntax is as simple as possible, especially when you have tons of these.

With booleans, the idea is the same, is when you have many condition checks each of which may generate an error, you could simply have:

(a < 1).context("too small")?;
(a > 100).context("too large")?;
(a % 2 = 0).context("too even")?;

(on the other hand, I have to admit that at least you can implement what's suggested above without overlapping with the existing WrapErr trait as long as you don't import ContextCompat; if WrapErr implemented some of the options above but not the other, it would be impossible due to name collision)

[yaahc]

I really enjoyed the fact that anyhow provided context on Option since it allows you to do things like

let x = y.checked_div(z).context("division error")?;

This goes back to one of the fundamental differences between how I like to mentally model errors and how anyhow / prior crates handle it. A big motivator for me writing eyre was to separate the ideas of "Errors" and "Context". To me error messages in the chain of errors are not context. This is why I renamed the trait from Context to WrapErr, to indicate more clearly that a new error is being introduced to the chain of errors, rather than new context being added to the Error type. wrap_err doesn't make sense in this context for Option or bool, because they have no pre-existing error to wrap. That's why I prefer to encourage users to utilize the builtin methods for converting Option to Result.

let x = y.checked_div(z).ok_or_else(|| eyre!("division error"))?;

Why? Because there's no macros and no extra function calls. The resulting syntax is as simple as possible, especially when you have tons of these.

I appreciate that this version is slightly more verbose, but the verbosity is not without value here. I feel it's important to differentiate between errors that are wrapped and those that are newly created when an option is None.

[aldanor]

@yaahc Apologies if not directly related (please let me know if I should post this to color-eyre instead), but I've tried the snippet you suggested above (also implemented it for Result<T, E> so it's all packed in one trait), and now testing how it works with color-eyre backtrace reporting - what happens is that:

• The actual location where the .with_context() was called is frame number 14, and there's three more non-hidden frames before it (the closure, .with_context() itself and .ok_or_else()). Now, this can be fixed with frame filters in HookBuilder, although it has to be done manually.
• The reported location is eyre/src/kind.rs:69? (that's Report::from_adhoc() that eyre!() calls) So it will pretty much never point at user code? It's interesting - the frame number 10 is the one at eyre/src/kind.rs actually, why is the location of eyre! attached to that?

Error:
   0: division error

Location:
   ~/.cargo/registry/src/.../eyre-0.6.2/src/kind.rs:69 <--------

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 10 frames hidden ⋮
  11: <core::option::Option<T> as context::Context>::with_context::{{closure}}::h6456ab3710539e03
      at ~/.cargo/registry/.../eyre-0.6.2/src/macros.rs:158
  12: core::option::Option<T>::ok_or_else::h0b53eff304633c44
      at ~/.rustup/toolchains/.../rust/src/libcore/option.rs:563
  13: <core::option::Option<T> as context::Context>::with_context::h8a4ca296296c13b2
      at src/context.rs:49
  14: test_context::h895334c0700b5494
      at src/context.rs:103
  15: ... (50 more frames)

Ideally, it should instead look like this:

Error:
   0: division error

Location:
   src/context.rs:103

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 13 frames hidden ⋮
  14: test_context::h895334c0700b5494
      at src/context.rs:103

Hence my initial thinking - if something like this was embedded into eyre itself, then color-eyre would probably know how to filter things out and present backtraces nicely?

Now, there's HookBuilder and you can manually customize it (it's a bit of a chore but.. w/e), but you'll have eyre/src/kind.rs:69 reported for all ad-hoc errors and never the userland code?

[aldanor]

Edit: the problem above is solved via

• marking context() and with_context() as #[track_caller]
• adding custom frame filter that filters them out
• not using .or_else(||) but rather a raw match
• marking some methods in eyre as #[track_caller] (Add #[track_caller] for adhoc errors #42)

Then you get Context that sort of behaves like it should be, backtrace-wise. (a bit of hassle, but w/e)

[yaahc]

Now, there's HookBuilder and you can manually customize it (it's a bit of a chore but.. w/e), but you'll have eyre/src/kind.rs:69 reported for all ad-hoc errors and never the userland code?

looks like you figured it out but yea, this is the second release in a row for eyre that has been only adding #[track_caller] to functions I missed the first time around. I wouldn't be surprised if this isn't the last time we find extra places to propagate callsite information.

As for the custom frame filters, I have for the most part been pretty conservative about which functions I add to the set of frame filters. Filtering out relevant frames would probably piss ppl off a lot more than having slightly noisier backtraces. That said, I think it might be a good idea to copy clippy on this one and setup a second group of filters that are easy to enable but off by default, something like default_frame_filter_nursery. Then we could drop or_else and ok_or_else in there, and whatever other frames you feel would be good defaults. I can probably also add a few items to the list from the frame filters we have setup in https://github.com/ZcashFoundation/zebra/blob/main/zebra-test/src/lib.rs#L37

[aldanor]

Yea - great idea re: filter nursery. I have a similar frame filter list (almost identical, in fact) to the one you've posted above, but with async-std stuff instead of tokio in it :) I think it's very likely most people will come up with something similar or identical, more or less, if configuring filters manually. Also, things like .or_else and .ok_or_else and probably a few more similar methods from core.

[yaahc]

I'm gonna go ahead and close this issue as resolved