ข้ามไปยังเนื้อหา

Review Checklist

นี่คือเวอร์ชัน checklist ของสองบทก่อนหน้า: หนึ่งหน้า หกหมวด แต่ละข้อเขียนเป็นคำถามที่คุณถามกับบรรทัดโค้ดที่เจาะจงได้ แต่ละข้อลิงก์ไปบทที่อธิบายว่าทำไมคำถามนั้นถึงสำคัญ เปิดไว้อีกแท็บระหว่างรีวิวจริง

  • ทุก .clone() มีเหตุผลที่คุณอธิบายได้ในหนึ่งประโยคไหม (“จำเป็นเพราะ X ยังใช้ตัวเดิมอยู่หลังจากนี้”)? — Ownership & Moves
  • parameter &Vec<T> หรือ &String ตรงนี้เป็น &[T] หรือ &str แทนได้ไหม? — Slices
  • Rc<RefCell<T>> ตัวนี้ต้องการหลายเจ้าของและ interior mutability จริงไหม หรือ field ธรรมดากับการยืมแบบ &mut ก็พอแล้ว? — Smart Pointers
  • ทุก unsafe block มี comment // SAFETY: อธิบายว่าข้อสมมติไหนทำให้มัน sound ไหม? — The Borrow Checker Model
  • ทุก lifetime annotation แบบชัดเจนเพิ่มข้อมูลที่ elision จะไม่ infer ให้อยู่แล้วไหม? — Lifetimes
  • 'static ตรงนี้เพราะข้อมูลมีชีวิตอยู่ตลอดโปรแกรมจริง ๆ หรือเพราะมันปิดปาก error? — Lifetimes
  • mem::forget หรือ Box::leak ปรากฏอยู่ไหม และถ้ามี leak นั้นตั้งใจและคุ้มกับ comment ที่อธิบายมันไหม? — Drop and RAII
  • ก่อนจะหยิบ dyn Trait trait นี้ต้องการ method ที่ return Self หรือ generic method ที่ทำให้มัน dyn-incompatible อยู่แล้วไหม? — Trait Objects
  • impl Trait ถูกใช้แค่ในตำแหน่ง argument หรือ return เท่านั้น ไม่เคยเป็น field type ของ struct ใช่ไหม? — Generics
  • ทุก #[derive(Copy)] อยู่บน type ที่เล็กพอที่ implicit copy จะถูกจริง ๆ ไหม? — Structs
  • float (f32/f64) ถูกใช้เป็น HashMap/HashSet key หรือถูกเปรียบเทียบด้วย == ตรง ๆ บ้างไหม? — HashMap
  • ทุก .unwrap()/.expect(...) อยู่บนค่าที่ infallible จริง ๆ ตรงนี้ ไม่ใช่ input ผู้ใช้, I/O, หรือ network response ใช่ไหม? — Panics
  • มี match บน enum ตัวไหนมี _ => arm ไหม และถ้ามี enum นั้นมีโอกาสโตขึ้นเป็น variant ใหม่ในอนาคตได้จริงไหม? — Pattern Matching
  • .is_some()/.is_ok() ตามด้วย .unwrap() แยกกันคนละขั้นบ้างไหม ในที่ที่ if let/match ทำทั้งสองอย่างในขั้นตอนเดียวได้? — Option
  • Box<dyn Error> ถูกใช้เป็น public return type ของ library ในที่ที่ผู้เรียกอาจต้อง match กับ failure เฉพาะเจาะจงไหม? — Custom Errors
  • มี panic! ตัวไหนครอบคลุมเงื่อนไขที่ผู้เรียกอาจอยากกู้คืนได้อย่างสมเหตุสมผลไหม? — Result
  • for i in 0..collection.len() loop ตัวไหนใช้ i แค่ index collection เดียวกันนั้นไหม? — Iterators
  • .collect() ถูกใช้ต่อทันทีด้วย method รูปร่าง iterator อีกตัว (.contains(...), .any(...), .len()) ไหม? — Iterators
  • string byte-slice ตัวไหน (&s[a..b]) สมมติว่าเป็น ASCII โดยไม่เช็คว่า a/b ตกบน char boundary สำหรับ input ที่ไม่ใช่ ASCII ไหม? — Vec and String
  • as cast ระหว่าง integer type ตัวไหนเสี่ยง truncate อย่างเงียบ ๆ บน input ที่ไม่ใช่ literal เล็ก ๆ ไหม? — Data Types
  • Arc<Mutex<T>> ตัวนี้ต้องมีชีวิตอยู่นานกว่าฟังก์ชันที่ spawn thread ของมันไหม หรือ thread::scope จะตัดความต้องการ Arc ออกได้? — Threads
  • integer ตัวเดียวที่ Mutex ปกป้องอยู่เป็น atomic ธรรมดาแทนได้ไหม? — Threads
  • std::sync::MutexGuard ตัวไหนยังอยู่ใน scope ข้าม .await point ไหม? — Async Intro
  • async fn ตัวไหนเรียก thread::sleep หรือ blocking call อื่นแทนที่จะเป็น async-aware equivalent ไหม? — Async Intro
  • mpsc::channel() (ไม่มีขอบเขต) ถูกใช้ในที่ที่ producer อาจเร็วกว่า consumer ได้จริงไหม? — Channels
  • ทุก #[should_panic] ระบุ expected = "..." ไหม? — Cargo Tests
  • ทุก body ของฟังก์ชัน #[test] มี assert!/assert_eq!/assert_ne! อย่างน้อยหนึ่งตัวไหม? — Cargo Tests
  • cargo clippy -- -D warnings รันสะอาดไหม และมันถูกรันด้วย AI-smell lint จาก tool ladder เปิดใช้งานจริงหรือยัง? — Verification Tools
  • resolver กับ rust-version ใน Cargo.toml ถูกตั้งไว้อย่างตั้งใจ ไม่ปล่อยให้เป็น default ใช่ไหม? — Cargo Tests

Compiler พูดว่าอย่างไร: การแก้ไข Vec ระหว่างที่กำลัง Iterate มันอยู่

หัวข้อที่มีชื่อว่า “Compiler พูดว่าอย่างไร: การแก้ไข Vec ระหว่างที่กำลัง Iterate มันอยู่”

checklist ข้อหนึ่ง — “มีอะไรดูเหมือนจะทำให้ iterator invalid กลาง loop ไหม” — คุ้มที่จะเห็นมันถูกปฏิเสธตรง ๆ เพราะ Rust ปฏิเสธสิ่งนี้ที่ compile time แทนที่จะปล่อยให้มันทำลาย memory ที่ runtime แบบที่จะเกิดในภาษาที่มีกฎ aliasing หลวมกว่านี้:

// @expect-error E0502
fn main() {
let mut v = vec![1, 2, 3];
for x in &v {
v.push(*x);
}
}
error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> src/main.rs:4:9
|
3 | for x in &v {
| --
| |
| immutable borrow occurs here
| immutable borrow later used here
4 | v.push(*x);
| ^^^^^^^^^^ mutable borrow occurs here

for x in &v ถือ immutable borrow ของ v ตลอดทั้ง loop; v.push(*x) ต้องการ mutable borrow ซึ่งอยู่ร่วมกับมันไม่ได้ ข้อผิดพลาดเฉพาะนี้ — การ append เข้า collection ระหว่างที่ iterate มันอยู่ — เป็น undefined behavior ในภาษาอื่นหลายภาษา (backing array ที่ resize อาจทำให้ x ชี้ไปที่ memory ที่ถูกปล่อยแล้วหรือเก่าไป) แต่ที่นี่เป็น compile error แทน

สำหรับการผ่านแบบเร็วหรือ PR description นี่อัดทุกคำถามข้างบนให้เหลือแค่วลี:

Ownership: unexplained .clone() / &Vec or &String params / Rc<RefCell> as default / unsafe
without SAFETY / decorative lifetimes / 'static-to-silence / mem::forget or Box::leak
Types: dyn where impl/enum would do / impl Trait in a field / Copy on a large struct /
float as a HashMap key or compared with ==
Errors: unwrap on fallible input / _ => on a growable enum / is_some+unwrap / Box<dyn
Error> in a library API / panic! on a recoverable condition
Iterators: index loop instead of iterator / needless collect / byte-slice on non-ASCII /
as-cast truncation risk
Concurrency: Arc<Mutex> where scope/atomic would do / lock held across .await /
thread::sleep in async / unbounded channel
Cargo & tests: should_panic without expected / test with no assert / clippy not run
with -D warnings / resolver and rust-version left to default

ก่อนจะอ่าน body ของฟังก์ชันแม้แต่ตัวเดียว รันคำสั่งนี้บนไฟล์ (หรือ diff) เพื่อดูว่ารายการที่ signal สูงสุดของ checklist กระจุกตัวอยู่ตรงไหน:

Terminal window
grep -nE 'clone\(\)|unwrap\(\)|expect\(|unsafe|'"'"'static|Rc<RefCell|Box<dyn Error|as u|_ =>|mem::forget|#\[allow|\.lock\(\)\.unwrap\(\)' src/**/*.rs

ไม่กี่ hit กระจายอยู่ในไฟล์ใหญ่ ๆ เป็นเรื่องปกติ hit ห้าหกตัวกระจุกอยู่ในฟังก์ชันเดียวคือวิธีที่เร็วที่สุดของ checklist ในการชี้ว่าควรเริ่มอ่านอย่างละเอียดตรงไหน

struct Order {
items: Vec<(String, u32)>, // (name, price in cents)
}
// &[...] not &Vec<...>, &str not &String -- accepts strictly more callers
fn total_cents(items: &[(String, u32)]) -> u32 {
items.iter().map(|(_, price)| price).sum()
}
fn format_receipt(order: &Order) -> String {
let mut lines = Vec::new();
for (name, price) in &order.items { // iterator, not an index loop
lines.push(format!("{name}: {price}"));
}
lines.push(format!("total: {}", total_cents(&order.items)));
lines.join("\n")
}
fn main() {
let order = Order {
items: vec![
("coffee".to_string(), 350),
("bagel".to_string(), 275),
],
};
println!("{}", format_receipt(&order));
}

โค้ดนี้ผ่านทุกข้อใน checklist ข้างบน: ไม่มี .clone() เลยสักที่, &[(String, u32)] แทน &Vec<...>, iterator ธรรมดาแทน index loop, ไม่มี .unwrap() บนอะไรที่ fallible เลย (ไม่มีอะไร fallible ตรงนี้เลย) และไม่มี suppression เลย การจำได้ว่าการผ่านแบบสะอาดหน้าตาเป็นอย่างไรมีประโยชน์พอ ๆ กับการจำได้ว่ากลิ่นไม่ดีหน้าตาเป็นอย่างไร — มันคือ baseline ที่ catalog ส่วนที่เหลือของโมดูลนี้ถูกวัดเทียบด้วย

Checklist แลก depth กับ coverage: มันจะไม่จับ logic bug ที่ subtle เฉพาะ domain ของ codebase นี้ แต่มันเผยรูปร่างของปัญหาสองโหลเดิมได้อย่างสม่ำเสมอในไฟล์ Rust ใด ๆ ในเวลาคงที่ ไม่ว่าคุณจะคุ้นเคยกับ codebase นี้แค่ไหน นั่นแหละคือการแลกที่การอ่านผ่านครั้งแรกบนโค้ดที่ไม่คุ้นเคย (หรือโค้ดที่ AI สร้าง) ควรทำ — หาก่อนว่าความเสี่ยงกระจุกอยู่ตรงไหนก่อนที่จะใช้เวลาอ่านลึกกับฟังก์ชันใดฟังก์ชันหนึ่ง

การรัน grep list กับ checklist ทุกครั้งที่รีวิวเพิ่มเวลาสองสามนาทีให้กับการผ่านที่ “แค่อ่าน diff” จะไม่ใช้เวลาแบบนั้น — และบน diff ที่กลายเป็นว่าสะอาดล้วน ๆ เวลานั้นไม่ให้ finding อะไรเลย ทางเลือกคือข้ามไปอ่าน logic ตรง ๆ เร็วกว่าบน diff ที่สะอาด และแย่กว่าพอดีบน diff ที่มันสำคัญที่สุด: ไฟล์ที่มี pattern พวกนี้หลายตัวกระจุกอยู่ด้วยกัน ซึ่งการอ่านแบบไม่มีโครงสร้างมีโอกาสน้อยที่สุดที่จะสังเกตว่ามันเป็นกลุ่มก้อน แทนที่จะเห็นเป็นทางเลือกแยกกันที่ให้อภัยได้ทีละอัน

  • “checklist ข้อไหนถูก flag แปลว่าโค้ดผิด” มันแปลว่าโค้ดนั้นสมควรได้รับคำถามที่เจาะจง หลายข้อ (.clone() ที่จำเป็นจริง ๆ, Box<dyn Error> ที่เปิดกว้างอย่างชอบธรรม) คาดว่าจะผ่านคำถามนั้นโดยไม่ถูก flag
  • “ถ้า clippy สะอาด checklist ก็ไม่จำเป็น” หลายข้อใน checklist — channel ไม่มีขอบเขต, Arc<Mutex<T>> ในที่ที่ scope จะพอ, test ที่ไม่มี assertion — ไม่มี clippy lint ที่ตรงกันเลย checklist กับ tool ladder (บทถัดไป) ครอบคลุมพื้นที่ที่ต่างกัน ทับกันแค่บางส่วน
  • “grep list จับทุกอย่างที่ checklist ครอบคลุม” มันจับเฉพาะส่วนที่ grep ได้ทาง syntax เท่านั้น _ => catch-all, lifetime ประดับ, และ “design นี้ต้องการหลายเจ้าของจริงไหม” ล้วนต้องการการอ่าน ไม่ใช่แค่การจับคู่ text

การรัน grep list บนไฟล์ที่ AI สร้างมักจะเผย hit หลายตัวในฟังก์ชันเดียว ซึ่งเป็น signal “กลุ่มก้อน” ที่ checklist ถูกออกแบบมาให้จับพอดี

🐛 หา bug ในโค้ดนี้

use std::rc::Rc;
use std::cell::RefCell;
struct Cache {
data: Rc<RefCell<Vec<String>>>,
}
fn add_entry(cache: Rc<RefCell<Vec<String>>>, entry: String) {
cache.borrow_mut().push(entry);
}
fn print_all(cache: Rc<RefCell<Vec<String>>>) {
for item in cache.borrow().iter() {
println!("{item}");
}
}
fn main() {
let cache = Rc::new(RefCell::new(Vec::new()));
add_entry(cache.clone(), "first".to_string());
add_entry(cache.clone(), "second".to_string());
print_all(cache.clone());
}
เฉลย

การ grep ไฟล์นี้หา term ของ checklist เจอ Rc<RefCell สามครั้งและ .clone() สามครั้ง ทั้งหมดอยู่ในไฟล์ 20 บรรทัดที่มี thread เดียวและไม่มีความต้องการ shared-ownership จริง ๆ เลย — main เป็นผู้เรียกตัวเดียว และมันเรียกทุกอย่างตามลำดับ ไม่มีอะไรตรงนี้ต้องการเจ้าของสองคนมีชีวิตอยู่พร้อมกันหรือ interior mutability ข้าม alias เลย &mut Vec<String> ธรรมดาที่ส่งแบบ mutable reference ทำงานเดียวกันได้โดยไม่มี Rc, ไม่มี RefCell, และไม่มี .clone() เลยสักที่: fn add_entry(cache: &mut Vec<String>, entry: String) เรียกเป็น add_entry(&mut cache, "first".to_string()) grep list ไม่ได้บอกสิ่งนี้ด้วยตัวมันเอง — Rc<RefCell สาม hit ในไฟล์เล็ก ๆ เดียวคือ signal ที่บอกว่า “ไปอ่านฟังก์ชันนี้แล้วถามว่ามันจำเป็นต้องอยู่ตรงนี้ไหม” และการอ่านมันคือสิ่งที่ยืนยันว่าคำตอบคือไม่

จุดประสงค์จริงของ checklist ตามบทนี้คืออะไร?
ทำไมโค้ดนี้ compile ไม่ผ่าน?
fn main() {
    let mut v = vec![1, 2, 3];
    for x in &v {
        v.push(*x);
    }
}
ข้อไหนใน checklist ที่ไม่มี clippy lint แบบ default หรือ off-by-default ที่ตรงกันเลย ตามที่โมดูลนี้บอก?
ทำไมบทนี้ถึงแนะนำให้รัน grep list ก่อนอ่าน body ของฟังก์ชัน แทนที่จะรันทีหลัง?
ในตัวอย่าง SpotTheBug สามฟังก์ชันแต่ละตัวรับ Rc<RefCell<Vec<String>>> โดย value และผู้เรียก clone Rc ก่อนทุก call จริง ๆ แล้ว hit ของ Rc<RefCell กับ clone() จาก grep list บอกอะไรผู้รีวิวตรงนี้?
ตาม checklist เมื่อไหร่ที่ lifetime annotation แบบชัดเจนอย่าง <'a> ควรถูกมองว่าเป็นกลิ่นไม่ดี?