Thoughts on error context in error-handling libraries

[dpc]

I decided to give snafu a chance, and quickly grew quite tired of how laborious specifying “error context” next to every single ? is.

I went back to some of my apps using failure, to see why it didn’t feel as bad, and I quickly realized, that it’s only because in failure context is optional… and I just don’t ever use it.

There’s something deeply redundant in how both of these libraries deal with context.

Let’s look at the failure documentation example

use failure::ResultExt;

let mut file = File::open(cargo_toml_path).context("Missing Cargo.toml")?;
file.read_to_end(&buffer).context("Could not read Cargo.toml")?;

It immediately strikes me how redundant handling of context here is: the message is almost the same in both .context calls. If what you’re doing requires N steps, you’ll have to write the same/similiar boring message that many times.

What is being missed here is that there’s just one context. cargo_toml_path is the core of that context. For all these "context"s not to be so laborious, the operations have to be grouped and code should have to look something like this (pseudo-Rust):

with_err_ctx!({ path: cargo_toml_path }, || {
  let mut file = File::open(cargo_toml_path)?;
  file.read_to_end(&buffer)?;
});

Moreover, when you think about where would this example code be, it most probably be in some fn read_cargo_manifest_data(path: &Path) -> Result<SomeData>. A function! A function in which I quite explicitly narrowed down the context with arguments and documentation. So why there’s no error library that uses all that information.

The way I would like things to be is something like (again: pay attention to bigger picture, not details):

/// Read cargo manifest data
///
/// Returns `SomeData` from the manifest
#[ctx("reading data from cargo manifest at {} in {} mode", path.display(), mode]
fn read_cargo_manifest_data(path: &Path, mode: SomeMode) -> Result<SomeData> {
  let mut file = File::open(path)?;
  file.read_to_end(&buffer)?;
}

And if something fails, I want the error to be something like:

Error: IO Error: File Not Found
when: opening cargo manifest at /home/dpc/someproject/Cargo.toml
when: reading metadata of crate `foobar`
when: updating graph of dependencies

It also seems to me that such contexts might be useful eg. for logging, and also serve as a form of documentation.

I don’t really know what to do with all that thoughts, so I wrote them down and share here. Feel free to comment and dicuss. Maybe someone with more time at hand, could protottype a library like this, or maintainers of existing error-managing crates, think if this could be implemented.

[josh]

I like the general idea you’ve outlined; it sounds promising to me. If that were combined with something a little less Box-happy than failure, I’d enjoy using it.

The other half of the picture: I’d love to have functions like File::open and file.read_to_end have the ability to provide additional context for their own errors. You’ve passed a path to File::open, it knows what it tried to open when it got the IO error for “File not found”. (OS errors being what they are, you can’t assume that File not found: /path/to/file is accurate, but you could at least get a when: opening file "/path/to/file" out of File::open.

That would require a fairly substantial overhaul of standard library error handling, though.

[dtolnay]

@yoshuawuyts’s context-attribute crate is in this direction.

[dpc]

Yeah. It’s really in this spirit. I like that it actually reuses the doc comment content. Really effortless. Though this additional formatting of arguments would be really useful for making detailed context messages… sometimes. It would probably have a cost in performance, so it might be up to a developer when better context messages are worth the allocation (?).

And it seems to me the imperative mode of docstrings does not fit that well into error context. But maybe it’s just a matter of formatting it differently.

I think extending context-attribute to default to function docstring title, but allow overwriting the message, including format_args!-like messages on function arguments, would pretty much be what I was thinking about.

I still think there’s more here to explore, with regards to other aspects of error managementthe function-annotation-centrism could help with the less laborious, but still chore-heavy aspect of maintaining list of error variants maybe.

[repax]

How about something like:

let buf = try {
  let mut file = File::open(path)?;
  file.read_to_end(&buffer)?
}.context("reading cargo manifest")?;

[dpc]

Definitely better than what I have sketched in my first post.

[dtolnay]

An inner attribute could be good to consider too:

#![feature(custom_inner_attributes)]

use context_attribute::am;
use failure::Fallible as Result;
use memmap::MmapOptions;
use serde::de::DeserializeOwned;
use std::fs::File;
use std::path::Path;

fn load<T: DeserializeOwned>(path: impl AsRef<Path>) -> Result<T> {
    #![am("reading config from {}", path.display())]

    let file = File::open(path.as_ref())?;
    let mmap = unsafe { MmapOptions::new().map(&file)? };
    let config = serde_json::from_slice(&mmap)?;
    Ok(config)
}

This would feel a lot like logging or println, but serves as the context for the block in case of error.

fn main() -> Result<()> {
    let x = {
        #![am("determining x from {}", ...)]
        let f = get_f()?;
        let g = get_g()?;
        f ⊗ g
    };

    if is_big(x) {
        #![am("analyzing big x: {}", x)]
        p(x)?;
        q(x)?;
    }

    Ok(())
}

[dhm]

@dpc

It would probably have a cost in performance, so it might be up to a developer when better context messages are worth the allocation (?).

I would imagine the Context thing being required to be Display + 'static, and then using something like lazy formatting for variables able to be 'static, else plain old format!.

[shepmaster]

@dpc

It immediately strikes me how redundant handling of context here is: the message is almost the same in both .context calls. If what you’re doing requires N steps, you’ll have to write the same/similiar boring message that many times.

What is being missed here is that there’s just one context. cargo_toml_path is the core of that context. For all these "context"s not to be so laborious, the operations have to be grouped

@repax showed the future with the try block, but you can use an IIFE for the same purposes today, although what rustfmt creates is less-than-ideal:

use snafu::{ResultExt, Snafu};
use std::{
    fs::File,
    io::{self, Read},
    path::{Path, PathBuf},
};

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("Could not read Cargo.toml from {}", path.display()))]
    CargoToml { source: io::Error, path: PathBuf },
}

fn example(cargo_toml_path: impl AsRef<Path>) -> Result<(), Error> {
    let cargo_toml_path = cargo_toml_path.as_ref();

    let mut buffer = Vec::new();

    (|| {
        let mut file = File::open(cargo_toml_path)?;
        file.read_to_end(&mut buffer)
    })()
    .context(CargoToml {
        path: cargo_toml_path,
    })?;

    println!("Read {} bytes", buffer.len());
    Ok(())
}

fn main() {
    let d = example("/etc/passwd");
    println!("{:?}", d);
}

However, I don't think that you need this all the time because we already have grouping mechanisms: functions.

fn example(cargo_toml_path: impl AsRef<Path>) -> Result<(), Error> {
    let cargo_toml_path = cargo_toml_path.as_ref();

    let buffer = fs::read(cargo_toml_path).context(CargoToml {
        path: cargo_toml_path,
    })?;

    println!("Read {} bytes", buffer.len());
    Ok(())
}

I believe you should absolutely be creating new functions to reflect your encapsulation boundaries. If your code doesn't care about the difference of an open failing and a read failing, then group them together.

[shepmaster]

@dpc

#[ctx("reading data from cargo manifest at {} in {} mode", path.display(), mode] 
fn read_cargo_manifest_data(path: &Path, mode: SomeMode) -> Result<SomeData> {

By my superficial knowledge of macros, there's no way to "collect" multiple attributes across a code base and then unify them. That is, there's no way to take

#[ctx(A)]
fn alpha() {}

#[ctx(B)]
fn beta() {}

and produce

enum Error {
    A,
    B,
}

If there is, I'd be excited to learn of it. This is where I sit back and wait for @dtolnay to work magic.

[dpc]

@shepmaster

@repax showed the future with the try block, but you can use an IIFE for the same purposes today, although what rustfmt creates is less-than-ideal:

try is great for ad-hoc grouping, but not great to span across a whole function.

@shepmaster

However, I don’t think that you need this all the time because we already have grouping mechanisms: functions.

I don't understand your example. It has only one failable operation. My point is a typical function "processing data from a file under path" is multiple steps - all with the same context path. I would like that .context on the whole function body. Even with try keyword implemented, I don't want to wrap the whole body into try manually.

BTW. I was thinking about it, and I don't really find "structured context" (vs formatted string) that useful. I have a hard time coming with common use-cases where in this case:

.context(CargoToml {
        path: cargo_toml_path,
    })

any caller would be able to do anything useful with that path in a returned error. First of all - most probably the calling code already has that path because it was the one passing it as an argument in the first place. And if it wasn't - then it would be an implementation detail that we probably wouldn't want to expose as a part of the API.

So most of the time it's actually just human-readable string that is important for debugging/logging and diagnostic messages.

[CAD97]

Structural context is best paired with structural logging such that the logs retain the structure, and you can do things like query your logs for “all unique paths mentioned in an error” from your production server to see if they have any commonalities.

When it’s just displayed as a string panic to the developer, it doesn’t matter when it’s stringified (though delaying the cost so it can be dismissed is often (not always) ideal).