improve errors for circular dependencies (#7940)

* improve errors for circular dependencies

* changelog

* fix

* Update rewatch/src/build/compile/dependency_cycle.rs

Co-authored-by: Copilot <[email protected]>

* remove

* fix error message

* remove replace

---------

Co-authored-by: Copilot <[email protected]>

Author: zth

CHANGELOG.md

@@ -24,6 +24,8 @@
 
 #### :nail_care: Polish
 
+- Improve circular dependency errors, and make sure they end up in the compiler log so the editor tooling can surface them. https://github.com/rescript-lang/rescript/pull/7940
+
 #### :house: Internal
 
 - Use AST nodes with locations for fn arguments in the typed tree. https://github.com/rescript-lang/rescript/pull/7873

rewatch/src/build/compile.rs

@@ -356,11 +356,28 @@ pub fn compile(
                     .collect::<Vec<(&String, &Module)>>(),
             );
 
-            compile_errors.push_str(&format!(
-                "\n{}\n{}\n",
+            let guidance = "Possible solutions:\n- Extract shared code into a new module both depend on.\n";
+            let message = format!(
+                "\n{}\n{}\n{}",
                 style("Can't continue... Found a circular dependency in your code:").red(),
-                dependency_cycle::format(&cycle)
-            ))
+                dependency_cycle::format(&cycle, build_state),
+                guidance
+            );
+
+            // Append the error to the logs of all packages involved in the cycle,
+            // so editor tooling can surface it from .compiler.log
+            let mut touched_packages = AHashSet::<String>::new();
+            for module_name in cycle.iter() {
+                if let Some(module) = build_state.get_module(module_name) {
+                    if touched_packages.insert(module.package_name.clone()) {
+                        if let Some(package) = build_state.get_package(&module.package_name) {
+                            logs::append(package, &message);
+                        }
+                    }
+                }
+            }
+
+            compile_errors.push_str(&message)
         }
         if !compile_errors.is_empty() {
             break;

rewatch/src/build/compile/dependency_cycle.rs

@@ -1,7 +1,10 @@
 use super::super::build_types::*;
 use crate::helpers;
 use ahash::AHashSet;
-use std::collections::{HashMap, HashSet, VecDeque};
+use std::{
+    collections::{HashMap, HashSet, VecDeque},
+    path::Path,
+};
 
 pub fn find(modules: &Vec<(&String, &Module)>) -> Vec<String> {
     find_shortest_cycle(modules)
@@ -139,15 +142,43 @@ fn find_cycle_bfs(
     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());
+pub fn format(cycle: &[String], build_state: &BuildCommandState) -> String {
+    let mut nodes = cycle.to_vec();
+    if nodes.is_empty() {
+        return String::new();
+    }
+    nodes.reverse();
+    nodes.push(nodes[0].clone());
+
+    let root_path = &build_state.get_root_config().path;
+    let root = root_path.parent().unwrap_or(root_path.as_path());
 
-    cycle
+    nodes
         .iter()
-        .map(|s| helpers::format_namespaced_module_name(s))
+        .map(|name| {
+            let display_name = helpers::format_namespaced_module_name(name);
+
+            match build_state.get_module(name) {
+                Some(Module {
+                    source_type: SourceType::SourceFile(source_file),
+                    package_name,
+                    ..
+                }) => {
+                    if let Some(package) = build_state.get_package(package_name) {
+                        let abs_path = Path::new(&package.path).join(&source_file.implementation.path);
+                        let rel_path = abs_path.strip_prefix(root).unwrap_or(&abs_path).to_string_lossy();
+                        format!("{display_name} ({rel_path})")
+                    } else {
+                        display_name
+                    }
+                }
+                Some(Module {
+                    source_type: SourceType::MlMap(_),
+                    ..
+                })
+                | None => display_name,
+            }
+        })
         .collect::<Vec<String>>()
         .join("\n → ")
 }

rewatch/tests/snapshots/dependency-cycle.txt

@@ -12,10 +12,12 @@ The field 'bsc-flags' found in the package config of '@testrepo/deprecated-confi
 Use 'compiler-flags' instead.
 
 Can't continue... Found a circular dependency in your code:
-Dep01
- → Dep02
- → NS
- → NewNamespace.NS_alias
- → Dep01
+Dep01 (packages/dep01/src/Dep01.res)
+ → Dep02 (packages/dep02/src/Dep02.res)
+ → NS (packages/new-namespace/src/NS.res)
+ → NewNamespace.NS_alias (packages/new-namespace/src/NS_alias.res)
+ → Dep01 (packages/dep01/src/Dep01.res)
+Possible solutions:
+- Extract shared code into a new module both depend on.
 
 Incremental build failed. Error:   Failed to Compile. See Errors Above