diff --git a/changes/dev-module-namespace-isolation.fix.md b/changes/dev-module-namespace-isolation.fix.md new file mode 100644 index 0000000000..d08f49c9cd --- /dev/null +++ b/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. diff --git a/rewatch-ocaml/compiler_args.ml b/rewatch-ocaml/compiler_args.ml index 6f061726d4..e471a277c5 100644 --- a/rewatch-ocaml/compiler_args.ml +++ b/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 diff --git a/rewatch-ocaml/compiler_process.ml b/rewatch-ocaml/compiler_process.ml index eec1a09c2c..056410f83c 100644 --- a/rewatch-ocaml/compiler_process.ml +++ b/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 diff --git a/rewatch/src/build/compile.rs b/rewatch/src/build/compile.rs index c4677632cf..0bab182b88 100644 --- a/rewatch/src/build/compile.rs +++ b/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(), diff --git a/rewatch/src/build/namespaces.rs b/rewatch/src/build/namespaces.rs index 9042e8a2ec..c5de54fc4d 100644 --- a/rewatch/src/build/namespaces.rs +++ b/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::>(); + modules.sort(); + modules.dedup(); + format!( + "randjbuildsystem\n{}", + modules.into_iter().map(|name| name + "\n").collect::() + ) +} + +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 = vec![]; // include `-runtime-path` arg diff --git a/rewatch/src/build/parse.rs b/rewatch/src/build/parse.rs index 51ba80aefe..757f00d9d4 100644 --- a/rewatch/src/build/parse.rs +++ b/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 } diff --git a/rewatch/tests/dev-module-shadowing.sh b/rewatch/tests/dev-module-shadowing.sh new file mode 100644 index 0000000000..f8a71bd69c --- /dev/null +++ b/rewatch/tests/dev-module-shadowing.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +set -euo pipefail + +: "${REWATCH_EXECUTABLE:?Set REWATCH_EXECUTABLE}" +project=$(mktemp -d) +trap 'rm -rf "$project"' EXIT + +node - "$project" <<'JS' +const fs = require('fs'); +const path = require('path'); +const root = process.argv[2]; +function write(file, value) { + const target = path.join(root, file); + fs.mkdirSync(path.dirname(target), {recursive: true}); + fs.writeFileSync(target, typeof value === 'string' ? value : JSON.stringify(value)); +} +write('package.json', {name: 'root', private: true, workspaces: ['consumer', 'provider']}); +write('rescript.json', {name: 'root', sources: ['src'], dependencies: ['consumer']}); +write('src/Main.res', 'let value = Consumer.Use.value\n'); +for (const name of ['consumer', 'provider']) { + write(name + '/package.json', {name, version: '1.0.0'}); + write(name + '/rescript.json', { + name, namespace: true, + sources: [{dir: 'src'}, {dir: 'test', type: 'dev'}], + dependencies: name === 'consumer' ? ['provider'] : [], + warnings: {number: '-33-44'}, + }); + fs.mkdirSync(root + '/node_modules', {recursive: true}); + fs.symlinkSync('../' + name, root + '/node_modules/' + name); +} +write('provider/src/Public.res', 'type t = {value: int}\nlet make = () => {value: 1}\n'); +write('provider/src/Alias.res', 'module Public = Public\nlet make = Public.make\n'); +write('provider/src/Alias.resi', 'module Public = Public\nlet make: unit => Public.t\n'); +write('provider/test/Config.res', 'let other = 2\n'); +write('provider/test/Local.res', 'let value = Config.other\nlet qualified = Provider.Config.other\n'); +write('consumer/src/Use.res', 'let value = Provider.Alias.make()\n'); +write('consumer/test/Config.res', 'let get = () => 3\n'); +write('consumer/test/Check.res', 'open Provider\nlet value = Config.get()\nlet qualified = Consumer.Config.get()\n'); +JS + +cd "$project" +"$REWATCH_EXECUTABLE" build +"$REWATCH_EXECUTABLE" build + +# Simulate a namespace published by an older rewatch, without changing sources. +for extension in mlmap cmi cmt cmj; do + cp "provider/lib/bs/Provider.$extension" "provider/lib/ocaml/Provider.$extension" +done +"$REWATCH_EXECUTABLE" build +if grep -qx Config provider/lib/ocaml/Provider.mlmap; then + echo 'Stale dependency dev alias was not removed' >&2 + exit 1 +fi + +# A dependency's dev module must not be accessible through its namespace. +printf 'let value = Provider.Config.other\n' > consumer/test/Leak.res +if "$REWATCH_EXECUTABLE" build > leak.log 2>&1; then + echo 'Dependency dev module was visible to a consumer' >&2 + exit 1 +fi +grep -E 'Config.*(found|unbound)|module or file Config' leak.log +rm consumer/test/Leak.res +"$REWATCH_EXECUTABLE" build + +echo 'Dev-module namespace isolation passed' + +# Namespace entries must expose interfaces compatible with the public namespace. +node <<'NODE' +const fs = require('fs'); +const config = JSON.parse(fs.readFileSync('provider/rescript.json', 'utf8')); +config['namespace-entry'] = 'Entry'; +fs.writeFileSync('provider/rescript.json', JSON.stringify(config)); +fs.writeFileSync('provider/src/Entry.res', 'module Public = Public\n'); +fs.writeFileSync('provider/test/Local.res', 'let value = Config.other\n'); +fs.writeFileSync('consumer/src/Use.res', 'let value = Entry.Public.make()\n'); +fs.writeFileSync('consumer/test/Check.res', 'let value = Config.get()\n'); +NODE +"$REWATCH_EXECUTABLE" clean +"$REWATCH_EXECUTABLE" build +"$REWATCH_EXECUTABLE" build + +echo 'Namespace-entry isolation passed' diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 757d440759..9a984c7ce5 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -65,6 +65,7 @@ else exit 1 fi +bash ./dev-module-shadowing.sh && # Individual test files # Comment out any test to skip it diff --git a/tests/rewatch_ounit_tests/compiler_args_tests.ml b/tests/rewatch_ounit_tests/compiler_args_tests.ml index a12877dd7b..62953b4e69 100644 --- a/tests/rewatch_ounit_tests/compiler_args_tests.ml +++ b/tests/rewatch_ounit_tests/compiler_args_tests.ml @@ -96,6 +96,24 @@ let tests = |> adjacent_positions "-I" |> position development |> Option.is_none) "source directory matching respects path-component boundaries"; let config = Config.load_root root in + let namespace_includes module_name = + let config = + { + config with + namespace = Namespace_with_entry {name = "Ns"; entry = "Entry"}; + } + in + Compiler_args.compiler_arguments_with_common ~config + ~common_args:["-I"; "../ocaml"] ~module_name + ~source_kind:Source.Implementation ~has_interface:false + ~path:"src/A.res" + |> adjacent_positions "-I" |> List.map fst + in + assert_equal ["."; "../ocaml"] (namespace_includes "A") + ~msg:"ordinary modules prefer the internal namespace"; + assert_equal ["../ocaml"] + (namespace_includes "Entry") + ~msg:"namespace entries use the exported namespace"; let gentype_config = {config with gentype_args = ["-bs-gentype"]} in let selected_regular = Filename.concat root "selected/regular" in check