From 465eccdeb62dde6f9a2ecf70dd71917268359b4d Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 7 Oct 2025 13:33:30 +0200 Subject: [PATCH 1/7] improve errors for circular dependencies --- rewatch/src/build/compile.rs | 25 +++++++-- rewatch/src/build/compile/dependency_cycle.rs | 51 ++++++++++++++++--- rewatch/tests/snapshots/dependency-cycle.txt | 14 +++-- 3 files changed, 73 insertions(+), 17 deletions(-) diff --git a/rewatch/src/build/compile.rs b/rewatch/src/build/compile.rs index c48457a5e1..d6e3eadf94 100644 --- a/rewatch/src/build/compile.rs +++ b/rewatch/src/build/compile.rs @@ -356,11 +356,28 @@ pub fn compile( .collect::>(), ); - compile_errors.push_str(&format!( - "\n{}\n{}\n", + let guidance = "\nHow to fix:\n- Extract shared code into a new module both depend on.\n- If a namespace entry is part of the cycle, import submodules directly instead of the entry.\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::::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; diff --git a/rewatch/src/build/compile/dependency_cycle.rs b/rewatch/src/build/compile/dependency_cycle.rs index a048d0ad8d..a8da483cae 100644 --- a/rewatch/src/build/compile/dependency_cycle.rs +++ b/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 { find_shortest_cycle(modules) @@ -139,15 +142,47 @@ 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 { + // Mirror dependency_cycle::format semantics: reverse then close the loop + let mut nodes = cycle.to_vec(); + if nodes.is_empty() { + return String::new(); + } + nodes.reverse(); + nodes.push(nodes[0].clone()); + + let root = build_state.get_root_config().path.parent().unwrap(); - 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() + .replace('\\', "/"); + format!("{} ({})", display_name, rel_path) + } else { + display_name + } + } + Some(Module { + source_type: SourceType::MlMap(_), + .. + }) + | None => display_name, + } + }) .collect::>() .join("\n → ") } diff --git a/rewatch/tests/snapshots/dependency-cycle.txt b/rewatch/tests/snapshots/dependency-cycle.txt index 7bf2339c9b..2c7a252f9f 100644 --- a/rewatch/tests/snapshots/dependency-cycle.txt +++ b/rewatch/tests/snapshots/dependency-cycle.txt @@ -12,10 +12,14 @@ 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) + +How to fix: +- Extract shared code into a new module both depend on. +- If a namespace entry is part of the cycle, import submodules directly instead of the entry. Incremental build failed. Error:  Failed to Compile. See Errors Above From cb5bfd3025456c65eba35cd3f223f429bf31627a Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 7 Oct 2025 13:36:06 +0200 Subject: [PATCH 2/7] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ed53baa56..deb232d09b 100644 --- a/CHANGELOG.md +++ b/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 From 41317cb28068437be51f4277d55e719987adeba5 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 7 Oct 2025 13:44:18 +0200 Subject: [PATCH 3/7] fix --- rewatch/src/build/compile/dependency_cycle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rewatch/src/build/compile/dependency_cycle.rs b/rewatch/src/build/compile/dependency_cycle.rs index a8da483cae..0e713e5d51 100644 --- a/rewatch/src/build/compile/dependency_cycle.rs +++ b/rewatch/src/build/compile/dependency_cycle.rs @@ -171,7 +171,7 @@ pub fn format(cycle: &[String], build_state: &BuildCommandState) -> String { .unwrap_or(&abs_path) .to_string_lossy() .replace('\\', "/"); - format!("{} ({})", display_name, rel_path) + format!("{display_name} ({rel_path})") } else { display_name } From cf90d0d9168bd012c3e1893701e809a0dfaba1c0 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 7 Oct 2025 13:45:11 +0200 Subject: [PATCH 4/7] Update rewatch/src/build/compile/dependency_cycle.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- rewatch/src/build/compile/dependency_cycle.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rewatch/src/build/compile/dependency_cycle.rs b/rewatch/src/build/compile/dependency_cycle.rs index 0e713e5d51..5977b51658 100644 --- a/rewatch/src/build/compile/dependency_cycle.rs +++ b/rewatch/src/build/compile/dependency_cycle.rs @@ -151,7 +151,8 @@ pub fn format(cycle: &[String], build_state: &BuildCommandState) -> String { nodes.reverse(); nodes.push(nodes[0].clone()); - let root = build_state.get_root_config().path.parent().unwrap(); + let root_path = &build_state.get_root_config().path; + let root = root_path.parent().unwrap_or(root_path.as_path()); nodes .iter() From e0046d9b00b44178f0ae5b99d879b425c2f83a37 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 7 Oct 2025 14:08:38 +0200 Subject: [PATCH 5/7] remove --- rewatch/src/build/compile/dependency_cycle.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rewatch/src/build/compile/dependency_cycle.rs b/rewatch/src/build/compile/dependency_cycle.rs index 5977b51658..396333fae1 100644 --- a/rewatch/src/build/compile/dependency_cycle.rs +++ b/rewatch/src/build/compile/dependency_cycle.rs @@ -143,7 +143,6 @@ fn find_cycle_bfs( } pub fn format(cycle: &[String], build_state: &BuildCommandState) -> String { - // Mirror dependency_cycle::format semantics: reverse then close the loop let mut nodes = cycle.to_vec(); if nodes.is_empty() { return String::new(); From 6c67205633735257ccdb1b084723c2840638b561 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 7 Oct 2025 18:30:50 +0200 Subject: [PATCH 6/7] fix error message --- rewatch/src/build/compile.rs | 2 +- rewatch/tests/snapshots/dependency-cycle.txt | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/rewatch/src/build/compile.rs b/rewatch/src/build/compile.rs index d6e3eadf94..8048764f09 100644 --- a/rewatch/src/build/compile.rs +++ b/rewatch/src/build/compile.rs @@ -356,7 +356,7 @@ pub fn compile( .collect::>(), ); - let guidance = "\nHow to fix:\n- Extract shared code into a new module both depend on.\n- If a namespace entry is part of the cycle, import submodules directly instead of the entry.\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(), diff --git a/rewatch/tests/snapshots/dependency-cycle.txt b/rewatch/tests/snapshots/dependency-cycle.txt index 2c7a252f9f..b5bf195403 100644 --- a/rewatch/tests/snapshots/dependency-cycle.txt +++ b/rewatch/tests/snapshots/dependency-cycle.txt @@ -17,9 +17,7 @@ Dep01 (packages/dep01/src/Dep01.res) → NS (packages/new-namespace/src/NS.res) → NewNamespace.NS_alias (packages/new-namespace/src/NS_alias.res) → Dep01 (packages/dep01/src/Dep01.res) - -How to fix: +Possible solutions: - Extract shared code into a new module both depend on. -- If a namespace entry is part of the cycle, import submodules directly instead of the entry. Incremental build failed. Error:  Failed to Compile. See Errors Above From 948f64d79897ffd5d6a65f524bceb795b2cfe437 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 8 Oct 2025 08:32:54 +0200 Subject: [PATCH 7/7] remove replace --- rewatch/src/build/compile/dependency_cycle.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/rewatch/src/build/compile/dependency_cycle.rs b/rewatch/src/build/compile/dependency_cycle.rs index 396333fae1..7fa763908b 100644 --- a/rewatch/src/build/compile/dependency_cycle.rs +++ b/rewatch/src/build/compile/dependency_cycle.rs @@ -166,11 +166,7 @@ pub fn format(cycle: &[String], build_state: &BuildCommandState) -> String { }) => { 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() - .replace('\\', "/"); + let rel_path = abs_path.strip_prefix(root).unwrap_or(&abs_path).to_string_lossy(); format!("{display_name} ({rel_path})") } else { display_name