The green checkmark is not the finish line
You merge a PR. The CI turns green. cargo test passed. cargo clippy was quiet. Two weeks later, production crashes because a raw pointer was dereferenced without checking alignment, or the API forces every caller to wrap their data in a RefCell just to get a mutable reference. The compiler caught the syntax errors. It didn't catch the design smell. Reviewing Rust code means looking past the green checkmark. You are checking for safety invariants, ergonomic APIs, and the subtle traps that only a human eye spots.
What the compiler checks versus what you check
Rust's compiler is a rigorous inspector. It checks types, lifetimes, and memory safety. It enforces the borrow checker rules and guarantees no data races at compile time. But the compiler doesn't know your domain logic. It doesn't know if unwrap() is acceptable in this context. It doesn't know if unsafe is justified. It doesn't know if a function should return Result or panic. Code review fills the gap between "compiles" and "correct."
Think of the compiler as a structural engineer checking bolt torque. It ensures every connection is tight. Code review is the architect verifying the bridge goes to the right place, uses the right materials for the environment, and can hold the expected traffic. The compiler guarantees the code is safe Rust. You guarantee the code is good Rust.
The API surface: ergonomics matter
A common mistake in Rust code is writing APIs that work but force callers into awkward patterns. The compiler accepts these APIs, but they create friction for everyone who uses the crate.
// BAD: Forces callers to own a String or clone.
// This rejects &str, string literals, and borrowed data.
fn parse_header(header: &String) -> Option<&str> {
header.strip_prefix("X-")
}
// GOOD: Accepts any string slice.
// Works with String, &str, literals, and borrowed data.
fn parse_header(header: &str) -> Option<&str> {
header.strip_prefix("X-")
}
The first version compiles. It even works if you pass a String. But it breaks composition. If a caller has a &str, they must allocate a String just to call this function. That allocation is unnecessary overhead. The reviewer's job is to spot these restrictions and demand the more general type.
Check for &String and &Vec<T> in function signatures. These are almost always wrong. Use &str and &[T] instead. The slice types accept the owned types via deref coercion, so you get flexibility without cost.
Convention aside: The community treats &String as a code smell. If you see it in a review, ask why. The answer is usually "I didn't know about &str" or "I copied this from a tutorial." Fix it.
Don't let ergonomic debt accumulate. A function that forces cloning today becomes a performance bottleneck tomorrow.
Running the toolchain: your first reviewer
Before you read a single line of code, run the automation. The tools catch idiomatic issues, performance hints, and correctness warnings that you might miss.
# Run all tests, including doctests and integration tests.
cargo test --all-targets
# Run clippy on all targets.
# --all-targets catches issues in tests and benchmarks that clippy might skip.
cargo clippy --all-targets -- -D warnings
# Build documentation and check for broken links.
# --no-deps focuses on your crate's docs.
cargo doc --no-deps
# Check for security vulnerabilities in dependencies.
cargo audit
cargo clippy is not optional. It lints for idiomatic Rust. It catches things like map followed by unwrap, unnecessary clones, and incorrect loop patterns. Treat clippy warnings as errors. If the code triggers a warning, the author needs to fix it or suppress it with a justification.
cargo test --doc is part of cargo test, but it's worth calling out. Documentation examples are code. If they don't compile, the docs are lying. Broken examples erode trust faster than missing docs.
Convention aside: cargo fmt formats every file the same way. Don't argue style. If the code is not formatted, ask the author to run cargo fmt and push again. Arguing about braces or indentation is a waste of cognitive load. Run the formatter and move on.
Run the tools. They are your first reviewer. If the tools are angry, the code is not ready.
Inspecting unsafe blocks
The compiler cannot verify unsafe code. When you see unsafe, you are the safety net. Every unsafe block must have a // SAFETY: comment that lists the invariants the code relies on. The comment is a proof. If you can't verify the proof, reject the PR.
/// Reads a value from a raw pointer.
///
/// # Safety
/// The pointer must be non-null, aligned, and valid for reads.
pub unsafe fn read_value(ptr: *const i32) -> i32 {
// SAFETY:
// 1. Caller guarantees ptr is non-null.
// 2. Caller guarantees ptr is aligned for i32.
// 3. Caller guarantees ptr points to valid memory.
unsafe { *ptr }
}
Review the // SAFETY: comment against the code. Does the code actually check the invariants? Or does it assume them? If the function is public and unsafe, the invariants are the contract. The caller must uphold them. If the function is private, the caller is inside the same module, and you must verify the caller upholds the invariants.
Check for the "minimum unsafe surface" rule. The unsafe block should be as small as possible. If the block contains logic that doesn't need unsafe, move it outside. Isolating unsafe makes the code easier to audit.
If you see unsafe without a // SAFETY: comment, ask for one. If the author can't write the invariants, they don't understand the risks.
Convention aside: The community calls this the "minimum unsafe surface" rule. Keep unsafe blocks tiny. Wrap them in safe functions with clear contracts. The goal is to minimize the amount of code that bypasses the compiler's checks.
Treat the SAFETY comment as a proof. If you can't write it, you don't have one.
Error handling and panics
Rust encourages explicit error handling with Result. Review code that uses unwrap(), expect(), or panic!. These are acceptable in specific contexts, but they leak implementation details to the caller.
// BAD: Panics if the key is missing.
// The caller has no way to handle the error.
fn get_user(id: u32) -> String {
let db = get_database();
db.query(id).unwrap()
}
// GOOD: Returns Result.
// The caller decides how to handle the error.
fn get_user(id: u32) -> Result<String, DbError> {
let db = get_database();
db.query(id).map_err(|e| DbError::QueryFailed(e))
}
unwrap() is fine in tests, main functions, or when the condition is guaranteed by logic. It is not fine in library code or public APIs. If a function can fail, it should return Result.
Check for #[must_use] on functions returning Result. If the caller ignores the error, the compiler should warn them. It's a small annotation that prevents silent failures.
/// Connects to the database.
///
/// # Panics
/// Panics if the connection string is invalid.
#[must_use]
pub fn connect(url: &str) -> Result<Connection, ConnectError> {
// ...
}
If you see a Result being ignored without let _ = ..., flag it. let _ = ... signals that the author considered the value and chose to drop it. Ignoring a Result silently is a bug waiting to happen.
Question every unwrap(). If it can panic, the API is leaking implementation details to the caller.
Concurrency and Send/Sync
Rust's type system enforces thread safety through the Send and Sync traits. Review code that shares data across threads. Does the struct implement Send? Should it?
use std::sync::Mutex;
// BAD: Mutex protects the data, but the struct is not Send.
// This prevents sharing across threads.
struct Counter {
value: Mutex<i32>,
}
// GOOD: Mutex makes the inner data safe to share.
// The struct is automatically Send and Sync.
struct Counter {
value: Mutex<i32>,
}
In this example, Mutex<T> is Send and Sync if T is Send. The struct is safe to share. But if the struct contains a Rc<T>, it is not Send. Rc is not thread-safe. The reviewer must check for Rc in shared state. Replace Rc with Arc for thread-safe reference counting.
Check for interior mutability. RefCell is not Sync. If you see RefCell in a struct that needs to be shared across threads, flag it. Use Mutex or RwLock instead.
Convention aside: Rc::clone(&data) vs data.clone(). Both compile. The convention is Rc::clone(&data) because data.clone() looks like a deep clone but isn't. It bumps the reference count. The explicit form makes the intent clear. Reviewers should enforce this convention for clarity.
Trust the borrow checker. It usually has a point. If the compiler rejects a shared struct, the author is likely missing a synchronization primitive.
Pitfalls and compiler errors
Reviewers should know the common errors that signal deeper issues.
- E0382 (use of moved value): The compiler rejects code that uses a value after it has been moved. In review, look for places where the author clones just to silence E0382. That's a performance leak. If the value is
Copy, use it directly. If it's not, check if ownership transfer is necessary. - E0277 (trait bound not satisfied): The compiler complains about a missing trait bound. Check if the bound is too broad.
T: ClonewhenT: Copywould suffice? Over-constraining types limits API usability. - E0133 (dereference of raw pointer requires unsafe): This error appears when dereferencing a raw pointer outside
unsafe. If the author wraps the deref inunsafe, check the invariants. If the author removes the deref, check if the logic is correct. - E0597 (borrowed value does not live long enough): Lifetime errors often indicate design issues. The author might be trying to return a reference to local data. The fix is usually to return an owned value or restructure the API.
Inline these errors in your review comments. "This triggers E0382 because data is moved into process. Consider passing a reference or cloning if ownership transfer is unintended." Specific error codes help the author understand the compiler's perspective.
Decision matrix: when to use what
Use cargo clippy --all-targets when you want to catch idiomatic issues, performance hints, and correctness warnings across tests and benchmarks. Use cargo test --doc when you need to verify that documentation examples compile and run. Broken examples erode trust faster than missing docs. Use manual inspection for every unsafe block. The compiler cannot verify your invariants. You must read the // SAFETY: comment and the code to ensure they match. Use cargo doc --no-deps when checking public API surface. It reveals missing doc comments and broken links that cargo test ignores. Reach for rustfmt output when style is the only disagreement. Run the formatter and move on. Arguing about braces is a waste of cognitive load. Use cargo audit when reviewing dependencies. Security vulnerabilities in third-party crates are a risk you cannot ignore.