S09: Error context lost (map_err(|_| ...))

S09Severity: Medium

The original error is replaced, so the cause is no longer available.

Clippy: map_err_ignore

What it looks like

#[derive(Debug)]
pub enum ImportError {
    CannotRead,
    BadId,
}

pub fn read_order_ids(path: &Path) -> Result<Vec<u64>, ImportError> {
    let text = std::fs::read_to_string(path).map_err(|_| ImportError::CannotRead)?;
    text.lines()
        .map(|line| line.trim().parse().map_err(|_| ImportError::BadId))
        .collect()
}

When the file does not exist, the error message is “cannot read file”. Which file? Not found, or no permission? When one ID is bad, the message is “bad order id”. Which line? What text?

Why AI agents produce it

It is the shortest code that compiles. The ? needs the error to have the function’s error type, and map_err(|_| ImportError::CannotRead) is the quickest way to get it.
The variant has no field for the cause. The agent first wrote the error enum with unit variants. To keep the cause, it must change the enum too.
It looks like Java code that the agent saw often: catch (IOException e) { throw new ImportException("cannot read file"); }. That code is also a smell in Java. The fix there is new ImportException("...", e).

Why it is a problem

The cause is gone. An io::Error says “not found”, “permission denied” or “is a directory”. ParseIntError says “invalid digit” or “number too large”. After map_err(|_| ...), none of this is available to the caller or to the log.
The details are gone. The error does not say which file or which line. Someone must reproduce the problem to find out.
Callers cannot react to the cause. A caller that wants to create a missing file, but report a permission error, cannot tell the two apart.

The better version

WorseUnit variants, and map_err(|_| ...): no file name, no line, no cause.

#[derive(Debug)]
pub enum ImportError {
    CannotRead,
    BadId,
}

pub fn read_order_ids(path: &Path) -> Result<Vec<u64>, ImportError> {
    let text = std::fs::read_to_string(path).map_err(|_| ImportError::CannotRead)?;
    text.lines()
        .map(|line| line.trim().parse().map_err(|_| ImportError::BadId))
        .collect()
}

BetterEach variant has the details (path, line, text) and the original error as source.

#[derive(Debug)]
pub enum ImportError {
    CannotRead {
        path: PathBuf,
        source: std::io::Error,
    },
    BadId {
        line: usize,
        text: String,
        source: ParseIntError,
    },
}

pub fn read_order_ids(path: &Path) -> Result<Vec<u64>, ImportError> {
    let text = std::fs::read_to_string(path).map_err(|source| ImportError::CannotRead {
        path: path.to_path_buf(),
        source,
    })?;
    text.lines()
        .enumerate()
        .map(|(index, line)| {
            line.trim().parse().map_err(|source| ImportError::BadId {
                line: index + 1,
                text: line.to_string(),
                source,
            })
        })
        .collect()
}

The message says what and where, and source() gives the cause:

impl fmt::Display for ImportError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            Self::CannotRead { path, .. } => write!(f, "cannot read {}", path.display()),
            Self::BadId { line, text, .. } => write!(f, "line {line}: bad order id {text:?}"),
        }
    }
}

impl std::error::Error for ImportError {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self {
            Self::CannotRead { source, .. } => Some(source),
            Self::BadId { source, .. } => Some(source),
        }
    }
}

With thiserror, the same is a #[source] attribute on the field. With anyhow, use .with_context(|| format!("cannot read {}", path.display())): it keeps the original error as the cause. See Result, not exceptions.

When it is acceptable

When the original error has no useful information, and the new error says everything. Example: "7".parse::<u64>() on a value that you already checked contains only digits.
When the caller only needs “yes or no”, and there is one cause. Converting to Option with .ok() is then honest:

/// The caller only needs "is this a number or not". There is one cause,
/// and the `None` says it.
pub fn parse_optional_id(field: &str) -> Option<u64> {
    field.trim().parse().ok()
}

At a security boundary, where the details must not go to the client (for example a login error). Then log the original error first, and return the generic one.

How to detect it

Search for map_err(|_|. Also search for .ok()? and .ok().ok_or( on a Result, which throw the error away in the same way.
Clippy map_err_ignore (restriction group, off by default) reports map_err(|_| ...).
Look at error enums. Unit variants (CannotRead, BadId) with no fields usually mean that the cause is thrown away somewhere.
In a review, read the error messages in the tests. If a message cannot answer “which file?” or “which line?”, ask for the context.

What to tell your agent

In read_order_ids, keep the original errors. Change ImportError so that
CannotRead has the path and the io::Error, and BadId has the line number,
the bad text and the ParseIntError. Return the original error from
Error::source(). Do not use map_err(|_| ...) anywhere in this module.