From 30d209d451e00660493f150e5cac6fdff235282d Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Sun, 5 Jan 2025 21:57:13 -0800 Subject: [PATCH] fix(melange): disallow private implementations of public virtual libs (#11253) * test(melange): show wrong require for private impl of public virtual lib Signed-off-by: Antonio Nuno Monteiro * fix(melange): disallow private implementations of public virtual libs Signed-off-by: Antonio Nuno Monteiro * changelog Signed-off-by: Antonio Nuno Monteiro --------- Signed-off-by: Antonio Nuno Monteiro --- doc/changes/11253.md | 2 + src/dune_rules/melange/melange_rules.ml | 77 +++++++++++-------- ...ib-public.t => virtual-lib-private-impl.t} | 33 +++++++- 3 files changed, 80 insertions(+), 32 deletions(-) create mode 100644 doc/changes/11253.md rename test/blackbox-tests/test-cases/melange/{virtual-lib-public.t => virtual-lib-private-impl.t} (54%) diff --git a/doc/changes/11253.md b/doc/changes/11253.md new file mode 100644 index 00000000000..5c29c108aa1 --- /dev/null +++ b/doc/changes/11253.md @@ -0,0 +1,2 @@ +- Disallow private implementations of public virtual libs in melange mode. + (@amonteiro, #11253) diff --git a/src/dune_rules/melange/melange_rules.ml b/src/dune_rules/melange/melange_rules.ml index 2812ddad4f1..1f79cff25cb 100644 --- a/src/dune_rules/melange/melange_rules.ml +++ b/src/dune_rules/melange/melange_rules.ml @@ -576,38 +576,53 @@ let setup_js_rules_libraries = | None -> Memo.return () | Some vlib -> let* vlib = Resolve.Memo.read_memo vlib in - let* includes = - let+ requires_link = - let+ requires_link = - Lib.Compile.for_lib - ~allow_overlaps:mel.allow_overlapping_dependencies - (Scope.libs scope) - vlib - |> Lib.Compile.requires_link - |> Memo.Lazy.force - in - let open Resolve.O in - let+ requires_link = requires_link in - (* Whenever a `concrete_lib` implementation contains a field - `(implements virt_lib)`, we also set up the JS targets for the - modules defined in `virt_lib`. + let vlib_output = output_of_lib ~target_dir vlib in + (match vlib_output, output with + | `Public_library _, `Private_library_or_emit _ -> + let info = Lib.info lib in + User_error.raise + ~loc:(Lib_info.loc info) + [ Pp.text + "Dune doesn't currently support building private implementations of \ + virtual public libaries for `(modes melange)`" + ] + ~hints: + [ Pp.textf + "Add a `public_name` to the library `%s'." + (Lib_name.to_string (Lib_info.name info)) + ] + | `Public_library _, `Public_library _ | `Private_library_or_emit _, _ -> + let* includes = + let+ requires_link = + let+ requires_link = + Lib.Compile.for_lib + ~allow_overlaps:mel.allow_overlapping_dependencies + (Scope.libs scope) + vlib + |> Lib.Compile.requires_link + |> Memo.Lazy.force + in + let open Resolve.O in + let+ requires_link = requires_link in + (* Whenever a `concrete_lib` implementation contains a field + `(implements virt_lib)`, we also set up the JS targets for the + modules defined in `virt_lib`. - In the cases where `virt_lib` (concrete) modules depend on any - virtual modules (i.e. programming against the interface), we - need to make sure that the JS rules that dune emits for - `virt_lib` depend on `concrete_lib`, such that Melange can find - the correct `.cmj` file, which is needed to emit the correct - path in `import` / `require`. *) - lib :: requires_link - in - cmj_includes ~requires_link ~scope lib_config - in - let output = output_of_lib ~target_dir vlib in - parallel_build_source_modules - ~sctx - ~scope - vlib - ~f:(build_js ~dir ~output ~includes ~compile_flags) + In the cases where `virt_lib` (concrete) modules depend on any + virtual modules (i.e. programming against the interface), we + need to make sure that the JS rules that dune emits for + `virt_lib` depend on `concrete_lib`, such that Melange can find + the correct `.cmj` file, which is needed to emit the correct + path in `import` / `require`. *) + lib :: requires_link + in + cmj_includes ~requires_link ~scope lib_config + in + parallel_build_source_modules + ~sctx + ~scope + vlib + ~f:(build_js ~dir ~output:vlib_output ~includes ~compile_flags)) and+ () = parallel_build_source_modules ~sctx diff --git a/test/blackbox-tests/test-cases/melange/virtual-lib-public.t b/test/blackbox-tests/test-cases/melange/virtual-lib-private-impl.t similarity index 54% rename from test/blackbox-tests/test-cases/melange/virtual-lib-public.t rename to test/blackbox-tests/test-cases/melange/virtual-lib-private-impl.t index 2713a975124..d9b666c7478 100644 --- a/test/blackbox-tests/test-cases/melange/virtual-lib-public.t +++ b/test/blackbox-tests/test-cases/melange/virtual-lib-private-impl.t @@ -1,4 +1,5 @@ -Test a case of virtual libraries where the virtual library is public +Test virtual libraries where the virtual lib is public and the concrete impl is +private $ mkdir -p vlib js_impl test $ cat > dune-project < EOF $ dune build @melange + File "js_impl/dune", lines 1-5, characters 0-95: + 1 | (library + 2 | (name timeJs) + 3 | (implements the_lib) + 4 | (modes melange) + 5 | (preprocess (pps melange.ppx))) + Error: Dune doesn't currently support building private implementations of + virtual public libaries for `(modes melange)` + Hint: Add a `public_name` to the library `timeJs'. + [1] + +Making timeJs a public library makes it work + + $ cat > dune-project < (lang dune 3.13) + > (using melange 0.1) + > (package (name the_lib)) + > (package (name concrete_lib)) + > EOF + + $ cat > js_impl/dune < (library + > (name timeJs) + > (public_name concrete_lib) + > (implements the_lib) + > (modes melange) + > (preprocess (pps melange.ppx))) + > EOF + + $ dune build @melange