Fix dev module shadowing

changes/dev-module-namespace-isolation.fix.md

@@ -0,0 +1 @@
+- Keep dependency dev modules out of exported namespaces, while preserving local dev access and compatible namespace-entry interfaces.

rewatch-ocaml/compiler_args.ml

@@ -126,8 +126,13 @@ let compiler_arguments_with_common ~(config : Config.t) ~common_args
         (fun spec -> ["-bs-package-output"; package_output config path spec])
         config.package_specs
   in
+  let local_namespace_args =
+    match config.namespace with
+    | Namespace_with_entry {entry; _} when entry = module_name -> []
+    | _ -> ["-I"; "."]
+  in
   namespace_args config module_name
-  @ interface_args @ common_args @ output_args
+  @ interface_args @ local_namespace_args @ common_args @ output_args
   @ [Source.ast_path path]
 
 let compiler_arguments ~project_root ~(config : Config.t) ~runtime

rewatch-ocaml/compiler_process.ml

@@ -95,15 +95,29 @@ let run ?poll request =
   Option.iter (fun poll -> poll ()) poll;
   result
 
-let task request =
+let sequential_task requests =
   let cancelled = Atomic.make false in
   Process.concurrent_task
     ~cancel:(fun () -> Atomic.set cancelled true)
     (fun () ->
-      run_in_process
-        ~poll:(fun () ->
-          if Atomic.get cancelled then raise (Process.Interrupted 15))
-        request)
+      let poll () =
+        if Atomic.get cancelled then raise (Process.Interrupted 15)
+      in
+      List.fold_left
+        (fun result request ->
+          if not (Process.succeeded result) then result
+          else (
+            poll ();
+            let next = run_in_process ~poll request in
+            {
+              next with
+              Process.stdout = result.stdout ^ next.stdout;
+              stderr = result.stderr ^ next.stderr;
+            }))
+        Process.{status = Unix.WEXITED 0; stdout = ""; stderr = ""}
+        requests)
+
+let task request = sequential_task [request]
 
 let run_requests ?poll ?on_complete requests =
   requests |> List.map task
@@ -158,7 +172,7 @@ let publish_compiler_artifacts ~artifact_dir ~ocaml_dir ~basename artifacts =
 let namespace_task ~runtime ~build_dir ~ocaml_dir ~entry ~package_dirty ~force
     namespace modules =
   let mlmap = Filename.concat build_dir (namespace ^ ".mlmap") in
-  let contents =
+  let namespace_contents modules =
     let buffer = Buffer.create 128 in
     Buffer.add_string buffer "randjbuildsystem\n";
     Source.namespace_members ~entry modules
@@ -169,6 +183,23 @@ let namespace_task ~runtime ~build_dir ~ocaml_dir ~entry ~package_dirty ~force
         Buffer.add_char buffer '\n');
     Buffer.contents buffer
   in
+  let contents = namespace_contents modules in
+  let public_contents =
+    namespace_contents
+      (List.filter (fun module_ -> not module_.Source.is_dev) modules)
+  in
+  (* Local dev sources need the full namespace; consumers and namespace entries
+     must share the filtered interface. Stage that interface before publication. *)
+  let artifact_dir =
+    if public_contents = contents then build_dir
+    else Filename.concat build_dir ".public-namespace"
+  in
+  let internal_cmi = Filename.concat build_dir (namespace ^ ".cmi") in
+  let previous_cmi =
+    if File_util.is_regular_file internal_cmi then
+      Some (File_util.digest_file internal_cmi)
+    else None
+  in
   let previous_contents =
     try Some (File_util.read_file mlmap)
     with Unix.Unix_error ((Unix.ENOENT | Unix.ENOTDIR), _, _) -> None
@@ -177,15 +208,16 @@ let namespace_task ~runtime ~build_dir ~ocaml_dir ~entry ~package_dirty ~force
   if mlmap_changed then
     File_util.write_file_atomic ~ensure_parent:false mlmap contents;
   let outputs_exist =
-    ["cmi"; "cmj"; "cmt"; "mlmap"]
-    |> List.for_all (fun extension ->
-        File_util.is_regular_file
-          (Filename.concat ocaml_dir (namespace ^ "." ^ extension)))
+    Option.is_some previous_cmi
+    && ["cmi"; "cmj"; "cmt"; "mlmap"]
+       |> List.for_all (fun extension ->
+           File_util.is_regular_file
+             (Filename.concat ocaml_dir (namespace ^ "." ^ extension)))
   in
   let published_mlmap_matches =
     try
       File_util.read_file (Filename.concat ocaml_dir (namespace ^ ".mlmap"))
-      = contents
+      = public_contents
     with Unix.Unix_error ((Unix.ENOENT | Unix.ENOTDIR), _, _) -> false
   in
   if
@@ -194,39 +226,62 @@ let namespace_task ~runtime ~build_dir ~ocaml_dir ~entry ~package_dirty ~force
       || (not published_mlmap_matches)
       || not outputs_exist)
   then None
-  else
+  else (
+    if artifact_dir <> build_dir then (
+      File_util.ensure_dir artifact_dir;
+      File_util.write_file_atomic ~ensure_parent:false
+        (Filename.concat artifact_dir (namespace ^ ".mlmap"))
+        public_contents);
+    let request cwd =
+      {
+        args =
+          [
+            "-runtime-path";
+            runtime;
+            "-w";
+            "-49";
+            "-color";
+            "always";
+            "-no-alias-deps";
+            Filename.basename mlmap;
+          ];
+        cwd;
+      }
+    in
+    let requests =
+      if artifact_dir = build_dir then [request build_dir]
+      else [request build_dir; request artifact_dir]
+    in
     Some
       Compiler_scheduler.
         {
-          task =
-            task
-              {
-                args =
-                  [
-                    "-runtime-path";
-                    runtime;
-                    "-w";
-                    "-49";
-                    "-color";
-                    "always";
-                    "-no-alias-deps";
-                    Filename.basename mlmap;
-                  ];
-                cwd = build_dir;
-              };
+          task = sequential_task requests;
           publish =
             (fun result ->
               if not (Process.succeeded result) then
                 raise
                   (Compiler_scheduler.Build_failure
                      (result.Process.stderr ^ result.stdout));
+              let internal_changed =
+                previous_cmi <> Some (File_util.digest_file internal_cmi)
+              in
+              let include_internal_change change =
+                if internal_changed then Compiler_scheduler.Cmi_changed
+                else change
+              in
               let cmi_change =
-                publish_compiler_artifacts ~artifact_dir:build_dir ~ocaml_dir
-                  ~basename:namespace
-                  [Cmi; Required "cmj"; Required "cmt"; Required "mlmap"]
+                try
+                  publish_compiler_artifacts ~artifact_dir ~ocaml_dir
+                    ~basename:namespace
+                    [Cmi; Required "cmj"; Required "cmt"; Required "mlmap"]
+                  |> include_internal_change
+                with Compiler_scheduler.Publication_failure (error, change) ->
+                  raise
+                    (Compiler_scheduler.Publication_failure
+                       (error, include_internal_change change))
               in
               Compiler_scheduler.{stderr = result.stderr; cmi_change});
-        }
+        })
 
 let post_build_tasks (config : Config.t) path =
   match config.js_post_build with

rewatch/src/build/compile.rs

@@ -777,6 +777,11 @@ pub fn compiler_args(
     let dependency_paths = get_dependency_paths(config, project_context, packages, is_type_dev);
     let module_name = helpers::file_path_to_module_name(file_path, &config.get_namespace());
 
+    let local_namespace_args = match &config.get_namespace() {
+        packages::Namespace::NamespaceWithEntry { entry, .. } if &module_name == entry => vec![],
+        _ => vec!["-I".to_string(), ".".to_string()],
+    };
+
     let namespace_args = match &config.get_namespace() {
         packages::Namespace::NamespaceWithEntry { namespace: _, entry } if &module_name == entry => {
             // if the module is the entry we just want to open the namespace
@@ -880,6 +885,7 @@ pub fn compiler_args(
     Ok(vec![
         namespace_args,
         read_cmi_args,
+        local_namespace_args,
         vec![
             "-I".to_string(),
             Path::new("..").join("ocaml").to_string_lossy().to_string(),

rewatch/src/build/namespaces.rs

@@ -1,5 +1,6 @@
 use crate::build::compile::get_runtime_path_args;
 use crate::build::packages;
+use crate::helpers;
 use crate::helpers::StrippedVerbatimPath;
 use crate::project_context::ProjectContext;
 use ahash::AHashSet;
@@ -59,7 +60,60 @@ pub fn compile_mlmap(
     namespace: &str,
     bsc_path: &Path,
 ) -> Result<()> {
-    let build_path_abs = package.get_build_path();
+    compile_mlmap_at(
+        project_context,
+        package,
+        namespace,
+        bsc_path,
+        &package.get_build_path(),
+    )?;
+
+    // Consumers must not import dev aliases when opening a dependency's namespace.
+    // Keep the full map in lib/bs for same-package compilation.
+    let public_path = package.get_ocaml_build_path();
+    std::fs::write(
+        public_path.join(format!("{namespace}.mlmap")),
+        public_mlmap_contents(package),
+    )?;
+    compile_mlmap_at(project_context, package, namespace, bsc_path, &public_path)
+}
+
+fn public_mlmap_contents(package: &packages::Package) -> String {
+    let mut modules = package
+        .source_files
+        .as_ref()
+        .unwrap()
+        .iter()
+        .filter(|(_, metadata)| !metadata.is_type_dev)
+        .map(|(path, _)| helpers::file_path_to_module_name(path, &packages::Namespace::NoNamespace))
+        .filter(|name| helpers::is_non_exotic_module_name(name))
+        .filter(|name| match &package.namespace {
+            packages::Namespace::NamespaceWithEntry { entry, .. } => name != entry,
+            _ => true,
+        })
+        .collect::<Vec<_>>();
+    modules.sort();
+    modules.dedup();
+    format!(
+        "randjbuildsystem\n{}",
+        modules.into_iter().map(|name| name + "\n").collect::<String>()
+    )
+}
+
+pub fn public_mlmap_is_current(package: &packages::Package, namespace: &str) -> bool {
+    let path = package.get_ocaml_build_path();
+    path.join(format!("{namespace}.cmi")).is_file()
+        && std::fs::read_to_string(path.join(format!("{namespace}.mlmap")))
+            .is_ok_and(|contents| contents == public_mlmap_contents(package))
+}
+
+fn compile_mlmap_at(
+    project_context: &ProjectContext,
+    package: &packages::Package,
+    namespace: &str,
+    bsc_path: &Path,
+    build_path_abs: &Path,
+) -> Result<()> {
     let mlmap_name = format!("{namespace}.mlmap");
     let mut args: Vec<String> = vec![];
     // include `-runtime-path` arg

rewatch/src/build/parse.rs

@@ -252,12 +252,14 @@ pub fn generate_asts(
         if let Some(module) = build_state.build_state.modules.get_mut(&module_name) {
             let is_dirty = match &module.source_type {
                 SourceType::MlMap(_) => {
-                    if dirty_packages.contains(&package_name) {
-                        let package = build_state
-                            .build_state
-                            .packages
-                            .get(&package_name)
-                            .expect("Package not found");
+                    let package = build_state
+                        .build_state
+                        .packages
+                        .get(&package_name)
+                        .expect("Package not found");
+                    let public_namespace_changed =
+                        !namespaces::public_mlmap_is_current(package, &module_name);
+                    if dirty_packages.contains(&package_name) || public_namespace_changed {
                         // probably better to do this in a different function
                         // specific to compiling mlmaps
                         let compile_path = package.get_mlmap_compile_path();
@@ -273,32 +275,11 @@ pub fn generate_asts(
                         }
                         let mlmap_hash_after = helpers::compute_file_hash(Path::new(&compile_path));
 
-                        let suffix = package
-                            .namespace
-                            .to_suffix()
-                            .expect("namespace should be set for mlmap module");
-                        let base_build_path = package.get_build_path().join(&suffix);
-                        let base_ocaml_build_path = package.get_ocaml_build_path().join(&suffix);
-                        let _ = std::fs::copy(
-                            base_build_path.with_extension("cmi"),
-                            base_ocaml_build_path.with_extension("cmi"),
-                        );
-                        let _ = std::fs::copy(
-                            base_build_path.with_extension("cmt"),
-                            base_ocaml_build_path.with_extension("cmt"),
-                        );
-                        let _ = std::fs::copy(
-                            base_build_path.with_extension("cmj"),
-                            base_ocaml_build_path.with_extension("cmj"),
-                        );
-                        let _ = std::fs::copy(
-                            base_build_path.with_extension("mlmap"),
-                            base_ocaml_build_path.with_extension("mlmap"),
-                        );
-                        match (mlmap_hash, mlmap_hash_after) {
-                            (Some(digest), Some(digest_after)) => !digest.eq(&digest_after),
-                            _ => true,
-                        }
+                        public_namespace_changed
+                            || match (mlmap_hash, mlmap_hash_after) {
+                                (Some(digest), Some(digest_after)) => !digest.eq(&digest_after),
+                                _ => true,
+                            }
                     } else {
                         false
                     }