Rust anti-patterns and common mistakes expert. Handles code review issues with clone abuse, unwrap in production, String misuse, index loops, and refactoring guidance.
| Rank | Mistake | Correct Approach |
|---|---|---|
| 1 | Using .clone() to escape borrow checker |
Use references |
| 2 | Using .unwrap() in production code |
Use ? or with_context() |
| 3 | Everything is String |
Use &str, Cow<str> when needed |
| 4 | Index-based loops | Use iterators .iter(), .enumerate() |
| 5 | Fighting lifetimes | Redesign data structure |
// โ Bad: escaping borrow checker
fn process(user: User) {
let name = user.name.clone(); // Why clone?
// ...
}
// โ
Good: use references
fn process(user: &User) {
let name = &user.name; // Just borrow
}
When clone is actually needed:
// โ Bad: may panic
let config = File::open("config.json").unwrap();
// โ
Good: propagate error
let config = File::open("config.json")?;
// โ
Good: with context
let config = File::open("config.json")
.context("failed to open config")?;
// โ Bad: unnecessary allocation
fn greet(name: String) {
println!("Hello, {}", name);
}
// โ
Good: borrow is enough
fn greet(name: &str) {
println!("Hello, {}", name);
}
// When String is actually needed: ownership or mutation required
// โ Bad: error-prone, inefficient
for i in 0..items.len() {
println!("{}: {}", i, items[i]);
}
// โ
Good: direct iteration
for item in &items {
println!("{}", item);
}
// โ
Good: with index
for (i, item) in items.iter().enumerate() {
println!("{}: {}", i, item);
}
// โ Bad: unsafe for convenience
unsafe {
let ptr = data.as_mut_ptr();
// ... complex memory operations
}
// โ
Good: find safe abstractions
let mut data: Vec<u8> = vec![0; size];
// Vec handles memory management
// โ Anti-pattern: clone to satisfy borrow checker
fn process_data(data: &Data) -> String {
let cloned = data.items.clone();
cloned.into_iter().map(|x| x.to_string()).collect()
}
// โ
Solution: use references properly
fn process_data(data: &Data) -> String {
data.items.iter().map(|x| x.to_string()).collect()
}
// โ Anti-pattern: unwrap chain
fn load_config() -> Config {
let content = std::fs::read_to_string("config.toml").unwrap();
toml::from_str(&content).unwrap()
}
// โ
Solution: Result propagation
fn load_config() -> Result<Config, Box<dyn Error>> {
let content = std::fs::read_to_string("config.toml")?;
Ok(toml::from_str(&content)?)
}
// โ
Solution: with context (anyhow)
fn load_config() -> anyhow::Result<Config> {
let content = std::fs::read_to_string("config.toml")
.context("failed to read config file")?;
toml::from_str(&content)
.context("failed to parse config")
}
// โ Anti-pattern: String parameters everywhere
struct Config {
host: String,
port: String,
path: String,
}
impl Config {
fn new(host: String, port: String, path: String) -> Self {
Self { host, port, path }
}
}
// โ
Solution: accept &str, store String
impl Config {
fn new(host: impl Into<String>, port: u16, path: impl Into<String>) -> Self {
Self {
host: host.into(),
port: port.to_string(),
path: path.into(),
}
}
}
// โ Anti-pattern: manual indexing
fn sum_even(nums: &[i32]) -> i32 {
let mut sum = 0;
for i in 0..nums.len() {
if nums[i] % 2 == 0 {
sum += nums[i];
}
}
sum
}
// โ
Solution: iterator chain
fn sum_even(nums: &[i32]) -> i32 {
nums.iter()
.filter(|&&n| n % 2 == 0)
.sum()
}
| Symptom | Indicates | Refactoring Direction |
|---|---|---|
Many .clone() |
Unclear ownership | Clarify data flow |
Many .unwrap() |
Missing error handling | Add Result handling |
Many pub fields |
Broken encapsulation | Private + accessors |
| Deep nesting | Complex logic | Extract methods |
| Long functions (>50 lines) | Too many responsibilities | Split responsibilities |
| Huge enums | Missing abstraction | Trait + types |
| Outdated | Modern |
|---|---|
Index loop .items[i] |
.iter().enumerate() |
collect::<Vec<_>>() then iterate |
Chain iterators |
lazy_static! |
std::sync::OnceLock |
mem::transmute conversion |
as or TryFrom |
| Custom linked list | Vec or VecDeque |
| Manual unsafe cell | Cell, RefCell |
Code review checklist:
โ Lots of .clone()? Check ownership design
โ .unwrap() in lib code? Need error handling
โ Index loops? Should use iterators
โ pub fields with invariants? Need encapsulation
โ >50 line functions? Should split
1. Is this fighting Rust or working with Rust?
Fighting โ Redesign
Working with โ Continue
2. Is this clone necessary?
Escaping borrow checker โ Warning sign
Actually need copy โ Keep
3. Will this unwrap panic?
Might panic โ Use ?
Never panics โ expect("reason")
4. Is there a more idiomatic way?
Check std library patterns
Review other Rust code
Identified anti-pattern?
โ
Understand the root cause
โ
Find idiomatic alternative
โ
Refactor incrementally
โ
Test thoroughly
When reviewing code:
.clone().unwrap()pub fields with invariants&str instead of String when sufficient#[must_use] warningsunsafe has SAFETY comments# Check for common issues
cargo clippy
# Specific anti-pattern lints
cargo clippy -- -W clippy::clone_on_copy \
-W clippy::unwrap_used \
-W clippy::expect_used
# Check for complexity
cargo clippy -- -W clippy::cognitive_complexity
# Find todos and fixmes
rg "TODO|FIXME|XXX|HACK" --type rust
Symptom: Lots of .clone() calls
// โ Bad: cloning to satisfy compiler
fn process(items: &Vec<Item>) -> Vec<String> {
let items_clone = items.clone();
items_clone.into_iter().map(|i| i.name).collect()
}
// โ
Good: proper borrowing
fn process(items: &[Item]) -> Vec<String> {
items.iter().map(|i| i.name.clone()).collect()
}
// โ
Better: no clone at all
fn process(items: &[Item]) -> Vec<&str> {
items.iter().map(|i| i.name.as_str()).collect()
}
Symptom: Unwrap/expect in production code
// โ Bad: panic on error
let data = fetch_data().unwrap();
let parsed: Config = serde_json::from_str(&data).expect("bad JSON");
// โ
Good: proper error propagation
fn load_data() -> Result<Config, Box<dyn Error>> {
let data = fetch_data()?;
let parsed = serde_json::from_str(&data)?;
Ok(parsed)
}
Symptom: Unnecessary String allocations
// โ Bad: allocating for no reason
fn log_message(level: String, msg: String) {
println!("[{}] {}", level, msg);
}
// โ
Good: borrow when possible
fn log_message(level: &str, msg: &str) {
println!("[{}] {}", level, msg);
}
?expect("reason")