diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 0f4c065e..c1f23f6a 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1091,11 +1091,18 @@ This transformation involves several steps: The implemented algorithm performs a depth-first traversal of the target graph and maintains a recursion stack. Order-only dependencies are ignored - during this search. Encountering an already visiting node indicates a cycle. - The stack slice from the first occurrence of that node forms the cycle and - is returned in `IrGenError::CircularDependency` for improved debugging. The - cycle list is rotated so the lexicographically smallest node appears first, - ensuring deterministic error messages. + during this search. Self-edges are rejected immediately, and encountering an + already visiting node indicates a cycle. The stack slice from the first + occurrence of that node forms the cycle and is returned in + `IrGenError::CircularDependency` for improved debugging. The cycle list is + rotated so the lexicographically smallest node appears first, ensuring + deterministic error messages. + + Traversal state is managed by a small `CycleDetector` helper struct. This + type holds the recursion stack and visitation map, allowing the traversal + functions to remain focused and easily testable. The detector borrows path + references from the `targets` map, so `targets` must remain unchanged during + detection. ### 5.4 Ninja File Synthesis (`ninja_gen.rs`) diff --git a/src/ir.rs b/src/ir.rs index 17ae6ebb..9dc91968 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -25,7 +25,7 @@ //! ``` // use std::collections::HashMap; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; /// The complete, static build graph. #[derive(Debug, Default, Clone)] @@ -479,80 +479,103 @@ enum VisitState { } fn should_visit_node<'a>( - states: &'a mut HashMap, - node: &'a PathBuf, -) -> Result { + states: &mut HashMap<&'a Path, VisitState>, + node: &'a Path, +) -> Result { match states.get(node) { Some(VisitState::Visited) => Ok(false), Some(VisitState::Visiting) => Err(node), None => { - states.insert(node.clone(), VisitState::Visiting); + states.insert(node, VisitState::Visiting); Ok(true) } } } -fn find_cycle(targets: &HashMap) -> Option> { - fn visit( - targets: &HashMap, - node: &PathBuf, - stack: &mut Vec, - states: &mut HashMap, - ) -> Option> { - match should_visit_node(states, node) { +/// Detects cycles in a dependency graph by tracking traversal state. +struct CycleDetector<'a> { + targets: &'a HashMap, + stack: Vec<&'a Path>, + states: HashMap<&'a Path, VisitState>, +} + +impl<'a> CycleDetector<'a> { + fn new(targets: &'a HashMap) -> Self { + Self { + targets, + stack: Vec::new(), + states: HashMap::new(), + } + } + + fn is_visited(&self, node: &Path) -> bool { + matches!(self.states.get(node), Some(VisitState::Visited)) + } + + fn visit_node(&mut self, node: &'a Path) -> Option> { + match should_visit_node(&mut self.states, node) { Ok(false) => return None, Err(path) => { - if let Some(idx) = stack.iter().position(|n| n == path) { - let mut cycle = stack.get(idx..).expect("slice").to_vec(); - cycle.push(path.clone()); + if let Some(idx) = self.stack.iter().position(|n| *n == path) { + let mut cycle: Vec = self + .stack + .get(idx..) + .expect("slice") + .iter() + .map(|p| (*p).to_path_buf()) + .collect(); + cycle.push(path.to_path_buf()); return Some(canonicalize_cycle(cycle)); } - return Some(vec![path.clone(), path.clone()]); + return Some(vec![path.to_path_buf(), path.to_path_buf()]); } Ok(true) => {} } - stack.push(node.clone()); + self.stack.push(node); - if let Some(cycle) = targets - .get(node) - .and_then(|edge| visit_dependencies(targets, &edge.inputs, stack, states)) + if let Some(edge) = self.targets.get(node) + && let Some(cycle) = self.visit_dependencies(&edge.inputs) { return Some(cycle); } - stack.pop(); - states.insert(node.clone(), VisitState::Visited); + self.stack.pop(); + self.states.insert(node, VisitState::Visited); None } - fn visit_dependencies( - targets: &HashMap, - deps: &[PathBuf], - stack: &mut Vec, - states: &mut HashMap, - ) -> Option> { + fn visit_dependencies(&mut self, deps: &'a [PathBuf]) -> Option> { for dep in deps { - if !targets.contains_key(dep) { + if let Some(&head) = self.stack.last() + && head == dep.as_path() + { + return Some(canonicalize_cycle(vec![dep.clone(), dep.clone()])); + } + + if !self.targets.contains_key(dep) { continue; } - if let Some(cycle) = visit(targets, dep, stack, states) { + if let Some(cycle) = self.visit_node(dep) { return Some(cycle); } } None } +} - let mut states = HashMap::new(); - let mut stack = Vec::new(); +fn find_cycle(targets: &HashMap) -> Option> { + // CycleDetector borrows paths from `targets`; `targets` must remain + // unmodified during detection to keep borrowed keys valid. + let mut detector = CycleDetector::new(targets); for node in targets.keys() { // Skip nodes we've already processed to avoid redundant traversal. - if states.contains_key(node) { + if detector.is_visited(node) { continue; } - if let Some(cycle) = visit(targets, node, &mut stack, &mut states) { + if let Some(cycle) = detector.visit_node(node) { return Some(cycle); } } @@ -570,7 +593,9 @@ fn canonicalize_cycle(mut cycle: Vec) -> Vec { .enumerate() .min_by(|(_, a), (_, b)| a.cmp(b)) .map_or(0, |(idx, _)| idx); - cycle.rotate_left(start); + if let Some(slice) = cycle.get_mut(..len) { + slice.rotate_left(start); + } if let (Some(first), Some(slot)) = (cycle.first().cloned(), cycle.get_mut(len)) { slot.clone_from(&first); } @@ -610,4 +635,40 @@ mod tests { let option_b = vec![PathBuf::from("b"), PathBuf::from("a"), PathBuf::from("b")]; assert!(cycle == option_a || cycle == option_b); } + + #[test] + fn canonicalize_cycle_rotates_smallest_node() { + let cycle = vec![ + PathBuf::from("c"), + PathBuf::from("a"), + PathBuf::from("b"), + PathBuf::from("c"), + ]; + let canonical = canonicalize_cycle(cycle); + let expected = vec![ + PathBuf::from("a"), + PathBuf::from("b"), + PathBuf::from("c"), + PathBuf::from("a"), + ]; + assert_eq!(canonical, expected); + } + + #[test] + fn canonicalize_cycle_handles_reverse_direction() { + let cycle = vec![ + PathBuf::from("c"), + PathBuf::from("b"), + PathBuf::from("a"), + PathBuf::from("c"), + ]; + let canonical = canonicalize_cycle(cycle); + let expected = vec![ + PathBuf::from("a"), + PathBuf::from("c"), + PathBuf::from("b"), + PathBuf::from("a"), + ]; + assert_eq!(canonical, expected); + } }