Fix dependency tracking and show shortest path dependency cycle (#151)

Author: jfrolich

src/build.rs

@@ -330,7 +330,7 @@ pub fn incremental_build(
                 pb.finish();
             }
 
-            log::error!("Could not parse source files: {}", &err);
+            println!("Could not parse source files: {}", &err);
             return Err(IncrementalBuildError::SourceFileParseError);
         }
     }
@@ -405,7 +405,7 @@ pub fn incremental_build(
     pb.finish();
     if !compile_errors.is_empty() {
         if helpers::contains_ascii_characters(&compile_warnings) {
-            log::error!("{}", &compile_warnings);
+            println!("{}", &compile_warnings);
         }
         if show_progress {
             println!(
@@ -417,7 +417,7 @@ pub fn incremental_build(
                 default_timing.unwrap_or(compile_duration).as_secs_f64()
             );
         }
-        log::error!("{}", &compile_errors);
+        println!("{}", &compile_errors);
         // mark the original files as dirty again, because we didn't complete a full build
         for (module_name, module) in build_state.modules.iter_mut() {
             if tracked_dirty_modules.contains(module_name) {

src/build/build_types.rs

@@ -70,6 +70,7 @@ pub struct Module {
     pub compile_dirty: bool,
     pub last_compiled_cmi: Option<SystemTime>,
     pub last_compiled_cmt: Option<SystemTime>,
+    pub deps_dirty: bool,
 }
 
 impl Module {

src/build/compile/dependency_cycle.rs

@@ -1,71 +1,153 @@
 use super::super::build_types::*;
 use crate::helpers;
 use ahash::AHashSet;
+use std::collections::{HashMap, HashSet, VecDeque};
 
 pub fn find(modules: &Vec<(&String, &Module)>) -> Vec<String> {
-    let mut visited: AHashSet<String> = AHashSet::new();
-    let mut stack: Vec<String> = vec![];
+    find_shortest_cycle(modules)
+}
 
-    // we want to sort the module names so that we always return the same
-    // dependency cycle (there can be more than one)
-    let mut module_names = modules
-        .iter()
-        .map(|(name, _)| name.to_string())
-        .collect::<Vec<String>>();
+fn find_shortest_cycle(modules: &Vec<(&String, &Module)>) -> Vec<String> {
+    let mut shortest_cycle: Vec<String> = Vec::new();
+
+    // Build a graph representation for easier traversal
 
-    module_names.sort();
-    for module_name in module_names {
-        if find_dependency_cycle_helper(&module_name, modules, &mut visited, &mut stack) {
-            return stack;
+    let mut graph: HashMap<&String, &AHashSet<String>> = HashMap::new();
+    let mut in_degrees: HashMap<&String, usize> = HashMap::new();
+
+    let empty = AHashSet::new();
+    // First pass: collect all nodes and initialize in-degrees
+    for (name, _) in modules {
+        graph.insert(name, &empty);
+        in_degrees.insert(name, 0);
+    }
+
+    // Second pass: build the graph and count in-degrees
+    for (name, module) in modules {
+        // Update in-degrees
+        for dep in module.deps.iter() {
+            if let Some(count) = in_degrees.get_mut(dep) {
+                *count += 1;
+            }
         }
-        visited.clear();
-        stack.clear();
+
+        // Update the graph
+        *graph.get_mut(*name).unwrap() = &module.deps;
     }
-    stack
-}
+    // Remove all nodes in the graph that have no incoming edges
+    graph.retain(|_, deps| !deps.is_empty());
 
-fn find_dependency_cycle_helper(
-    module_name: &String,
-    modules: &Vec<(&String, &Module)>,
-    visited: &mut AHashSet<String>,
-    stack: &mut Vec<String>,
-) -> bool {
-    if let Some(module) = modules
-        .iter()
-        .find(|(name, _)| *name == module_name)
-        .map(|(_, module)| module)
-    {
-        visited.insert(module_name.to_string());
-        // if the module is a mlmap (namespace), we don't want to show this in the path
-        // because the namespace is not a module the user created, so only add source files
-        // to the stack
-        if let SourceType::SourceFile(_) = module.source_type {
-            stack.push(module_name.to_string())
+    // OPTIMIZATION 1: Start with nodes that are more likely to be in cycles
+    // Sort nodes by their connectivity (in-degree + out-degree)
+    let mut start_nodes: Vec<&String> = graph.keys().cloned().collect();
+    start_nodes.sort_by(|a, b| {
+        let a_connectivity = in_degrees.get(a).unwrap_or(&0) + graph.get(a).map_or(0, |v| v.len());
+        let b_connectivity = in_degrees.get(b).unwrap_or(&0) + graph.get(b).map_or(0, |v| v.len());
+        b_connectivity.cmp(&a_connectivity) // Sort in descending order
+    });
+
+    // OPTIMIZATION 2: Keep track of the current shortest cycle length for early termination
+    let mut current_shortest_length = usize::MAX;
+
+    // OPTIMIZATION 3: Cache nodes that have been checked and don't have cycles
+    let mut no_cycle_cache: HashSet<String> = HashSet::new();
+
+    // Try BFS from each node to find the shortest cycle
+    for start_node in start_nodes {
+        // Skip nodes that we know don't have cycles
+        if no_cycle_cache.contains(start_node) {
+            continue;
         }
-        for dep in &module.deps {
-            if !visited.contains(dep) {
-                if find_dependency_cycle_helper(dep, modules, visited, stack) {
-                    return true;
+
+        // Skip nodes with no incoming edges
+        if in_degrees.get(&start_node).map_or(true, |&d| d == 0) {
+            no_cycle_cache.insert(start_node.clone());
+            continue;
+        }
+
+        if let Some(cycle) = find_cycle_bfs(&start_node, &graph, current_shortest_length) {
+            if shortest_cycle.is_empty() || cycle.len() < shortest_cycle.len() {
+                shortest_cycle = cycle.clone();
+                current_shortest_length = cycle.len();
+
+                // OPTIMIZATION 4: If we find a very short cycle (length <= 3), we can stop early
+                // as it's unlikely to find a shorter one
+                if cycle.len() <= 3 {
+                    break;
                 }
-            } else if stack.contains(dep) {
-                stack.push(dep.to_string());
-                return true;
             }
+        } else {
+            // Cache this node as not having a cycle
+            no_cycle_cache.insert(start_node.to_string());
         }
-        // because we only pushed source files to the stack, we also only need to
-        // pop these from the stack if we don't find a dependency cycle
-        if let SourceType::SourceFile(_) = module.source_type {
-            let _ = stack.pop();
+    }
+
+    shortest_cycle
+}
+
+fn find_cycle_bfs(
+    start: &String,
+    graph: &HashMap<&String, &AHashSet<String>>,
+    max_length: usize,
+) -> Option<Vec<String>> {
+    // Use a BFS to find the shortest cycle
+    let mut queue = VecDeque::new();
+    // Store node -> (distance, parent)
+    let mut visited: HashMap<String, (usize, Option<String>)> = HashMap::new();
+
+    // Initialize with start node
+    visited.insert(start.clone(), (0, None));
+    queue.push_back(start.clone());
+
+    while let Some(current) = queue.pop_front() {
+        let (dist, _) = *visited.get(&current).unwrap();
+
+        // OPTIMIZATION: Early termination if we've gone too far
+        // If we're already at max_length, we won't find a shorter cycle from here
+        if dist >= max_length - 1 {
+            continue;
+        }
+
+        // Check all neighbors
+        if let Some(neighbors) = graph.get(&current) {
+            for neighbor in neighbors.iter() {
+                // If we found the start node again, we have a cycle
+                if neighbor == start {
+                    // Reconstruct the cycle
+                    let mut path = Vec::new();
+                    path.push(start.clone());
+
+                    // Backtrack from current to start using parent pointers
+                    let mut curr = current.clone();
+                    while curr != *start {
+                        path.push(curr.clone());
+                        curr = visited.get(&curr).unwrap().1.clone().unwrap();
+                    }
+
+                    return Some(path);
+                }
+
+                // If not visited, add to queue
+                if !visited.contains_key(neighbor) {
+                    visited.insert(neighbor.clone(), (dist + 1, Some(current.clone())));
+                    queue.push_back(neighbor.clone());
+                }
+            }
         }
-        return false;
     }
-    false
+
+    None
 }
 
 pub fn format(cycle: &[String]) -> String {
+    let mut cycle = cycle.to_vec();
+    cycle.reverse();
+    // add the first module to the end of the cycle
+    cycle.push(cycle[0].clone());
+
     cycle
         .iter()
         .map(|s| helpers::format_namespaced_module_name(s))
         .collect::<Vec<String>>()
-        .join(" -> ")
+        .join("\n → ")
 }

src/build/deps.rs

@@ -1,5 +1,4 @@
 use super::build_types::*;
-use super::is_dirty;
 use super::packages;
 use crate::helpers;
 use ahash::AHashSet;
@@ -76,7 +75,7 @@ pub fn get_deps(build_state: &mut BuildState, deleted_modules: &AHashSet<String>
                     .get_package(&module.package_name)
                     .expect("Package not found");
                 let ast_path = package.get_ast_path(&source_file.implementation.path);
-                if is_dirty(module) || !build_state.deps_initialized {
+                if module.deps_dirty || !build_state.deps_initialized {
                     let mut deps = get_dep_modules(
                         &ast_path,
                         package.namespace.to_suffix(),
@@ -114,6 +113,7 @@ pub fn get_deps(build_state: &mut BuildState, deleted_modules: &AHashSet<String>
         .for_each(|(module_name, deps)| {
             if let Some(module) = build_state.modules.get_mut(&module_name) {
                 module.deps = deps.clone();
+                module.deps_dirty = false;
             }
             deps.iter().for_each(|dep_name| {
                 if let Some(module) = build_state.modules.get_mut(dep_name) {

src/build/packages.rs

@@ -645,6 +645,7 @@ pub fn parse_packages(build_state: &mut BuildState) {
                 build_state.insert_module(
                     &helpers::file_path_to_module_name(&mlmap.to_owned(), &packages::Namespace::NoNamespace),
                     Module {
+                        deps_dirty: false,
                         source_type: SourceType::MlMap(MlMap { parse_dirty: false }),
                         deps,
                         dependents: AHashSet::new(),
@@ -685,6 +686,7 @@ pub fn parse_packages(build_state: &mut BuildState) {
                                 }
                             })
                             .or_insert(Module {
+                                deps_dirty: true,
                                 source_type: SourceType::SourceFile(SourceFile {
                                     implementation: Implementation {
                                         path: file.to_owned(),
@@ -732,6 +734,7 @@ pub fn parse_packages(build_state: &mut BuildState) {
                                         }
                                     })
                                     .or_insert(Module {
+                                        deps_dirty: true,
                                         source_type: SourceType::SourceFile(SourceFile {
                                             // this will be overwritten later
                                             implementation: Implementation {

src/build/parse.rs

@@ -98,6 +98,7 @@ pub fn generate_asts(
                 // the compile_dirty flag if it was set before
                 if is_dirty {
                     module.compile_dirty = true;
+                    module.deps_dirty = true;
                 }
                 let package = build_state
                     .packages
@@ -113,9 +114,6 @@ pub fn generate_asts(
                         Ok((_path, Some(stderr_warnings))) if package.is_pinned_dep => {
                             source_file.implementation.parse_state = ParseState::Warning;
                             source_file.implementation.parse_dirty = true;
-                            if let Some(interface) = source_file.interface.as_mut() {
-                                interface.parse_dirty = false;
-                            }
                             logs::append(package, &stderr_warnings);
                             stderr.push_str(&stderr_warnings);
                         }
@@ -124,9 +122,6 @@ pub fn generate_asts(
                             // dependency (so some external dep). We can ignore those
                             source_file.implementation.parse_state = ParseState::Success;
                             source_file.implementation.parse_dirty = false;
-                            if let Some(interface) = source_file.interface.as_mut() {
-                                interface.parse_dirty = false;
-                            }
                         }
                         Err(err) => {
                             // Some compilation error

src/main.rs

@@ -114,7 +114,7 @@ fn main() -> Result<()> {
 
     match lock::get(&folder) {
         lock::Lock::Error(ref e) => {
-            log::error!("Could not start Rewatch: {e}");
+            println!("Could not start Rewatch: {e}");
             std::process::exit(1)
         }
         lock::Lock::Aquired(_) => match command {
@@ -130,7 +130,7 @@ fn main() -> Result<()> {
                     args.dev,
                 ) {
                     Err(e) => {
-                        log::error!("{e}");
+                        println!("{e}");
                         std::process::exit(1)
                     }
                     Ok(_) => {

src/watcher.rs

@@ -282,7 +282,7 @@ pub fn start(
         )
         .await
         {
-            log::error!("{:?}", e)
+            println!("{:?}", e)
         }
     })
 }

tests/snapshots/dependency-cycle.txt

@@ -1,18 +1,16 @@
-[1/7] 📦 Building package tree...
-[1/7] 📦 Built package tree in 0.00s
-[2/7] 🕵️ Finding source files...
-[2/7] 🕵️ Found source files in 0.00s
-[3/7] 📝 Reading compile state...
-[3/7] 📝 Read compile state 0.00s
-[4/7] 🧹 Cleaning up previous build...
-[4/7] 🧹 Cleaned 0/11 0.00s
+[1/7] 📦 Building package tree...[1/7] 📦 Built package tree in 0.00s
+[2/7] 👀 Finding source files...[2/7] 👀 Found source files in 0.00s
+[3/7] 📝 Reading compile state...[3/7] 📝 Read compile state 0.00s
+[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 0/11 0.00s
 [5/7] 🧱 Parsed 1 source files in 0.00s
-[6/7] ️🌴 Collected deps in 0.00s
-[7/7] ️🛑 Compiled 0 modules in 0.00s
-ERROR:
+[6/7] 🌴 Collected deps in 0.00s
+[7/7] ❌ Compiled 0 modules in 0.00s
 
 Can't continue... Found a circular dependency in your code:
-NewNamespace.NS_alias -> Dep01 -> Dep02 -> NS -> NewNamespace.NS_alias
+Dep01
+ → Dep02
+ → NS
+ → NewNamespace.NS_alias
+ → Dep01
 
-ERROR:
-Incremental build failed. Error:   ️🛑 Failed to Compile. See Errors Above
+Incremental build failed. Error:   ❌ Failed to Compile. See Errors Above

tests/snapshots/remove-file.txt

@@ -1,15 +1,10 @@
-[1/7] 📦 Building package tree...
-[1/7] 📦 Built package tree in 0.00s
-[2/7] 🕵️ Finding source files...
-[2/7] 🕵️ Found source files in 0.00s
-[3/7] 📝 Reading compile state...
-[3/7] 📝 Read compile state 0.00s
-[4/7] 🧹 Cleaning up previous build...
-[4/7] 🧹 Cleaned 1/11 0.00s
+[1/7] 📦 Building package tree...[1/7] 📦 Built package tree in 0.00s
+[2/7] 👀 Finding source files...[2/7] 👀 Found source files in 0.00s
+[3/7] 📝 Reading compile state...[3/7] 📝 Read compile state 0.00s
+[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 1/11 0.00s
 [5/7] 🧱 Parsed 0 source files in 0.00s
-[6/7] ️🌴 Collected deps in 0.00s
-[7/7] ️🛑 Compiled 1 modules in 0.00s
-ERROR:
+[6/7] 🌴 Collected deps in 0.00s
+[7/7] ❌ Compiled 1 modules in 0.00s
 
   We've found a bug for you!
   /packages/dep01/src/Dep01.res:3:9-17
@@ -27,5 +22,4 @@ ERROR:
 
 
 
-ERROR:
-Incremental build failed. Error:   ️🛑 Failed to Compile. See Errors Above
+Incremental build failed. Error:   ❌ Failed to Compile. See Errors Above