Error-Handling Anti-Patterns

Every ERR rule the audits report, with the reason, a before/after fix, and when the pattern is acceptable. After that, smells the scripts cannot see. Examples use thiserror 2 and anyhow 1.

#Contents

• ERR001-ERR003: panics in library code
• ERR004: Box<dyn Error> without Send + Sync
• ERR005: anyhow in a library API
• ERR006: map_err(|_| ..)
• ERR007: stringly map_err
• ERR008: Result<T, String>
• ERR009: source printed twice
• ERR010: exhaustive public error enums
• ERR011: discarded results
• ERR012: ? without context
• ERR013: process::exit instead of returning
• Smells the scripts cannot see

#ERR001-ERR003: panics in library code

A library cannot know whether its caller can recover; panicking takes that decision away and loses the error value.

// Before
pub fn port(cfg: &HashMap<String, String>) -> u16 {
    cfg.get("port").unwrap().parse().expect("port must be a number")
}

// After
#[derive(Debug, thiserror::Error)]
#[non_exhaustive]
pub enum ConfigError {
    #[error("missing key {0:?}")]
    Missing(&'static str),
    #[error("invalid value {value:?} for {key}")]
    Invalid { key: &'static str, value: String, #[source] source: std::num::ParseIntError },
}

pub fn port(cfg: &HashMap<String, String>) -> Result<u16, ConfigError> {
    let value = cfg.get("port").ok_or(ConfigError::Missing("port"))?;
    value.parse().map_err(|source| ConfigError::Invalid { key: "port", value: value.clone(), source })
}

Acceptable: expect for genuine invariants, phrased as the invariant (.expect("regex literal is valid"), .expect("index < len checked above")); unwrap in tests, examples, build scripts and main of small tools. Document deliberate panics under a # Panics heading. todo!/unimplemented! should not ship.

#ERR004: Box<dyn Error> without Send + Sync

Box<dyn Error> cannot be sent to another thread or task, cannot be returned from a tokio::spawned future, and does not convert into anyhow::Error (which requires Send + Sync + 'static).

// Before: pub fn load(p: &str) -> Result<String, Box<dyn std::error::Error>>
pub type BoxError = Box<dyn std::error::Error + Send + Sync + 'static>;

pub fn load(p: &str) -> Result<String, BoxError> {
    Ok(std::fs::read_to_string(p)?)
}

Acceptable: fn main() -> Result<(), Box<dyn Error>> in a small binary (the audits exempt main).

#ERR005: anyhow in a library API

Callers of a library need to react to failures (retry on timeout, 404 on not-found). With anyhow::Error they must downcast to guess what happened, and every new internal error becomes invisible API.

// Before
pub fn fetch(id: u64) -> anyhow::Result<Item> { .. }

// After: typed at the boundary, anyhow stays inside the binary
#[derive(Debug, thiserror::Error)]
#[non_exhaustive]
pub enum FetchError {
    #[error("item {0} not found")]
    NotFound(u64),
    #[error("request timed out")]
    Timeout,
    #[error("transport error")]
    Transport(#[source] std::io::Error),
}
pub fn fetch(id: u64) -> Result<Item, FetchError> { .. }

Acceptable: internal (pub(crate)) functions, workspace-internal crates owned by one binary, CLI-only crates.

#ERR006: map_err(|_| ..)

The original error is thrown away: no source(), no cause in logs.

// Before
let n: usize = raw.parse().map_err(|_| StoreError::BadCount(raw.to_owned()))?;

// After: keep the cause
#[error("invalid count {input:?}")]
BadCount { input: String, #[source] source: std::num::ParseIntError },

let n: usize = raw.parse().map_err(|source| StoreError::BadCount { input: raw.to_owned(), source })?;

Acceptable: when the source genuinely adds nothing and is huge or sensitive (rare); then say so in a comment.

#ERR007: stringly map_err

e.to_string() or format!("..: {e}") keeps the message but loses the type, the chain and any backtrace.

// Before
fs::write(dest, json).map_err(|e| format!("write failed: {e}"))?;

// After (application code)
fs::write(&dest, json).with_context(|| format!("writing snapshot to {}", dest.display()))?;

// After (library code)
fs::write(&dest, json).map_err(|source| StoreError::Write { path: dest.to_owned(), source })?;

#ERR008: Result<T, String>

String errors cannot be matched, do not implement std::error::Error (no ? into anyhow, no source), and push formatting into every call site.

• Library: a thiserror enum (generate one with scaffold_errors).
• Application: anyhow::Result<T> with bail!("..")/.context("..").
• Result<T, ()>: use Option<T> if there is only one failure, or a unit-variant enum.

#ERR009: source printed twice

thiserror's Display comes from #[error(..)]; source() comes from the #[from]/#[source]/source field. Reporters (anyhow's {:#}, {:?}, tracing with std::error::Error chains, eyre) print each layer's Display and then walk source(), so a message that interpolates its own source prints it twice:

Error: i/o error: No such file or directory (os error 2)

Caused by:
    No such file or directory (os error 2)

// Before
#[error("i/o error: {0}")]
Io(#[from] std::io::Error),

// After: describe this layer only
#[error("i/o error on the store log")]
Io(#[from] std::io::Error),

Use #[error(transparent)] when the wrapper adds nothing and should forward both Display and source().

#ERR010: exhaustive public error enums

Adding a variant to an exhaustive public enum breaks every downstream match (a major release). #[non_exhaustive] lets you add variants in minor releases; callers write a _ => arm. Inside the defining crate, matches stay exhaustive.

#ERR011: discarded results

let _ = f(); and f().ok(); silence the #[must_use] warning without handling the failure.

// Before
let _ = self.compact();

// After: decide explicitly
if let Err(e) = self.compact() {
    tracing::warn!(error = %e, "compaction failed; continuing with an uncompacted log");
}

Acceptable: best-effort cleanup where failure is expected and harmless (let _ = fs::remove_file(tmp); in a Drop), ideally with a comment. Note let _ = x; drops x immediately, whereas let _guard = x; keeps it.

#ERR012: ? without context

? on File::open produces No such file or directory (os error 2) with no path. In application code, add what was being attempted:

let text = fs::read_to_string(&path)
    .with_context(|| format!("reading config {}", path.display()))?;
let cfg: Config = serde_json::from_str(&text)
    .with_context(|| format!("parsing config {}", path.display()))?;

Use .context("static message") when nothing needs formatting; with_context avoids building the string on success. In libraries, put the path in a variant field instead.

#ERR013: process::exit instead of returning

std::process::exit does not run destructors (buffered writers are not flushed, temp files stay), cannot be tested, and makes the function unusable as a library.

// Before
fn load_or_exit(path: &str) -> Store {
    match Store::open(path) {
        Ok(s) => s,
        Err(e) => { eprintln!("cannot open store: {e}"); std::process::exit(1) }
    }
}

// After
fn main() -> anyhow::Result<()> {
    let store = Store::open(path).context("opening store")?; // main prints the chain, exits with 1
    run(store)
}

For custom exit codes return std::process::ExitCode from main (see anyhow-context-reporting.md).

#Smells the scripts cannot see