Rust Primer

Articles

Reviewing AI-written Rust: six things to check

· 4 min read

Coding assistants are good at producing Rust that compiles. Producing Rust that compiles is not the same as producing Rust you should merge, and the gap shows up in a small number of recurring patterns. Each one makes a compiler error go away, which is why an assistant reaches for it.

Every “before” example below compiles, and cargo clippy with its default settings reports nothing for any of them. Every “after” example compiles and passes its tests.

1. A clone to keep the borrow checker quiet

pub fn count_long(names: Vec<String>) -> usize {
    names.iter().filter(|name| name.len() > 5).count()
}

let long = count_long(names.clone());
println!("{long} of {} names are long", names.len());

The function only reads names, but its signature demands ownership. The caller still needs the vector afterwards, so the move fails, and the quickest fix is .clone(): a full copy of every string, purely to satisfy a signature that should not have asked for ownership.

Fix the signature, not the call site:

pub fn count_long(names: &[String]) -> usize {
    names.iter().filter(|name| name.len() > 5).count()
}

let long = count_long(&names);

Ask: for every .clone(), why does the receiver need its own copy? Sometimes the answer is good: the value is sent to another thread, or kept after the original is dropped. Often there is no answer. clippy::needless_pass_by_value, in the pedantic group, flags the original signature.

2. unwrap() instead of modelling failure

pub fn parse_port(input: &str) -> u16 {
    input.trim().parse().unwrap()
}

This works on every input the assistant tried. On an empty string, "65536" or "http", it panics, and in a library a panic is a decision made on the caller’s behalf. Return a type that states the contract instead:

use std::num::ParseIntError;

#[derive(Debug, PartialEq, Eq)]
pub enum PortError {
    NotANumber(ParseIntError),
    Zero,
}

pub fn parse_port(input: &str) -> Result<u16, PortError> {
    match input.trim().parse() {
        Ok(0) => Err(PortError::Zero),
        Ok(port) => Ok(port),
        Err(e) => Err(PortError::NotANumber(e)),
    }
}

u16 parsing already rejects values above 65,535 and negative numbers, so the type does some of the work. Port 0 is valid as a u16 but means “pick any port” to the operating system, which is rarely what a configuration file intends, so it gets its own variant.

unwrap() is fine in tests, and in main while prototyping. In library code, each one should come with a reason why it cannot fail. To make that a rule, enable clippy::unwrap_used and clippy::expect_used from the restriction group, and set allow-unwrap-in-tests = true and allow-expect-in-tests = true in clippy.toml so that tests are not affected.

3. Errors as strings

The other quick way to silence a ? is Result<T, String> or Box<dyn Error>:

pub fn parse_port(input: &str) -> Result<u16, String> {
    input.trim().parse().map_err(|e| format!("bad port {input:?}: {e}"))
}

The message is fine for a person reading a log. A caller cannot act on it without parsing the string, so every failure looks the same to the code. Libraries should return error types that callers can match on, like PortError above. Catch-all error types such as anyhow::Error are a good fit at the top of an application, where the only remaining job is to report the error. They are a poor fit for a library’s public API.

Ask: what will a caller do with this error, and can it tell this case apart from the others?

4. Returning a copy when a reference says more

#[derive(Debug, Clone, PartialEq)]
pub struct User {
    pub id: u32,
    pub name: String,
}

pub fn find_user(users: &[User], id: u32) -> Option<User> {
    users.iter().find(|user| user.id == id).cloned()
}

Watch for a Clone derive that appears in the same change as the code that needs it: sometimes it was added only to make a .cloned() compile. The lookup should hand back a reference to the user it found:

pub fn find_user(users: &[User], id: u32) -> Option<&User> {
    users.iter().find(|user| user.id == id)
}

The signature now says that the result points into users and that nothing was copied. The caller can still call .cloned() if it really needs its own copy, but that becomes the caller’s choice.

5. Arc<Mutex<T>> with nothing to share

use std::sync::{Arc, Mutex};

pub fn word_count(text: &str) -> usize {
    let count = Arc::new(Mutex::new(0));
    for _word in text.split_whitespace() {
        *count.lock().unwrap() += 1;
    }
    *count.lock().unwrap()
}

Arc<Mutex<T>> is the universal answer to “cannot borrow as mutable”, so it turns up in code that never crosses a thread boundary. Here it adds a heap allocation, reference counting, a lock on every word and two unwrap() calls that can only fail if a thread panicked while holding the lock. In this code, no other thread exists. The whole function is:

pub fn word_count(text: &str) -> usize {
    text.split_whitespace().count()
}

Ask: which threads share this value? If the answer is “none”, plain ownership or &mut will do. If the answer is “several”, ask whether passing messages or splitting the data would avoid sharing it at all.

6. Tests that only cover the happy path

Assistants often write tests that repeat the example from the prompt:

#[test]
fn parses_port() {
    assert_eq!(parse_port("8080"), Ok(8080));
}

That proves the code works on the one input everyone already thought about. The interesting tests are at the edges, where the contract actually lives:

#[test]
fn trims_surrounding_whitespace() {
    assert_eq!(parse_port(" 443\n"), Ok(443));
}

#[test]
fn rejects_port_zero() {
    assert_eq!(parse_port("0"), Err(PortError::Zero));
}

#[test]
fn rejects_empty_and_out_of_range_input() {
    assert!(matches!(parse_port(""), Err(PortError::NotANumber(_))));
    assert!(matches!(parse_port("65536"), Err(PortError::NotANumber(_))));
    assert!(matches!(parse_port("-1"), Err(PortError::NotANumber(_))));
}

Ask: what inputs would break this, and is there a test for each one? Then write the tests before you read the implementation, so that they test the contract rather than the code.

Make the tools do some of the work

Default clippy catches none of the six patterns above, but some of it can be switched on:

# Pedantic lints, including needless_pass_by_value.
cargo clippy --all-targets -- -W clippy::pedantic

# No unwrap() or expect() outside tests
# (with allow-unwrap-in-tests = true and allow-expect-in-tests = true in clippy.toml).
cargo clippy --all-targets -- -W clippy::unwrap_used -W clippy::expect_used

pedantic is noisy, so you might run it occasionally rather than in CI. It is still worth knowing what it would have said. The rest, such as whether an error type is useful or whether a value needs to be shared, needs a person who understands ownership well enough to imagine the simpler version. That is the skill Rust Primer is built to practise, and the case for learning it has not gone away just because the first draft is free.