rust-review

Rust review

Review Rust code against the same rules the rust-best-practices skill uses for writing it, and report what to change. The output is a review, not a rewrite: the reader decides what to fix. Only rewrite code when the user asks for that.

1. Decide the scope

• What to read. Review exactly what the user pointed at: a file, a module, a crate, or a diff. For a diff, review the changed lines and anything they break, not the whole file.
• Library or application. Check for a lib.rs, pub items used by other crates, or a published Cargo.toml. Library code is held to the API-design rules strictly. Application code gets the simplicity rules instead: concrete types, anyhow-style errors, and no speculative generality. Say which you assumed at the top of the report.
• The edition and MSRV, from Cargo.toml, so you don't recommend features the project can't use.

2. Run the tools first, if you can

If a Rust toolchain is available, run these from the crate root and fold the results into the report. They catch the mechanical issues, which leaves you free for design.

cargo fmt --check
cargo clippy --all-targets --all-features -- -W clippy::pedantic
cargo test

Don't paste raw clippy output. Group repeated warnings into one finding, and skip pedantic lints that don't matter for this code. If the tools aren't available, say so in the report and review by reading.

3. What to look for, in priority order

Work through these in order. The first two categories are where real bugs live.

Correctness and soundness

• Undefined behavior: an unsafe block without a // SAFETY: comment, transmute to cast enums or extend lifetimes, unsafe impl Send/Sync used to silence the compiler, or a safe function that can cause UB through safe calls (unsound).
• Panics on reachable paths: unwrap(), expect(), indexing, as casts that wrap, or integer overflow, wherever input from outside can reach them.
• Trait contracts: a hand-written Ord with a derived PartialEq, or Hash that disagrees with Eq. These silently corrupt BTreeMap, HashMap, sort, and dedup.
• Lock hazards: a MutexGuard held across .await or returned to a caller, user closures called under a lock, or a poisoned lock recovered with into_inner.
• Destructors that panic, block, or can fail.

Error handling

• Errors that callers can't use: (), String, or Box<dyn Error> as a library's public error type, an error enum without Display and std::error::Error, or anyhow::Error in a library's public API.
• Errors that lose information: map_err(|_| ...), no source(), or messages without the path, line, or value that failed.
• Panics used for expected failures, or catch_unwind used as control flow.

API design (strict for libraries)

• Parameters: &String, &Vec<T>, or &Box<T> instead of &str, &[T], or &T; a bool parameter in a public signature; ownership taken and then cloned, or a borrow cloned internally to get ownership.
• Visibility: public fields that should be private, Rc, RefCell, or Arc in public signatures, or third-party types leaking into the API.
• Missing standard traits: Debug on public types, Display on errors, Default next to a zero-argument new(), and FromStr instead of an ad-hoc from_str method.
• Naming: get_ prefixes on getters, as_/to_/into_ that don't match their cost and ownership, and Manager/Service/Util names.
• Evolution: public enums or structs that should be #[non_exhaustive], or traits that should be sealed.

Idiom and clarity

• Clones that exist only to satisfy the borrow checker.
• Check-then-unwrap (if x.is_some() { x.unwrap() }), and match where a combinator is clearer, or the reverse.
• String building with +, hand loops that are clearer as iterators, or iterator chains long enough to need a loop.
• HashMap iteration order leaking into output or tests.
• #![deny(warnings)] in source, #[allow] without a reason, and glob imports of external crates.

Documentation and tests

• Public items without docs, and docs that restate the name.
• Missing # Errors, # Panics, or # Safety sections where they apply.
• Doc examples that use unwrap() instead of ?.
• Logic with no tests, or tests that only restate constants.

The rust-best-practices skill and its references/ folder hold the full rules and their reasoning. If it is installed, read the relevant reference when a finding needs a rule you're unsure of. Otherwise, rely on the summary above.

4. Write the report

Use this structure:

# Rust review: <what was reviewed>

<One paragraph: what the code does, library or application, overall verdict, and the
single most important thing to fix. Say whether clippy and tests were run.>

## Must fix
### 1. <Short title> (`src/file.rs:42`)
**Rule:** <rule name and source, e.g. "Errors implement std::error::Error (C-GOOD-ERR,
Rust API Guidelines)">
**Problem:** <what is wrong and the concrete consequence, one or two sentences>
**Fix:**
```rust
<the smallest code change that fixes it>
```

## Should fix
### 2. ...

## Consider
### 3. ...

## What's already good
- <2 to 4 specific things the code does well>

How to sort findings:

• Must fix means a bug, a soundness hole, a reachable panic, or an API mistake that becomes a breaking change once published.
• Should fix means it works but violates a rule with a real cost, such as worse error messages, harder testing, or a confusing API.
• Consider means style, naming, and polish.

Rules for each finding:

• Cite a location as file:line for every finding (write whole file for a finding that has no single location, such as missing tests), and cite the rule with its source: an API Guidelines code such as C-GETTER, an Effective Rust item number, a Microsoft guideline code such as M-PANIC-ON-BUG, a clippy lint name, or "Google Rust Style Guide". A reader should be able to look the rule up.
• Show the fix as code, kept to the lines that change. Don't rewrite whole functions.
• Group repeats. One finding for "eight unwrap() calls on parse results", listing the lines, rather than eight findings.
• Respect intent. If something looks deliberate, such as a documented quirk or a comment explaining it, mention it under Consider with the trade-off. Don't mark it Must fix.
• Keep it proportionate. For a short file, five to ten findings is plenty. Don't pad the list, and don't hide a real bug among style nits.
• "What's already good" is required. It tells the author what to keep and makes the rest credible.

5. Offer the lints

If the project lacks a [lints] table, end the report by suggesting the BeautifulRust lints config (lints/lints.toml and lints/clippy.toml in the BeautifulRust repo, https://github.com/morganlinton/BeautifulRust). It makes clippy catch most of the mechanical findings above automatically. Name two or three findings from this review that it would have caught.