Rust Reviewer

Rust Reviewer Skill

Purpose

Reviews Rust code for ownership, lifetimes, error handling, safety, and idiomatic patterns.

When to Use

• Rust project code review
• Ownership/borrowing review
• Lifetime annotation review
• Unsafe code audit
• Error handling patterns

Project Detection

• Cargo.toml in project root
• .rs files
• src/lib.rs or src/main.rs
• tests/ directory

Workflow

Step 1: Analyze Project

**Rust Edition**: 2021
**Type**: Library / Binary
**Dependencies**: Key crates
**Testing**: cargo test
**Linter**: clippy

Step 2: Select Review Areas

AskUserQuestion:

"Which areas to review?"
Options:
- Full Rust review (recommended)
- Ownership and borrowing
- Lifetimes and references
- Error handling (Result/Option)
- Unsafe code audit
multiSelect: true

Detection Rules

Ownership & Borrowing

// BAD: Unnecessary clone
fn process(data: Vec<String>) {
    for item in data.clone().iter() {  // Clone not needed!
        println!("{}", item);
    }
}

// GOOD: Borrow
fn process(data: &[String]) {
    for item in data {
        println!("{}", item);
    }
}

// BAD: &String parameter
fn greet(name: &String) {
    println!("Hello, {}", name);
}

// GOOD: &str parameter (more flexible)
fn greet(name: &str) {
    println!("Hello, {}", name);
}

// BAD: Taking ownership unnecessarily
fn sum(numbers: Vec<i32>) -> i32 {
    numbers.iter().sum()
}

// GOOD: Borrow slice
fn sum(numbers: &[i32]) -> i32 {
    numbers.iter().sum()
}

Lifetimes

// BAD: Missing lifetime
struct Parser {
    input: &str,  // Error: missing lifetime
}

// GOOD: Explicit lifetime
struct Parser<'a> {
    input: &'a str,
}

impl<'a> Parser<'a> {
    fn new(input: &'a str) -> Self {
        Parser { input }
    }

    fn parse(&self) -> Result<Ast<'a>, Error> {
        // ...
    }
}

// BAD: 'static when not needed
fn process(data: &'static str) {
    // Requires static lifetime!
}

// GOOD: Generic lifetime
fn process(data: &str) {
    // Works with any lifetime
}

// Consider: Ownership instead of lifetime
struct Config {
    name: String,  // Owned, no lifetime needed
}

Error Handling

// BAD: unwrap in library code
pub fn parse_config(path: &str) -> Config {
    let content = fs::read_to_string(path).unwrap();  // Will panic!
    serde_json::from_str(&content).unwrap()
}

// GOOD: Return Result
pub fn parse_config(path: &str) -> Result<Config, ConfigError> {
    let content = fs::read_to_string(path)
        .map_err(|e| ConfigError::IoError(e))?;
    serde_json::from_str(&content)
        .map_err(|e| ConfigError::ParseError(e))
}

// GOOD: Custom error type
#[derive(Debug, thiserror::Error)]
pub enum ConfigError {
    #[error("failed to read config file: {0}")]
    IoError(#[from] std::io::Error),

    #[error("failed to parse config: {0}")]
    ParseError(#[from] serde_json::Error),

    #[error("missing required field: {0}")]
    MissingField(String),
}

// BAD: expect without message
let value = map.get("key").expect("failed");

// GOOD: Descriptive expect
let value = map.get("key")
    .expect("config must contain 'key' field");

Option Handling

// BAD: Verbose Option handling
let result = match value {
    Some(v) => v.to_string(),
    None => "default".to_string(),
};

// GOOD: Using combinators
let result = value
    .map(|v| v.to_string())
    .unwrap_or_else(|| "default".to_string());

// Or even simpler
let result = value.map_or("default".to_string(), |v| v.to_string());

// BAD: Nested Options
let result: Option<Option<i32>> = Some(Some(42));
let value = match result {
    Some(Some(v)) => v,
    _ => 0,
};

// GOOD: Flatten
let value = result.flatten().unwrap_or(0);

// Using and_then for chaining
let value = get_user(id)
    .and_then(|user| user.profile)
    .and_then(|profile| profile.avatar)
    .map(|avatar| avatar.url);

Unsafe Code

// BAD: Unsafe without documentation
unsafe fn get_value(ptr: *const i32) -> i32 {
    *ptr
}

// GOOD: Documented unsafe
/// Gets the value at the given pointer.
///
/// # Safety
///
/// - `ptr` must be valid and properly aligned
/// - `ptr` must point to initialized memory
/// - The memory must not be mutated while this reference exists
unsafe fn get_value(ptr: *const i32) -> i32 {
    debug_assert!(!ptr.is_null(), "ptr must not be null");
    *ptr
}

// BAD: Transmute for casting
let bytes: [u8; 4] = unsafe { std::mem::transmute(value) };

// GOOD: Safe alternative
let bytes = value.to_ne_bytes();

// Encapsulating unsafe in safe API
pub struct Buffer {
    ptr: *mut u8,
    len: usize,
}

impl Buffer {
    /// Creates a new buffer. Safe because we control allocation.
    pub fn new(size: usize) -> Self {
        let ptr = unsafe { alloc(Layout::array::<u8>(size).unwrap()) };
        Self { ptr, len: size }
    }

    /// Safe accessor - bounds checked
    pub fn get(&self, index: usize) -> Option<u8> {
        if index < self.len {
            // SAFETY: index is bounds-checked above
            Some(unsafe { *self.ptr.add(index) })
        } else {
            None
        }
    }
}

Idiomatic Rust

// BAD: C-style loop
let mut i = 0;
while i < items.len() {
    process(&items[i]);
    i += 1;
}

// GOOD: Iterator
for item in &items {
    process(item);
}

// With index
for (i, item) in items.iter().enumerate() {
    println!("{}: {}", i, item);
}

// BAD: Redundant type
let numbers: Vec<i32> = vec![1, 2, 3];
let sum: i32 = numbers.iter().sum();

// GOOD: Type inference
let numbers = vec![1, 2, 3];
let sum: i32 = numbers.iter().sum();  // Only needed here

// BAD: Return in match arms
fn classify(n: i32) -> &'static str {
    match n {
        0 => { return "zero"; }
        _ if n > 0 => { return "positive"; }
        _ => { return "negative"; }
    }
}

// GOOD: Match as expression
fn classify(n: i32) -> &'static str {
    match n {
        0 => "zero",
        _ if n > 0 => "positive",
        _ => "negative",
    }
}

Response Template

## Rust Code Review Results

**Project**: [name]
**Rust Edition**: 2021 | **Clippy**: Enabled

### Ownership & Borrowing
| Status | File | Issue |
|--------|------|-------|
| MEDIUM | parser.rs:45 | Unnecessary clone() |

### Lifetimes
| Status | File | Issue |
|--------|------|-------|
| HIGH | cache.rs:23 | Missing lifetime on struct field |

### Error Handling
| Status | File | Issue |
|--------|------|-------|
| HIGH | lib.rs:67 | unwrap() in public function |

### Unsafe
| Status | File | Issue |
|--------|------|-------|
| CRITICAL | ffi.rs:12 | Unsafe block without safety comment |

### Recommended Actions
1. [ ] Replace clone() with borrowing
2. [ ] Add lifetime annotations to Cache struct
3. [ ] Return Result instead of panicking
4. [ ] Document all unsafe blocks with safety invariants

Best Practices

1. Ownership: Borrow when possible, clone when needed
2. Lifetimes: Explicit > implicit, owned > referenced
3. Errors: thiserror for libs, anyhow for apps
4. Unsafe: Minimize, document, encapsulate in safe API
5. Clippy: Run with cargo clippy -- -W clippy::pedantic

Integration

• rust-api-reviewer: Web framework patterns
• security-scanner: Rust security audit
• perf-analyzer: Performance profiling