Allow clean with invalid module graphs

changes/clean-invalid-module-graphs.fix.md

@@ -0,0 +1 @@
+- Allow `rescript clean` to remove generated files when the source module graph is invalid, including out-of-source `lib/es6` and `lib/js` outputs in the project and its dependencies. https://github.com/rescript-lang/rescript/pull/8640

rewatch/src/build/clean.rs

@@ -1,12 +1,11 @@
 use super::build_types::*;
 use super::packages;
-use crate::build;
 use crate::build::packages::Package;
-use crate::config::{Config, SourceMapCommand};
+use crate::config::Config;
 use crate::helpers;
 use crate::helpers::emojis::*;
 use crate::project_context::ProjectContext;
-use ahash::AHashSet;
+use ahash::{AHashMap, AHashSet};
 use anyhow::Result;
 use console::style;
 use rayon::prelude::*;
@@ -68,37 +67,36 @@ fn remove_compile_assets(package: &packages::Package, source_file: &Path) {
     }
 }
 
-fn clean_source_files(build_state: &BuildState, root_config: &Config) {
-    // get all rescript file locations
-    let rescript_file_locations = build_state
-        .modules
-        .values()
-        .filter_map(|module| match &module.source_type {
-            SourceType::SourceFile(source_file) => {
-                build_state.packages.get(&module.package_name).map(|package| {
-                    root_config
-                        .get_package_specs()
-                        .into_iter()
-                        .filter_map(|spec| {
-                            if spec.in_source {
-                                Some((
-                                    package.path.join(&source_file.implementation.path),
-                                    match spec.suffix {
-                                        None => root_config.get_suffix(&spec),
-                                        Some(suffix) => suffix,
-                                    },
-                                ))
-                            } else {
-                                None
-                            }
-                        })
-                        .collect::<Vec<(PathBuf, String)>>()
-                })
+fn clean_source_files(packages: &AHashMap<String, Package>, root_config: &Config) {
+    let mut rescript_file_locations = Vec::new();
+    for package in packages.values() {
+        let Some(source_files) = &package.source_files else {
+            continue;
+        };
+        for source_file in source_files.keys().filter(|source_file| {
+            source_file
+                .extension()
+                .and_then(|extension| extension.to_str())
+                .is_some_and(helpers::is_implementation_file)
+        }) {
+            for spec in root_config.get_package_specs() {
+                let output_base = if spec.in_source {
+                    package.path.join(source_file)
+                } else {
+                    package
+                        .path
+                        .join("lib")
+                        .join(spec.get_out_of_source_dir())
+                        .join(source_file)
+                };
+                let suffix = match &spec.suffix {
+                    None => root_config.get_suffix(&spec),
+                    Some(suffix) => suffix.clone(),
+                };
+                rescript_file_locations.push((output_base, suffix));
             }
-            _ => None,
-        })
-        .flatten()
-        .collect::<Vec<(PathBuf, String)>>();
+        }
+    }
 
     rescript_file_locations
         .par_iter()
@@ -342,7 +340,6 @@ pub fn cleanup_after_build(build_state: &BuildCommandState) {
 #[instrument(name = "clean.clean", skip_all)]
 pub fn clean(path: &Path, show_progress: bool, plain_output: bool, prod: bool) -> Result<()> {
     let project_context = ProjectContext::new(path)?;
-    let compiler_info = build::get_compiler_info(&project_context)?;
     // `clean` always acts on the full set of source directories regardless of which features are
     // active. We explicitly pass `None` so every tagged source folder is included and its
     // artifacts can be removed, even for features the user hasn't enabled for this build.
@@ -376,27 +373,13 @@ pub fn clean(path: &Path, show_progress: bool, plain_output: bool, prod: bool) -
     }
 
     let timing_clean_mjs = Instant::now();
-    let mut build_state = BuildState::new(project_context, packages, compiler_info, SourceMapCommand::Build);
-    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 {
-            None => String::from(".js"),
-            Some(suffix) => suffix.clone(),
-        },
-        Some(_) => root_config
-            .get_package_specs()
-            .into_iter()
-            .filter_map(|spec| {
-                if spec.in_source {
-                    spec.suffix.or_else(|| root_config.suffix.clone())
-                } else {
-                    None
-                }
-            })
-            .collect::<Vec<String>>()
-            .join(", "),
-    };
+    let root_config = project_context.get_root_config();
+    let suffix_for_print = root_config
+        .get_package_specs()
+        .iter()
+        .map(|spec| root_config.get_suffix(spec))
+        .collect::<Vec<String>>()
+        .join(", ");
 
     if !plain_output && show_progress {
         print!(
@@ -408,7 +391,7 @@ pub fn clean(path: &Path, show_progress: bool, plain_output: bool, prod: bool) -
         let _ = std::io::stdout().flush();
     }
 
-    clean_source_files(&build_state, root_config);
+    clean_source_files(&packages, root_config);
     let timing_clean_mjs_elapsed = timing_clean_mjs.elapsed();
 
     if !plain_output && show_progress {

rewatch/tests/clean/05-clean-invalid-module-graph.sh

@@ -0,0 +1,58 @@
+#!/bin/bash
+cd $(dirname $0)
+source "../utils.sh"
+cd ../../testrepo
+
+bold "Test: Clean succeeds when the module graph is invalid"
+
+rewatch clean > /dev/null 2>&1
+cp rescript.json rescript.json.clean-invalid.bak
+
+restore_fixture() {
+  rm -rf src/dupe-a src/dupe-b
+  if [ -f rescript.json.clean-invalid.bak ]; then
+    mv rescript.json.clean-invalid.bak rescript.json
+  fi
+  rewatch build > /dev/null 2>&1 || true
+}
+trap restore_fixture EXIT
+
+node -e '
+  const fs = require("fs");
+  const path = process.argv[1];
+  const config = JSON.parse(fs.readFileSync(path, "utf8"));
+  config["package-specs"] = [
+    {module: "esmodule", "in-source": true},
+    {module: "esmodule", "in-source": false, suffix: ".out.mjs"},
+  ];
+  fs.writeFileSync(path, `${JSON.stringify(config, null, 2)}\n`);
+' rescript.json
+
+rewatch build > /dev/null 2>&1
+
+if [ ! -f src/Test.mjs ] || [ ! -f lib/es6/src/Test.out.mjs ]; then
+  error "Test setup did not produce both in-source and out-of-source outputs"
+  exit 1
+fi
+
+mkdir -p src/dupe-a src/dupe-b
+echo 'let value = 1' > src/dupe-a/DuplicateModule.res
+echo 'let value = 2' > src/dupe-b/DuplicateModule.res
+
+error_output=$(rewatch clean 2>&1)
+clean_status=$?
+
+if [ $clean_status -ne 0 ]; then
+  error "Clean failed for an invalid module graph"
+  printf "%s\n" "$error_output" >&2
+  exit 1
+fi
+
+if [ -f src/Test.mjs ] || [ -f lib/es6/src/Test.out.mjs ]; then
+  error "Clean left a generated output behind"
+  exit 1
+fi
+
+restore_fixture
+trap - EXIT
+success "Invalid module graph cleaned"

rewatch/tests/clean/06-clean-duplicate-names-across-packages.sh

@@ -0,0 +1,61 @@
+#!/bin/bash
+set -e
+cd "$(dirname "$0")"
+source "../utils.sh"
+
+bold "Test: Clean removes same-named modules from every package"
+
+test_dir=$(pwd)
+fixture=$(mktemp -d)
+trap 'cd "$test_dir"; rm -rf "$fixture"' EXIT
+cd "$fixture"
+
+node <<'NODE'
+const fs = require("fs");
+const config = name => ({
+  name,
+  sources: ["src"],
+  namespace: false,
+  "package-specs": {module: "esmodule", "in-source": true},
+  suffix: ".mjs",
+});
+const dependencies = ["clean-duplicate-a", "clean-duplicate-b"];
+fs.mkdirSync("src");
+fs.writeFileSync("src/Main.res", "let value = 0\n");
+fs.writeFileSync("rescript.json", JSON.stringify({...config("clean-duplicates"), dependencies}));
+for (const [index, name] of dependencies.entries()) {
+  const directory = `node_modules/${name}`;
+  fs.mkdirSync(`${directory}/src`, {recursive: true});
+  fs.writeFileSync(`${directory}/package.json`, JSON.stringify({name, version: "1.0.0"}));
+  fs.writeFileSync(`${directory}/rescript.json`, JSON.stringify(config(name)));
+  fs.writeFileSync(`${directory}/src/Foo.res`, `let value = ${index}\n`);
+}
+NODE
+
+# Build separately so both packages have real output before root-level cleanup.
+for package in clean-duplicate-a clean-duplicate-b; do
+  if ! output=$(cd "node_modules/$package" && rewatch build 2>&1); then
+    error "Could not build $package"
+    printf '%s\n' "$output" >&2
+    exit 1
+  fi
+  if [ ! -f "node_modules/$package/src/Foo.mjs" ]; then
+    error "Test setup did not produce $package/src/Foo.mjs"
+    exit 1
+  fi
+done
+
+if ! output=$(rewatch clean 2>&1); then
+  error "Could not clean packages with same-named modules"
+  printf '%s\n' "$output" >&2
+  exit 1
+fi
+
+for package in clean-duplicate-a clean-duplicate-b; do
+  if [ -f "node_modules/$package/src/Foo.mjs" ]; then
+    error "Clean left $package/src/Foo.mjs behind"
+    exit 1
+  fi
+done
+
+success "Same-named modules cleaned from both packages"

rewatch/tests/suite.sh

@@ -163,6 +163,8 @@ fi
 ./clean/02-clean-dev-dependencies.sh &&
 ./clean/03-clean-node-modules.sh &&
 ./clean/04-clean-rebuild-no-compiler-update.sh &&
+./clean/05-clean-invalid-module-graph.sh &&
+./clean/06-clean-duplicate-names-across-packages.sh &&
 
 # Experimental tests
 ./experimental/01-experimental-features-emit.sh &&