diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index ea15e8a9..9820e8e0 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -989,6 +989,14 @@ This transformation involves several steps: search maintaining a visitation state) on the dependency graph to fail compilation if a circular dependency is found. + 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. + ### 5.4 Ninja File Synthesis (`ninja_gen.rs`) The final step is to synthesize the `build.ninja` file from the `BuildGraph` @@ -1199,8 +1207,8 @@ libraries.[^27] # RuleNotFound { target_name: String, rule_name: String, }, - #[error("A circular dependency was detected involving target '{path}'.")] - CircularDependency { path: PathBuf, }, + #[error("circular dependency detected: {cycle:?}")] + CircularDependency { cycle: Vec, }, # DependencyNotFound { target_name: String, dependency_name: String, }, } diff --git a/docs/roadmap.md b/docs/roadmap.md index 46bfcbb5..8e1dc7b2 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -50,8 +50,8 @@ compilation pipeline from parsing to execution. referenced by a target is valid and that they are mutually exclusive. *(done)* - - [ ] Implement a cycle detection algorithm (e.g., depth-first search) to fail - compilation if a circular dependency is found in the target graph. + - [x] Implement a cycle detection algorithm (e.g., depth-first search) to fail + compilation if a circular dependency is found in the target graph. *(done)* - [ ] **Code Generation and Execution:** diff --git a/src/ir.rs b/src/ir.rs index 36b3deef..ca6bba19 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -93,6 +93,9 @@ pub enum IrGenError { #[error("duplicate target outputs: {outputs:?}")] DuplicateOutput { outputs: Vec }, + + #[error("circular dependency detected: {cycle:?}")] + CircularDependency { cycle: Vec }, } impl BuildGraph { @@ -110,6 +113,8 @@ impl BuildGraph { Self::process_targets(manifest, &mut graph.actions, &mut graph.targets, &rule_map)?; Self::process_defaults(manifest, &mut graph.default_targets); + graph.detect_cycles()?; + Ok(graph) } @@ -165,6 +170,13 @@ impl BuildGraph { defaults.push(PathBuf::from(name)); } } + + fn detect_cycles(&self) -> Result<(), IrGenError> { + if let Some(cycle) = find_cycle(&self.targets) { + return Err(IrGenError::CircularDependency { cycle }); + } + Ok(()) + } } fn register_action( @@ -267,3 +279,138 @@ fn get_target_display_name(paths: &[PathBuf]) -> String { .map(|p| p.display().to_string()) .unwrap_or_default() } + +#[derive(Clone, Copy)] +enum VisitState { + Visiting, + Visited, +} + +fn should_visit_node<'a>( + states: &'a mut HashMap, + node: &'a PathBuf, +) -> Result { + match states.get(node) { + Some(VisitState::Visited) => Ok(false), + Some(VisitState::Visiting) => Err(node), + None => { + states.insert(node.clone(), 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) { + 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()); + return Some(canonicalize_cycle(cycle)); + } + return Some(vec![path.clone(), path.clone()]); + } + Ok(true) => {} + } + + stack.push(node.clone()); + + if let Some(edge) = targets.get(node) + && let Some(cycle) = visit_dependencies(targets, &edge.inputs, stack, states) + { + return Some(cycle); + } + + stack.pop(); + states.insert(node.clone(), VisitState::Visited); + None + } + + fn visit_dependencies( + targets: &HashMap, + deps: &[PathBuf], + stack: &mut Vec, + states: &mut HashMap, + ) -> Option> { + for dep in deps { + if targets.contains_key(dep) + && let Some(cycle) = visit(targets, dep, stack, states) + { + return Some(cycle); + } + } + None + } + + let mut states = HashMap::new(); + let mut stack = Vec::new(); + + for node in targets.keys() { + if !states.contains_key(node) + && let Some(cycle) = visit(targets, node, &mut stack, &mut states) + { + return Some(cycle); + } + } + None +} + +fn canonicalize_cycle(mut cycle: Vec) -> Vec { + if cycle.len() < 2 { + return cycle; + } + let len = cycle.len() - 1; + let start = cycle + .iter() + .take(len) + .enumerate() + .min_by(|(_, a), (_, b)| a.cmp(b)) + .map_or(0, |(idx, _)| idx); + cycle.rotate_left(start); + if let (Some(first), Some(slot)) = (cycle.first().cloned(), cycle.get_mut(len)) { + slot.clone_from(&first); + } + cycle +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn find_cycle_identifies_cycle() { + let mut targets = HashMap::new(); + let edge_a = BuildEdge { + action_id: "id".into(), + inputs: vec![PathBuf::from("b")], + explicit_outputs: vec![PathBuf::from("a")], + implicit_outputs: Vec::new(), + order_only_deps: Vec::new(), + phony: false, + always: false, + }; + let edge_b = BuildEdge { + action_id: "id".into(), + inputs: vec![PathBuf::from("a")], + explicit_outputs: vec![PathBuf::from("b")], + implicit_outputs: Vec::new(), + order_only_deps: Vec::new(), + phony: false, + always: false, + }; + targets.insert(PathBuf::from("a"), edge_a); + targets.insert(PathBuf::from("b"), edge_b); + + let cycle = find_cycle(&targets).expect("cycle"); + let option_a = vec![PathBuf::from("a"), PathBuf::from("b"), PathBuf::from("a")]; + let option_b = vec![PathBuf::from("b"), PathBuf::from("a"), PathBuf::from("b")]; + assert!(cycle == option_a || cycle == option_b); + } +} diff --git a/tests/data/circular.yml b/tests/data/circular.yml new file mode 100644 index 00000000..024fb55a --- /dev/null +++ b/tests/data/circular.yml @@ -0,0 +1,12 @@ +netsuke_version: "1.0.0" +targets: + - name: a + sources: b + recipe: + kind: command + command: "touch a" + - name: b + sources: a + recipe: + kind: command + command: "touch b" diff --git a/tests/features/ir.feature b/tests/features/ir.feature index 9ff8d7e3..c4bd70b0 100644 --- a/tests/features/ir.feature +++ b/tests/features/ir.feature @@ -28,3 +28,7 @@ Feature: BuildGraph Scenario: Duplicate target outputs When the manifest file "tests/data/duplicate_outputs.yml" is compiled to IR Then IR generation fails + + Scenario: Circular dependency detection + When the manifest file "tests/data/circular.yml" is compiled to IR + Then IR generation fails diff --git a/tests/ir_from_manifest_tests.rs b/tests/ir_from_manifest_tests.rs index 597ba75e..3bc08c04 100644 --- a/tests/ir_from_manifest_tests.rs +++ b/tests/ir_from_manifest_tests.rs @@ -5,6 +5,7 @@ use netsuke::{ manifest, }; use rstest::rstest; +use std::path::PathBuf; #[rstest] fn minimal_manifest_to_ir() { @@ -38,6 +39,7 @@ enum ExpectedError { }, EmptyRule(String), RuleNotFound(String), + CircularDependency(Vec), } #[rstest] @@ -64,6 +66,10 @@ enum ExpectedError { "tests/data/rule_not_found.yml", ExpectedError::RuleNotFound("missing_rule".into()) )] +#[case( + "tests/data/circular.yml", + ExpectedError::CircularDependency(vec!["a".into(), "b".into(), "a".into()]) +)] fn manifest_error_cases(#[case] manifest_path: &str, #[case] expected: ExpectedError) { let manifest = manifest::from_path(manifest_path).expect("load"); let err = BuildGraph::from_manifest(&manifest).expect_err("error"); @@ -87,6 +93,16 @@ fn manifest_error_cases(#[case] manifest_path: &str, #[case] expected: ExpectedE (IrGenError::RuleNotFound { rule_name, .. }, ExpectedError::RuleNotFound(exp_rule)) => { assert_eq!(rule_name, exp_rule); } + ( + IrGenError::CircularDependency { cycle }, + ExpectedError::CircularDependency(exp_cycle), + ) => { + let mut expected: Vec = exp_cycle.iter().map(PathBuf::from).collect(); + let mut actual = cycle; + expected.sort(); + actual.sort(); + assert_eq!(actual, expected); + } (other, exp) => panic!("expected {exp:?} but got {other:?}"), } }