---

name: rust-review description: | Reviewing Rust code - what to flag, and what rustc and clippy already flag for you

USE WHEN: you are reviewing, critiquing or auditing existing Rust code - a "code review", a "review" of a .rs file, a diff, a PR or a pull request; deciding what to comment on in Rust; avoiding false positives on Rust code

DO NOT USE FOR: writing, explaining or learning Rust - use languages/rust; anything rustc or a default clippy run already reports (this skill lists those so you can stay silent about them); async runtime specifics - use the tokio material; FFI binding generation - use languages/uniffi allowed-tools: Read, Grep, Glob

Reviewing Rust

A reviewer's leverage is what the toolchain cannot say — and in Rust the toolchain says an unusual amount. rustc plus a default clippy run covers most of what a reviewer would flag in other languages, so the "already covered" list is long and the check list is short. Reviewing Rust as if it were C++ produces almost entirely noise.

What remains clusters in four places the compiler cannot reach by construction: runtime-checked interior mutability, reference cycles, deadlock, and unsafe.

The snippets are fragments cut down to the defect, not runnable programs.

Already covered - do not spend review on it

Defect	Reported by
Use after move, use after free, dangling reference	rustc — borrow checker
Data race between threads	rustc — Send/Sync
Unhandled Result	rustc unused_must_use (deny-by-default warning)
Non-exhaustive match	rustc
Unused variable, unused import	rustc
unwrap() on an Option in obvious shapes	clippy unnecessary_unwrap, option_map_unit_fn
Needless clone, needless borrow, needless range loop	clippy redundant_clone, needless_borrow, needless_range_loop
Inefficient string building, &String parameters	clippy ptr_arg, format_push_string
Integer overflow in debug builds	runtime panic — but silent wrap in release
Missing #[must_use] on a builder	clippy return_self_not_must_use

Clippy's default set is broad and on in most CI. Before commenting, assume it ran; check #![allow(...)] at crate root and clippy.toml for what was switched off, because that is where the interesting exemptions hide.

The table above dates; the tools do not. Verify a default set with mcp__documentation__fetch_docs on technology rust-quality — topics clippy-lints, rustc-lints, cargo-profiles. Those entries point at the indexes the tool authors maintain, so they follow the version the project actually pins rather than the one this skill was written against.

The checks that earn their place

RefCell moving an aliasing error to runtime

let a = cell.borrow_mut();
let b = cell.borrow();      // panics: already mutably borrowed

When you see it: two borrow/borrow_mut calls whose guards can be alive at once — often one held across a function call that reaches back into the same cell.

Ask: what is the longest a guard lives here? RefCell does not remove the aliasing rule, it moves the check from compile time to a panic — so the review question is whether every path releases before re-entering. Re-entrancy through a callback or an observer is the usual source, and it is invisible locally.

An Rc cycle that never drops

struct Node { parent: Rc<Node>, children: Vec<Rc<Node>> }   // both strong

When you see it: Rc (or Arc) in both directions of a relationship — parent/child, observer/subject, a graph edge stored on both nodes.

Ask: which direction should be Weak? Reference counting cannot collect a cycle, so this is a leak the borrow checker is entirely happy with. Nothing in the default toolchain reports it.

A lock held across an await

let guard = state.lock().unwrap();      // std::sync::Mutex
do_io(&guard).await;                    // task can be parked while holding it

When you see it: a std::sync guard alive across an .await, or two locks acquired in different orders in different functions.

Ask: can another task need this lock while the first is parked? A blocking mutex held across a yield point stalls the executor thread; clippy's await_holding_lock catches the direct shape, but not the case where the guard is passed into a helper that awaits. Lock ordering it cannot see at all.

Integer arithmetic that wraps in release

let remaining = total - used;   // panics in debug, wraps silently in release

When you see it: subtraction on unsigned types, or any arithmetic on values derived from input, in code that ships as --release.

Ask: is underflow reachable? Overflow checks are on in debug and off in release by default, so the test suite proves nothing about production. This is the one Rust defect that behaves differently in the two profiles, which makes it worth flagging even when tests pass. checked_sub/saturating_sub say which behaviour was meant.

unsafe whose invariant is not written down

unsafe { slice::from_raw_parts(ptr, len) }   // no comment on why this is sound

When you see it: any unsafe block without a // SAFETY: comment stating the invariant the caller must uphold.

Ask: what makes this sound, and who guarantees it? The compiler verifies nothing inside unsafe; the block is a claim, and an unstated claim cannot be re-checked when the surrounding code changes. clippy::undocumented_unsafe_blocks exists but is not in the default set.

unwrap on a value whose invariant lives elsewhere

let cfg = map.get("timeout").unwrap();   // sound only if some other code inserted it

When you see it: unwrap/expect on a lookup, a parse, or an index whose success depends on state established in a different function.

Ask: is the invariant enforced or assumed? unwrap right after an is_some() on the same value is fine and clippy often rewrites it. unwrap on a map key that came from configuration is a panic waiting for a deployment. An expect with a message naming the invariant is the minimum; propagating the error is usually better.

A blocking call inside an async task

async fn handler() {
    std::fs::read_to_string(path).unwrap();   // blocks the executor thread
}

When you see it: std::fs, std::net, thread::sleep, or a CPU-heavy loop inside an async fn.

Ask: which executor thread runs this? Tokio's worker count is small, so one blocking call stalls unrelated tasks. spawn_blocking is the escape hatch. No default lint covers this — tokio::task::block_in_place misuse included.

Drop order relied upon, or a guard bound to _

let _ = mutex.lock();          // dropped IMMEDIATELY: the lock is not held
let _guard = mutex.lock();     // held to end of scope

When you see it: let _ = on anything returning a guard, a tempfile::TempDir, or a span/tracing handle.

Ask: was this meant to live to the end of the scope? _ and _name differ here and read almost identically — let _ = drops at the end of the statement. This is the single most common way a lock silently stops locking.

An error type that erases the cause

.map_err(|_| MyError::Failed)?;   // the original error is discarded

When you see it: map_err with a closure that ignores its argument, or a From impl that drops the source.

Ask: how will this be debugged in production? #[source] (thiserror) or .context(...) (anyhow) preserves the chain. Discarding it turns a specific I/O error into a message that names only the layer that gave up.

Config-dependent - read Cargo.toml and the crate root before commenting

What to read	Why it changes the review
[profile.release] overflow-checks	Off by default. On, the arithmetic check above becomes a panic rather than a silent wrap, and the finding changes shape
edition	2021 changed closure capture to per-field and made IntoIterator for arrays work; 2024 changed unsafe attribute rules and RPIT lifetime capture. Do not suggest what the edition forbids
#![allow(...)] at crate root, clippy.toml	Where the interesting exemptions hide. A crate-wide allow(clippy::unwrap_used) tells you the unwrap findings are deliberate policy, not oversight
unsafe_code = "forbid" in [lints]	If present, the unsafe check does not apply — and if absent in a crate that has unsafe, that is worth one comment
Whether CI runs cargo clippy -- -D warnings	If it does not, the entire "already covered" table above moves into your column

cargo has no equivalent of a "default lint set is narrow" problem, but it does have a default profile problem: cargo test runs in debug, so overflow panics there and wraps in the binary users run.

What to say

Anchor the comment to the line, name the condition that triggers the defect, and say what breaks: "let _ = mutex.lock() drops the guard at the end of this statement, so the critical section below is unlocked" beats "be careful with guards". If you cannot state the input that fails, it is a preference, not a defect.

Rust rewards restraint more than most languages here. The compiler has already made the review; the value you add is the handful of things it structurally cannot check, not a second opinion on what it approved.