Rust Code Review

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 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:

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

Quick Reference

For development guidance on performance, pointer types, type state, clippy config, iterators, generics, and documentation, load the rust-best-practices 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

• [ ] 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 "inheritance" — inherent-method ambiguity is a real bug class
• [ ] Fallible cleanup uses an explicit close()/shutdown() returning Result; Drop is best-effort fallback only
• [ ] No block_on(...) or new runtime in Drop (deadlock under async runtimes)
• [ ] Public types have a compile-time fn is_normal<T: Sized + Send + Sync + Unpin>() {} test so auto-trait regressions surface at build time
• [ ] Re-exported foreign types in public API are flagged — downstream major bumps become this crate's breaking change
• [ ] Getter methods follow the convention fn name(&self) not fn get_name(&self) (reserve get_* for Option-returning or interesting lookups)
• [ ] as_* is cheap reference-to-reference, to_* may allocate, into_* consumes — verify the cost matches the prefix
• [ ] Standard derives (Debug, Clone, Default, PartialEq, Eq, Hash) considered for every public type; Copy only when truly cheap and value-like (removing it later is breaking)

Patterns in the Wild

Detailed guidance: references/patterns-in-the-wild.md

• [ ] Index-pointer graphs use generational indices (slotmap::DefaultKey, petgraph::NodeIndex) — bare usize orphans after Vec::swap_remove
• [ ] Drop guards bound to let _guard = ..., never let _ = ... (the second drops immediately, not at scope end)
• [ ] Drop-guard cleanup not relied upon under panic = "abort" (destructors do not run)
• [ ] Extension traits used only when the type is foreign; for owned types use inherent impl directly
• [ ] Extension-trait methods don't shadow popular existing methods on the same type (ambiguity at call sites)
• [ ] Crate prelude module additions treated as semver-minor (RFC 1105); reserve for major releases when possible

Concurrency Design (Models)

Detailed guidance: references/concurrency-models.md

• [ ] Concurrency model (shared memory vs worker pool vs actor) named explicitly in the design; primitives match the model
• [ ] Mutex critical section is short, measurable, and excludes I/O / network calls / .await points
• [ ] Worker-pool queues are bounded; backpressure strategy is named
• [ ] Actor mailbox channels are sized to expected load; cross-actor cycles reviewed for deadlock
• [ ] async is not conflated with parallelism — join! interleaves on one thread; only tokio::spawn (or equivalent on a multi-threaded runtime) parallelizes
• [ ] CAS retry loops on a hot atomic considered for replacement with fetch_add/sharding (CAS is O(N²) under contention)
• [ ] println! / dbg! not used as a debugging tool for race conditions (the Stdout mutex changes the race)

Unsafe Code

• [ ] unsafe blocks have safety comments explaining invariants
• [ ] unsafe is minimal — only the truly unsafe operation is inside the block
• [ ] Safety invariants are documented and upheld by surrounding safe code
• [ ] No undefined behavior (null pointer deref, data races, invalid memory access)
• [ ] unsafe trait implementations justify why the contract is upheld
• [ ] Edition 2024: unsafe fn bodies use explicit unsafe {} blocks around unsafe ops (unsafe_op_in_unsafe_fn is deny)
• [ ] Edition 2024: extern "C" {} blocks written as unsafe extern "C" {}
• [ ] Edition 2024: #[no_mangle] and #[export_name] written as #[unsafe(no_mangle)] and #[unsafe(export_name)]

Concurrency (Memory Ordering and Lock-Free Patterns)

Detailed guidance: references/memory-ordering.md, references/lock-free-patterns.md

• [ ] Types shared across threads have correct Send / Sync bounds; unsafe impl Send/Sync carries a comment naming the invariant
• [ ] Each atomic operation pairs with a named happens-before edge (spawn/join, Release/Acquire on the same atomic, or a fence); Release publishes data, Acquire observes it
• [ ] No SeqCst by default — only when two or more independent atomics need a single global total order, with a comment naming the requirement
• [ ] No store(.., Acquire) / load(.., Release) / load(.., AcqRel) (rejected by the type half they occupy)
• [ ] Relaxed not used to publish or observe non-atomic data (use Release / Acquire)
• [ ] compare_exchange_weak used inside retry loops; strong compare_exchange reserved for one-shot updates; success ordering at least Acquire when acquiring a critical section
• [ ] Hand-rolled spinlocks include std::hint::spin_loop() in the busy wait, exponential backoff, and an eventual thread::yield_now(); not used in normal user-space binaries without a documented reason a Mutex is unsuitable
• [ ] Hand-rolled Arc clones with Relaxed, drops with Release + fence(Acquire) on the last decrement, and includes an overflow guard
• [ ] Arc<Mutex<...>> cycles broken with Weak; Arc<Mutex<Copy>> reviewed for Arc<AtomicT> replacement
• [ ] Hand-rolled lock-free primitives have a #[cfg(loom)] test module and a Miri-runnable test (no blanket cfg_attr(miri, ignore))
• [ ] OnceLock / LazyLock preferred over once_cell / lazy_static for new code (MSRV ≥ 1.80)
• [ ] No MutexGuard held across .await (use tokio::sync::Mutex or drop the guard first)
• [ ] Hot atomics on contended cache lines wrapped with CachePadded or #[repr(align(64))] to avoid false sharing
• [ ] Shared mutation goes through UnsafeCell<T> (not bare *mut T or transmuted & to &mut)
• [ ] Pointer-based CAS (AtomicPtr<Node>) uses epoch / hazard-pointer / tagged-pointer reclamation; ABA hazards considered

Naming and Style

• [ ] Types are PascalCase, functions/methods snake_case, constants SCREAMING_SNAKE_CASE
• [ ] Modules use snake_case
• [ ] is_, has_, can_ prefixes for boolean-returning methods
• [ ] Builder pattern methods take and return self (not &mut self) for chaining
• [ ] Public items have doc comments (///)
• [ ] #[must_use] on functions where ignoring the return value is likely a bug
• [ ] Imports ordered: std → external crates → workspace → crate/super
• [ ] #[expect(clippy::...)] preferred over #[allow(...)] for lint suppression

Performance

Detailed guidance: the rust-best-practices skill (references/performance.md)

• [ ] No unnecessary allocations in hot paths (prefer &str over String, &[T] over Vec<T>)
• [ ] collect() type is specified or inferable
• [ ] Iterators preferred over indexed loops for collection transforms
• [ ] Vec::with_capacity() used when size is known
• [ ] No redundant .to_string() / .to_owned() chains
• [ ] No intermediate .collect() when passing iterators directly works
• [ ] .sum() preferred over .fold() for summation
• [ ] Static dispatch (impl Trait) used over dynamic (dyn Trait) unless flexibility required

Clippy Configuration

Detailed guidance: the rust-best-practices skill (references/clippy-config.md)

• [ ] Workspace-level lints configured in Cargo.toml ([workspace.lints.clippy] or [lints.clippy])
• [ ] #[expect(clippy::lint)] used over #[allow(...)] — warns when suppression becomes stale
• [ ] Justification comment present when suppressing any lint
• [ ] Key lints enforced: redundant_clone, large_enum_variant, needless_collect, perf group
• [ ] cargo clippy --all-targets --all-features -- -D warnings passes
• [ ] Doc lints enabled for library crates (missing_docs, broken_intra_doc_links)

Type State Pattern

Detailed guidance: the rust-best-practices skill (references/type-state-pattern.md)

• [ ] PhantomData<State> used for zero-cost compile-time state machines (not runtime enums/booleans)
• [ ] State transitions consume self and return new state type (prevents reuse of old state)
• [ ] Only applicable methods available per state (invalid operations are compile errors)
• [ ] Pattern used where it adds safety value (builders with required fields, connection states, workflows)
• [ ] Not overused for trivial state (simple enums are fine when runtime flexibility needed)

Severity Calibration

Critical (Block Merge)

• unsafe code with unsound invariants or undefined behavior
• Use-after-free or dangling reference patterns
• unwrap() on user input or external data in production code
• Data races (concurrent mutation without synchronization)
• Wrong memory ordering on an atomic that gates other shared data (data race)
• Memory leaks via circular Arc<Mutex<...>> without weak references

Major (Should Fix)

• Errors returned without context (bare return err equivalent)
• .clone() masking ownership design issues in hot paths
• Missing Send/Sync bounds on types used across threads
• panic! for recoverable errors in library code
• Overly broad 'static lifetimes hiding API design issues

Minor (Consider Fixing)

• Missing doc comments on public items
• String parameter where &str or impl AsRef<str> would work
• Derive macros missing for types that should have them
• Unused feature flags in Cargo.toml
• Suboptimal iterator chains (multiple allocations where one suffices)

Informational (Note Only)

• Suggestions to introduce newtypes for domain modeling
• Refactoring ideas for trait design
• Performance optimizations without measured impact
• Suggestions to add #[must_use] or #[non_exhaustive]

When to Load References

• Reviewing ownership, borrows, lifetimes, clone traps → ownership-borrowing.md
• Reviewing lifetime variance, covariance/invariance, multiple lifetime params → lifetime-variance.md
• Reviewing Result/Option handling, error types, opaque vs enumerated, deferred-cleanup, Error trait impls → error-handling.md
• Reviewing async code, poll contract, Pin mechanics, cancellation soundness, cross-runtime → async-concurrency.md
• Reviewing concurrency design decisions (shared memory vs worker pool vs actor, async vs threads), data race vs race condition → concurrency-models.md
• Reviewing Send/Sync, atomics, mutexes, lock patterns → concurrency-primitives.md
• Reviewing memory ordering decisions, fences, ABA, out-of-thin-air → memory-ordering.md
• Reviewing hand-rolled spinlocks, channels, Arc, seqlock, CAS retry patterns → lock-free-patterns.md
• Reviewing type layout, alignment, repr, PhantomData, wide pointers, auto-trait leakage, Sized/?Sized → types-layout.md
• Reviewing interface design — object safety, ergonomic blanket impls, Deref discipline, fallible/blocking destructors, hidden contracts, naming → interface-design.md
• Reviewing index-pointer graphs, drop guards, extension traits, crate preludes → patterns-in-the-wild.md
• Reviewing unsafe code, API design, derive macros, clippy patterns → common-mistakes.md
• Reviewing validity vs safety, drop check + may_dangle, provenance, panic safety in unsafe, MaybeUninit → unsafe-deep.md
• Reviewing performance, pointer types, type state, generics, iterators, documentation → the rust-best-practices skill

Valid Patterns (Do NOT Flag)

These are acceptable Rust patterns — reporting them wastes developer time:

• `.clone()` in tests — Clarity over performance in test code
• `unwrap()` in tests and examples — Acceptable where panicking on failure is intentional
• `Box<dyn Error>` in simple binaries — Not every application needs custom error types
• `String` fields in structs — Owned data in structs is correct; &str fields require lifetime parameters
• `#[allow(dead_code)]` during development — Common during iteration
• `todo!()` / `unimplemented!()` in new code — Valid placeholder during active development
• `.expect("reason")` with clear message — Self-documenting and acceptable for invariants
• *`use super:: in test modules** — Standard pattern for #[cfg(test)]` modules
• Type aliases for complex types — type Result<T> = std::result::Result<T, MyError> is idiomatic
• `impl Trait` in return position — Zero-cost abstraction, standard pattern
• Turbofish syntax — collect::<Vec<_>>() is idiomatic when type inference needs help
• `_` prefix for intentionally unused variables — Compiler convention
• `#[expect(clippy::...)]` with justification — Self-cleaning lint suppression
• `Arc::clone(&arc)` — Explicit Arc cloning is idiomatic and recommended
• `std::sync::Mutex` for short critical sections in async — Tokio docs recommend this
• `for` loops over iterators — When early exit or side effects are needed
• `async fn` in trait definitions — Stable since 1.75; async-trait crate only needed for dyn Trait or pre-1.75 MSRV
• `LazyCell` / `LazyLock` from std — Stable since 1.80; replaces once_cell and lazy_static for new code
• `+ use<'a, T>` precise capture syntax — Edition 2024 syntax for controlling RPIT lifetime capture

Context-Sensitive Rules

Only flag these issues when the specific conditions apply:

Before Submitting Findings

Satisfy Gates § verification protocol (step 4). Load and follow the review-verification-protocol skill before reporting any issue.

Async and Concurrency

Critical Anti-Patterns

1. Blocking in Async Context

Blocking operations inside async functions starve the tokio runtime's thread pool, causing latency spikes and potential deadlocks.

// BAD - blocks the async runtime thread
async fn read_config() -> Config {
    let data = std::fs::read_to_string("config.toml").unwrap(); // BLOCKING!
    toml::from_str(&data).unwrap()
}

// GOOD - use async I/O
async fn read_config() -> Result<Config, Error> {
    let data = tokio::fs::read_to_string("config.toml").await?;
    let config: Config = toml::from_str(&data)?;
    Ok(config)
}

// GOOD - offload blocking work to a dedicated thread
async fn compute_hash(data: Vec<u8>) -> Result<Hash, Error> {
    tokio::task::spawn_blocking(move || {
        expensive_hash(&data)
    }).await?
}

Common blockers to watch for: std::fs, std::net, std::thread::sleep, CPU-heavy computation, synchronous database drivers.

2. Holding Locks Across Await Points

A MutexGuard held across an .await can cause deadlocks and prevents Send bounds from being satisfied.

// BAD - guard held across await
async fn update(state: &Mutex<State>) {
    let mut guard = state.lock().await;
    let data = fetch_data().await; // guard still held!
    guard.data = data;
}

// GOOD - drop guard before await
async fn update(state: &Mutex<State>) {
    let current = {
        let guard = state.lock().await;
        guard.data.clone()
    }; // guard dropped here
    let new_data = fetch_data().await;
    let mut guard = state.lock().await;
    guard.data = new_data;
}

3. Using std::sync::Mutex in Async Code

std::sync::Mutex blocks the thread while waiting. In async code, use tokio::sync::Mutex which yields to the runtime, or use std::sync::Mutex only for short, non-async critical sections.

// RISKY - std mutex in async context
use std::sync::Mutex;
async fn process(shared: &Mutex<Vec<Item>>) {
    let mut guard = shared.lock().unwrap(); // blocks thread
    guard.push(item);
}

// GOOD - tokio mutex for async-aware locking
use tokio::sync::Mutex;
async fn process(shared: &Mutex<Vec<Item>>) {
    let mut guard = shared.lock().await; // yields to runtime
    guard.push(item);
}

Exception: std::sync::Mutex is fine when the critical section is very short (no async operations, just field access) because it avoids the overhead of tokio's async mutex. The tokio docs themselves recommend this pattern.

For a detailed comparison of tokio::sync::Mutex vs std::sync::Mutex and other sync primitives (RwLock, Semaphore, Notify), see the tokio-async-code-review skill (references/sync-primitives.md).

4. Spawning Tasks Without Join Handles

Fire-and-forget tasks can silently fail, leak resources, or outlive their logical scope.

// BAD - task error is lost, no lifecycle management
tokio::spawn(async {
    process_batch(items).await;
});

// GOOD - handle tracked for cancellation and error reporting
let handle = tokio::spawn(async move {
    process_batch(items).await
});
// ... later
match handle.await {
    Ok(result) => result?,
    Err(e) => tracing::error!(error = %e, "batch processing panicked"),
}

5. Missing Cancellation Safety

When a future is dropped (e.g., via tokio::select!), partially completed operations may leave state inconsistent.

// RISKY - if timeout fires, partial write may have occurred
tokio::select! {
    result = write_to_db(&data) => { ... }
    _ = tokio::time::sleep(timeout) => {
        return Err(Error::Timeout);
    }
}

// SAFER - use cancellation-safe operations or checkpoints
tokio::select! {
    result = write_to_db_atomic(&data) => { ... }
    _ = tokio::time::sleep(timeout) => {
        // write_to_db_atomic either completes fully or not at all
        return Err(Error::Timeout);
    }
}

6. Send/Sync Bound Violations

Types shared across tasks must be Send. Types shared across threads must be Send + Sync. Rc, RefCell, and raw pointers are not Send.

// WON'T COMPILE - Rc is not Send
let data = Rc::new(vec![1, 2, 3]);
tokio::spawn(async move {
    println!("{:?}", data); // Rc is !Send
});

// GOOD - Arc is Send + Sync
let data = Arc::new(vec![1, 2, 3]);
tokio::spawn(async move {
    println!("{:?}", data);
});

async fn in Traits (Stable Since 1.75)

Native async fn in trait definitions is stable since Rust 1.75. The async-trait crate is no longer needed for most use cases.

// BAD — unnecessary dependency on async-trait (if MSRV >= 1.75)
#[async_trait::async_trait]
trait Service {
    async fn call(&self, req: Request) -> Response;
}

// GOOD — native async fn in trait
trait Service {
    async fn call(&self, req: Request) -> Response;
}

When `async-trait` is still needed:

• `dyn Trait`: Native async traits don't support dynamic dispatch (dyn Service). Use async-trait or the trait_variant crate for object-safe async traits.
• MSRV < 1.75: Projects that must compile on older Rust versions.

When reviewing, check whether async-trait usage can be replaced with native syntax. The crate adds a heap allocation per call (Box::pin), which native async traits avoid.

Channel Patterns

Choose channels based on communication shape: mpsc for back-pressure, broadcast for fan-out, oneshot for request-response, watch for latest-value. Ensure bounded channels are sized to avoid OOM risks with unbounded alternatives.

For detailed channel patterns, usage examples, and pitfalls, see the tokio-async-code-review skill (references/channels.md).

Graceful Shutdown

Use CancellationToken from tokio_util with child tokens for hierarchical shutdown. Combine with tokio::select! to listen for cancellation alongside work.

For full shutdown patterns and cancellation token usage, see the tokio-async-code-review skill (references/task-management.md).

Review Questions

1. Are there any blocking operations (std::fs, std::net, thread::sleep) in async functions? 2. Are mutex guards dropped before .await points? 3. Is tokio::sync::Mutex used when locks are held across await points? 4. Are spawned tasks tracked via join handles? 5. Is select! used with cancellation-safe futures? 6. Do types shared across tasks satisfy Send + Sync bounds? 7. Can async-trait be replaced with native async fn in traits (MSRV >= 1.75, no dyn Trait needed)?

The Poll Contract — Register Before Checking

A leaf Future::poll impl must store the latest cx.waker() BEFORE checking whether the resource is ready. If the order is reversed (check first, then store), there is a TOCTOU race: a producer can publish data and call the OLD waker between the check and the store, and the consumer parks holding a waker that will never fire again. The task sleeps forever.

// WRONG — race window between check and waker store.
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
    if let Some(v) = self.queue.try_pop() { return Poll::Ready(v); }
    *self.waker.lock() = Some(cx.waker().clone()); // too late
    Poll::Pending
}

// RIGHT — register first, then re-check (spurious wake is harmless).
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
    *self.waker.lock() = Some(cx.waker().clone());
    if let Some(v) = self.queue.try_pop() { return Poll::Ready(v); }
    Poll::Pending
}

Executors are free to construct a fresh Waker per poll, so caching the first waker forever is also a bug — see WAKER_STORED_ONCE_NEVER_REFRESHED. Compare with Waker::will_wake before cloning to skip redundant atomic increments. See concurrency-primitives.md for the underlying Arc/atomic mechanics.

Pin Wraps a Pointer, Not a Value

Pin<P> is always Pin<P> where P is a pointer type: Pin<&mut T>, Pin<Box<T>>, Pin<Arc<T>>, Pin<&T>. The invariant Pin enforces is "the memory address of *P will not change until T is dropped." Wrapping a pointer keeps the pointee in place while the Pin itself can move freely. If Pin held a T by value, moving the Pin would move the T and break self-references.

The safety surface extends to P's Deref, DerefMut, and Drop impls: if any of them move the pointee via the &mut self they receive, the pin invariant is violated from outside an unsafe block. Custom smart pointers that intend to be wrapped in Pin must keep these impls move-free or document the hazard.

Box::pin vs std::pin::pin!

Box has an unconditional Unpin impl: moving a Box<T> does not move the T. But Pin<Box<T>> does NOT let you move a !Unpin T out — only the box itself. Prefer pin! for stack-local futures inside select! arms or driver loops to avoid per-iteration allocation.

Structural vs Non-Structural Pinning

When a struct opts out of Unpin (via PhantomPinned) and is pinned, each field is either structurally pinned — pinning the outer pins this field, projecting Pin<&mut Self> to Pin<&mut Field> — or non-structural — the field can be accessed as plain &mut Field and moved out. Pick one per field and stick to it; Drop must not move structurally-pinned fields. Hand-rolling this is error-prone; use pin-project-lite.

use std::marker::PhantomPinned;
use pin_project_lite::pin_project;

pin_project! {
    struct SelfRef {
        #[pin] inner: InnerFuture,   // structurally pinned
        counter: u64,                // not structural, freely accessible
        _pin: PhantomPinned,
    }
}

Without PhantomPinned, a struct of all-Unpin fields auto-derives Unpin and Pin provides zero protection — callers can Pin::new(&mut x) and then move it.

Cancellation Soundness — Drop Equals Cancel

There is no async Drop. When an async fn is dropped mid-poll, its generated state machine drops every local currently held across the latest await point, in reverse order of construction. No "cleanup" coroutine runs. A future that has already written half a request and is then dropped (timeout, select!, JoinHandle::abort) leaves external state in whatever partial form was reached.

Cancel-safety taxonomy for review:

• Cancel-safe: futures that hold no "I started but didn't finish" state across .await. Examples: mpsc::Receiver::recv, broadcast::Receiver::recv, tokio::time::sleep, Mutex::lock, read (returns whatever arrived), accept.
• Cancel-unsafe: futures with internal partial-buffer state that gets dropped. Examples: AsyncReadExt::read_exact, read_to_end, read_line, AsyncWriteExt::write_all, HTTP body reads from reqwest/hyper, anything with a per-call invariant spanning multiple polls.

Defensive pattern for cancel-unsafe work: spawn it onto a detached task and .await the JoinHandle. The JoinHandle future is cancel-safe; the work behind it runs to a clean checkpoint even if the awaiter is cancelled. See ../../tokio-async-code-review/references/pinning-cancellation.md for tokio-specific cancellation primitives and concurrency-models.md for choosing between channel-driven supervisors and direct spawn.

Send Propagation Through .await

The auto-trait Send on the compiler-generated future is decided by every local that is live across any .await point. A single non-Send local — Rc<T>, RefCell<T> (held), std::sync::MutexGuard<'_, T> from std::sync, a raw pointer, a thread-local handle — downgrades the whole future to !Send. tokio::spawn then rejects it with an error pointing at the await, not at the offending local.

Fix patterns:

// BAD — Rc lives across the await; whole future is !Send.
let local = Rc::new(state);
let _ = use_local(&local);
fetch().await;

// GOOD — scope the !Send local so it drops before the await.
{
    let local = Rc::new(state);
    let _ = use_local(&local);
} // Rc dropped here
fetch().await;

The same pattern fixes std::sync::MutexGuard held across .await — scope it. parking_lot::MutexGuard is Send, which means the compiler does NOT catch the hazard; review for it manually.

Cross-Runtime Compatibility

A leaf future built against tokio's reactor (e.g. tokio::net::TcpStream::read, tokio::time::sleep) panics with "there is no reactor running" when polled under smol, async-std, or monoio. For library code that should be runtime-agnostic, depend on futures crate primitives (AsyncRead, AsyncWrite, Stream), expose async fn or impl Future from your public API, and do NOT spawn internally — let the caller's runtime own the task graph. If you must depend on tokio, declare it loudly in the crate docs and gate alternative runtimes behind feature flags.

Additional Review Checks

• [FILE:LINE] LEAF_POLL_CHECK_BEFORE_REGISTER — poll impl checks readiness BEFORE installing cx.waker(). Producer can publish + wake in the gap; consumer parks forever. Fix: store waker first, then re-check.
• [FILE:LINE] WAKER_STORED_ONCE_NEVER_REFRESHED — Leaf future stashes the first waker and never updates it. Executor may rebind the task (FuturesUnordered, task migration) and produce a fresh waker per poll; the cached one wakes nothing. Refresh on every Pending return, or compare via Waker::will_wake.
• [FILE:LINE] WAKER_CLONED_EVERY_POLL_HOT_PATH — Leaf future calls cx.waker().clone() unconditionally each poll, even when the previously stored waker would wake the same task. Use Waker::will_wake(&old, cx.waker()) to skip the atomic increment on hot paths.
• [FILE:LINE] PIN_HOLDS_VALUE_NOT_POINTER — Code constructs Pin<T> directly or treats Pin as a value wrapper. Pin must wrap a pointer type (&mut T, Box<T>, Arc<T>). Likely a misuse of Pin::new_unchecked on a non-pointer.
• [FILE:LINE] CUSTOM_SMART_POINTER_MOVES_IN_DEREF — A Deref/DerefMut/Drop impl on a pointer type intended to be wrapped in Pin moves the pointee (e.g. via mem::replace in Drop). Breaks the pin invariant from safe code.
• [FILE:LINE] BOX_PIN_IN_HOT_LOOP — Box::pin(async { ... }) allocates on the heap every iteration of a polling loop. Replace with std::pin::pin! for stack-local pinning unless the future must escape the scope.
• [FILE:LINE] PIN_BOX_USED_TO_MOVE_OUT_OF_T — Code calls *pin_box or mem::replace on Pin<Box<T>> expecting to move T. Box: Unpin lets you move the box, not the T inside. Use into_inner_unchecked only with the same justification as Pin::new_unchecked.
• [FILE:LINE] STRUCTURAL_PIN_DROP_MOVES_FIELD — A !Unpin struct's hand-written Drop impl moves a structurally-pinned field (e.g. via mem::take). Violates the structural pinning contract — switch to pin-project-lite or document why the field isn't structural.
• [FILE:LINE] PIN_PROJECT_MIXED_STRUCTURAL — Same field accessed as both Pin<&mut Field> (structural) and &mut Field (non-structural) across different methods of an impl. Pick one per field. Use pin-project-lite to enforce.
• [FILE:LINE] PHANTOMPINNED_MISSING_ON_SELFREF — A struct holds raw pointers or references into its own data but is auto-Unpin (all fields are Unpin, no PhantomPinned). Callers can move it freely via Pin::new. Add _pin: PhantomPinned.
• [FILE:LINE] ASYNC_DROP_ATTEMPT — A struct's Drop impl needs to flush a channel, send a goodbye message, or .await cleanup. There is no async Drop; the work is silently skipped or blocks the runtime thread. Expose async fn close(self) and require callers to invoke it explicitly.
• [FILE:LINE] CANCEL_UNSAFE_IN_SELECT_BRANCH — tokio::select! branch holds read_exact, read_to_end, read_line, write_all, or an HTTP body read. Cancellation drops partial buffers → silent data loss or wire-protocol corruption. Move the call into a spawned task and select on its JoinHandle.
• [FILE:LINE] CRITICAL_WRITE_BEHIND_TIMEOUT — tokio::time::timeout(d, work) wraps a write/commit that cannot be undone (DB transaction, file write, network publish). On timeout the work is mid-flight and dropped. Spawn the work and timeout the JoinHandle instead.
• [FILE:LINE] NON_SEND_LOCAL_ACROSS_AWAIT — A Rc<T>, RefCell borrow, raw pointer, thread-local handle, or std::sync::MutexGuard is held across an .await. The future becomes !Send and cannot be spawned on a multi-thread runtime. Scope the local so it drops before the await.
• [FILE:LINE] PARKING_LOT_GUARD_ACROSS_AWAIT — A parking_lot::MutexGuard (which is Send) is held across .await. Compiler does NOT reject the spawn; logic still deadlocks if the resumed task tries to re-acquire the same lock on a different worker thread. Flag manually.
• [FILE:LINE] LIBRARY_HARDCODES_TOKIO_REACTOR — A crate's public async fn API internally calls tokio::net::*, tokio::time::sleep, or tokio::spawn without declaring tokio as a hard requirement or gating it behind a feature. Callers on smol/async-std get a runtime-mismatch panic. Either document loudly or abstract over AsyncRead + AsyncWrite.
• [FILE:LINE] INTERNAL_SPAWN_IN_LIBRARY — Library code calls tokio::spawn internally, locking callers into tokio and stealing task ownership from the caller's supervisor. Expose impl Future and let the caller spawn.

Cross-References

• concurrency-primitives.md — Arc, atomic ordering, and the synchronization primitives that underpin Waker storage and cross-task handoff.
• concurrency-models.md — choosing between actor/supervisor models, channel-driven concurrency, and shared-state mutexes for cancel-safe designs.
• ../../tokio-async-code-review/references/pinning-cancellation.md — tokio-specific cancellation primitives (CancellationToken, JoinSet, select! semantics, spawn_blocking bridge).

Unsafe Code, API Design, and Derive Patterns

For performance, pointer types, clippy config, iterators, generics, and documentation guidance, see the rust-best-practices skill.

Unsafe Code

Missing Safety Comments

Every unsafe block must explain why the invariants are upheld. This isn't a style preference — it's how future maintainers verify the code is correct.

// BAD - no justification
let value = unsafe { &*ptr };

// GOOD - documents the invariant
// SAFETY: `ptr` was allocated by `Box::into_raw` in `new()` and
// is guaranteed to be valid until `drop()` is called. We hold &self,
// which prevents concurrent mutation.
let value = unsafe { &*ptr };

Unsafe Operations in unsafe fn (Edition 2024)

In edition 2024, unsafe_op_in_unsafe_fn is deny by default. Being inside an unsafe fn no longer implicitly permits unsafe operations — each one needs its own unsafe {} block with a safety comment.

// BAD in edition 2024 — unsafe ops without explicit blocks
unsafe fn process_raw(ptr: *const u8, len: usize) -> &[u8] {
    std::slice::from_raw_parts(ptr, len) // ERROR: requires unsafe block
}

// GOOD — explicit unsafe block inside unsafe fn
unsafe fn process_raw(ptr: *const u8, len: usize) -> &[u8] {
    // SAFETY: caller guarantees ptr is valid for len bytes and
    // the resulting slice does not outlive the allocation.
    unsafe { std::slice::from_raw_parts(ptr, len) }
}

This makes unsafe fn bodies auditable at the same granularity as regular functions. Every unsafe operation gets its own safety justification.

unsafe extern Blocks (Edition 2024)

In edition 2024, extern blocks must be marked unsafe because declaring foreign functions is inherently unsafe (the compiler cannot verify the signatures are correct).

// BAD in edition 2024
extern "C" {
    fn strlen(s: *const c_char) -> usize;
}

// GOOD in edition 2024
unsafe extern "C" {
    fn strlen(s: *const c_char) -> usize;
}

unsafe Attributes (Edition 2024)

Attributes that affect ABI or symbol names are now safety-sensitive and must be wrapped in unsafe(...):

// BAD in edition 2024
#[no_mangle]
pub extern "C" fn my_func() {}

#[export_name = "custom_name"]
pub fn another_func() {}

// GOOD in edition 2024
#[unsafe(no_mangle)]
pub extern "C" fn my_func() {}

#[unsafe(export_name = "custom_name")]
pub fn another_func() {}

Overly Broad Unsafe Blocks

Only the minimum necessary code should be inside unsafe. Surrounding safe code makes it harder to audit.

// BAD - safe operations inside unsafe block
unsafe {
    let len = data.len();          // safe
    let ptr = data.as_ptr();       // safe
    std::slice::from_raw_parts(ptr, len)  // this is the only unsafe part
}

// GOOD - narrow unsafe boundary
let len = data.len();
let ptr = data.as_ptr();
// SAFETY: ptr and len come from the same slice, which is still alive
unsafe { std::slice::from_raw_parts(ptr, len) }

API Design

Non-Exhaustive Enums

Public enums should be #[non_exhaustive] if variants may be added in the future. Without it, adding a variant is a breaking change.

// GOOD - allows adding variants without breaking downstream
#[derive(Debug)]
#[non_exhaustive]
pub enum Status {
    Pending,
    Active,
    Complete,
}

Builder Pattern

For types with many optional fields, builders prevent argument confusion and allow incremental construction.

// Builder takes ownership for chaining
pub struct ServerBuilder {
    port: u16,
    host: String,
    workers: Option<usize>,
}

impl ServerBuilder {
    pub fn new(port: u16) -> Self {
        Self { port, host: "0.0.0.0".into(), workers: None }
    }

    pub fn host(mut self, host: impl Into<String>) -> Self {
        self.host = host.into();
        self
    }

    pub fn workers(mut self, n: usize) -> Self {
        self.workers = Some(n);
        self
    }

    pub fn build(self) -> Result<Server, Error> { ... }
}

Clippy Patterns Worth Flagging

These are patterns that clippy warns about but are easy to miss during review:

• manual_map — match arms that just wrap in Some/Ok; use .map() instead
• needless_borrow — & on values that already implement the trait for references
• redundant_closure — closures that just call a function: |x| foo(x) -> foo
• single_match — match with one arm + wildcard; use if let instead
• or_fun_call — .unwrap_or(Vec::new()) allocates even on the happy path; use .unwrap_or_default()

#[expect] Over #[allow]

#[expect(clippy::lint)] warns when the suppression is no longer needed. #[allow] stays forever unnoticed:

// BAD - stale suppression goes undetected
#[allow(clippy::large_enum_variant)]
enum Message { /* ... */ }

// GOOD - compiler warns when lint no longer triggers
// Justification: Content variant intentionally large for fast matching
#[expect(clippy::large_enum_variant)]
enum Message { /* ... */ }

Always add a justification comment when suppressing lints.

Derive Macro Guidelines

LazyCell / LazyLock (Stable Since 1.80)

std::cell::LazyCell and std::sync::LazyLock replace the once_cell and lazy_static crates for lazy initialization. Prefer the std types in new code.

// BAD — external dependency for something std now provides
use once_cell::sync::Lazy;
static CONFIG: Lazy<Config> = Lazy::new(|| load_config());

// BAD — macro-based, no longer needed
lazy_static::lazy_static! {
    static ref CONFIG: Config = load_config();
}

// GOOD — std::sync::LazyLock for thread-safe global lazy init
use std::sync::LazyLock;
static CONFIG: LazyLock<Config> = LazyLock::new(|| load_config());

// GOOD — std::cell::LazyCell for single-threaded lazy init
use std::cell::LazyCell;
let value: LazyCell<String> = LazyCell::new(|| expensive_compute());

When to flag: New code (or code with MSRV >= 1.80) using once_cell or lazy_static when LazyCell/LazyLock would work. Existing code using these crates is fine if the MSRV prevents migration.

Review Questions

1. Does every unsafe block have a safety comment? 2. Are unsafe blocks as narrow as possible? 3. Are public enums #[non_exhaustive] if they may grow? 4. Are appropriate derive macros present for each type's usage? 5. Is #[expect] used instead of #[allow] for lint suppression? 6. Would clippy flag any of these patterns? 7. Are builders used for types with many optional fields? 8. Edition 2024: Do unsafe fn bodies use explicit unsafe {} blocks? 9. Edition 2024: Are extern blocks marked unsafe extern? 10. Edition 2024: Are #[no_mangle] / #[export_name] wrapped in #[unsafe(...)]? 11. Is LazyLock/LazyCell used instead of once_cell/lazy_static when MSRV allows?

Concurrency Models

Concurrency is a design decision, not a tactical choice. Pick the model — shared memory, worker pool, or actor — before reaching for primitives, then let the model dictate which primitives belong. Jon Gjengset's framing: most concurrency bugs come from accidentally mixing two models in one design. This file covers model selection and the review hazards specific to each. For primitive-level review (Mutex vs RwLock, atomics, UnsafeCell, OnceLock/LazyLock), see concurrency-primitives.md. For ordering semantics, see memory-ordering.md. For async-specific patterns, see async-concurrency.md and ../../tokio-async-code-review/SKILL.md.

Shared memory model

Threads cooperate by reading and writing common memory protected by mutexes, RW-locks, or atomics.

Valid when: workload accesses small shared state from many threads (cache, counter, config, in-memory registry); critical sections are short (microseconds); updates are non-commutative (f(g(s)) != g(f(s))).

Dangerous when: the critical section holds I/O, network calls, allocator pressure, or unbounded work; a lock is held across .await on an async runtime (deadlocks the executor); one lock guards composite state where independent fields are touched by independent code paths.

Review questions: who holds the lock? for how long is it held in the worst case? is the critical section measured, or assumed-short? could this be an AtomicX, arc-swap, or a sharded design instead? Mutex (mutual exclusion, ~40 bytes on Linux), RwLock (read-heavy only — writer overhead exceeds Mutex if writes are frequent), or parking_lot variants (smaller, faster, fair) — which tradeoff is the code actually paying for?

The deadly shape in async code is a sync mutex held across .await:

// BAD: std::sync::Mutex guard alive across .await.
let mut g = state.lock().unwrap();
g.pending += 1;
let row = db.fetch(g.id).await?; // executor may suspend with guard held
g.cache.insert(row.key, row);

// GOOD: drop the guard first, or use tokio::sync::Mutex.
let id = { let g = state.lock().unwrap(); g.id };
let row = db.fetch(id).await?;
state.lock().unwrap().cache.insert(row.key, row);

Worker pool model

N identical threads (or tasks) pull jobs from a shared queue and execute them independently. Every worker runs the same code; jobs differ only in data. Web servers, rayon parallel iterators, tokio multi-threaded runtime, DB connection pools.

Valid when: CPU-bound work that partitions cleanly (rayon, crossbeam-channel + threads, tokio::task::spawn_blocking); I/O multiplexing on a fixed pool; resource pooling (DB connections, HTTP clients) with N established handles.

Dangerous when: the queue is unbounded (producer outruns workers → OOM); tasks hold per-thread state that breaks when reassigned (rayon work-stealing moves tasks between threads); connection pool sized smaller than concurrent users without explicit backpressure plan; worker code branches on thread::current().id() or a worker index (that is an actor-shaped design wearing a pool's clothes); connections returned with open transactions, session variables, or warm prepared-statement caches.

Review questions: pool size and how it was chosen? queue bounded? backpressure strategy when full (block, drop, error)? does any task hold thread-local state? are pooled resources reset between borrows? is the queue itself a single MPSC bottleneck (consider work-stealing deques per worker)?

Actor model

N independent inboxes, one per "topic" of state. Each actor owns its data exclusively; communication is messages. No locks needed inside an actor — exclusivity is structural.

Valid when: state is naturally encapsulated per-thing (per-connection, per-user, per-document, per-device-driver); messages are small and infrequent; cross-actor coordination is rare. Often combined with a worker pool: each actor maps to a tokio task, the pool polls tasks, exclusivity holds because only one task body runs at a time.

Dangerous when: message rate is high (the channel becomes the bottleneck); actors send to each other in cycles (deadlock or unbounded mailbox growth); one actor grows hot and serializes the entire workload (cannot parallelize within an actor); the design re-implements an actor framework (Actix and friends) for state where plain shared memory would have been simpler; cross-actor invariants are required (no atomicity guarantee across actor boundaries).

Canonical pattern: mpsc::channel to the actor for commands, oneshot::Sender embedded in the message for the reply. Rich enum Msg { Get(K, oneshot::Sender<V>), Set(K, V) } reads clearly; Box<dyn FnOnce()> hides what the actor does.

enum Msg { Get(Key, oneshot::Sender<Val>), Set(Key, Val) }

async fn actor(mut rx: mpsc::Receiver<Msg>) {
    let mut state = HashMap::new();
    while let Some(msg) = rx.recv().await {
        match msg {
            Msg::Get(k, tx) => { let _ = tx.send(state.get(&k).cloned().unwrap_or_default()); }
            Msg::Set(k, v) => { state.insert(k, v); }
        }
    }
}

Async vs threads vs hybrid

The decision tree:

• Mostly I/O-bound, many concurrent waits → async (per-task overhead in bytes, not KB).
• Mostly CPU-bound, parallel speedup needed → threads or rayon.
• Mixed (I/O handler that occasionally crunches data) → async runtime + tokio::task::spawn_blocking for the CPU section.

Async is not parallelism. async fn on a single-threaded runtime gives interleaving, not speedup. tokio::join! polls all its futures on the same task — concurrent on one thread, never parallel. Only tokio::spawn (or equivalent) hands a future to the runtime as a distinct task that the multi-threaded runtime can poll on another worker. Two conditions for actual parallelism: (1) the future is Send, and (2) the work is split into multiple spawned tasks.

// BAD: concurrent on one task, never parallel.
let (a, b) = tokio::join!(crunch(x), crunch(y));

// GOOD: each task can land on a separate worker.
let ha = tokio::spawn(crunch(x));
let hb = tokio::spawn(crunch(y));
let (a, b) = (ha.await?, hb.await?);

Data race vs race condition

These are not synonyms.

• Data race = two threads access the same memory, at least one writes, with no synchronization. Always UB in Rust. The borrow checker prevents data races in safe code.
• Race condition = observable behavior depends on thread scheduling. Not UB, not always a bug, but a frequent source of business-logic defects. Safe code can still have race conditions.

A Mutex resolves data races (synchronization established) but not race conditions (logic still depends on order of locking). The classic TOCTOU shape: if map.lock().contains_key(&k) { map.lock().get(&k).unwrap() } — the lock is released between the check and the use; another thread may remove k in the window. Use entry() to fuse check-and-act under one lock.

CAS as hardware mutex

compare_exchange on a single atomic location looks lock-free, but under MESI the cache line still requires exclusive ownership for the duration of the CAS. Under N-CPU contention on one hot atomic, coordination scales quadratically in N. Splitting one contended atomic into N less-contended atomics is almost always a better optimization than tuning the ordering.

fetch_add, fetch_sub, fetch_and, fetch_or are preferable to compare_exchange when the operation commutes. They run unconditionally, no retry loop, and have dedicated hardware fast paths (LDADD on ARM, LOCK XADD on x86). fetch_update is not in this family — it is implemented as a compare_exchange_weak loop. Review questions: is this CAS loop on a hot atomic? would fetch_add/fetch_or do? could this counter be sharded per-thread with periodic summation?

The println! heisenbug

Stdout holds a Mutex. Adding println! to investigate a race changes the race — the print's lock and milliseconds-scale latency dwarf the race window. Reviewer instinct: when a flaky test "fixes itself" once dbg! or println! is added, the bug is still there, just hidden. Use a per-thread in-memory log printed after the bug triggers, tracing with a low-overhead subscriber, rr on Linux, or loom/TSan, all of which observe without reshaping timing.

Review checks

• [FILE:LINE] WRONG_MODEL_FOR_WORKLOAD — shared-memory Mutex<HashMap> used where each entry is independent and naturally per-actor. Convert to actor-per-key or DashMap.
• [FILE:LINE] LOCK_ACROSS_AWAIT — std::sync::Mutex guard alive at a .await point. Drop guard before await, or switch to tokio::sync::Mutex.
• [FILE:LINE] LOCK_WRAPS_IO — critical section holds the lock across a network/DB/filesystem call. Restructure to release before I/O, or stage I/O outside the lock.
• [FILE:LINE] LOCK_GUARDS_INDEPENDENT_STATE — single Mutex wraps a struct whose fields are touched by independent code paths. Split or shard.
• [FILE:LINE] UNBOUNDED_WORKER_QUEUE — mpsc::unbounded_channel() or VecDeque feeding a worker pool with no backpressure. Bound it and define overflow behavior.
• [FILE:LINE] WORKER_POOL_BRANCHES_ON_TID — worker code selects behavior on thread::current().id() or worker index. The actor model fits; convert.
• [FILE:LINE] CONNECTION_POOL_NO_RESET — pooled connection returned without resetting transaction state, session variables, or prepared-statement cache.
• [FILE:LINE] CONNECTION_POOL_UNDERSIZED — pool size < expected concurrent users with no documented backpressure or queue-wait budget.
• [FILE:LINE] UNBOUNDED_ACTOR_MAILBOX — mpsc::unbounded_channel() feeding an actor with no backpressure. Bound it or document why infinite buffering is safe.
• [FILE:LINE] ACTOR_HOLDS_AWAIT_ON_SHARED — actor task awaits external I/O while exclusively holding state others want to query. Spawn the I/O as a sub-task and return a future, or shard the actor.
• [FILE:LINE] ACTOR_CYCLIC_SEND — actor A sends to B which sends back to A on the same call path. Deadlock or unbounded queue growth; restructure to one-way or use oneshot reply channels.
• [FILE:LINE] JOIN_INSTEAD_OF_SPAWN_FOR_PARALLELISM — tokio::join!(a, b) where the intent was parallelism. join! polls both on one task; use tokio::spawn for each then await the handles.
• [FILE:LINE] CHECK_THEN_ACT_ON_LOCKED_MAP — if !map.lock().contains_key(k) { map.lock().insert(k, v); }. Lock dropped between read and write. Use entry.
• [FILE:LINE] CAS_LOOP_FOR_COMMUTATIVE_OP — compare_exchange_weak loop computing a commutative op (counter, OR-mask of flags). Replace with fetch_add/fetch_or.
• [FILE:LINE] FETCH_UPDATE_AS_FAST_PATH — fetch_update used in hot-path code under the assumption it is a hardware fast path. It is a CAS loop. If the op commutes, use fetch_add/fetch_or.
• [FILE:LINE] CONCURRENCY_DEBUG_VIA_PRINTLN — println! added to investigate a race. The print's Stdout mutex reshapes timing and may mask the bug. Use in-memory logging, tracing, rr, or loom.

Anti-patterns

• Mixing models in one design. A worker pool whose tasks reach into shared mutable state without going through the queue. An actor that also holds a Mutex "for the hot path." Pick one model per subsystem and stay inside it.
• One giant `Future` that you expect to run in parallel. Without spawn, it is one task; the runtime cannot poll one future from two threads.
• `std::sync::Mutex` in async code held across `.await`. Either use the async mutex or drop the guard before the await point.
• One `Mutex` over composite state. Almost always becomes a contention bottleneck. Split by sub-state or shard by key.
• Unbounded channels as the default. Lose backpressure and produce silent OOM under load. Bound everything; raise the bound after measurement.
• Actor for everything. When messages are high-rate and state is small, the channel becomes the bottleneck. Shared memory with a short critical section is faster.
• Lock-free for the sake of lock-free. Hand-rolled atomic algorithms usually scale worse than a well-placed Mutex until benchmarks prove otherwise. Start with locks; optimize on evidence.
• Adding `println!` to debug races. Stdout's lock and latency change the very interleavings being investigated.

Concurrency Primitives

For async-specific patterns (tokio, channels, cancellation), see async-concurrency.md. For deep dives on Ordering and happens-before reasoning, see memory-ordering.md. For hand-rolled spinlocks, channels, and Arc, see lock-free-patterns.md.

Send and Sync Semantics

• `Send`: a type can be transferred to another thread. Most types are Send. Notable exceptions: Rc, MutexGuard (on some platforms).
• `Sync`: a type can be shared (via &T) between threads. T is Sync if &T is Send. Notable exceptions: Cell, RefCell, Rc.

Both are auto-traits: the compiler implements them if all fields are Send/Sync. Raw pointers block auto-implementation as a safety guard.

Manual Implementation

Flag when: unsafe impl Send or unsafe impl Sync appears without: 1. A safety comment explaining why the invariant holds 2. Bounds on generic parameters

// BAD — missing bound allows T: !Send to cross threads
unsafe impl<T> Send for MyWrapper<T> {}

// GOOD — bound ensures inner T is also Send
unsafe impl<T: Send> Send for MyWrapper<T> {}

Check for: types containing Rc, Cell, RefCell, or raw pointers that manually implement Send/Sync — these need extra scrutiny.

Why Rc is !Send + !Sync, but Arc is both

Rc<T> uses a non-atomic counter — two threads cloning concurrently would corrupt it. Arc<T> uses AtomicUsize for the strong/weak counts, so Arc<T>: Send + Sync when `T: Send + Sync`. Both bounds are required: sending an Arc clone to another thread implicitly shares &T (so T: Sync), and the receiver may drop the last reference (so T: Send). Arc<Cell<u32>> is rejected because Cell: !Sync.

Why MutexGuard<'_, T> is !Send

On platforms where std::sync::Mutex wraps pthread_mutex_t, POSIX requires the unlocking thread to be the locking thread. Rust encodes this by making MutexGuard: !Send. parking_lot::MutexGuard is !Send by default; opting into the send_guard feature drops priority inheritance on platforms that need it. Reviewers should not "fix" a !Send compile error by transmuting or wrapping the guard.

unsafe impl for wrappers around UnsafeCell

When you wrap Arc<UnsafeCell<T>> or similar to share interior-mutable data, both bounds matter:

struct Shared<T> { inner: Arc<UnsafeCell<T>> }
// SAFETY: All access to T is serialized through the spin-lock in `inner`.
// T: Send is required because ownership crosses threads via the lock handoff.
unsafe impl<T: Send> Sync for Shared<T> {}
unsafe impl<T: Send> Send for Shared<T> {}

Note T: Send (not T: Sync) is the typical bound for a lock-like wrapper: only one thread accesses T at a time, but ownership transfers across threads. A wrapper that exposes &T concurrently (e.g., an RwLock reader) needs T: Sync.

Opting out of auto traits

Use PhantomData<*const ()> to make a type !Send + !Sync (raw pointers are !Send + !Sync, and PhantomData propagates). Use PhantomData<Cell<()>> to keep Send but drop Sync. Reviewers should flag PhantomData<T> used purely as an auto-trait switch when the type does not actually own a T — preferred forms above are clearer.

• [FILE:LINE] UNSAFE_SEND_SYNC_WITHOUT_SAFETY_COMMENT — unsafe impl (Send|Sync) with no preceding // SAFETY: justification.
• [FILE:LINE] UNSAFE_SEND_SYNC_MISSING_GENERIC_BOUND — unsafe impl<T> Send for W<T> {} with no bound; almost always wrong.
• [FILE:LINE] PHANTOMDATA_AUTO_TRAIT_HACK — PhantomData<T> used to block Send/Sync when the type doesn't own a T; prefer PhantomData<*const ()> or PhantomData<Cell<()>>.

Atomics and Memory Ordering

Atomic types (AtomicBool, AtomicUsize, AtomicPtr, etc.) provide lock-free concurrent access. Every operation takes an Ordering argument.

Ordering Guide

Common Patterns

Acquire/Release pair (most common for synchronization):

// Writer thread
data.store(42, Ordering::Relaxed);
flag.store(true, Ordering::Release); // all prior stores visible to Acquire readers

// Reader thread
if flag.load(Ordering::Acquire) {
    // guaranteed to see data == 42
    let val = data.load(Ordering::Relaxed);
}

Flag when:

• Relaxed used where the value gates access to other shared data — needs at least Acquire/Release
• SeqCst used everywhere "to be safe" — this is correct but may be unnecessarily costly on non-x86 architectures. Flag as informational if Acquire/Release would suffice.
• compare_exchange success ordering is Relaxed when it guards a critical section — needs AcqRel

Valid pattern: Relaxed for metrics counters, reference counts (when paired with Acquire on the final decrement), and statistics.

UnsafeCell and Interior Mutability Invariants

UnsafeCell<T> is the only legitimate primitive that lets safe code obtain &mut T from &T. Every interior-mutability type in std (Cell, RefCell, Mutex, RwLock, OnceLock, atomics) is built on it. The rule: a shared reference to an UnsafeCell is the only way to obtain a mutable raw pointer to data behind a shared reference. Any wrapper that hands out &mut T from &T without going through UnsafeCell::get() is unsound and violates the aliasing rules Miri checks.

use std::cell::UnsafeCell;

pub struct OneShot<T> {
    cell: UnsafeCell<Option<T>>,
}
// SAFETY: callers must serialize access externally (e.g., via an AtomicBool).
unsafe impl<T: Send> Sync for OneShot<T> {}

impl<T> OneShot<T> {
    pub fn set(&self, value: T) {
        // Caller has proven exclusive write access.
        unsafe { *self.cell.get() = Some(value) };
    }
}

A public type containing UnsafeCell<T> must have an unsafe impl Sync (or be explicitly !Sync) with a written safety argument; the auto-trait default would leave the type !Sync for the wrong reason.

• [FILE:LINE] UNSAFECELL_NO_SYNC_RATIONALE — public type holds UnsafeCell<T> with unsafe impl Sync but no // SAFETY: comment describing the access-serialization scheme.
• [FILE:LINE] UNSAFECELL_GET_MUT_ALIASING — &mut *cell.get() produced while another &T or &mut T derived from the same cell is live; flag any place where two such pointers' lifetimes overlap.
• [FILE:LINE] PUB_UNSAFECELL_FIELD — pub field of type UnsafeCell<T> on an otherwise safe API; external callers can mint aliasing pointers.

MaybeUninit and Atomic Initialization Patterns

MaybeUninit<T> represents possibly-uninitialized storage. The "atomic initialization" pattern uses an AtomicBool (or AtomicU8 with multiple states) to gate access to a MaybeUninit<T> until it has been written:

use std::cell::UnsafeCell;
use std::mem::MaybeUninit;
use std::sync::atomic::{AtomicBool, Ordering::{Acquire, Release}};

pub struct LazyInit<T> {
    value: UnsafeCell<MaybeUninit<T>>,
    ready: AtomicBool,
}

impl<T> LazyInit<T> {
    pub fn set(&self, v: T) {
        unsafe { (*self.value.get()).write(v) };
        self.ready.store(true, Release); // publishes the write above
    }
    pub fn get(&self) -> Option<&T> {
        if self.ready.load(Acquire) {
            // SAFETY: `ready == true` happens-after `set`'s write.
            Some(unsafe { (*self.value.get()).assume_init_ref() })
        } else { None }
    }
}

Pitfalls to flag:

• MaybeUninit::uninit().assume_init() called without ever writing the storage — UB even for Copy types when their bit pattern is invalid (bool, char, references, enums).
• assume_init_read called twice on the same MaybeUninit — double-drop or use-after-move; the value's destructor may already have run.
• A Drop impl that unconditionally calls assume_init_drop on a MaybeUninit<T> field — drops uninitialized memory when the gate flag is false. The correct shape is if *self.ready.get_mut() { unsafe { self.value.get_mut().assume_init_drop() } }.
• For lazy one-shot init, prefer OnceLock::get_or_init over hand-rolled MaybeUninit + AtomicBool unless allocation or const fn construction is a hard requirement.

• [FILE:LINE] MAYBEUNINIT_UNCONDITIONAL_DROP — assume_init_drop in a Drop impl without checking an init flag.
• [FILE:LINE] MAYBEUNINIT_DOUBLE_READ — two assume_init_read calls reachable on the same MaybeUninit storage.

Mutex and RwLock Poisoning

A Mutex<T> poisons when a thread panics while holding the guard. RwLock<T> poisons only when a write-guard holder panics; a panic with a read guard does not poison. Subsequent lock() / write() calls return Err(PoisonError<MutexGuard<T>>). PoisonError::into_inner() yields the (possibly inconsistent) guard so you can repair the data and continue.

std::sync::Mutex::clear_poison() and RwLock::clear_poison() (stable in 1.77) let code recover after repairing the invariant:

let guard = m.lock().unwrap_or_else(|mut e| {
    **e.get_mut() = T::default(); // restore the invariant
    m.clear_poison();
    e.into_inner()
});

When .unwrap() is fine vs. when it is not:

• Fine: the protected data has no multi-field invariant a partial update could break — e.g., a Mutex<Vec<Event>> where any prefix of events is valid.
• Flag: a Mutex<State> where State has fields that must agree (cached count vs. underlying collection length, two halves of a struct). Recommend documenting the rationale or using clear_poison with a repair step.

Important asymmetry: LazyLock<T, F> poisoning is unrecoverable. An init-closure panic propagates, and every subsequent access also panics with no clear_poison analogue. For fallible init in long-running services, use OnceLock::get_or_init (or the unstable get_or_try_init), which is not poisoned by a panicking closure — the next caller may retry.

is_poisoned() is advisory only: it races against other threads and can be stale by the time you act on it. Branching on is_poisoned() to decide control flow is almost always wrong; act on the PoisonError returned by lock() directly.

• [FILE:LINE] UNWRAP_ON_LOCK_DROPS_RECOVERY — lock().unwrap() on data with a multi-field invariant and no rationale comment. Recommend unwrap_or_else(|e| { repair; clear_poison(); e.into_inner() }).
• [FILE:LINE] BRANCH_ON_IS_POISONED — control flow branches on mutex.is_poisoned(); race-prone. Act on lock()'s PoisonError instead.
• [FILE:LINE] LAZYLOCK_FALLIBLE_INIT — LazyLock::new(|| ...) with an initializer that can panic (network calls, parsing user input). One panic permanently breaks the cell. Use OnceLock and explicit retry.

Mutex vs RwLock vs Atomics

Check for:

• RwLock where writes are frequent — reader/writer lock overhead may exceed a simple Mutex
• Mutex protecting a single integer — an atomic is simpler and lock-free
• std::sync::Mutex in async code held across .await — use tokio::sync::Mutex instead (see async-concurrency.md)

Lock Ordering and Deadlock Prevention

Flag when: code acquires multiple locks without a documented ordering. Two threads acquiring locks in different orders will deadlock.

// DEADLOCK RISK — thread 1 locks A then B, thread 2 locks B then A
let _a = lock_a.lock().unwrap();
let _b = lock_b.lock().unwrap();

// SAFE — document and enforce a global lock ordering
// Rule: always acquire lock_a before lock_b

Prevention strategies:

• Global lock ordering: document which locks must be acquired first. Enforce in code review.
• Lock splitting: use finer-grained locks that are never held simultaneously
• Lock-free algorithms: avoid locks entirely with atomics and compare_exchange
• `try_lock` with backoff: detect contention and retry

Common Concurrency Bugs to Flag

1. Data races: mutation of non-atomic shared state without synchronization — always undefined behavior 2. Lock held across await: MutexGuard alive at .await point — see async-concurrency.md. Block-scope the guard or switch to tokio::sync::Mutex; drop(guard) before .await does not always convince the compiler the borrow region has closed. 3. Incorrect `Send`/`Sync`: manual implementations missing generic bounds 4. TOCTOU (time-of-check-to-time-of-use): checking a condition then acting on it without holding the lock 5. Forgetting to join spawned threads: fire-and-forget threads with thread::spawn may outlive the data they reference 6. `compare_exchange` without loop: CAS can spuriously fail on some architectures — use compare_exchange_weak in a loop for better performance 7. ABA on pointer-tagged CAS: between a load(A) and compare_exchange(A, B), the value goes A → X → A. CAS succeeds and the caller misses an intermediate state. When this is a real bug: lock-free stacks, queues, linked lists where A is a pointer to a freed-and-reused node — silent use-after-free or lost insertions. When it is benign: integer refcounts, ID counters, or anywhere the value identity is the value. Fix by using crossbeam-epoch, hazard pointers, or a packed (ptr, generation) AtomicU64. See lock-free-patterns.md. 8. "Out of thin air" panic-mongering: the formal C++/Rust memory model permits Relaxed reads to materialize circular-dependency values. No real compiler or CPU has ever exhibited this. Do not let it scare you off Relaxed for counters, statistics, or correctly-paired publish flags. The legitimate concerns with Relaxed are missing happens-before, not thin-air values. See memory-ordering.md for the full story.

// BAD — single compare_exchange may fail spuriously
let old = val.compare_exchange(0, 1, Ordering::AcqRel, Ordering::Relaxed);

// GOOD — loop for retry (unless you handle failure explicitly)
loop {
    match val.compare_exchange_weak(0, 1, Ordering::AcqRel, Ordering::Relaxed) {
        Ok(_) => break,
        Err(_) => continue,
    }
}

Atomic Types Survey

std::sync::atomic exposes one atomic per primitive width:

Target gating

Availability is gated by #[cfg(target_has_atomic = "8" | "16" | "32" | "64" | "ptr")]. Notable gaps:

• 32-bit PowerPC and MIPS: no AtomicU64.
• thumbv6m-* and thumbv8m.base-*: only load/store are available — no CAS, no fetch_*.

Portable code must #[cfg(target_has_atomic = "64")]-gate any 64-bit atomic use, with a fallback (typically Mutex<u64>).

Convenience methods worth knowing

• fetch_update(set_ordering, fetch_ordering, f) — built-in CAS loop. Replace hand-rolled loop { let cur = a.load(...); a.compare_exchange_weak(cur, f(cur), ...) } with it when f is pure.
• get_mut(&mut self) -> &mut T, into_inner(self) -> T — no ordering needed; the borrow checker proves exclusivity. Prefer these in constructors and &mut self methods over load(SeqCst).
• as_ptr() -> *mut T — raw pointer for FFI.

Nightly-only (do not flag absence)

• Atomic*::from_ptr(*mut T) (feature atomic_from_ptr) — view a raw pointer as an atomic.
• Atomic*::from_mut(&mut T) (feature atomic_from_mut) — view a unique borrow as an atomic.

Concurrent atomic / non-atomic accesses

Rust permits a Relaxed atomic load to race with a non-atomic read of the same address (this differs from C++). Concurrent atomic writes vs. non-atomic accesses of any kind, and mixed-size atomic accesses to overlapping memory, are UB. Atomic accesses to read-only memory (statics in .rodata) are UB except for "small" Relaxed loads (≤4 bytes on 32-bit, ≤8 bytes on 64-bit) — use load(Relaxed) followed by fence(Acquire) instead of load(Acquire) on a static.

• [FILE:LINE] ATOMIC_NO_TARGET_HAS_ATOMIC_CFG — AtomicU64 / AtomicI64 used in a library targeting no_std or embedded without a #[cfg(target_has_atomic = "64")] gate.
• [FILE:LINE] HAND_ROLLED_FETCH_UPDATE — loop { let cur = a.load(...); if a.compare_exchange_weak(cur, f(cur), ...).is_ok() { break; } } where fetch_update would express the same intent.

std::thread::scope for Bounded Thread Lifetimes

Scoped threads (stable since 1.63) borrow non-'static data safely by guaranteeing all threads join before the scope exits.

let mut data = vec![1, 2, 3];
std::thread::scope(|s| {
    s.spawn(|| {
        println!("{:?}", &data); // borrows data — no Arc needed
    });
    s.spawn(|| {
        println!("len: {}", data.len());
    });
}); // all threads joined here — data is safe to use again

Flag when: Arc<T> is used to share data with threads that are joined before the function returns — thread::scope is simpler and avoids the allocation.

OnceLock / LazyLock Patterns

OnceLock stable since 1.70, LazyLock stable since 1.80. Replace once_cell and lazy_static for new code.

use std::sync::{LazyLock, OnceLock};

// LazyLock: initialize with a closure, computed on first access
static CONFIG: LazyLock<Config> = LazyLock::new(|| load_config());

// OnceLock: initialize at runtime, set exactly once
static DB: OnceLock<Database> = OnceLock::new();
fn init_db(conn_str: &str) {
    DB.set(Database::connect(conn_str)).expect("DB already initialized");
}

Flag when: new code (MSRV >= 1.80) uses once_cell::sync::Lazy or lazy_static! — prefer LazyLock/OnceLock from std.

crossbeam and parking_lot Patterns

crossbeam

• crossbeam::channel: faster, more ergonomic channels than std::sync::mpsc. Supports select! over multiple channels.
• crossbeam::epoch: epoch-based memory reclamation for lock-free data structures
• crossbeam::utils::CachePadded: wraps a value to occupy a full cache line, preventing false sharing

Flag when: hot concurrent counters or flags are in adjacent memory without cache-line padding — likely false sharing.

parking_lot

• Drop-in replacements for std::sync::{Mutex, RwLock, Condvar, Once}
• Faster on contended workloads, smaller Mutex size (1 byte vs 40+ bytes on Linux)
• Provides MutexGuard::map for projecting through a lock

Valid pattern: parking_lot::Mutex over std::sync::Mutex when benchmarks show contention is a bottleneck.

Review Questions

1. Are Send/Sync implementations bounded on generic parameters? 2. Is the memory ordering for each atomic operation sufficient for its use case? 3. Are multiple locks acquired in a consistent, documented order? 4. Could thread::scope replace Arc for data shared with joined threads? 5. Are once_cell/lazy_static uses replaceable with OnceLock/LazyLock (MSRV >= 1.80)? 6. Is there false sharing risk from adjacent atomic values without cache-line padding? 7. Are compare_exchange operations in a retry loop when spurious failure is possible? 8. Does every UnsafeCell<T> field have an accompanying unsafe impl Sync (or explicit !Sync) with a written safety argument naming the access-serialization mechanism? 9. Does every MaybeUninit<T> field have a Drop impl that checks an init flag before calling assume_init_drop? 10. For each lock().unwrap(), is poisoning genuinely unrecoverable, or should the code repair the invariant and call clear_poison() (1.77+)? Is any LazyLock initializer fallible (a one-shot panic permanently breaks the cell)? 11. For each pointer CAS, is ABA addressed via crossbeam-epoch, hazard pointers, or a generation counter? Or is the data integer-like enough for ABA to be benign? 12. Is any AtomicU64 / AtomicI64 use gated on #[cfg(target_has_atomic = "64")] for portable / no_std crates?

Error Handling

Critical Anti-Patterns

1. Unwrap in Production Code

unwrap() and expect() panic on None/Err, crashing the program. They bypass the type system's error safety guarantees.

// BAD - panics on invalid input
fn parse_config(input: &str) -> Config {
    let value: Config = serde_json::from_str(input).unwrap();
    value
}

// GOOD - propagates error to caller
fn parse_config(input: &str) -> Result<Config, serde_json::Error> {
    serde_json::from_str(input)
}

unwrap() is acceptable in: tests, examples, and after a check that guarantees success (e.g., if option.is_some() { option.unwrap() } — though .unwrap() after match/if-let is cleaner).

2. Errors Without Context

Bare ? propagation loses the "what was being attempted" context, making debugging difficult.

// BAD - caller sees "file not found" with no context
fn load_config(path: &Path) -> Result<Config, Error> {
    let contents = std::fs::read_to_string(path)?;
    let config: Config = toml::from_str(&contents)?;
    Ok(config)
}

// GOOD - each operation adds context
fn load_config(path: &Path) -> Result<Config, Error> {
    let contents = std::fs::read_to_string(path)
        .map_err(|e| Error::ConfigRead { path: path.to_owned(), source: e })?;
    let config: Config = toml::from_str(&contents)
        .map_err(|e| Error::ConfigParse { path: path.to_owned(), source: e })?;
    Ok(config)
}

With anyhow, use .context() / .with_context():

use anyhow::Context;

fn load_config(path: &Path) -> anyhow::Result<Config> {
    let contents = std::fs::read_to_string(path)
        .with_context(|| format!("reading config from {}", path.display()))?;
    let config: Config = toml::from_str(&contents)
        .context("parsing config TOML")?;
    Ok(config)
}

3. Stringly-Typed Errors

Using String as an error type loses structured information and makes error matching impossible.

// BAD - callers can't match on error types
fn validate(input: &str) -> Result<(), String> {
    if input.is_empty() {
        return Err("input is empty".to_string());
    }
    Ok(())
}

// GOOD - structured error types
#[derive(Debug, thiserror::Error)]
pub enum ValidationError {
    #[error("input is empty")]
    Empty,
    #[error("input too long: {len} chars (max {max})")]
    TooLong { len: usize, max: usize },
}

fn validate(input: &str) -> Result<(), ValidationError> {
    if input.is_empty() {
        return Err(ValidationError::Empty);
    }
    Ok(())
}

4. Panic for Recoverable Errors

panic! should be reserved for unrecoverable states (violated invariants, programmer bugs). Expected failures like I/O errors, parse failures, or network issues should return Result.

// BAD
fn connect(url: &str) -> Connection {
    TcpStream::connect(url).unwrap_or_else(|e| panic!("connection failed: {e}"))
}

// GOOD
fn connect(url: &str) -> Result<Connection, ConnectionError> {
    let stream = TcpStream::connect(url)
        .map_err(|e| ConnectionError::TcpFailed { url: url.to_owned(), source: e })?;
    Ok(Connection::new(stream))
}

5. Swallowing Errors

Discarding errors silently makes failures invisible.

// BAD - error silently ignored
let _ = save_to_disk(&data);

// GOOD - log if you can't propagate
if let Err(e) = save_to_disk(&data) {
    tracing::error!(error = %e, "failed to save data to disk");
}

The exception: some errors are genuinely unactionable (e.g., write! to stderr, close() on a file you're done with). In those cases, let _ = with a brief comment is acceptable.

Let-Else for Early Returns

Rust's let-else pattern (stable since 1.65) is cleaner than match for early returns on failure:

// GOOD - flat, readable early return
let Ok(json) = serde_json::from_str(&input) else {
    return Err(MyError::InvalidJson);
};

// GOOD - continue/break in loops
for item in items {
    let Some(value) = item.value() else {
        continue;
    };
    process(value);
}

// Use if-let when the else branch needs computation
if let Some(result) = cache.get(&key) {
    return Ok(result.clone());
} else {
    let computed = expensive_compute(&key)?;
    cache.insert(key, computed.clone());
    return Ok(computed);
}

// NOTE (Edition 2024): Temporaries in if-let conditions are dropped
// at the end of the condition, not the end of the block. If the
// matched value borrows a temporary, bind it explicitly first.
// See ownership-borrowing.md for details.

Prevent Early Allocation

Use _else variants when the fallback involves allocation or computation:

// BAD - format! runs even when x is Some
let val = x.ok_or(ParseError::Missing(format!("key {key}")));

// GOOD - closure only runs on None
let val = x.ok_or_else(|| ParseError::Missing(format!("key {key}")));

// BAD - Vec::new() allocates even on Ok path
let items = result.unwrap_or(Vec::new());

// GOOD - use unwrap_or_default for Default types
let items = result.unwrap_or_default();

Logging and Transforming Errors

Use inspect_err to log and map_err to transform errors in a chain:

let result = do_something()
    .inspect_err(|err| tracing::error!("do_something failed: {err}"))
    .map_err(|err| AppError::from(("do_something", err)))?;

Custom Error Structs

When a module has only one error type, a struct is simpler than an enum:

#[derive(Debug, thiserror::Error, PartialEq)]
#[error("Request failed with code `{code}`: {message}")]
struct HttpError {
    code: u16,
    message: String,
}

Async Error Bounds

Errors in async code must be Send + Sync + 'static for spawned tasks:

// Ensure error types work across await boundaries
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
    Ok(())
}

Avoid Box<dyn std::error::Error> (without Send + Sync) in libraries.

Panic Alternatives

Prefer these over panic! for expected incomplete code:

thiserror Patterns

thiserror generates Display and Error implementations from derive macros. It's the standard choice for library error types.

#[derive(Debug, thiserror::Error)]
pub enum Error {
    // Transparent: delegates Display and source() to inner error
    #[error(transparent)]
    Io(#[from] std::io::Error),

    // Structured: carries context alongside the cause
    #[error("failed to parse config at {path}")]
    ConfigParse {
        path: PathBuf,
        #[source]
        source: toml::de::Error,
    },

    // Simple: no underlying cause
    #[error("workflow not found: {0}")]
    NotFound(Uuid),

    // Multiple sources via transparent wrapping
    #[error(transparent)]
    Database(#[from] sqlx::Error),
}

Hierarchical errors: subsystem error types wrap into a top-level error via #[from]:

#[derive(Debug, thiserror::Error)]
pub enum AppError {
    #[error(transparent)]
    Workflow(#[from] WorkflowError),
    #[error(transparent)]
    Driver(#[from] DriverError),
}

Result Type Alias Pattern

Crates commonly define a local Result alias to reduce boilerplate:

pub type Result<T> = std::result::Result<T, Error>;

// Now functions in this module just use:
pub fn load(path: &Path) -> Result<Config> { ... }

Option Handling

Option<T> represents absence, not failure. Converting between Option and Result should be explicit about what "missing" means:

// BAD - ok_or with allocated string
let user = users.get(id).ok_or("user not found".to_string())?;

// GOOD - specific error type
let user = users.get(id).ok_or(Error::NotFound(id))?;

// GOOD - ok_or_else for expensive error construction
let user = users.get(id).ok_or_else(|| Error::NotFound(id))?;

Error Trait Implementation Rules

Custom error types should implement the full Error contract for ecosystem compatibility:

1. `Error` trait: implement std::error::Error 2. `Display`: one-line, lowercase, no trailing punctuation — fits into larger error reports 3. `Debug`: usually #[derive(Debug)] is sufficient; include auxiliary info (ports, paths, request IDs) 4. `Send + Sync`: required for multithreaded contexts and std::io::Error wrapping 5. `'static`: enables downcasting and easy propagation up the call stack

#[derive(Debug)]
pub struct DecodeError {
    offset: usize,
    kind: DecodeErrorKind,
}

impl std::fmt::Display for DecodeError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        // lowercase, no trailing punctuation
        write!(f, "decode failed at offset {}: {}", self.offset, self.kind)
    }
}

impl std::error::Error for DecodeError {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        self.kind.source()
    }
}

Flag when: a custom error type is missing Display, Debug, or Error implementations. With thiserror, these are derived automatically.

Enumerated vs Opaque Error Strategy

Choose between enumerated and opaque errors based on whether callers need to distinguish error cases:

Flag when:

• A library exposes Box<dyn Error> when callers demonstrably need to match on specific error cases
• An error enum has 20+ variants that callers never match on — consider an opaque wrapper to simplify the API

Error Chain Traversal with source()

Error::source() provides the underlying cause, enabling error chain traversal for backtraces and diagnostics.

fn print_error_chain(err: &dyn std::error::Error) {
    let mut current = Some(err);
    while let Some(e) = current {
        eprintln!("  caused by: {e}");
        current = e.source();
    }
}

Check for: error types that wrap an inner error but don't implement source() — the chain breaks and root cause is hidden.

With thiserror, #[source] and #[from] attributes handle this automatically:

#[derive(Debug, thiserror::Error)]
pub enum AppError {
    #[error("database query failed")]
    Database {
        #[source]  // wires up Error::source()
        source: sqlx::Error,
    },
}

Type-Erased Error Composition

Box<dyn Error + Send + Sync + 'static> enables heterogeneous error handling in applications:

fn process() -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> {
    let data = std::fs::read_to_string("input.txt")?; // io::Error
    let parsed: Config = toml::from_str(&data)?;       // toml::de::Error
    Ok(())
}

Check for: Box<dyn Error> (without Send + Sync) in library code — this prevents use in multithreaded contexts. Always prefer Box<dyn Error + Send + Sync + 'static> or a concrete type.

Note: Box<dyn Error + Send + Sync + 'static> itself does not implement Error. If you need a type-erased error that also implements Error, define a wrapper type or use anyhow::Error.

Downcasting with Error::downcast_ref()

Downcasting recovers the concrete error type from a dyn Error. Requires the 'static bound.

fn handle_error(err: &(dyn std::error::Error + 'static)) {
    if let Some(io_err) = err.downcast_ref::<std::io::Error>() {
        if io_err.kind() == std::io::ErrorKind::WouldBlock {
            // handle non-blocking retry
            return;
        }
    }
    // generic error handling
    eprintln!("error: {err}");
}

Flag when: error types are not 'static — this prevents downcasting and limits composability. Avoid placing non-static references in error types unless strictly necessary.

From Implementations for ? Ergonomics

The ? operator uses From to convert between error types. Implementing From<SourceError> for MyError enables seamless ? propagation.

// Manual From implementation
impl From<std::io::Error> for AppError {
    fn from(err: std::io::Error) -> Self {
        AppError::Io(err)
    }
}

// With thiserror — #[from] generates the From impl
#[derive(Debug, thiserror::Error)]
pub enum AppError {
    #[error(transparent)]
    Io(#[from] std::io::Error),
}

Check for:

• Missing From implementations causing verbose .map_err() chains when ? would suffice
• Implement From, not Into — the ? operator uses From internally
• Conflicting #[from] attributes: two variants with #[from] for the same source type won't compile

Try Blocks for Scoped Error Handling

Try blocks (still unstable as of Edition 2024, behind #![feature(try_blocks)]) scope ? to a block instead of the entire function:

#![feature(try_blocks)]

fn do_work() -> Result<(), Error> {
    let resource = Resource::acquire()?;
    let result: Result<(), Error> = try {
        step_one(&resource)?;
        step_two(&resource)?;
    };
    resource.cleanup(); // always runs, even if steps failed
    result
}

Check for: functions that need cleanup before returning errors — try blocks avoid the pattern of manually catching and re-raising. Until stabilized, the drop-guard or RAII pattern is the stable alternative.

Opaque vs Enumerated Errors

Jon Gjengset frames this as a binary choice driven by caller need, not author convenience: will the caller branch on the variant, or only log and propagate?

• Enumerated (pub enum Error { ... }) — caller takes different action per cause. Each variant should carry the underlying cause (e.g. In(io::Error) and Out(io::Error) are two io::Errors but the discriminant tells the caller which stream failed).
• Opaque — caller will only log/propagate. Two flavors: a struct with private fields and limited accessors (when bounded info is still useful — offset, status code), or Box<dyn Error + Send + Sync + 'static> (full erasure).

// Enumerated: callers branch on this
pub enum CopyError {
    In(std::io::Error),
    Out(std::io::Error),
}

// Opaque wrapper: callers cannot match, but can downcast if documented
pub struct MyError(Box<dyn std::error::Error + Send + Sync + 'static>);

Tradeoffs: enum locks in your variant set as a public API (adding a non-#[non_exhaustive] variant is a breaking change), but enables exhaustive pattern matching. Opaque keeps the variant set internal but forfeits pattern matching — callers can only inspect via source() or documented downcasts. See interface-design.md for #[non_exhaustive] rules on public error enums.

Flag:

• [FILE:LINE] ENUM_VARIANTS_DROP_CAUSE — error enum variants like In / Out with no inner io::Error — wraps lose the only useful information.
• [FILE:LINE] OPAQUE_HIDES_BRANCH_INFO — opaque error hides info callers demonstrably branch on (retryable vs fatal) — promote to enum or expose an inspector.

The Custom Error Trait Set

Any custom error type that ships in a public API should satisfy Error + Display + Debug + Send + Sync + 'static. Each bound is load-bearing:

• `Error` — source() is how the chain-traversal machinery walks to the root cause.
• `Display` — one-line, lowercase, no trailing punctuation, so it composes into larger error reports.
• `Debug` — carries auxiliary diagnostic info (ports, paths, request IDs) that doesn't belong in Display.
• `Send + Sync` — required in async/multithreaded code; without it the error can't cross an await or a spawn, and it won't compose with std::io::Error cleanly.
• `'static` — required by dyn Error for downcasting (Any::type_id vtable comparison) and to avoid lifetime contamination across the call stack.

// Audit fields: Rc<...>, RefCell<...>, raw pointers, or borrowed refs
// break Send/Sync/'static respectively.
#[derive(Debug, thiserror::Error)]
#[error("decode failed at offset {offset}")]
pub struct DecodeError { offset: usize, source_id: u64 }

Flag:

• [FILE:LINE] ERROR_FIELD_NOT_SEND — error type contains Rc<T> or *const T — breaks Send + Sync. Switch to Arc or owned data.
• [FILE:LINE] ERROR_BORROWS_INPUT — error type holds &'a str from request data — blocks 'static, prevents downcasting and propagation. Own the data (String).

Special Error Cases

Jon calls out four special cases worth flagging in review:

• `Result<T, ()>` is usually wrong. () doesn't implement Error, so the value can't be type-erased to Box<dyn Error> and is painful with ?. If the failure carries no detail, switch to Option<T> (which says "nothing to return, no handling required"). If you must keep Result semantics for #[must_use], define a unit struct that implements Error.
• `std::thread::Result<T>` is `Result<T, Box<dyn Any + Send + 'static>>` — note Any, not Error. The Err value is whatever was passed to panic!, often a &'static str or String. Calling .source() or Display on it is wrong; either downcast to a known payload type, propagate it via resume_unwind, or just .unwrap().
• The never type `!` / `Infallible` for errors that cannot occur. Result<T, Infallible> lets you match a trait signature without panicking on unwrap — the compiler knows no Err value can ever be constructed.
• Boxing on the hot path has allocation cost. Errors are rare, so boxing a large variant keeps Result<T, E> small — but a fallible function called in a tight loop will pay the allocation cost on every error. Use Cow<'static, str> or fixed-shape errors for hot loops.

Flag:

• [FILE:LINE] RESULT_UNIT_USED_AS_OPTION — fn f() -> Result<T, ()> returned and callers treat it as absence — switch to Option<T> or a unit-struct error.
• [FILE:LINE] THREAD_RESULT_AS_ERROR — JoinHandle::join() payload treated as dyn Error (calling .source() or Display) — it's Box<dyn Any>. Downcast or resume_unwind.
• [FILE:LINE] BOXED_ERROR_HOT_LOOP — Result<_, Box<MyError>> returned from a function called per-iteration in a hot loop — allocation per failure; consider a fixed-shape error or unboxed enum.

? Uses From, Not Into

? desugars to (roughly) match expr { Ok(v) => v, Err(e) => return Err(From::from(e)) }. The conversion is literally From::from, not Into::into. Implementing only Into<OuterErr> for InnerErr will not enable ? propagation, even though From implies Into.

// Will NOT enable `?` from InnerErr to OuterErr
impl Into<OuterErr> for InnerErr { /* ... */ }

// Correct: implement From, get Into for free, and `?` works
impl From<InnerErr> for OuterErr { fn from(e: InnerErr) -> Self { /* ... */ } }

Old crates that only implement Into are common offenders — ? will fail to compile against them with a confusing message about From not being implemented.

Flag:

• [FILE:LINE] INTO_NOT_FROM — error conversion implemented as impl Into<Outer> for Inner — ? calls From::from, not Into::into. Reverse the impl direction.

Deferred Cleanup with ? — The Surprising Bug

A function that needs cleanup on the error path before propagating will skip the cleanup whenever ? short-circuits:

fn do_the_thing() -> Result<(), Error> {
    let thing = Thing::setup()?;
    step_one(&thing)?;          // if this errors, cleanup is skipped
    step_two(&thing)?;
    thing.cleanup();            // only runs on the success path
    Ok(())
}

Three fixes, in order of preference:

• RAII guard (stable, idiomatic). Move cleanup into Drop. The compiler runs destructors on every exit path, including ? early returns.
• `try` blocks (still unstable behind #![feature(try_blocks)]). Scope ? to a block; cleanup runs after.
• Explicit `match`. Verbose but stable: bind the intermediate Result, run cleanup, then propagate.

struct Thing;
impl Drop for Thing { fn drop(&mut self) { /* cleanup */ } }
// Now `?` cannot skip cleanup — Drop fires on every exit.

Flag:

• [FILE:LINE] CLEANUP_SKIPPED_BY_QMARK — resource setup followed by ? calls and a manual cleanup() / close() / release() after — the ? early-return skips cleanup. Use Drop (RAII) or scope cleanup in a try block.
• [FILE:LINE] MANUAL_DROP_GUARD_LEAK — explicit mem::forget or ManuallyDrop on a guard followed by ? — leaks the resource on error. Restructure so the guard owns the cleanup.

#[error(transparent)] (thiserror)

#[error(transparent)] marks a variant that is a thin wrapper around an inner error type. Display and source() delegate to the inner error unchanged — the wrapper is invisible to logs and chain traversal.

#[derive(Debug, thiserror::Error)]
pub enum AppError {
    #[error(transparent)]
    Io(#[from] std::io::Error),     // displays exactly like the io::Error
    #[error("config invalid at {path}")]
    Config { path: PathBuf, #[source] source: toml::de::Error },
}

Use it when the variant adds no semantic information — the inner type is already the "real" error. Do not use it when you have context to add (a path, a request ID); use #[error("...")] with a #[source] field instead so the wrapping shows up in logs.

Flag:

• [FILE:LINE] TRANSPARENT_HIDES_CONTEXT — #[error(transparent)] on a variant that should be carrying context (path, ID) — the inner error's message alone won't tell users what was being attempted. Switch to #[error("...")] + #[source].
• [FILE:LINE] TRANSPARENT_DUPLICATE_DISPLAY — #[error(transparent)] paired with a manual Display impl that also prints the inner error — double-prints in error chains. Drop one.

Review Questions

1. Are all unwrap() / expect() calls in production code justified? 2. Do errors carry context about what operation failed? 3. Are error types structured (enums/structs) rather than stringly-typed? 4. Is panic! reserved for unrecoverable invariant violations? 5. Are errors propagated or logged, not silently swallowed? 6. Is thiserror used for library errors, anyhow for application errors? 7. Are _else variants used when fallbacks involve allocation? 8. Do async error types satisfy Send + Sync + 'static bounds? 9. Is inspect_err used for error logging instead of match arms? 10. Do custom error types implement the full contract (Error, Display, Debug, Send + Sync + 'static)? 11. Is Error::source() implemented for wrapped errors to enable chain traversal? 12. Are From implementations provided for ? ergonomics instead of verbose .map_err() chains? 13. Is the error strategy (enumerated vs opaque) appropriate for how callers interact with errors?

Lifetime Variance

Variance Fundamentals

Variance describes when a subtype can substitute for a supertype. A lifetime 'b is a subtype of 'a if 'b: 'a (outlives). There are three kinds:

• Covariant: a subtype can substitute freely. &'a T is covariant in both 'a and T.
• Invariant: must match exactly. &mut T is invariant in T; Cell<T> is invariant in T.
• Contravariant: flipped relationship. fn(T) is contravariant in T (a function accepting less-specific args is more useful).

Quick Reference Table

When Variance Causes Bugs

Invariance Behind &mut

Flag when: a type uses a single lifetime where two are needed, and one appears behind &mut.

// BAD — single lifetime forces invariance, won't compile
struct MutStr<'a> {
    s: &'a mut &'a str,
}

// GOOD — two lifetimes decouple the mutable borrow from the inner reference
struct MutStr<'a, 'b> {
    s: &'a mut &'b str,
}

With one lifetime, the compiler cannot shorten the mutable borrow independently of the inner &str because &mut T is invariant in T. Two lifetimes let the outer borrow end while the inner &str lifetime remains 'static.

Cell<&'a T> Is Invariant

Flag when: Cell or RefCell wraps a reference and code assumes lifetime covariance.

// This won't compile — Cell<&'a T> is invariant in 'a
fn bad<'a>(cell: &Cell<&'a str>) {
    let local = String::from("temp");
    cell.set(&local); // would allow dangling ref if covariant
}

Invariance is correct here: if Cell<&'a str> were covariant, you could store a short-lived reference that outlives its source.

Memory Regions and Lifetime Implications

• Stack: references to stack frames cannot outlive the frame. Check that returned references don't point to locals.
• Heap (Box, Arc): lifetime is unconstrained until deallocation. Box::leak produces &'static.
• Static memory: 'static references to static variables or string literals are always valid.

Check for: 'static bounds on type parameters (e.g., T: 'static) — this means T must be owned or itself 'static, not that it lives forever. Common in thread::spawn and tokio::spawn closures.

Lifetime Annotation Review Rules

Flag When

• Unnecessary `'static` on parameters: fn process(name: &'static str) when &str suffices. This forces callers to provide only compile-time strings or leaked allocations.
• Missing multiple lifetimes: a struct holds references to two independent sources but uses one lifetime, causing invariance-related compilation failures or overly restrictive APIs.
• Lifetime on return but not needed: functions returning owned data (String, Vec<T>) that carry a phantom lifetime parameter.
• Single lifetime on iterator types: types like StrSplit that yield references from one field but borrow a different field need separate lifetimes so the yielded reference isn't tied to the shorter-lived field.

Valid Patterns

• Elided lifetimes: when the three elision rules apply, explicit annotations are noise. Don't flag missing annotations when elision handles it.
• `'a` on `&self` returns: rule 3 of elision assigns self's lifetime to outputs. Explicit annotation is optional.
• `'static` for `thread::spawn` and `tokio::spawn`: required by the API, not a code smell.
• `'static` trait bounds: T: 'static on generic parameters is standard for owned, self-sufficient types.

Common Mistakes

Conflating 'static with "lives forever"

T: 'static means T contains no non-static borrows. An owned String is 'static. This is a bound, not a lifetime annotation on a reference.

Ignoring Drop's interaction with lifetimes

If a type implements Drop and is generic over 'a, dropping counts as a use of 'a. Code that shortens borrows before the drop site may fail to compile.

// If Wrapper<'a> implements Drop, this won't compile:
let mut x = 42;
let w = Wrapper(&mut x);
x = 0; // x is still mutably borrowed because w.drop() might use it

Check for: types with Drop that hold references — the borrow extends to the drop point, not the last explicit use.

Edition 2024: RPIT captures all lifetimes by default

In edition 2024, -> impl Trait captures all in-scope lifetime parameters. Use + use<'a> to narrow capture when the return value doesn't actually borrow all parameters.

Review Questions

1. Does the type need multiple lifetime parameters, or does a single lifetime cause invariance issues? 2. Are 'static annotations on parameters genuinely required, or would an elided lifetime work? 3. Do types implementing Drop account for the extended borrow at the drop site? 4. Are Cell/RefCell wrapping references with correct variance expectations? 5. Edition 2024: Do RPIT return types need + use<...> to avoid capturing unrelated lifetimes?

Ownership and Borrowing

For pointer type selection, Copy trait guidance, Cow patterns, and iterator idioms, see the rust-best-practices skill.

Critical Anti-Patterns

1. Clone to Silence the Borrow Checker

When .clone() appears primarily to resolve borrow checker errors, it often hides a design issue. The borrow checker is pointing at a real ownership conflict that cloning papers over.

// BAD - cloning to work around borrow conflict
fn process(data: &mut Vec<String>) {
    let items = data.clone(); // expensive, hides design issue
    for item in &items {
        data.push(item.to_uppercase());
    }
}

// GOOD - restructure to avoid the conflict
fn process(data: &mut Vec<String>) {
    let uppercased: Vec<String> = data.iter().map(|s| s.to_uppercase()).collect();
    data.extend(uppercased);
}

The exception: .clone() is fine when you genuinely need an independent copy (e.g., sending data to another thread, storing in a cache alongside the original).

2. Overly Broad Lifetimes

Using 'static when a shorter lifetime works makes APIs inflexible and can hide real ownership issues.

// BAD - forces callers to own their data forever
fn process(name: &'static str) {
    println!("{name}");
}

// GOOD - any borrowed string works
fn process(name: &str) {
    println!("{name}");
}

'static is appropriate for: compile-time constants, leaked allocations (intentional), thread-spawned closures that must outlive the caller.

3. Taking Ownership When Borrowing Suffices

Functions that take String when they only read the data force unnecessary allocations at call sites.

// BAD - forces callers to allocate
fn greet(name: String) {
    println!("Hello, {name}");
}
greet(some_str.to_string()); // unnecessary allocation

// GOOD - borrows are cheaper
fn greet(name: &str) {
    println!("Hello, {name}");
}
greet(some_str); // works with &str, String, &String

For maximum flexibility in public APIs, consider impl AsRef<str> which accepts &str, String, &String, and other types that deref to str.

4. Returning References to Local Data

The borrow checker catches this at compile time, but it indicates a misunderstanding of ownership.

// WON'T COMPILE - but indicates design confusion
fn create_name() -> &str {
    let name = String::from("hello");
    &name // name is dropped at end of function
}

// GOOD - return owned data
fn create_name() -> String {
    String::from("hello")
}

5. Interior Mutability Overuse

RefCell, Cell, and Mutex bypass compile-time borrow checking. Overusing them suggests the ownership model needs rethinking.

// SUSPICIOUS - RefCell to work around borrow rules
struct Service {
    cache: RefCell<HashMap<String, Data>>,
    config: RefCell<Config>,
}

// BETTER - separate mutable and immutable state
struct Service {
    cache: HashMap<String, Data>,  // mutated via &mut self
    config: Config,                // set at construction
}

RefCell is appropriate for: observer patterns, graph structures with shared nodes, runtime-polymorphic mutation. In multithreaded code, Mutex/RwLock serve a similar role but with thread safety.

Lifetime Elision Rules

Rust elides lifetimes when the rules are unambiguous. Understanding them prevents unnecessary lifetime annotations:

1. Each input reference gets its own lifetime: fn f(a: &T, b: &U) becomes fn f<'a, 'b>(a: &'a T, b: &'b U) 2. If there's exactly one input lifetime, it's assigned to all output references 3. If &self or &mut self is an input, its lifetime is assigned to outputs

// Elision handles this - no annotations needed
fn first_word(s: &str) -> &str { ... }

// Multiple inputs, ambiguous - must annotate
fn longest<'a>(a: &'a str, b: &'a str) -> &'a str { ... }

RPIT Lifetime Capture (Edition 2024)

In edition 2024, -> impl Trait return types capture all in-scope generic parameters and lifetimes by default. In edition 2021, only type parameters used in the bounds were captured.

// Edition 2021: this compiled — 'a is NOT captured by impl Display
fn foo<'a>(x: &'a str, y: String) -> impl Display {
    y // fine: returned value doesn't borrow 'a
}

// Edition 2024: same code captures 'a — returned impl Display now
// borrows 'a even though it doesn't use it. This can cause
// unexpected borrow checker errors at call sites.

// GOOD — use precise capturing to opt out of capturing 'a
fn foo<'a>(x: &'a str, y: String) -> impl Display + use<> {
    y // explicitly captures nothing
}

// GOOD — capture only what you need
fn bar<'a, 'b>(x: &'a str, y: &'b str) -> impl Display + use<'b> {
    y.to_uppercase() // only captures 'b
}

When to flag: Functions returning impl Trait that take multiple lifetime parameters — check whether the edition 2024 default capture causes unintended borrowing at call sites. If callers get unexpected "borrowed value does not live long enough" errors, add + use<...> to narrow the capture.

if let Temporary Scope (Edition 2024)

In edition 2024, temporaries created in if let conditions are dropped at the end of the condition, not at the end of the if block. This breaks patterns that relied on temporaries living through the else branch.

// BAD in edition 2024 — MutexGuard dropped before else branch
if let Some(val) = mutex.lock().unwrap().get("key") {
    use_val(val);
} else {
    // mutex is already unlocked here — was locked in 2021
}

// GOOD — bind the guard explicitly to control its lifetime
let guard = mutex.lock().unwrap();
if let Some(val) = guard.get("key") {
    use_val(val);
} else {
    // guard still alive — explicit control
}

Tail Expression Temporary Scope (Edition 2024)

In edition 2024, temporaries in tail expressions (the final expression in a block without a semicolon) are dropped before local variables. This can break code where a temporary borrows a local.

// BAD in edition 2024 — temporary String dropped before local
fn example() -> &str {
    let s = String::from("hello");
    s.as_str() // temporary borrow dropped before s in 2024
}

// GOOD — return owned data or bind explicitly
fn example() -> String {
    String::from("hello")
}

When to flag: Tail expressions that create temporaries referencing local variables — in edition 2024 the drop order changed and this may cause borrow checker errors that didn't exist in 2021.

IntoIterator for Box<[T]> (Edition 2024)

Box<[T]> now implements IntoIterator directly in edition 2024, yielding owned T values without converting to Vec first.

// BEFORE edition 2024 — had to convert to Vec
let boxed: Box<[i32]> = vec![1, 2, 3].into_boxed_slice();
for item in boxed.into_vec() {
    process(item);
}

// GOOD in edition 2024 — iterate directly
let boxed: Box<[i32]> = vec![1, 2, 3].into_boxed_slice();
for item in boxed {
    process(item);
}

Review Questions

1. Are .clone() calls necessary, or do they mask ownership design issues? 2. Are lifetimes as narrow as possible (not overly 'static)? 3. Do functions borrow when they don't need ownership? 4. Is interior mutability (RefCell, Cell) used only when compile-time borrowing is genuinely insufficient? 5. Are smart pointers chosen appropriately for the sharing and threading model? 6. Edition 2024: Do -> impl Trait returns use + use<...> when default lifetime capture is too broad? 7. Edition 2024: Are if let temporaries explicitly bound when their lifetime matters? 8. Edition 2024: Are tail expression temporaries safe given the new drop order?