S07: Result<T, String> or stringly-typed errors

S07Severity: Medium

The caller cannot match on the kind of error, only read a message.

Clippy: no lint finds this. Look for it in review.

What it looks like

pub fn parse_order(line: &str) -> Result<Order, String> {
    let fields: Vec<&str> = line.split(',').collect();
    if fields.len() != 3 {
        return Err(format!("expected 3 fields, got {}", fields.len()));
    }
    let id = fields[0].parse().map_err(|e| format!("bad id: {e}"))?;
    let total_cents = fields[2].parse().map_err(|e| format!("bad total: {e}"))?;
    Ok(Order::new(id, fields[1], total_cents))
}

And a caller that needs to treat one failure differently:

pub fn import_line(line: &str) -> Option<Order> {
    match parse_order(line) {
        Ok(order) => Some(order),
        Err(msg) if msg.starts_with("bad total") => None, // skip, fix later
        Err(msg) => panic!("{msg}"),
    }
}

Why AI agents produce it

It is the fastest way to make ? work. format!(...) makes a String, and every error converts to a String with .to_string(). No error type is necessary.
It looks like Java. throw new IllegalArgumentException("bad total: " + e) with a message string is common in Java code.

Why it is a problem

The caller cannot match on the error. In the example above, the caller tests msg.starts_with("bad total"). If someone changes the message to “invalid total”, the code silently changes from “skip this line” to “panic”.
The cause is lost. format!("bad id: {e}") keeps the text of the ParseIntError but not the error itself. Code that wants the cause, or a logger that prints the error chain, gets only a string.
It does not compose. String does not implement std::error::Error. Box<dyn Error> and anyhow can wrap it, but only as text, with no cause to follow.

The better version

Define an error enum with one variant for each failure that a caller can handle differently. Keep the cause inside the variant:

WorseErrors are String values. The caller can only parse the message.

pub fn parse_order(line: &str) -> Result<Order, String> {
    let fields: Vec<&str> = line.split(',').collect();
    if fields.len() != 3 {
        return Err(format!("expected 3 fields, got {}", fields.len()));
    }
    let id = fields[0].parse().map_err(|e| format!("bad id: {e}"))?;
    let total_cents = fields[2].parse().map_err(|e| format!("bad total: {e}"))?;
    Ok(Order::new(id, fields[1], total_cents))
}

BetterAn enum: the caller matches on the variant, and the cause is kept.

#[derive(Debug, PartialEq)]
pub enum ParseOrderError {
    FieldCount(usize),
    BadId(ParseIntError),
    BadTotal(ParseIntError),
}

pub fn parse_order(line: &str) -> Result<Order, ParseOrderError> {
    let fields: Vec<&str> = line.split(',').collect();
    let [id, customer, total] = fields[..] else {
        return Err(ParseOrderError::FieldCount(fields.len()));
    };
    let id = id.parse().map_err(ParseOrderError::BadId)?;
    let total_cents = total.parse().map_err(ParseOrderError::BadTotal)?;
    Ok(Order::new(id, customer, total_cents))
}

The caller now matches on a variant, and the compiler checks it:

pub fn import_line(line: &str) -> Result<Option<Order>, ParseOrderError> {
    match parse_order(line) {
        Ok(order) => Ok(Some(order)),
        Err(ParseOrderError::BadTotal(_)) => Ok(None), // skip, fix later
        Err(other) => Err(other),
    }
}

The message text and the cause go in the Display and Error implementations:

impl fmt::Display for ParseOrderError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            Self::FieldCount(n) => write!(f, "expected 3 fields, got {n}"),
            Self::BadId(_) => f.write_str("order id is not a number"),
            Self::BadTotal(_) => f.write_str("total is not a number of cents"),
        }
    }
}

impl std::error::Error for ParseOrderError {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self {
            Self::BadId(e) | Self::BadTotal(e) => Some(e),
            Self::FieldCount(_) => None,
        }
    }
}

When it is acceptable

In a small binary or script where every error is printed and the program stops, a message is enough. Even there, anyhow costs little and keeps the cause.
In tests, Result<(), String> as a test return type is fine.

How to detect it

No clippy lint finds this. Search for Result< followed by , String> or , &str> (for example with grep -rnE 'Result<.*, (String|&.*str)>').
Look for map_err(|e| format!(...)) and map_err(|e| e.to_string()).
Look for callers that test error text: .contains(, .starts_with(, or == on an error message.

What to tell your agent

Replace Result<Order, String> in parse_order with a ParseOrderError enum:
FieldCount(usize), BadId(ParseIntError), BadTotal(ParseIntError). Keep the
original errors as the variant data, and implement Display and
std::error::Error (use thiserror if the crate already depends on it).
Change import_line to match on the variants, not on the message text.