From 50467ae679708884dcb3aae1b9b3d74fb87b3034 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Tue, 23 Dec 2025 17:11:10 +0100 Subject: [PATCH 1/6] Fix rewatch panic on duplicate module name --- rewatch/src/build.rs | 2 +- rewatch/src/build/clean.rs | 2 +- rewatch/src/build/packages.rs | 408 +++++++++--------- rewatch/tests/compile.sh | 8 + .../tests/snapshots/duplicate-module-name.txt | 1 + 5 files changed, 218 insertions(+), 203 deletions(-) create mode 100644 rewatch/tests/snapshots/duplicate-module-name.txt diff --git a/rewatch/src/build.rs b/rewatch/src/build.rs index 5c381d3239..2f4d5ca5d7 100644 --- a/rewatch/src/build.rs +++ b/rewatch/src/build.rs @@ -147,7 +147,7 @@ pub fn initialize_build( } let mut build_state = BuildCommandState::new(project_context, packages, compiler, warn_error); - packages::parse_packages(&mut build_state); + packages::parse_packages(&mut build_state)?; let compile_assets_state = read_compile_state::read(&mut build_state)?; diff --git a/rewatch/src/build/clean.rs b/rewatch/src/build/clean.rs index 13bc797ed9..f114dca46c 100644 --- a/rewatch/src/build/clean.rs +++ b/rewatch/src/build/clean.rs @@ -366,7 +366,7 @@ pub fn clean(path: &Path, show_progress: bool, plain_output: bool) -> Result<()> let timing_clean_mjs = Instant::now(); let mut build_state = BuildState::new(project_context, packages, compiler_info); - packages::parse_packages(&mut build_state); + packages::parse_packages(&mut build_state)?; let root_config = build_state.get_root_config(); let suffix_for_print = match root_config.package_specs { None => match &root_config.suffix { diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index c9ab7ae65b..2f05d8de5a 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -10,8 +10,9 @@ use crate::project_context::{MonoRepoContext, ProjectContext}; use ahash::{AHashMap, AHashSet}; use anyhow::{Result, anyhow}; use console::style; -use log::{debug, error}; +use log::debug; use rayon::prelude::*; +use std::collections::hash_map::Entry; use std::error; use std::fs::{self}; use std::hash::{Hash, Hasher}; @@ -625,150 +626,153 @@ pub fn make( Ok(result) } -pub fn parse_packages(build_state: &mut BuildState) { - build_state - .packages - .clone() - .iter() - .for_each(|(package_name, package)| { - debug!("Parsing package: {package_name}"); - if let Some(package_modules) = package.modules.to_owned() { - build_state.module_names.extend(package_modules) - } - let build_path_abs = package.get_build_path(); - let bs_build_path = package.get_ocaml_build_path(); - helpers::create_path(&build_path_abs); - helpers::create_path(&bs_build_path); - let root_config = build_state.get_root_config(); - - root_config.get_package_specs().iter().for_each(|spec| { - if !spec.in_source { - // we don't want to calculate this if we don't have out of source specs - // we do this twice, but we almost never have multiple package specs - // so this optimization is less important - let relative_dirs: AHashSet = match &package.source_files { - Some(source_files) => source_files - .keys() - .map(|source_file| { - Path::new(source_file) - .parent() - .expect("parent dir not found") - .to_owned() - }) - .collect(), - _ => AHashSet::new(), - }; - if spec.is_common_js() { - helpers::create_path(&package.get_js_path()); - relative_dirs.iter().for_each(|path_buf| { - helpers::create_path_for_path(&Path::join(&package.get_js_path(), path_buf)) - }) - } else { - helpers::create_path(&package.get_es6_path()); - relative_dirs.iter().for_each(|path_buf| { - helpers::create_path_for_path(&Path::join(&package.get_es6_path(), path_buf)) - }) - } - } - }); - - package.namespace.to_suffix().iter().for_each(|namespace| { - // generate the mlmap "AST" file for modules that have a namespace configured - let source_files = match package.source_files.to_owned() { +pub fn parse_packages(build_state: &mut BuildState) -> Result<()> { + let packages = build_state.packages.clone(); + for (package_name, package) in packages.iter() { + debug!("Parsing package: {package_name}"); + if let Some(package_modules) = package.modules.to_owned() { + build_state.module_names.extend(package_modules) + } + let build_path_abs = package.get_build_path(); + let bs_build_path = package.get_ocaml_build_path(); + helpers::create_path(&build_path_abs); + helpers::create_path(&bs_build_path); + let root_config = build_state.get_root_config(); + + root_config.get_package_specs().iter().for_each(|spec| { + if !spec.in_source { + // we don't want to calculate this if we don't have out of source specs + // we do this twice, but we almost never have multiple package specs + // so this optimization is less important + let relative_dirs: AHashSet = match &package.source_files { Some(source_files) => source_files .keys() - .map(|key| key.to_owned()) - .collect::>(), - None => unreachable!(), - }; - let entry = match &package.namespace { - packages::Namespace::NamespaceWithEntry { entry, namespace: _ } => Some(entry), - _ => None, + .map(|source_file| { + Path::new(source_file) + .parent() + .expect("parent dir not found") + .to_owned() + }) + .collect(), + _ => AHashSet::new(), }; - - let depending_modules = source_files - .iter() - .map(|path| helpers::file_path_to_module_name(path, &packages::Namespace::NoNamespace)) - .filter(|module_name| { - if let Some(entry) = entry { - module_name != entry - } else { - true - } + if spec.is_common_js() { + helpers::create_path(&package.get_js_path()); + relative_dirs.iter().for_each(|path_buf| { + helpers::create_path_for_path(&Path::join(&package.get_js_path(), path_buf)) }) - .filter(|module_name| helpers::is_non_exotic_module_name(module_name)) - .collect::>(); - - let mlmap = namespaces::gen_mlmap(package, namespace, &depending_modules); - - // mlmap will be compiled in the AST generation step - // compile_mlmap(&package, namespace, &project_root); - let deps = source_files - .iter() - .filter(|path| { - helpers::is_non_exotic_module_name(&helpers::file_path_to_module_name( - path, - &packages::Namespace::NoNamespace, - )) + } else { + helpers::create_path(&package.get_es6_path()); + relative_dirs.iter().for_each(|path_buf| { + helpers::create_path_for_path(&Path::join(&package.get_es6_path(), path_buf)) }) - .map(|path| helpers::file_path_to_module_name(path, &package.namespace)) - .filter(|module_name| { - if let Some(entry) = entry { - module_name != entry - } else { - true - } - }) - .collect::>(); - - 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(), - package_name: package.name.to_owned(), - compile_dirty: false, - last_compiled_cmt: None, - last_compiled_cmi: None, - // Not sure if this is correct - is_type_dev: false, - }, - ); - }); - - debug!("Building source file-tree for package: {}", package.name); - match &package.source_files { - None => (), - Some(source_files) => source_files.iter().for_each(|(file, metadata)| { - let namespace = package.namespace.to_owned(); - - let extension = file.extension().unwrap().to_str().unwrap(); - let module_name = helpers::file_path_to_module_name(file, &namespace); - - if helpers::is_implementation_file(extension) { - build_state - .modules - .entry(module_name.to_string()) - .and_modify(|module| { - if let SourceType::SourceFile(ref mut source_file) = module.source_type { - if &source_file.implementation.path != file { - error!("Duplicate files found for module: {}", &module_name); - error!( - "file 1: {}", - source_file.implementation.path.to_string_lossy() - ); - error!("file 2: {}", file.to_string_lossy()); - - panic!("Unable to continue... See log output above..."); - } - source_file.implementation.path = file.to_owned(); - source_file.implementation.last_modified = metadata.modified; - source_file.implementation.parse_dirty = true; + } + } + }); + + package.namespace.to_suffix().iter().for_each(|namespace| { + // generate the mlmap "AST" file for modules that have a namespace configured + let source_files = match package.source_files.to_owned() { + Some(source_files) => source_files + .keys() + .map(|key| key.to_owned()) + .collect::>(), + None => unreachable!(), + }; + let entry = match &package.namespace { + packages::Namespace::NamespaceWithEntry { entry, namespace: _ } => Some(entry), + _ => None, + }; + + let depending_modules = source_files + .iter() + .map(|path| helpers::file_path_to_module_name(path, &packages::Namespace::NoNamespace)) + .filter(|module_name| { + if let Some(entry) = entry { + module_name != entry + } else { + true + } + }) + .filter(|module_name| helpers::is_non_exotic_module_name(module_name)) + .collect::>(); + + let mlmap = namespaces::gen_mlmap(package, namespace, &depending_modules); + + // mlmap will be compiled in the AST generation step + // compile_mlmap(&package, namespace, &project_root); + let deps = source_files + .iter() + .filter(|path| { + helpers::is_non_exotic_module_name(&helpers::file_path_to_module_name( + path, + &packages::Namespace::NoNamespace, + )) + }) + .map(|path| helpers::file_path_to_module_name(path, &package.namespace)) + .filter(|module_name| { + if let Some(entry) = entry { + module_name != entry + } else { + true + } + }) + .collect::>(); + + 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(), + package_name: package.name.to_owned(), + compile_dirty: false, + last_compiled_cmt: None, + last_compiled_cmi: None, + // Not sure if this is correct + is_type_dev: false, + }, + ); + }); + + let root_path = build_state.get_root_config().path.clone(); + let root = root_path.parent().map(PathBuf::from).unwrap_or(root_path); + + debug!("Building source file-tree for package: {}", package.name); + if let Some(source_files) = &package.source_files { + for (file, metadata) in source_files.iter() { + let namespace = package.namespace.to_owned(); + + let extension = file.extension().unwrap().to_str().unwrap(); + let module_name = helpers::file_path_to_module_name(file, &namespace); + + if helpers::is_implementation_file(extension) { + match build_state.modules.entry(module_name.to_string()) { + Entry::Occupied(mut entry) => { + let module = entry.get_mut(); + if let SourceType::SourceFile(ref mut source_file) = module.source_type { + if &source_file.implementation.path != file { + let existing_path = + Path::new(&package.path).join(&source_file.implementation.path); + let duplicate_path = Path::new(&package.path).join(file); + let existing_display = + existing_path.strip_prefix(&root).unwrap_or(&existing_path); + let duplicate_display = + duplicate_path.strip_prefix(&root).unwrap_or(&duplicate_path); + return Err(anyhow!( + "Duplicate module name: {module_name}. Found in {} and {}. Rename one of the files.", + existing_display.to_string_lossy(), + duplicate_display.to_string_lossy() + )); } - }) - .or_insert(Module { + source_file.implementation.path = file.to_owned(); + source_file.implementation.last_modified = metadata.modified; + source_file.implementation.parse_dirty = true; + } + } + Entry::Vacant(entry) => { + entry.insert(Module { deps_dirty: true, source_type: SourceType::SourceFile(SourceFile { implementation: Implementation { @@ -788,72 +792,74 @@ pub fn parse_packages(build_state: &mut BuildState) { last_compiled_cmi: None, is_type_dev: metadata.is_type_dev, }); - } else { - // remove last character of string: resi -> res - let mut implementation_filename = file.to_owned(); - let extension = implementation_filename.extension().unwrap().to_str().unwrap(); - implementation_filename = match extension { - "resi" => implementation_filename.with_extension("res"), - _ => implementation_filename, - }; - match source_files.get(&implementation_filename) { - None => { - println!( - "{} No implementation file found for interface file (skipping): {}", - LINE_CLEAR, - file.to_string_lossy() - ) - } - Some(_) => { - build_state - .modules - .entry(module_name.to_string()) - .and_modify(|module| { - if let SourceType::SourceFile(ref mut source_file) = - module.source_type - { - source_file.interface = Some(Interface { - path: file.to_owned(), - parse_state: ParseState::Pending, - compile_state: CompileState::Pending, - last_modified: metadata.modified, - parse_dirty: true, - }); - } - }) - .or_insert(Module { - deps_dirty: true, - source_type: SourceType::SourceFile(SourceFile { - // this will be overwritten later - implementation: Implementation { - path: implementation_filename, - parse_state: ParseState::Pending, - compile_state: CompileState::Pending, - last_modified: metadata.modified, - parse_dirty: true, - }, - interface: Some(Interface { - path: file.to_owned(), - parse_state: ParseState::Pending, - compile_state: CompileState::Pending, - last_modified: metadata.modified, - parse_dirty: true, - }), + } + } + } else { + // remove last character of string: resi -> res + let mut implementation_filename = file.to_owned(); + let extension = implementation_filename.extension().unwrap().to_str().unwrap(); + implementation_filename = match extension { + "resi" => implementation_filename.with_extension("res"), + _ => implementation_filename, + }; + match source_files.get(&implementation_filename) { + None => { + println!( + "{} No implementation file found for interface file (skipping): {}", + LINE_CLEAR, + file.to_string_lossy() + ) + } + Some(_) => { + build_state + .modules + .entry(module_name.to_string()) + .and_modify(|module| { + if let SourceType::SourceFile(ref mut source_file) = module.source_type { + source_file.interface = Some(Interface { + path: file.to_owned(), + parse_state: ParseState::Pending, + compile_state: CompileState::Pending, + last_modified: metadata.modified, + parse_dirty: true, + }); + } + }) + .or_insert(Module { + deps_dirty: true, + source_type: SourceType::SourceFile(SourceFile { + // this will be overwritten later + implementation: Implementation { + path: implementation_filename, + parse_state: ParseState::Pending, + compile_state: CompileState::Pending, + last_modified: metadata.modified, + parse_dirty: true, + }, + interface: Some(Interface { + path: file.to_owned(), + parse_state: ParseState::Pending, + compile_state: CompileState::Pending, + last_modified: metadata.modified, + parse_dirty: true, }), - deps: AHashSet::new(), - dependents: AHashSet::new(), - package_name: package.name.to_owned(), - compile_dirty: true, - last_compiled_cmt: None, - last_compiled_cmi: None, - is_type_dev: metadata.is_type_dev, - }); - } + }), + deps: AHashSet::new(), + dependents: AHashSet::new(), + package_name: package.name.to_owned(), + compile_dirty: true, + last_compiled_cmt: None, + last_compiled_cmi: None, + is_type_dev: metadata.is_type_dev, + }); } } - }), + } } - }); + } + } + + Ok(()) } impl Package { diff --git a/rewatch/tests/compile.sh b/rewatch/tests/compile.sh index 3c99d9bbe4..be90268676 100755 --- a/rewatch/tests/compile.sh +++ b/rewatch/tests/compile.sh @@ -93,6 +93,14 @@ rewatch build &> ../tests/snapshots/dependency-cycle.txt normalize_paths ../tests/snapshots/dependency-cycle.txt git checkout -- packages/new-namespace/src/NS_alias.res +# it should show an error for duplicate module names +mkdir -p packages/main/src/dupe-a packages/main/src/dupe-b +echo 'let value = 1' > packages/main/src/dupe-a/DuplicateModule.res +echo 'let value = 2' > packages/main/src/dupe-b/DuplicateModule.res +rewatch build &> ../tests/snapshots/duplicate-module-name.txt +normalize_paths ../tests/snapshots/duplicate-module-name.txt +rm -rf packages/main/src/dupe-a packages/main/src/dupe-b + # this should not compile because "@rescript/webapi" is part of dev-dependencies # and FileToTest.res is not listed as "type":"dev" echo 'open WebAPI' >> packages/with-dev-deps/src/FileToTest.res diff --git a/rewatch/tests/snapshots/duplicate-module-name.txt b/rewatch/tests/snapshots/duplicate-module-name.txt new file mode 100644 index 0000000000..be2606d4d1 --- /dev/null +++ b/rewatch/tests/snapshots/duplicate-module-name.txt @@ -0,0 +1 @@ +Could not initialize build: Duplicate module name: DuplicateModule. Found in packages/main/src/dupe-a/DuplicateModule.res and packages/main/src/dupe-b/DuplicateModule.res. Rename one of the files. From e1598f0ec053736f75fe4373cca9f6049ff48cd3 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Wed, 24 Dec 2025 07:25:50 +0100 Subject: [PATCH 2/6] Deterministic order of file/module names in error message --- rewatch/src/build/packages.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index 2f05d8de5a..2024cf1e78 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -760,10 +760,15 @@ pub fn parse_packages(build_state: &mut BuildState) -> Result<()> { existing_path.strip_prefix(&root).unwrap_or(&existing_path); let duplicate_display = duplicate_path.strip_prefix(&root).unwrap_or(&duplicate_path); + let mut first = existing_display.to_string_lossy().to_string(); + let mut second = duplicate_display.to_string_lossy().to_string(); + if second < first { + std::mem::swap(&mut first, &mut second); + } return Err(anyhow!( "Duplicate module name: {module_name}. Found in {} and {}. Rename one of the files.", - existing_display.to_string_lossy(), - duplicate_display.to_string_lossy() + first, + second )); } source_file.implementation.path = file.to_owned(); From a288fd1857dc08e8eb9db203907ed7cb194a0bb6 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Wed, 24 Dec 2025 12:16:42 +0100 Subject: [PATCH 3/6] Update rewatch/src/build/packages.rs Co-authored-by: Florian Verdonck --- rewatch/src/build/packages.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index 2024cf1e78..82168f25f8 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -766,7 +766,7 @@ pub fn parse_packages(build_state: &mut BuildState) -> Result<()> { std::mem::swap(&mut first, &mut second); } return Err(anyhow!( - "Duplicate module name: {module_name}. Found in {} and {}. Rename one of the files.", + "Duplicate module name: {module_name}. Found in {} and {}. Rename one of these files.", first, second )); From d1c46ec5a63eb48504399a0d55713b87d8e72498 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Wed, 24 Dec 2025 12:17:52 +0100 Subject: [PATCH 4/6] Fix snapshot --- rewatch/tests/snapshots/duplicate-module-name.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rewatch/tests/snapshots/duplicate-module-name.txt b/rewatch/tests/snapshots/duplicate-module-name.txt index be2606d4d1..a4a17b1026 100644 --- a/rewatch/tests/snapshots/duplicate-module-name.txt +++ b/rewatch/tests/snapshots/duplicate-module-name.txt @@ -1 +1 @@ -Could not initialize build: Duplicate module name: DuplicateModule. Found in packages/main/src/dupe-a/DuplicateModule.res and packages/main/src/dupe-b/DuplicateModule.res. Rename one of the files. +Could not initialize build: Duplicate module name: DuplicateModule. Found in packages/main/src/dupe-a/DuplicateModule.res and packages/main/src/dupe-b/DuplicateModule.res. Rename one of these files. From fd59a190e3e208ff461decdbcf197497b1b9f503 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Thu, 25 Dec 2025 08:51:43 +0100 Subject: [PATCH 5/6] Move cloning root_path into error handling code --- rewatch/src/build/packages.rs | 40 ++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index 82168f25f8..da6e585ec1 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -736,9 +736,6 @@ pub fn parse_packages(build_state: &mut BuildState) -> Result<()> { ); }); - let root_path = build_state.get_root_config().path.clone(); - let root = root_path.parent().map(PathBuf::from).unwrap_or(root_path); - debug!("Building source file-tree for package: {}", package.name); if let Some(source_files) = &package.source_files { for (file, metadata) in source_files.iter() { @@ -748,27 +745,16 @@ pub fn parse_packages(build_state: &mut BuildState) -> Result<()> { let module_name = helpers::file_path_to_module_name(file, &namespace); if helpers::is_implementation_file(extension) { + // Store duplicate paths in an Option so we can build the error after the entry borrow ends. + let mut duplicate_paths: Option<(PathBuf, PathBuf)> = None; match build_state.modules.entry(module_name.to_string()) { Entry::Occupied(mut entry) => { let module = entry.get_mut(); if let SourceType::SourceFile(ref mut source_file) = module.source_type { if &source_file.implementation.path != file { - let existing_path = - Path::new(&package.path).join(&source_file.implementation.path); - let duplicate_path = Path::new(&package.path).join(file); - let existing_display = - existing_path.strip_prefix(&root).unwrap_or(&existing_path); - let duplicate_display = - duplicate_path.strip_prefix(&root).unwrap_or(&duplicate_path); - let mut first = existing_display.to_string_lossy().to_string(); - let mut second = duplicate_display.to_string_lossy().to_string(); - if second < first { - std::mem::swap(&mut first, &mut second); - } - return Err(anyhow!( - "Duplicate module name: {module_name}. Found in {} and {}. Rename one of these files.", - first, - second + duplicate_paths = Some(( + Path::new(&package.path).join(&source_file.implementation.path), + Path::new(&package.path).join(file), )); } source_file.implementation.path = file.to_owned(); @@ -799,6 +785,22 @@ pub fn parse_packages(build_state: &mut BuildState) -> Result<()> { }); } } + if let Some((existing_path, duplicate_path)) = duplicate_paths { + let root_path = build_state.get_root_config().path.clone(); + let root = root_path.parent().map(PathBuf::from).unwrap_or(root_path); + let existing_display = existing_path.strip_prefix(&root).unwrap_or(&existing_path); + let duplicate_display = duplicate_path.strip_prefix(&root).unwrap_or(&duplicate_path); + let mut first = existing_display.to_string_lossy().to_string(); + let mut second = duplicate_display.to_string_lossy().to_string(); + if second < first { + std::mem::swap(&mut first, &mut second); + } + return Err(anyhow!( + "Duplicate module name: {module_name}. Found in {} and {}. Rename one of these files.", + first, + second + )); + } } else { // remove last character of string: resi -> res let mut implementation_filename = file.to_owned(); From 8e05b8a0e4eccbcd2780a8c9fc63d02246f4e602 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Thu, 25 Dec 2025 08:52:42 +0100 Subject: [PATCH 6/6] CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c7e109122..72d908019a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ - Fix: do not warn for "editor" field in `rescript.json`. https://github.com/rescript-lang/rescript/pull/8084 - Fix `@val` shadowing (rewrite using `globalThis`). https://github.com/rescript-lang/rescript/pull/8098 - Fix `@scope` shadowing (rewrite using `globalThis`). https://github.com/rescript-lang/rescript/pull/8100 +- Fix rewatch panic on duplicate module name. https://github.com/rescript-lang/rescript/pull/8102 #### :memo: Documentation