rust-code-review

What this skill does

# Rust Code Review

## Review Workflow

Follow this sequence to avoid false positives and catch edition-specific issues:

1. **Check `Cargo.toml`** — Note the Rust edition (2018, 2021, 2024) and MSRV if set. Edition 2024 introduces breaking changes to unsafe semantics, RPIT lifetime capture, temporary scoping, and `!` type fallback. This determines which patterns apply. Check workspace structure if present.
2. **Check dependencies** — Note key crates (thiserror vs anyhow, tokio features, serde features). These inform which patterns are expected.
3. **Scan changed files** — Read full functions, not just diffs. Many Rust bugs hide in ownership flow across a function.
4. **Check each category** — Work through the checklist below, loading references as needed.
5. **Verify before reporting** — Complete **Gates** (below), including the verification-protocol gate, before submitting findings.

## Gates

These steps are **sequenced**: do not skip ahead with “mental verification.” Each step has an objective **Pass** you can satisfy from files on disk and your own read path.

1. **Crate context** — Before relying on edition-specific checklist rows (Edition 2024, MSRV-sensitive APIs) or dependency assumptions. **Pass:** You opened the relevant `Cargo.toml` (package or workspace manifest) and can state `edition` and `rust-version` (if set) in one line.
2. **Expanded read** — Before reporting a **Major** or **Critical** finding. **Pass:** You read the full function, `unsafe` block, or `impl` / trait item that contains the cited line (not only a diff hunk).
3. **Severity match** — Before each finding line in the report. **Pass:** The **Severity** label matches **Severity Calibration** for that issue class, or you use **Informational** and give a one-line rationale.
4. **Verification protocol** — Before finalizing the report. **Pass:** the [review-verification-protocol](../review-verification-protocol/SKILL.md) skill is loaded and every step in it that applies to this review is completed (do not substitute a vague “I checked”).

## Output Format

Report findings as:

```text
[FILE:LINE] ISSUE_TITLE
Severity: Critical | Major | Minor | Informational
Description of the issue and why it matters.
```

## Quick Reference

| Issue Type | Reference |
|------------|-----------|
| Ownership transfers, borrowing, lifetimes, clone traps, iterators | [references/ownership-borrowing.md](references/ownership-borrowing.md) |
| Lifetime variance, covariance/invariance, memory regions | [references/lifetime-variance.md](references/lifetime-variance.md) |
| Result/Option handling, thiserror, anyhow, opaque vs enumerated errors, deferred-cleanup with `?` | [references/error-handling.md](references/error-handling.md) |
| Async pitfalls, Send/Sync bounds, poll contract, Pin mechanics, cancellation soundness | [references/async-concurrency.md](references/async-concurrency.md) |
| Send/Sync semantics, atomics, memory ordering, lock patterns | [references/concurrency-primitives.md](references/concurrency-primitives.md) |
| Memory ordering decision tree, fences, ABA, out-of-thin-air | [references/memory-ordering.md](references/memory-ordering.md) |
| Hand-rolled spinlocks, channels, Arc, seqlock, CAS retry patterns | [references/lock-free-patterns.md](references/lock-free-patterns.md) |
| Shared-memory vs worker-pool vs actor design, async vs threads, race-condition vs data race | [references/concurrency-models.md](references/concurrency-models.md) |
| Type layout, alignment, repr, PhantomData, generics vs dyn Trait, wide pointers, auto-trait leakage | [references/types-layout.md](references/types-layout.md) |
| Object safety, ergonomic trait impls, Deref discipline, fallible destructors, hidden contracts, is_normal | [references/interface-design.md](references/interface-design.md) |
| Index pointers, drop guards, extension traits, crate preludes | [references/patterns-in-the-wild.md](references/patterns-in-the-wild.md) |
| Unsafe code, API design, derive patterns, clippy patterns | [references/common-mistakes.md](references/common-mistakes.md) |
| Validity vs safety, drop check, may_dangle, provenance, panic safety in unsafe, MaybeUninit, Miri | [references/unsafe-deep.md](references/unsafe-deep.md) |

> For development guidance on performance, pointer types, type state, clippy config, iterators, generics, and documentation, load the [rust-best-practices](../rust-best-practices/SKILL.md) skill.

## Review Checklist

### Ownership and Borrowing
- [ ] No unnecessary `.clone()` to silence the borrow checker (hiding design issues)
- [ ] No `.clone()` inside loops — prefer `.cloned()` or `.copied()` on iterators
- [ ] No cloning to avoid lifetime annotations (take ownership explicitly or restructure)
- [ ] References have appropriate lifetimes (not overly broad `'static` when shorter lifetime works)
- [ ] **Edition 2024**: RPIT (`-> impl Trait`) captures all in-scope lifetimes by default; use `+ use<'a>` for precise capture control
- [ ] `&str` preferred over `String`, `&[T]` over `Vec<T>` in function parameters
- [ ] `impl AsRef<T>` or `Into<T>` used for flexible API parameters
- [ ] No dangling references or use-after-move
- [ ] Interior mutability (`Cell`, `RefCell`, `Mutex`) used only when shared mutation is genuinely needed
- [ ] Small types (≤24 bytes) derive `Copy` and are passed by value
- [ ] `Cow<'_, T>` used when ownership is ambiguous
- [ ] Iterator chains preferred over index-based loops for collection transforms
- [ ] No premature `.collect()` — pass iterators directly when the consumer accepts them
- [ ] `.sum()` preferred over `.fold()` for summation (compiler optimizes better)
- [ ] `_or_else` variants used when fallbacks involve allocation
- [ ] **Edition 2024**: `if let` temporaries drop at end of the `if let` — code relying on temporaries living through the else branch needs restructuring
- [ ] **Edition 2024**: `Box<[T]>` implements `IntoIterator` — prefer direct iteration over `into_vec()` first

### Error Handling
- [ ] `Result<T, E>` used for recoverable errors, not `panic!`/`unwrap`/`expect`
- [ ] Error types provide context (thiserror with `#[error("...")]` or manual `Display`)
- [ ] `?` operator used with proper `From` implementations or `.map_err()`
- [ ] `unwrap()` / `expect()` only in tests, examples, or provably-safe contexts
- [ ] Error variants are specific enough to be actionable by callers
- [ ] `anyhow` used in applications, `thiserror` in libraries (or clear rationale for alternatives)
- [ ] `_or_else` variants used when fallbacks involve allocation (`ok_or_else`, `unwrap_or_else`)
- [ ] `let-else` used for early returns on failure (`let Ok(x) = expr else { return ... }`)
- [ ] `inspect_err` used for error logging, `map_err` for error transformation

### Traits and Types
- [ ] Traits are minimal and cohesive (single responsibility)
- [ ] `derive` macros appropriate for the type (`Clone`, `Debug`, `PartialEq` used correctly)
- [ ] Newtypes used to prevent primitive obsession (e.g., `struct UserId(Uuid)` not bare `Uuid`)
- [ ] `From`/`Into` implementations are lossless and infallible; `TryFrom` for fallible conversions
- [ ] Sealed traits used when external implementations shouldn't be allowed
- [ ] Default implementations provided where they make sense
- [ ] `Send + Sync` bounds verified for types shared across threads
- [ ] `#[diagnostic::on_unimplemented]` used on public traits to provide clear error messages when users forget to implement them

### Interface Design
> Detailed guidance: [references/interface-design.md](references/interface-design.md)
- [ ] Methods on `dyn`-intended traits don't use `Self` by value, generic params, or associated constants (or are gated `where Self: Sized`)
- [ ] New traits ship with blanket impls for `&T`, `&mut T`, `Box<T>` so reference and smart-pointer arguments work
- [ ] Iterable types implement `IntoIterator` for `&Self` and `&mut Self`, not just `Self`
- [ ] `Deref` only used for transparent forwarding, never as "i