From a559fd009d4bbbac7a33976901b43d807dc438b3 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 14 Nov 2026 08:57:10 +0000 Subject: [PATCH 1/3] Keep a namespaced package's dev modules out of its namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: The namespace map aliased every module of a package, its dev modules too. A package that opened a dependency's namespace got the dependency's dev modules in scope, which shadowed its own modules of the same name (#9089, #6679). The namespace map also depended on the dev modules, so a dev test that used the namespace entry was a dependency cycle. The namespace map now holds only the regular modules. The dev modules get a second map, Ns-dev, which only the package's own dev modules open. bsc takes the alias targets from the map's file name, so it is dev/Ns.mlmap, compiled with -o Ns-dev. The new bsc flag -bs-open-alias-map opens it without recording its interface CRC, as -bs-ns does for the namespace; otherwise adding a dev module would break every dev module compiled against the previous dev map. The module graph and the editor analysis follow the same rule. Diagnostic removed (open of a dependency's namespace): Example: // provider/test/Config.res (dev): let other = 2 // consumer/test/Config.res (dev): let get = () => 3 // consumer/test/Check.res (dev): open Provider let value = Config.get() Before (diagnostic): The value get can't be found in Config Diagnostic removed (dev test using the namespace entry): Before (diagnostic): Can't continue... Found a circular dependency in your code: @Provider (provider namespace map) → Provider.Local (provider/test/Local.res) → Entry (provider/src/Entry.res) → @Provider (provider namespace map) Representation modified (namespace maps of provider): Before (text): lib/rescript/Provider.mlmap: Config Local Public After (text): lib/rescript/Provider.mlmap: Public lib/rescript/dev/Provider.mlmap (compiled to Provider-dev): Config Local Breaking: Another package, or a non-dev module of the same package, can no longer use a namespaced package's dev modules ("The module or file Provider.Config can't be found"). Move shared test helpers to a regular source directory. Packages without a namespace are unchanged. Review: Why a compiler flag and not a graph node for the dev map? The dev map only holds aliases, whose targets are recorded themselves, so its CRC does not need recording, the same reasoning as for -bs-ns. A graph node would recompile every dev module whenever one is added. Fixes #9089 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gHAzsdStoCEpBgx2tS Signed-off-by: Christoph Knittel --- analysis/src/packages.ml | 12 +- ...-modules-stay-in-their-package.breaking.md | 1 + compiler/bsc/rescript_compiler_driver.ml | 4 + compiler/core/js_implementation.ml | 5 +- compiler/core/js_packages_state.ml | 6 + compiler/core/js_packages_state.mli | 2 + compiler/ml/clflags.ml | 3 + compiler/ml/clflags.mli | 3 + compiler/ml/env.ml | 23 +-- rewatch-ocaml/build_artifacts.ml | 4 + rewatch-ocaml/compiler_args.ml | 32 ++-- rewatch-ocaml/compiler_args.mli | 6 + rewatch-ocaml/compiler_args_command.ml | 7 +- rewatch-ocaml/compiler_process.ml | 159 ++++++++++++------ rewatch-ocaml/compiler_process.mli | 1 + rewatch-ocaml/config.ml | 3 + rewatch-ocaml/config.mli | 5 + rewatch-ocaml/module_graph.ml | 10 +- rewatch-ocaml/module_graph.mli | 1 + rewatch-ocaml/package_compilation.ml | 5 +- rewatch-ocaml/source.ml | 13 +- rewatch-ocaml/source.mli | 7 +- rewatch/MonorepoSupport.md | 1 + .../24-dev-modules-stay-in-their-package.sh | 120 +++++++++++++ rewatch/tests/suite.sh | 1 + .../tests-namespaced-references/rescript.json | 4 + .../tests-namespaced-references/test.sh | 6 +- .../test/MyDevHelper.res | 1 + .../test/MyDevTest.res | 4 + .../test/expected/MyDevHelper.res.txt | 0 .../test/expected/MyDevTest.res.txt | 12 ++ .../rewatch_ounit_tests/build_state_tests.ml | 69 ++++++++ .../compiler_args_tests.ml | 19 +++ .../compiler_process_tests.ml | 5 +- tests/rewatch_ounit_tests/source_tests.ml | 27 ++- 35 files changed, 486 insertions(+), 95 deletions(-) create mode 100644 changes/dev-modules-stay-in-their-package.breaking.md create mode 100755 rewatch/tests/compile/24-dev-modules-stay-in-their-package.sh create mode 100644 tests/analysis_tests/tests-namespaced-references/test/MyDevHelper.res create mode 100644 tests/analysis_tests/tests-namespaced-references/test/MyDevTest.res create mode 100644 tests/analysis_tests/tests-namespaced-references/test/expected/MyDevHelper.res.txt create mode 100644 tests/analysis_tests/tests-namespaced-references/test/expected/MyDevTest.res.txt diff --git a/analysis/src/packages.ml b/analysis/src/packages.ml index 4cfd7d37514..f14ffbd3097 100644 --- a/analysis/src/packages.ml +++ b/analysis/src/packages.ml @@ -108,7 +108,17 @@ let new_bs_package ~root_path = let cmt = Filename.concat artifact_dir namespace ^ ".cmt" in Hashtbl.replace paths_for_module namespace (Namespace {cmt}); let path = [Find_files.name_space_to_name namespace] in - [path] + (* The package's dev modules are aliased by its dev map, which + the compiler opens in dev modules only. *) + let dev_namespace = namespace ^ "-dev" in + let dev_cmt = + Filename.concat artifact_dir dev_namespace ^ ".cmt" + in + if Files.exists dev_cmt then ( + Hashtbl.replace paths_for_module dev_namespace + (Namespace {cmt = dev_cmt}); + [path; [dev_namespace]]) + else [path] in let bind f x = Option.bind x f in let compiler_flags = diff --git a/changes/dev-modules-stay-in-their-package.breaking.md b/changes/dev-modules-stay-in-their-package.breaking.md new file mode 100644 index 00000000000..965d53e3aad --- /dev/null +++ b/changes/dev-modules-stay-in-their-package.breaking.md @@ -0,0 +1 @@ +- Keep a namespaced package's dev modules out of its namespace, so that `open` of a dependency's namespace no longer shadows your own modules. Dev modules are now only visible to the dev modules of their own package; move test helpers that other packages or non-dev code use to a regular source directory. diff --git a/compiler/bsc/rescript_compiler_driver.ml b/compiler/bsc/rescript_compiler_driver.ml index 9a5f16088a3..7775c7b394c 100644 --- a/compiler/bsc/rescript_compiler_driver.ml +++ b/compiler/bsc/rescript_compiler_driver.ml @@ -333,6 +333,10 @@ let command_line_flags () : (string * Bsc_args.spec * string) array = string_call Js_packages_state.set_package_map, "*internal* Set package map, not only set package name but also use it \ as a namespace" ); + ( "-bs-open-alias-map", + string_call Js_packages_state.open_alias_map, + "*internal* Open the alias map (compiled from an \ + .mlmap) before typing, without recording its interface CRC" ); ( "-as-pp", unit_call (fun _ -> (Js_config.current ()).as_pp := true; diff --git a/compiler/core/js_implementation.ml b/compiler/core/js_implementation.ml index 73f1e4a8a64..ab4076b1cba 100644 --- a/compiler/core/js_implementation.ml +++ b/compiler/core/js_implementation.ml @@ -178,7 +178,10 @@ let make_structure_item ~ns cunit : Parsetree.structure_item = (** decoding [.mlmap] keep in sync with [namespace_task] in rewatch-ocaml/compiler_process.ml -*) + + The file name gives the namespace of the alias targets, and [-o] can give + the compiled unit another name: rewatch compiles a package's dev modules + from [dev/Ns.mlmap] with [-o Ns-dev]. *) let implementation_map ppf sourcefile = let () = (Js_config.current ()).cmj_only := true in let ichan = open_in_bin (Compiler_request_state.resolve_path sourcefile) in diff --git a/compiler/core/js_packages_state.ml b/compiler/core/js_packages_state.ml index 3e6b3b7b8e1..eaeb32ef93f 100644 --- a/compiler/core/js_packages_state.ml +++ b/compiler/core/js_packages_state.ml @@ -51,6 +51,12 @@ let set_package_map module_name = (Clflags.current ()).open_modules := module_name :: !((Clflags.current ()).open_modules) +(* Opened after the namespace of [-bs-ns], like a [-open] that follows it. *) +let open_alias_map module_name = + let flags = Clflags.current () in + flags.alias_maps := module_name :: !(flags.alias_maps); + flags.open_modules := module_name :: !(flags.open_modules) + let update_npm_package_path s = let state = current () in state.packages_info <- diff --git a/compiler/core/js_packages_state.mli b/compiler/core/js_packages_state.mli index ee0c754a73a..1107204809f 100644 --- a/compiler/core/js_packages_state.mli +++ b/compiler/core/js_packages_state.mli @@ -28,6 +28,8 @@ val make_runtime : unit -> unit val set_package_map : string -> unit +val open_alias_map : string -> unit + val get_packages_info : unit -> Js_packages_info.t val update_npm_package_path : string -> unit diff --git a/compiler/ml/clflags.ml b/compiler/ml/clflags.ml index b32e92b1d83..6918dbae24c 100644 --- a/compiler/ml/clflags.ml +++ b/compiler/ml/clflags.ml @@ -24,6 +24,7 @@ type t = { color: Misc.Color.setting option ref; assume_no_mli: mli_status ref; dont_record_crc_unit: string option ref; + alias_maps: string list ref; bs_gentype: bool ref; jsx_preserve: bool ref; no_assert_false: bool ref; @@ -55,6 +56,7 @@ let create () = color = ref None (* -color *); assume_no_mli = ref Mli_non_exists; dont_record_crc_unit = ref None; + alias_maps = ref [] (* -bs-open-alias-map *); bs_gentype = ref false; jsx_preserve = ref false (* -bs-jsx-preserve *); no_assert_false = ref false; @@ -101,6 +103,7 @@ let reset () = state.color := Some Misc.Color.Always; state.assume_no_mli := Mli_non_exists; state.dont_record_crc_unit := None; + state.alias_maps := []; state.bs_gentype := false; state.jsx_preserve := false; state.no_assert_false := false; diff --git a/compiler/ml/clflags.mli b/compiler/ml/clflags.mli index 2844282e751..833b95f48be 100644 --- a/compiler/ml/clflags.mli +++ b/compiler/ml/clflags.mli @@ -24,6 +24,9 @@ type t = { color: Misc.Color.setting option ref; assume_no_mli: mli_status ref; dont_record_crc_unit: string option ref; + alias_maps: string list ref; + (** Alias maps opened with [-bs-open-alias-map]; like the namespace of + [-bs-ns], their interface CRCs are not recorded *) bs_gentype: bool ref; jsx_preserve: bool ref; (** Keep JSX in the output ([-bs-jsx-preserve]); read by translcore, and diff --git a/compiler/ml/env.ml b/compiler/ml/env.ml index 6278d4c23af..ce90a5e6e58 100644 --- a/compiler/ml/env.ml +++ b/compiler/ml/env.ml @@ -1919,17 +1919,18 @@ let read_signature modname filename = (* Return the CRC of the interface of the given compilation unit *) let imports () = - let dont_record_crc_unit = !((Clflags.current ()).dont_record_crc_unit) in - match dont_record_crc_unit with - | None -> - Consistbl.extract (String_set.elements !(imported_units ())) (crc_units ()) - | Some x -> - Consistbl.extract - (String_set.fold - (fun m acc -> if m = x then acc else m :: acc) - !(imported_units ()) - []) - (crc_units ()) + let flags = Clflags.current () in + (* A namespace or alias map only holds module aliases, whose targets are + recorded themselves, so a unit does not depend on the map's interface. *) + let unrecorded = + Option.to_list !(flags.dont_record_crc_unit) @ !(flags.alias_maps) + in + Consistbl.extract + (String_set.fold + (fun m acc -> if List.mem m unrecorded then acc else m :: acc) + !(imported_units ()) + []) + (crc_units ()) (* Save a signature to a file *) diff --git a/rewatch-ocaml/build_artifacts.ml b/rewatch-ocaml/build_artifacts.ml index 4d50c717daf..b699b0f236a 100644 --- a/rewatch-ocaml/build_artifacts.ml +++ b/rewatch-ocaml/build_artifacts.ml @@ -208,6 +208,10 @@ let cleanup_stale ~artifact_files ~ast_sources ~present_source_files Config.namespace_compiler_name config.namespace |> Option.iter (fun namespace -> add_expected namespace [".cmi"; ".cmj"; ".cmt"; ".mlmap"]); + (* The dev map's .mlmap is in dev/, which is not scanned for stale files. *) + Source.dev_namespace_compiler_name config modules + |> Option.iter (fun dev_namespace -> + add_expected dev_namespace [".cmi"; ".cmj"; ".cmt"]); let removed_modules = ref [] in artifact_files |> List.iter (fun path -> diff --git a/rewatch-ocaml/compiler_args.ml b/rewatch-ocaml/compiler_args.ml index dbc16c555a2..aa730e48250 100644 --- a/rewatch-ocaml/compiler_args.ml +++ b/rewatch-ocaml/compiler_args.ml @@ -82,13 +82,23 @@ let gentype_dependency_args_from_paths (config : Config.t) dependencies = | None -> [] | Some path -> ["-bs-gentype-dep-path"; dependency.name ^ "=" ^ path]) -let namespace_args (config : Config.t) module_name = - match config.namespace with - | Config.No_namespace -> [] - | Config.Namespace namespace -> ["-bs-ns"; namespace] - | Config.Namespace_with_entry {name; entry} -> - if entry = module_name then ["-open"; "@" ^ name] - else ["-bs-ns"; "@" ^ name] +(* Dev modules also open the dev map, which aliases the package's dev modules + (Source.namespace_members). *) +let namespace_args (config : Config.t) ~is_dev module_name = + let namespace = + match config.namespace with + | Config.No_namespace -> [] + | Config.Namespace namespace -> ["-bs-ns"; namespace] + | Config.Namespace_with_entry {name; entry} -> + if entry = module_name then ["-open"; "@" ^ name] + else ["-bs-ns"; "@" ^ name] + in + let dev_namespace = + match Config.dev_namespace_compiler_name config.namespace with + | Some dev_namespace when is_dev -> ["-bs-open-alias-map"; dev_namespace] + | Some _ | None -> [] + in + namespace @ dev_namespace let parser_arguments ~(config : Config.t) ~contents ~path = parser_flags ~ppx_flags:(filter_ppx_flags config.ppx_flags contents) config @@ -112,7 +122,7 @@ let compiler_common_arguments ~project_root ~(config : Config.t) ~runtime @ ["-bs-package-name"; config.name; "-bs-project-root"; project_root] let compiler_arguments_with_common ~(config : Config.t) ~common_args - ~module_name ~source_kind ~has_interface ~path = + ~module_name ~is_dev ~source_kind ~has_interface ~path = let interface_args = match source_kind with | Source.Implementation when has_interface -> ["-bs-read-cmi"] @@ -126,15 +136,15 @@ let compiler_arguments_with_common ~(config : Config.t) ~common_args (fun spec -> ["-bs-package-output"; package_output config path spec]) config.package_specs in - namespace_args config module_name + namespace_args config ~is_dev module_name @ interface_args @ common_args @ output_args @ [Source.ast_path path] let compiler_arguments ~project_root ~(config : Config.t) ~runtime - ~dependency_dirs ~module_name ~source_kind ~has_interface ~watch + ~dependency_dirs ~module_name ~is_dev ~source_kind ~has_interface ~watch ~gentype_dependency_args ~path = compiler_arguments_with_common ~config ~common_args: (compiler_common_arguments ~project_root ~config ~runtime ~dependency_dirs ~watch ~gentype_dependency_args) - ~module_name ~source_kind ~has_interface ~path + ~module_name ~is_dev ~source_kind ~has_interface ~path diff --git a/rewatch-ocaml/compiler_args.mli b/rewatch-ocaml/compiler_args.mli index e70b49c81dd..b2da74d4414 100644 --- a/rewatch-ocaml/compiler_args.mli +++ b/rewatch-ocaml/compiler_args.mli @@ -13,6 +13,10 @@ val with_local_warning_policy : is_local:bool -> Config.t -> Config.t val gentype_dependency_args_from_paths : Config.t -> (Config.dependency * string) list -> string list +val namespace_args : Config.t -> is_dev:bool -> string -> string list +(** The namespace arguments of the module with the given name. Dev modules + also open their package's dev map. *) + val parser_arguments : config:Config.t -> contents:string -> path:string -> string list @@ -22,6 +26,7 @@ val compiler_arguments : runtime:string -> dependency_dirs:string list -> module_name:string -> + is_dev:bool -> source_kind:Source.source_kind -> has_interface:bool -> watch:bool -> @@ -42,6 +47,7 @@ val compiler_arguments_with_common : config:Config.t -> common_args:string list -> module_name:string -> + is_dev:bool -> source_kind:Source.source_kind -> has_interface:bool -> path:string -> diff --git a/rewatch-ocaml/compiler_args_command.ml b/rewatch-ocaml/compiler_args_command.ml index ff84a7a5b23..28eaaae37e6 100644 --- a/rewatch-ocaml/compiler_args_command.ml +++ b/rewatch-ocaml/compiler_args_command.ml @@ -80,8 +80,9 @@ let run ~verbosity path = | Source.Implementation -> File_util.exists (source ^ "i") | Source.Interface -> false in + let is_dev = Config.source_is_dev config relative in let dependencies = - (if Config.source_is_dev config relative then + (if is_dev then List.map (fun dependency -> (false, dependency)) config.dev_dependencies else []) @ List.map (fun dependency -> (true, dependency)) config.dependencies @@ -113,8 +114,8 @@ let run ~verbosity path = Compiler_args.compiler_arguments ~project_root ~config ~runtime ~dependency_dirs ~module_name:(Source.module_name source) - ~source_kind ~has_interface ~watch:false ~gentype_dependency_args:[] - ~path:relative + ~is_dev ~source_kind ~has_interface ~watch:false + ~gentype_dependency_args:[] ~path:relative in Yojson.Safe.pretty_to_string (`Assoc diff --git a/rewatch-ocaml/compiler_process.ml b/rewatch-ocaml/compiler_process.ml index d38435b28b4..9aad0fd949a 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 = +(* Runs [requests] one after another until one fails. *) +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 : Process.result) request -> + if not (Process.succeeded result) then result + else + let next = run_in_process ~poll request in + { + next with + stdout = result.stdout ^ next.stdout; + stderr = result.stderr ^ next.stderr; + }) + {status = Unix.WEXITED 0; stdout = ""; stderr = ""} + requests) + +let task request = sequential_task [request] let run_tasks ?poll ?on_complete tasks = Process.run_tasks @@ -225,74 +239,109 @@ let publish_compiler_artifacts ~artifact_dir ~basename artifacts = with error -> raise (Compiler_scheduler.Publication_failure (error, !cmi_change)) -let namespace_task ~runtime ~artifact_dir ~entry ~package_dirty ~force namespace - modules = - let mlmap = Filename.concat artifact_dir (namespace ^ ".mlmap") in - let contents = - let buffer = Buffer.create 128 in - Buffer.add_string buffer "randjbuildsystem\n"; - Source.namespace_members ~entry modules - |> List.map (fun module_ -> module_.Source.name) - |> List.sort String.compare - |> List.iter (fun name -> - Buffer.add_string buffer name; - Buffer.add_char buffer '\n'); - Buffer.contents buffer +type namespace_map = {basename: string; mlmap: string; dev: bool} + +(* A package's dev modules are aliased by a second map, [Ns-dev], which only + they open (Compiler_args.namespace_args). The compiler takes the namespace + of the alias targets from the map's file name, so the dev map is + [dev/Ns.mlmap], compiled with [-o Ns-dev]. Both maps are compiled by one + task, so that a build that fails or is killed before both are published + compiles both again. *) +let namespace_task ~runtime ~artifact_dir ~entry ~package_dirty ~force + ~dev_namespace namespace modules = + let namespace_map = + {basename = namespace; mlmap = namespace ^ ".mlmap"; dev = false} + in + let dev_map = + dev_namespace + |> Option.map (fun basename -> + { + basename; + mlmap = Filename.concat "dev" (namespace ^ ".mlmap"); + dev = true; + }) + in + let maps = namespace_map :: Option.to_list dev_map in + let write_mlmap {mlmap; dev} = + let mlmap = Filename.concat artifact_dir mlmap in + let contents = + let buffer = Buffer.create 128 in + Buffer.add_string buffer "randjbuildsystem\n"; + Source.namespace_members ~entry ~dev modules + |> List.map (fun module_ -> module_.Source.name) + |> List.sort String.compare + |> List.iter (fun name -> + Buffer.add_string buffer name; + Buffer.add_char buffer '\n'); + Buffer.contents buffer + in + let previous_contents = + try Some (File_util.read_file mlmap) + with Unix.Unix_error ((Unix.ENOENT | Unix.ENOTDIR), _, _) -> None + in + let changed = previous_contents <> Some contents in + if changed then + File_util.write_file_atomic ~ensure_parent:dev mlmap contents; + changed + in + (* Every map is written, even when an earlier one changed. *) + let mlmap_changed = + List.fold_left (fun changed map -> write_mlmap map || changed) false maps in - let previous_contents = - try Some (File_util.read_file mlmap) - with Unix.Unix_error ((Unix.ENOENT | Unix.ENOTDIR), _, _) -> None + let artifact {basename} extension = + Filename.concat artifact_dir (basename ^ extension) in - let mlmap_changed = previous_contents <> Some contents in - if mlmap_changed then - File_util.write_file_atomic ~ensure_parent:false mlmap contents; - let typed_tree = Filename.concat artifact_dir (namespace ^ ".cmt") in let outputs_current = - ["cmi"; "cmj"; "cmt"] - |> List.for_all (fun extension -> - File_util.is_regular_file - (Filename.concat artifact_dir (namespace ^ "." ^ extension))) + maps + |> List.for_all (fun map -> + [".cmi"; ".cmj"; ".cmt"] + |> List.for_all (fun extension -> + File_util.is_regular_file (artifact map extension))) in if not (force || package_dirty || mlmap_changed || not outputs_current) then None else ( (* The typed tree, published last, marks the namespace compiled: without it, a build that fails or is killed before then compiles it again. *) - File_util.remove_file typed_tree; + List.iter (fun map -> File_util.remove_file (artifact map ".cmt")) maps; + let request {basename; mlmap; dev} = + { + args = + ["-runtime-path"; runtime; "-w"; "-49"; "-color"; "always"] + @ (if dev then ["-o"; basename] else []) + @ ["-no-alias-deps"; mlmap]; + cwd = artifact_dir; + } + in + let publish {basename} = + publish_compiler_artifacts ~artifact_dir ~basename + [Cmi; Required "cmj"; Typed_tree "cmt"] + in Some Compiler_scheduler. { - task = - task - { - args = - [ - "-runtime-path"; - runtime; - "-w"; - "-49"; - "-color"; - "always"; - "-no-alias-deps"; - Filename.basename mlmap; - ]; - cwd = artifact_dir; - }; + task = sequential_task (List.map request maps); publish = (fun result -> if not (Process.succeeded result) then ( List.iter - (fun extension -> - Build_file_store.forget - (Filename.concat artifact_dir (namespace ^ extension))) - [".cmi"; ".cmt"]; + (fun map -> + List.iter + (fun extension -> + Build_file_store.forget (artifact map extension)) + [".cmi"; ".cmt"]) + maps; raise (Compiler_scheduler.Build_failure (result.Process.stderr ^ result.stdout))); - let cmi_change = - publish_compiler_artifacts ~artifact_dir ~basename:namespace - [Cmi; Required "cmj"; Typed_tree "cmt"] - in + let cmi_change = publish namespace_map in + (* Only the namespace map's interface matters to dependents: the + modules that open the dev map do not record its interface + (-bs-open-alias-map). *) + (try Option.iter (fun map -> ignore (publish map)) dev_map + with Compiler_scheduler.Publication_failure (error, _) -> + raise + (Compiler_scheduler.Publication_failure (error, cmi_change))); Compiler_scheduler.{stderr = result.stderr; cmi_change}); }) @@ -322,7 +371,7 @@ let compile_request ~artifact_dir ~(config : Config.t) ~common_args (module_ : Source.module_) ~source_kind path = let args = Compiler_args.compiler_arguments_with_common ~config ~common_args - ~module_name:module_.name ~source_kind + ~module_name:module_.name ~is_dev:module_.is_dev ~source_kind ~has_interface:(Option.is_some module_.interface) ~path in diff --git a/rewatch-ocaml/compiler_process.mli b/rewatch-ocaml/compiler_process.mli index 9b7e2b0bd30..0b9b8b4c69f 100644 --- a/rewatch-ocaml/compiler_process.mli +++ b/rewatch-ocaml/compiler_process.mli @@ -35,6 +35,7 @@ val namespace_task : entry:string option -> package_dirty:bool -> force:bool -> + dev_namespace:string option -> string -> Source.module_ list -> Compiler_scheduler.namespace_task option diff --git a/rewatch-ocaml/config.ml b/rewatch-ocaml/config.ml index 6ec6f833598..48e8314b3ef 100644 --- a/rewatch-ocaml/config.ml +++ b/rewatch-ocaml/config.ml @@ -15,6 +15,9 @@ let namespace_compiler_name = function | Namespace name -> Some name | Namespace_with_entry {name; entry = _} -> Some ("@" ^ name) +let dev_namespace_compiler_name namespace = + namespace_compiler_name namespace |> Option.map (fun name -> name ^ "-dev") + let namespaced_module_name namespace module_name = match namespace with | No_namespace -> module_name diff --git a/rewatch-ocaml/config.mli b/rewatch-ocaml/config.mli index 36299039226..6307261a883 100644 --- a/rewatch-ocaml/config.mli +++ b/rewatch-ocaml/config.mli @@ -3,6 +3,11 @@ include module type of Config_types val namespace_name : namespace -> string option val namespace_entry : namespace -> string option val namespace_compiler_name : namespace -> string option + +val dev_namespace_compiler_name : namespace -> string option +(** The unit that aliases a package's dev modules, which only its dev modules + open: [Ns-dev] for namespace [Ns]. *) + val namespaced_module_name : namespace -> string -> string val path_in_root : string -> string val exists_in_root : string -> bool diff --git a/rewatch-ocaml/module_graph.ml b/rewatch-ocaml/module_graph.ml index 0b1d801ee5c..ac82e4bcf01 100644 --- a/rewatch-ocaml/module_graph.ml +++ b/rewatch-ocaml/module_graph.ml @@ -17,6 +17,7 @@ type module_node = { package_root: string; source_path: string; namespace: Config.namespace; + is_dev: bool; visible_packages: (string, unit) Hashtbl.t; mutable raw_dependencies: string list; } @@ -94,8 +95,13 @@ let resolve_dependency ~find_module ~find_namespace_maps (node : module_node) | _ -> raw_name in let local_key = Config.namespaced_module_name node.namespace local_name in + (* A namespaced package's dev modules are only aliased by its dev map, which + only its own dev modules open (Source.namespace_members). *) let is_visible (dependency_node : module_node) = Hashtbl.mem node.visible_packages dependency_node.package_name + && ((not dependency_node.is_dev) + || dependency_node.namespace = Config.No_namespace + || (node.is_dev && dependency_node.package_root = node.package_root)) in match find_module local_key with | Some dependency_node when is_visible dependency_node -> [local_key] @@ -233,6 +239,7 @@ let initialize ~(root_config : Config.t) ~package_plans ~compile_assets package_root = package.root; source_path = module_.Source.implementation; namespace = package.compile_config.namespace; + is_dev = module_.Source.is_dev; visible_packages; raw_dependencies; } @@ -270,7 +277,8 @@ let initialize ~(root_config : Config.t) ~package_plans ~compile_assets namespace_details |> Option.map (fun (compiler_name, name, namespace_entry) -> let members = - Source.namespace_members ~entry:namespace_entry package.modules + Source.namespace_members ~entry:namespace_entry ~dev:false + package.modules |> List.map (fun module_ -> Source.compiler_basename package.compile_config module_.Source.name) diff --git a/rewatch-ocaml/module_graph.mli b/rewatch-ocaml/module_graph.mli index 7b867904ac5..60c6b252ea9 100644 --- a/rewatch-ocaml/module_graph.mli +++ b/rewatch-ocaml/module_graph.mli @@ -22,6 +22,7 @@ type module_node = { package_root: string; source_path: string; namespace: Config.namespace; + is_dev: bool; visible_packages: (string, unit) Hashtbl.t; mutable raw_dependencies: string list; } diff --git a/rewatch-ocaml/package_compilation.ml b/rewatch-ocaml/package_compilation.ml index b9c850c1875..cacbb4db651 100644 --- a/rewatch-ocaml/package_compilation.ml +++ b/rewatch-ocaml/package_compilation.ml @@ -162,8 +162,9 @@ let prepare ~(package : Package_plan.t) ~(prepared : Build_session.prepared) Compiler_process.namespace_task ~runtime:prepared.compiler_context.runtime_path ~artifact_dir ~entry:(Config.namespace_entry config.namespace) - ~package_dirty ~force:namespace_state.compile_dirty compiler_name - modules + ~package_dirty ~force:namespace_state.compile_dirty + ~dev_namespace:(Source.dev_namespace_compiler_name config modules) + compiler_name modules |> Option.iter (fun namespace_task -> namespace_state.compile_dirty <- true; let cmi_path = diff --git a/rewatch-ocaml/source.ml b/rewatch-ocaml/source.ml index 73b7283877f..1da43b7f7c0 100644 --- a/rewatch-ocaml/source.ml +++ b/rewatch-ocaml/source.ml @@ -283,11 +283,20 @@ let active_sources (config : Config.t) ~prod ~features = (source_is_active ~prod ~all_features ~active_features) config.sources -let namespace_members ~entry modules = +(* The namespace map leaves out dev modules, so that a package opening the + namespace does not see them. They are in the dev map, which only the + package's own dev modules open. *) +let namespace_members ~entry ~dev modules = modules - |> List.filter (fun module_ -> Some module_.name <> entry) + |> List.filter (fun (module_ : module_) -> + module_.is_dev = dev && Some module_.name <> entry) |> List.filter (fun module_ -> is_non_exotic_module_name module_.name) +let dev_namespace_compiler_name (config : Config.t) modules = + if List.exists (fun (module_ : module_) -> module_.is_dev) modules then + Config.dev_namespace_compiler_name config.namespace + else None + let scan_sources ~on_missing (config : Config.t) ~prod ~features ~collect_inventory ~collect_gentype = let active_features = diff --git a/rewatch-ocaml/source.mli b/rewatch-ocaml/source.mli index 511fadc3b81..369fa57155a 100644 --- a/rewatch-ocaml/source.mli +++ b/rewatch-ocaml/source.mli @@ -27,7 +27,12 @@ val module_name : string -> string val without_current_dir_segments : string -> string (** [path] without its "." segments: "./src/./A.res" is "src/A.res". *) -val namespace_members : entry:string option -> module_ list -> module_ list +val namespace_members : + entry:string option -> dev:bool -> module_ list -> module_ list +(** The modules of the namespace map, or with [~dev:true] of the dev map. *) + +val dev_namespace_compiler_name : Config.t -> module_ list -> string option +(** The dev map of a namespaced package with dev modules. *) val duplicate_error : display_root:string -> string -> string -> string -> string -> exn diff --git a/rewatch/MonorepoSupport.md b/rewatch/MonorepoSupport.md index f4c8bc24d03..62af33fab65 100644 --- a/rewatch/MonorepoSupport.md +++ b/rewatch/MonorepoSupport.md @@ -145,6 +145,7 @@ For package `A` depending on package `B`: Module dependencies discovered from `.ast` files are **filtered**: - A module in another package is only valid if that package is declared in `dependencies` or `dev-dependencies` - "It compiles locally because the module exists" is **not** sufficient +- The dev modules (`"type": "dev"` sources) of a namespaced package are only visible to the dev modules of the same package. The namespace `Ns` aliases the other modules; the dev modules are aliased by a separate map `Ns-dev` (compiled from `dev/Ns.mlmap`), which only the package's dev modules open. `open Ns` in another package therefore never brings a dev module into scope. --- diff --git a/rewatch/tests/compile/24-dev-modules-stay-in-their-package.sh b/rewatch/tests/compile/24-dev-modules-stay-in-their-package.sh new file mode 100755 index 00000000000..dd632f8ef7f --- /dev/null +++ b/rewatch/tests/compile/24-dev-modules-stay-in-their-package.sh @@ -0,0 +1,120 @@ +#!/bin/bash +# A namespaced package's dev modules are aliased by its dev map, which only +# its own dev modules open: they neither shadow a consumer's modules after +# `open Provider` nor are reachable from other packages or non-dev modules. + +cd "$(dirname "$0")" +source "../utils.sh" + +bold "Test: Dev modules stay in their package" + +test_dir=$(pwd) +fixture=$(mktemp -d 2>/dev/null || mktemp -d -t rewatch-dev-modules) +trap 'cd "$test_dir"; rm -rf "$fixture"' EXIT +cd "$fixture" + +node <<'NODE' +const fs = require("fs"); +const path = require("path"); +const write = (file, contents) => { + fs.mkdirSync(path.dirname(file), {recursive: true}); + fs.writeFileSync(file, contents); +}; +const json = value => JSON.stringify(value, null, 2) + "\n"; +const specs = { + "package-specs": {module: "esmodule", "in-source": true}, + suffix: ".mjs", +}; +write("package.json", json({name: "root", private: true, workspaces: ["consumer", "provider"]})); +write("rescript.json", json({name: "root", sources: ["src"], dependencies: ["consumer"], ...specs})); +write("src/Main.res", "let value = Consumer.Use.value\n"); +for (const name of ["consumer", "provider"]) { + write(`${name}/package.json`, json({name, version: "1.0.0"})); + write(`${name}/rescript.json`, json({ + name, + namespace: true, + sources: [{dir: "src"}, {dir: "test", type: "dev"}], + dependencies: name === "consumer" ? ["provider"] : [], + ...specs, + })); + fs.mkdirSync("node_modules", {recursive: true}); + // A junction on Windows, which needs no privileges; a symlink elsewhere. + fs.symlinkSync(path.resolve(name), `node_modules/${name}`, "junction"); +} +write("provider/src/Public.res", "let make = () => 1\n"); +write("provider/test/Config.res", "let other = 2\n"); +// A dev module reaches its package's dev modules without qualification. +write("provider/test/Local.res", "let value = Config.other + Public.make()\n"); +write("consumer/src/Use.res", "let value = Provider.Public.make()\n"); +write("consumer/test/Config.res", "let get = () => 3\n"); +// Opening Provider must not shadow the consumer's own Config (#9089). +write("consumer/test/Check.res", "open Provider\nlet value = Config.get() + Public.make()\n"); +NODE + +# Usage: build_succeeds [build options] +build_succeeds() { + local message=$1 + shift + if ! output=$(rewatch build "$@" 2>&1); then + error "$message" + printf '%s\n' "$output" >&2 + exit 1 + fi +} + +# Usage: build_fails +build_fails() { + if output=$(rewatch build 2>&1); then + error "$1" + exit 1 + fi + if ! printf '%s\n' "$output" | grep -q "$2"; then + error "$1: unexpected error" + printf '%s\n' "$output" >&2 + exit 1 + fi +} + +build_succeeds "Opening a dependency's namespace let its dev modules shadow the consumer's" + +# The new dev module changes the dev map. Local, compiled again, then reads +# Config's interface, compiled against the previous dev map: the dev map's +# interface must not be recorded as an import. +printf 'let value = 4\n' > provider/test/Extra.res +build_succeeds "Adding a dev module failed" +printf 'let value = Config.other + Public.make() + 1\n' > provider/test/Local.res +build_succeeds "A dev module compiled against the previous dev map broke a dependent" + +printf 'let value = Provider.Config.other\n' > consumer/test/Leak.res +build_fails "A dependency's dev module was reachable through its namespace" \ + "Provider.Config can't be found" +rm consumer/test/Leak.res + +printf 'let value = Config.other\n' > provider/src/Bad.res +build_fails "A non-dev module reached a dev module of its package" \ + "Config can't be found" +rm provider/src/Bad.res +build_succeeds "Build failed after removing the invalid modules" + +build_succeeds "Production build failed" --prod +if [ -e provider/lib/rescript/Provider-dev.cmi ]; then + error "The production build kept the dev map" + exit 1 +fi + +# With a namespace entry, a dev test can use the entry: the namespace map no +# longer depends on the dev modules, so this is not a dependency cycle. +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, null, 2) + "\n"); +fs.writeFileSync("provider/src/Entry.res", "module Public = Public\n"); +fs.writeFileSync("provider/test/Local.res", "let value = Config.other + Entry.Public.make()\n"); +fs.writeFileSync("consumer/src/Use.res", "let value = Entry.Public.make()\n"); +fs.writeFileSync("consumer/test/Check.res", "open Entry\nlet value = Config.get() + Public.make()\n"); +NODE +rewatch clean &> /dev/null +build_succeeds "A dev test using the namespace entry failed to build" + +success "Dev modules stay in their package" diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index b1adfd134e2..b1c2fc89db9 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -86,6 +86,7 @@ fi ./compile/21-rebuild-dependency-for-package-specs.sh && ./compile/22-qualified-own-namespace-dependency.sh && ./compile/23-root-source-dir-rebuild.sh && +./compile/24-dev-modules-stay-in-their-package.sh && ./compile/14-no-testrepo-changes.sh && ./compile/15-no-new-files.sh && ./compile/16-snapshots-unchanged.sh && diff --git a/tests/analysis_tests/tests-namespaced-references/rescript.json b/tests/analysis_tests/tests-namespaced-references/rescript.json index 283f6934730..59e4de75ff9 100644 --- a/tests/analysis_tests/tests-namespaced-references/rescript.json +++ b/tests/analysis_tests/tests-namespaced-references/rescript.json @@ -5,6 +5,10 @@ { "dir": "src", "subdirs": true + }, + { + "dir": "test", + "type": "dev" } ], "package-specs": [ diff --git a/tests/analysis_tests/tests-namespaced-references/test.sh b/tests/analysis_tests/tests-namespaced-references/test.sh index e03b10c7773..4bca48d7798 100755 --- a/tests/analysis_tests/tests-namespaced-references/test.sh +++ b/tests/analysis_tests/tests-namespaced-references/test.sh @@ -1,4 +1,4 @@ -for file in src/*.res; do +for file in src/*.res test/*.res; do output="$(dirname $file)/expected/$(basename $file).txt" ../../../_build/install/default/bin/rescript-editor-analysis test $file &> $output # CI. We use LF, and the CI OCaml fork prints CRLF. Convert. @@ -20,11 +20,11 @@ warningYellow='\033[0;33m' successGreen='\033[0;32m' reset='\033[0m' -diff=$(git ls-files --modified src/expected) +diff=$(git ls-files --modified src/expected test/expected) if [[ $diff = "" ]]; then printf "${successGreen}✅ No unstaged tests difference.${reset}\n" else printf "${warningYellow}⚠️ There are unstaged differences in tests/! Did you break a test?\n${diff}\n${reset}" - git --no-pager diff src/expected + git --no-pager diff src/expected test/expected exit 1 fi diff --git a/tests/analysis_tests/tests-namespaced-references/test/MyDevHelper.res b/tests/analysis_tests/tests-namespaced-references/test/MyDevHelper.res new file mode 100644 index 00000000000..c174b12a3f6 --- /dev/null +++ b/tests/analysis_tests/tests-namespaced-references/test/MyDevHelper.res @@ -0,0 +1 @@ +let devValue = 7 diff --git a/tests/analysis_tests/tests-namespaced-references/test/MyDevTest.res b/tests/analysis_tests/tests-namespaced-references/test/MyDevTest.res new file mode 100644 index 00000000000..f72805677a2 --- /dev/null +++ b/tests/analysis_tests/tests-namespaced-references/test/MyDevTest.res @@ -0,0 +1,4 @@ +let value = MyDevHelper.devValue +// ^hov +// MyDevHelper.d +// ^com diff --git a/tests/analysis_tests/tests-namespaced-references/test/expected/MyDevHelper.res.txt b/tests/analysis_tests/tests-namespaced-references/test/expected/MyDevHelper.res.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/analysis_tests/tests-namespaced-references/test/expected/MyDevTest.res.txt b/tests/analysis_tests/tests-namespaced-references/test/expected/MyDevTest.res.txt new file mode 100644 index 00000000000..10d9388383d --- /dev/null +++ b/tests/analysis_tests/tests-namespaced-references/test/expected/MyDevTest.res.txt @@ -0,0 +1,12 @@ +Hover test/MyDevTest.res 0:24 +{ "contents": { "kind": "markdown", "value": "```rescript\nint\n```" } } + +Complete test/MyDevTest.res 2:16 +pos_cursor:[2:16] pos_no_white:[2:15] Found expr:[2:3->2:16] +Pexp_ident MyDevHelper.d:[2:3->2:16] +Completable: Cpath Value[MyDevHelper, d] +Package opens Stdlib.place holder Pervasives.JsxModules.place holder MyNamespace.place holder MyNamespace-dev.place holder +Resolved opens 3 MyNamespace-dev MyNamespace Stdlib +ContextPath Value[MyDevHelper, d] +Path MyDevHelper.d +[ { "detail": "int", "kind": 12, "label": "devValue", "tags": [] } ] diff --git a/tests/rewatch_ounit_tests/build_state_tests.ml b/tests/rewatch_ounit_tests/build_state_tests.ml index 17e4267eeff..c0ba9ee42a1 100644 --- a/tests/rewatch_ounit_tests/build_state_tests.ml +++ b/tests/rewatch_ounit_tests/build_state_tests.ml @@ -88,6 +88,7 @@ let dependency_state_tests _context = source_path = entry_source.implementation; namespace = Config.Namespace_with_entry {name = "Namespace"; entry = "Entry"}; + is_dev = false; visible_packages = Hashtbl.create 0; raw_dependencies = []; } @@ -113,6 +114,73 @@ let dependency_state_tests _context = = [namespace_map.key]) "namespace entry implicitly depends on its namespace map" +let dev_module_visibility_tests _context = + let visible packages = + let table = Hashtbl.create 2 in + List.iter (fun package -> Hashtbl.replace table package ()) packages; + table + in + let node ~package ~namespace ~is_dev key raw_dependencies : + Module_graph.module_node = + { + key; + package_name = package; + package_root = package; + source_path = "src/" ^ key ^ ".res"; + namespace; + is_dev; + visible_packages = visible ["provider"; "plain"; package]; + raw_dependencies; + } + in + let provider = + node ~package:"provider" ~namespace:(Config.Namespace "Provider") + in + let consumer = + node ~package:"consumer" ~namespace:(Config.Namespace "Consumer") + in + let plain = node ~package:"plain" ~namespace:Config.No_namespace in + let provider_map : Module_graph.namespace_map = + { + key = Module_graph.namespace_map_key "provider"; + compiler_name = "Provider"; + namespace = "Provider"; + package_name = "provider"; + package_root = "provider"; + members = ["Public-Provider"]; + } + in + let modules = Hashtbl.create 4 in + List.iter + (fun (node : Module_graph.module_node) -> Hashtbl.add modules node.key node) + [ + provider ~is_dev:false "Public-Provider" []; + provider ~is_dev:true "Config-Provider" []; + plain ~is_dev:true "Helper" []; + ]; + let namespace_maps = Hashtbl.create 1 in + Hashtbl.add namespace_maps "Provider" [provider_map]; + let resolved (node : Module_graph.module_node) = + Module_graph.resolved_dependencies ~find_module:(Hashtbl.find_opt modules) + ~find_namespace_maps:(Hashtbl.find_opt namespace_maps) + node + in + assert_equal ["Config-Provider"] + (resolved (provider ~is_dev:true "Local-Provider" ["Config"])) + ~msg:"a dev module depends on a dev module of its package"; + assert_equal [] + (resolved (provider ~is_dev:false "Bad-Provider" ["Config"])) + ~msg:"a non-dev module does not see the dev modules of its package"; + assert_equal [provider_map.key] + (resolved (consumer ~is_dev:true "Leak-Consumer" ["Provider.Config"])) + ~msg:"another package does not see a namespaced package's dev modules"; + assert_equal ["Public-Provider"] + (resolved (consumer ~is_dev:true "Use-Consumer" ["Provider.Public"])) + ~msg:"another package sees a namespaced package's other modules"; + assert_equal ["Helper"] + (resolved (consumer ~is_dev:false "Plain-Consumer" ["Helper"])) + ~msg:"dev modules of a package without a namespace stay global" + let carries_forward_dirty_state_only_for_matching_sources _context = let previous = Build_state.create 2 in let next = Build_state.create 2 in @@ -138,6 +206,7 @@ let tests = "build_state_tests" >::: [ "dependency state" >:: dependency_state_tests; + "dev module visibility" >:: dev_module_visibility_tests; "carry forward blocked work" >:: carries_forward_dirty_state_only_for_matching_sources; ] diff --git a/tests/rewatch_ounit_tests/compiler_args_tests.ml b/tests/rewatch_ounit_tests/compiler_args_tests.ml index d1bf39991bf..97bd6f861b1 100644 --- a/tests/rewatch_ounit_tests/compiler_args_tests.ml +++ b/tests/rewatch_ounit_tests/compiler_args_tests.ml @@ -97,6 +97,25 @@ 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_args namespace = + Compiler_args.namespace_args {config with namespace} + in + assert_equal ["-bs-ns"; "Ns"] + (namespace_args (Config.Namespace "Ns") ~is_dev:false "A") + ~msg:"non-dev modules do not open the dev map"; + assert_equal + ["-bs-ns"; "Ns"; "-bs-open-alias-map"; "Ns-dev"] + (namespace_args (Config.Namespace "Ns") ~is_dev:true "A") + ~msg:"dev modules open the dev map after the namespace"; + assert_equal + ["-open"; "@Ns"; "-bs-open-alias-map"; "@Ns-dev"] + (namespace_args + (Config.Namespace_with_entry {name = "Ns"; entry = "Entry"}) + ~is_dev:true "Entry") + ~msg:"a dev namespace entry opens the dev map after the namespace"; + assert_equal [] + (namespace_args Config.No_namespace ~is_dev:true "A") + ~msg:"packages without a namespace have no dev map"; let gentype_config = {config with gentype_args = ["-bs-gentype"]} in let selected_regular = Filename.concat root "selected/regular" in check diff --git a/tests/rewatch_ounit_tests/compiler_process_tests.ml b/tests/rewatch_ounit_tests/compiler_process_tests.ml index 4b77a198895..b5e19e91739 100644 --- a/tests/rewatch_ounit_tests/compiler_process_tests.ml +++ b/tests/rewatch_ounit_tests/compiler_process_tests.ml @@ -21,7 +21,8 @@ let publication_tests _context = in_build (fun () -> let namespace_task = Compiler_process.namespace_task ~runtime:"unused" ~artifact_dir - ~entry:None ~package_dirty:true ~force:false "Ns" [] + ~entry:None ~package_dirty:true ~force:false ~dev_namespace:None + "Ns" [] |> Option.get in (* The compiler wrote an interface, but no implementation. *) @@ -50,7 +51,7 @@ let publication_tests _context = ["cmi"; "cmj"; "cmt"]; let task ~package_dirty ~force = Compiler_process.namespace_task ~runtime:"unused" ~artifact_dir - ~entry:None ~package_dirty ~force "Ns" [] + ~entry:None ~package_dirty ~force ~dev_namespace:None "Ns" [] in check (Option.is_some (task ~package_dirty:false ~force:false)) diff --git a/tests/rewatch_ounit_tests/source_tests.ml b/tests/rewatch_ounit_tests/source_tests.ml index 8af0de3e271..a900f05331b 100644 --- a/tests/rewatch_ounit_tests/source_tests.ml +++ b/tests/rewatch_ounit_tests/source_tests.ml @@ -49,12 +49,35 @@ let tests = interface = None; is_dev = false; }; + Source. + { + name = "Fixture"; + implementation = "test/Fixture.res"; + interface = None; + is_dev = true; + }; + Source. + { + name = "Fixture-with-dash"; + implementation = "test/Fixture-with-dash.res"; + interface = None; + is_dev = true; + }; ] in check - (names (Source.namespace_members ~entry:(Some "Entry") namespace_modules) + (names + (Source.namespace_members ~entry:(Some "Entry") ~dev:false + namespace_modules) = ["Member"]) - "namespace membership excludes the entry and exotic module names"; + "namespace membership excludes the entry, dev modules and exotic module \ + names"; + check + (names + (Source.namespace_members ~entry:(Some "Entry") ~dev:true + namespace_modules) + = ["Fixture"]) + "dev map membership holds only dev modules with plain names"; let root = Filename.temp_file "rewatch-ocaml-sources-" "" in Sys.remove root; Unix.mkdir root 0o755; From ebf93b752b6d89c9246dff04fc23b4e4173061c3 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 14 Nov 2026 09:10:57 +0000 Subject: [PATCH 2/3] Give the output namespace and the alias maps separate flags fields Clflags.dont_record_crc_unit did two jobs: it named the outputs (Config-Ns) and kept the namespace's CRC out of the recorded imports. It is now Clflags.namespace, for the names only. -bs-ns adds the namespace to Clflags.alias_maps like -bs-open-alias-map, and Env.imports skips exactly the alias maps. The command line and the outputs are unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gHAzsdStoCEpBgx2tS Signed-off-by: Christoph Knittel --- compiler/core/config_util.ml | 2 +- compiler/core/js_packages_state.ml | 11 +++++------ compiler/ml/clflags.ml | 8 ++++---- compiler/ml/clflags.mli | 8 +++++--- compiler/ml/env.ml | 12 +++++------- 5 files changed, 20 insertions(+), 21 deletions(-) diff --git a/compiler/core/config_util.ml b/compiler/core/config_util.ml index 7c041c0c303..ee265801f46 100644 --- a/compiler/core/config_util.ml +++ b/compiler/core/config_util.ml @@ -30,5 +30,5 @@ let output_prefix name = | None -> Ext_namespace_encode.make (Filename.remove_extension name) - ?ns:!((Clflags.current ()).dont_record_crc_unit) + ?ns:!((Clflags.current ()).namespace) | Some oname -> Filename.remove_extension oname diff --git a/compiler/core/js_packages_state.ml b/compiler/core/js_packages_state.ml index eaeb32ef93f..adf66215f1a 100644 --- a/compiler/core/js_packages_state.ml +++ b/compiler/core/js_packages_state.ml @@ -46,17 +46,16 @@ let make_runtime () : unit = state.making_runtime <- true; state.packages_info <- Js_packages_info.runtime_package_specs -let set_package_map module_name = - (Clflags.current ()).dont_record_crc_unit := Some module_name; - (Clflags.current ()).open_modules := - module_name :: !((Clflags.current ()).open_modules) - -(* Opened after the namespace of [-bs-ns], like a [-open] that follows it. *) +(* Opened in flag order, like [-open]. *) let open_alias_map module_name = let flags = Clflags.current () in flags.alias_maps := module_name :: !(flags.alias_maps); flags.open_modules := module_name :: !(flags.open_modules) +let set_package_map module_name = + (Clflags.current ()).namespace := Some module_name; + open_alias_map module_name + let update_npm_package_path s = let state = current () in state.packages_info <- diff --git a/compiler/ml/clflags.ml b/compiler/ml/clflags.ml index 6918dbae24c..2aa417bee21 100644 --- a/compiler/ml/clflags.ml +++ b/compiler/ml/clflags.ml @@ -23,7 +23,7 @@ type t = { keep_locs: bool ref; color: Misc.Color.setting option ref; assume_no_mli: mli_status ref; - dont_record_crc_unit: string option ref; + namespace: string option ref; alias_maps: string list ref; bs_gentype: bool ref; jsx_preserve: bool ref; @@ -55,8 +55,8 @@ let create () = keep_locs = ref true (* -keep-locs *); color = ref None (* -color *); assume_no_mli = ref Mli_non_exists; - dont_record_crc_unit = ref None; - alias_maps = ref [] (* -bs-open-alias-map *); + namespace = ref None (* -bs-ns *); + alias_maps = ref [] (* -bs-ns, -bs-open-alias-map *); bs_gentype = ref false; jsx_preserve = ref false (* -bs-jsx-preserve *); no_assert_false = ref false; @@ -102,7 +102,7 @@ let reset () = state.keep_locs := true; state.color := Some Misc.Color.Always; state.assume_no_mli := Mli_non_exists; - state.dont_record_crc_unit := None; + state.namespace := None; state.alias_maps := []; state.bs_gentype := false; state.jsx_preserve := false; diff --git a/compiler/ml/clflags.mli b/compiler/ml/clflags.mli index 833b95f48be..bf0f7f2c979 100644 --- a/compiler/ml/clflags.mli +++ b/compiler/ml/clflags.mli @@ -23,10 +23,12 @@ type t = { keep_locs: bool ref; color: Misc.Color.setting option ref; assume_no_mli: mli_status ref; - dont_record_crc_unit: string option ref; + namespace: string option ref; + (** Appended to the names of the outputs ([-bs-ns]): [Config] becomes + [Config-Ns] *) alias_maps: string list ref; - (** Alias maps opened with [-bs-open-alias-map]; like the namespace of - [-bs-ns], their interface CRCs are not recorded *) + (** Opened alias maps, the namespace among them, whose interface CRCs are + not recorded ([-bs-ns], [-bs-open-alias-map]) *) bs_gentype: bool ref; jsx_preserve: bool ref; (** Keep JSX in the output ([-bs-jsx-preserve]); read by translcore, and diff --git a/compiler/ml/env.ml b/compiler/ml/env.ml index ce90a5e6e58..ee5245a21a2 100644 --- a/compiler/ml/env.ml +++ b/compiler/ml/env.ml @@ -1919,15 +1919,13 @@ let read_signature modname filename = (* Return the CRC of the interface of the given compilation unit *) let imports () = - let flags = Clflags.current () in - (* A namespace or alias map only holds module aliases, whose targets are - recorded themselves, so a unit does not depend on the map's interface. *) - let unrecorded = - Option.to_list !(flags.dont_record_crc_unit) @ !(flags.alias_maps) - in + (* An alias map, such as a namespace, only holds module aliases, whose + targets are recorded themselves, so a unit does not depend on the map's + interface. *) + let alias_maps = !((Clflags.current ()).alias_maps) in Consistbl.extract (String_set.fold - (fun m acc -> if List.mem m unrecorded then acc else m :: acc) + (fun m acc -> if List.mem m alias_maps then acc else m :: acc) !(imported_units ()) []) (crc_units ()) From 80e2db1e976bb3f87323a930da253c3054b94c71 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 14 Nov 2026 09:19:02 +0000 Subject: [PATCH 3/3] Open the dev map in the editor analysis for dev files only Codex review: the analysis opened the dev map for every file of the package, so completion in a regular module suggested dev modules that the compiler now rejects there. The package keeps the dev map's open apart, and get_package adds it only for a file in a dev source directory, as the compiler does. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gHAzsdStoCEpBgx2tS Signed-off-by: Christoph Knittel --- analysis/src/packages.ml | 52 +++++++++++++++---- analysis/src/shared_types.ml | 5 ++ .../src/MyRegularModule.res | 3 ++ .../src/expected/MyRegularModule.res.txt | 9 ++++ 4 files changed, 60 insertions(+), 9 deletions(-) create mode 100644 tests/analysis_tests/tests-namespaced-references/src/MyRegularModule.res create mode 100644 tests/analysis_tests/tests-namespaced-references/src/expected/MyRegularModule.res.txt diff --git a/analysis/src/packages.ml b/analysis/src/packages.ml index f14ffbd3097..ecfd8c25641 100644 --- a/analysis/src/packages.ml +++ b/analysis/src/packages.ml @@ -12,6 +12,11 @@ let make_paths_for_module ~project_files_and_paths ~dependencies_files_and_paths Hashtbl.replace paths_for_module mod_name paths); paths_for_module +(* Without "." segments, so that "./test" and "test" compare equal. *) +let path_components path = + Files.split Filename.dir_sep path + |> List.filter (fun component -> component <> "" && component <> ".") + let new_bs_package ~root_path = let rescript_json = Filename.concat root_path "rescript.json" in @@ -108,17 +113,31 @@ let new_bs_package ~root_path = let cmt = Filename.concat artifact_dir namespace ^ ".cmt" in Hashtbl.replace paths_for_module namespace (Namespace {cmt}); let path = [Find_files.name_space_to_name namespace] in - (* The package's dev modules are aliased by its dev map, which - the compiler opens in dev modules only. *) + [path] + in + (* The package's dev modules are aliased by its dev map, which the + compiler opens in dev modules only (get_package). *) + let dev_opens = + match namespace with + | None -> [] + | Some namespace -> let dev_namespace = namespace ^ "-dev" in - let dev_cmt = - Filename.concat artifact_dir dev_namespace ^ ".cmt" - in - if Files.exists dev_cmt then ( + let cmt = Filename.concat artifact_dir dev_namespace ^ ".cmt" in + if Files.exists cmt then ( Hashtbl.replace paths_for_module dev_namespace - (Namespace {cmt = dev_cmt}); - [path; [dev_namespace]]) - else [path] + (Namespace {cmt}); + [[dev_namespace; "place holder"]]) + else [] + in + let dev_source_directories = + let directories ~include_dev = + Find_files.get_source_directories ~include_dev + ~base_dir:root_path config + |> List.map path_components + in + let regular = directories ~include_dev:false in + directories ~include_dev:true + |> List.filter (fun directory -> not (List.mem directory regular)) in let bind f x = Option.bind x f in let compiler_flags = @@ -170,6 +189,8 @@ let new_bs_package ~root_path = dependencies_files; paths_for_module; opens; + dev_opens; + dev_source_directories; namespace; autocomplete; })) @@ -195,7 +216,20 @@ let find_root ~uri packages_by_root = in loop (if Sys.is_directory path then path else Filename.dirname path) +(* A dev module also opens its package's dev map. *) +let for_file ~uri (package : Shared_types.package) = + if + package.dev_opens <> [] + && List.mem + (Files.relpath package.root_path (Filename.dirname (Uri.to_path uri)) + |> path_components) + package.dev_source_directories + then {package with opens = package.opens @ package.dev_opens} + else package + let get_package ~state ~uri = + Option.map (for_file ~uri) + @@ let open Shared_types in if Hashtbl.mem state.root_for_uri uri then Some diff --git a/analysis/src/shared_types.ml b/analysis/src/shared_types.ml index f376f000d1a..62632b9a473 100644 --- a/analysis/src/shared_types.ml +++ b/analysis/src/shared_types.ml @@ -503,6 +503,11 @@ and package = { paths_for_module: (file, paths) Hashtbl.t; namespace: string option; opens: path list; + dev_opens: path list; + (** Opened in the files of [dev_source_directories] only, as the compiler + opens a package's dev map in its dev modules only *) + dev_source_directories: string list list; + (** Relative to [root_path], as path components *) autocomplete: file list Misc.String_map.t; } diff --git a/tests/analysis_tests/tests-namespaced-references/src/MyRegularModule.res b/tests/analysis_tests/tests-namespaced-references/src/MyRegularModule.res new file mode 100644 index 00000000000..a66ac7dac11 --- /dev/null +++ b/tests/analysis_tests/tests-namespaced-references/src/MyRegularModule.res @@ -0,0 +1,3 @@ +// A regular module does not see the dev modules. +// MyDevHelper.d +// ^com diff --git a/tests/analysis_tests/tests-namespaced-references/src/expected/MyRegularModule.res.txt b/tests/analysis_tests/tests-namespaced-references/src/expected/MyRegularModule.res.txt new file mode 100644 index 00000000000..aecfb659fc2 --- /dev/null +++ b/tests/analysis_tests/tests-namespaced-references/src/expected/MyRegularModule.res.txt @@ -0,0 +1,9 @@ +Complete src/MyRegularModule.res 1:16 +pos_cursor:[1:16] pos_no_white:[1:15] Found expr:[1:3->1:16] +Pexp_ident MyDevHelper.d:[1:3->1:16] +Completable: Cpath Value[MyDevHelper, d] +Package opens Stdlib.place holder Pervasives.JsxModules.place holder MyNamespace.place holder +Resolved opens 2 MyNamespace Stdlib +ContextPath Value[MyDevHelper, d] +Path MyDevHelper.d +[]