Implement cycle detection in IR

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<PathBuf>, },
 
       #
       DependencyNotFound { target_name: String, dependency_name: String, }, }

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:**
 

src/ir.rs

@@ -93,6 +93,9 @@ pub enum IrGenError {
 
     #[error("duplicate target outputs: {outputs:?}")]
     DuplicateOutput { outputs: Vec<String> },
+
+    #[error("circular dependency detected: {cycle:?}")]
+    CircularDependency { cycle: Vec<PathBuf> },
 }
 
 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<PathBuf, VisitState>,
+    node: &'a PathBuf,
+) -> Result<bool, &'a PathBuf> {
+    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<PathBuf, BuildEdge>) -> Option<Vec<PathBuf>> {
+    fn visit(
+        targets: &HashMap<PathBuf, BuildEdge>,
+        node: &PathBuf,
+        stack: &mut Vec<PathBuf>,
+        states: &mut HashMap<PathBuf, VisitState>,
+    ) -> Option<Vec<PathBuf>> {
+        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<PathBuf, BuildEdge>,
+        deps: &[PathBuf],
+        stack: &mut Vec<PathBuf>,
+        states: &mut HashMap<PathBuf, VisitState>,
+    ) -> Option<Vec<PathBuf>> {
+        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<PathBuf>) -> Vec<PathBuf> {
+    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);
+    }
+}

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"

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

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<String>),
 }
 
 #[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<PathBuf> = 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:?}"),
     }
 }