diff --git a/CHANGES.md b/CHANGES.md index cfc4064dce6..0c236840d01 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -26,6 +26,11 @@ Unreleased - Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary (#7469, fixes #2572, @anmonteiro) +- Modules that were declared in `(modules_without_implementation)`, + `(private_modules)` or `(virtual_modules)` but not declared in `(modules)` + will cause Dune to emit a warning which will become an error in 3.9. (#7608, + fixes #7026, @Alizter) + - Preliminary support for Coq compiled intefaces (`.vos` files) enabled via `(mode vos)` in `coq.theory` stanzas. This can be used in combination with `dune coq top` to obtain fast re-building of dependencies (with no checking diff --git a/src/dune_rules/ml_sources.ml b/src/dune_rules/ml_sources.ml index 9fd903246ac..ccf1366e5b0 100644 --- a/src/dune_rules/ml_sources.ml +++ b/src/dune_rules/ml_sources.ml @@ -290,7 +290,7 @@ let make_lib_modules ~dir ~libs ~lookup_vlib ~(lib : Library.t) ~modules ~include_subdirs: (loc_include_subdirs, (include_subdirs : Dune_file.Include_subdirs.t)) = let open Resolve.Memo.O in - let+ kind, main_module_name, wrapped = + let* kind, main_module_name, wrapped = match lib.implements with | None -> (* In the two following pattern matching, we can only get [From _] if @@ -334,7 +334,8 @@ let make_lib_modules ~dir ~libs ~lookup_vlib ~(lib : Library.t) ~modules let kind : Modules_field_evaluator.kind = Implementation impl in (kind, main_module_name, wrapped) in - let sources, modules = + let open Memo.O in + let* sources, modules = let { Buildable.loc = stanza_loc; modules = modules_settings; _ } = lib.buildable in @@ -366,9 +367,10 @@ let make_lib_modules ~dir ~libs ~lookup_vlib ~(lib : Library.t) ~modules in let implements = Option.is_some lib.implements in let _loc, lib_name = lib.name in - ( sources - , Modules_group.lib ~stdlib:lib.stdlib ~implements ~lib_name ~obj_dir:dir - ~modules ~main_module_name ~wrapped ) + Resolve.Memo.return + ( sources + , Modules_group.lib ~stdlib:lib.stdlib ~implements ~lib_name ~obj_dir:dir + ~modules ~main_module_name ~wrapped ) let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules ~include_subdirs = @@ -413,14 +415,13 @@ let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules `Library (lib, sources, modules, obj_dir) | Executables exes | Tests { exes; _ } -> let obj_dir = Dune_file.Executables.obj_dir ~dir exes in - let sources, modules = + let+ sources, modules = let { Buildable.loc = stanza_loc; modules = modules_settings; _ } = exes.buildable in - Modules_field_evaluator.eval ~modules ~stanza_loc + Modules_field_evaluator.eval ~modules ~stanza_loc ~src_dir:dir ~kind:Modules_field_evaluator.Exe_or_normal_lib - ~private_modules:Ordered_set_lang.standard ~src_dir:dir - modules_settings + ~private_modules:Ordered_set_lang.standard modules_settings in let modules = let project = Scope.project scope in @@ -429,10 +430,10 @@ let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules Modules_group.make_wrapped ~obj_dir ~modules `Exe else Modules_group.exe_unwrapped modules ~obj_dir in - Memo.return (`Executables (exes, sources, modules, obj_dir)) + `Executables (exes, sources, modules, obj_dir) | Melange_stanzas.Emit.T mel -> let obj_dir = Obj_dir.make_melange_emit ~dir ~name:mel.target in - let sources, modules = + let+ sources, modules = Modules_field_evaluator.eval ~modules ~stanza_loc:mel.loc ~kind:Modules_field_evaluator.Exe_or_normal_lib ~private_modules:Ordered_set_lang.standard ~src_dir:dir mel.modules @@ -441,7 +442,7 @@ let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules Modules_group.make_wrapped ~obj_dir:(Obj_dir.obj_dir obj_dir) ~modules `Melange in - Memo.return (`Melange_emit (mel, sources, modules, obj_dir)) + `Melange_emit (mel, sources, modules, obj_dir) | _ -> Memo.return `Skip) >>| filter_partition_map diff --git a/src/dune_rules/modules_field_evaluator.ml b/src/dune_rules/modules_field_evaluator.ml index 4c4209f9481..1beb2a7ccd6 100644 --- a/src/dune_rules/modules_field_evaluator.ml +++ b/src/dune_rules/modules_field_evaluator.ml @@ -60,6 +60,9 @@ type single_module_error = | Vmodule_impl_missing_impl | Forbidden_new_public_module | Vmodule_impls_with_own_intf + | Undeclared_module_without_implementation + | Undeclared_private_module + | Undeclared_virtual_module type errors = { errors : (single_module_error * Loc.t * Module_name.Path.t) list @@ -97,14 +100,19 @@ let find_errors ~modules ~intf_only ~virtual_modules ~private_modules in let ( ++ ) f g loc acc = f loc (g loc acc) in let ( !? ) = Option.is_some in - with_property private_ (add_if impl_vmodule Private_impl_of_vmodule) + with_property private_ + (add_if impl_vmodule Private_impl_of_vmodule + ++ add_if (not !?modules) Undeclared_private_module) @@ with_property intf_only (add_if has_impl Spurious_module_intf - ++ add_if impl_vmodule Vmodule_impl_intf_only_exclusion) + ++ add_if impl_vmodule Vmodule_impl_intf_only_exclusion + ++ add_if (not !?modules) Undeclared_module_without_implementation + ) @@ with_property virtual_ (add_if has_impl Spurious_module_virtual ++ add_if !?intf_only Virt_intf_overlap - ++ add_if !?private_ Private_virt_module) + ++ add_if !?private_ Private_virt_module + ++ add_if (not !?modules) Undeclared_virtual_module) @@ with_property modules (add_if ((not !?private_) @@ -128,7 +136,7 @@ let find_errors ~modules ~intf_only ~virtual_modules ~private_modules let check_invalid_module_listing ~stanza_loc ~modules_without_implementation ~intf_only ~modules ~virtual_modules ~private_modules - ~existing_virtual_modules ~allow_new_public_modules = + ~existing_virtual_modules ~allow_new_public_modules ~is_vendored = let { errors; unimplemented_virt_modules } = find_errors ~modules ~intf_only ~virtual_modules ~private_modules ~existing_virtual_modules ~allow_new_public_modules @@ -154,6 +162,11 @@ let check_invalid_module_listing ~stanza_loc ~modules_without_implementation let missing_intf_only = get Missing_intf_only in let spurious_modules_intf = get Spurious_module_intf in let spurious_modules_virtual = get Spurious_module_virtual in + let undeclared_modules_without_implementation = + get Undeclared_module_without_implementation + in + let undeclared_private_modules = get Undeclared_private_module in + let undeclared_virtual_modules = get Undeclared_virtual_module in let uncapitalized = List.map ~f:(fun (_, m) -> Module_name.Path.uncapitalize m) in @@ -161,11 +174,12 @@ let check_invalid_module_listing ~stanza_loc ~modules_without_implementation Pp.enumerate modules ~f:(fun (_, m) -> Pp.verbatim (Module_name.Path.to_string m)) in - let print before l after = + let print ?(is_error = true) before l after = match l with | [] -> () | (loc, _) :: _ -> - User_error.raise ~loc (List.concat [ before; [ line_list l ]; after ]) + User_warning.emit ~is_error ~loc + (List.concat [ before; [ line_list l ]; after ]) in print [ Pp.text "The following modules are implementations of virtual modules:" @@ -213,6 +227,20 @@ let check_invalid_module_listing ~stanza_loc ~modules_without_implementation (unimplemented_virt_modules |> Module_name.Path.Set.to_list |> List.map ~f:(fun name -> (stanza_loc, name))) [ Pp.text "You must provide an implementation for all of these modules." ]; + (* Checking that (modules) includes all declared modules *) + let print_undelared_modules field mods = + (* TODO: this is a warning for now, change to an error in 3.9. *) + (* If we are in a vendored stanza we do nothing. *) + if not is_vendored then + print ~is_error:false + [ Pp.textf "These modules appear in the %s field:" field ] + mods + [ Pp.text "They must also appear in the modules field." ] + in + print_undelared_modules "modules_without_implementation" + undeclared_modules_without_implementation; + print_undelared_modules "private_modules" undeclared_private_modules; + print_undelared_modules "virtual_modules" undeclared_virtual_modules; (if missing_intf_only <> [] then match Ordered_set_lang.loc modules_without_implementation with | None -> @@ -263,7 +291,7 @@ let eval0 ~modules:(all_modules : Module.Source.t Module_trie.t) ~stanza_loc { modules; fake_modules = !fake_modules } let eval ~modules:(all_modules : Module.Source.t Module_trie.t) ~stanza_loc - ~private_modules ~kind ~src_dir + ~private_modules ~kind ~src_dir ~is_vendored { Stanza_common.Modules_settings.modules = _ ; root_module ; modules_without_implementation @@ -299,7 +327,7 @@ let eval ~modules:(all_modules : Module.Source.t Module_trie.t) ~stanza_loc ]); check_invalid_module_listing ~stanza_loc ~modules_without_implementation ~intf_only ~modules ~virtual_modules ~private_modules - ~existing_virtual_modules ~allow_new_public_modules; + ~existing_virtual_modules ~allow_new_public_modules ~is_vendored; let all_modules = Module_trie.mapi modules ~f:(fun _path (_, m) -> let name = [ Module.Source.name m ] in @@ -333,8 +361,14 @@ let eval ~modules:(all_modules : Module.Source.t Module_trie.t) ~stanza_loc ~modules:(all_modules : Module.Source.t Module_trie.t) ~stanza_loc settings.modules in + let open Memo.O in + let+ is_vendored = + match Path.Build.drop_build_context src_dir with + | Some src_dir -> Source_tree.is_vendored src_dir + | None -> Memo.return false + in let modules = eval ~modules:all_modules ~stanza_loc ~private_modules ~kind ~src_dir - settings eval0 + ~is_vendored settings eval0 in (eval0.modules, modules) diff --git a/src/dune_rules/modules_field_evaluator.mli b/src/dune_rules/modules_field_evaluator.mli index 7165f2a98ba..4c0dd446150 100644 --- a/src/dune_rules/modules_field_evaluator.mli +++ b/src/dune_rules/modules_field_evaluator.mli @@ -26,4 +26,4 @@ val eval : -> kind:kind -> src_dir:Path.Build.t -> Stanza_common.Modules_settings.t - -> (Loc.t * Module.Source.t) Module_trie.t * Module.t Module_trie.t + -> ((Loc.t * Module.Source.t) Module_trie.t * Module.t Module_trie.t) Memo.t diff --git a/test/blackbox-tests/test-cases/intf-only/excluded-by-modules-field.t b/test/blackbox-tests/test-cases/intf-only/excluded-by-modules-field.t new file mode 100644 index 00000000000..90baabf89b8 --- /dev/null +++ b/test/blackbox-tests/test-cases/intf-only/excluded-by-modules-field.t @@ -0,0 +1,54 @@ +Specifying a module without implementation that isn't inside the (modules ..) +field + + $ cat > dune-project << EOF + > (lang dune 3.7) + > EOF + + $ mkdir src + $ cat > src/dune << EOF + > (library + > (name foo) + > (wrapped false) + > (modules_without_implementation x) + > (modules y)) + > EOF + + $ touch src/x.mli + + $ cat > src/y.ml << EOF + > module type F = X + > EOF + +X is warned about: + + $ dune build --display short + File "src/dune", line 4, characters 33-34: + 4 | (modules_without_implementation x) + ^ + Warning: These modules appear in the modules_without_implementation field: + - X + They must also appear in the modules field. + ocamlc src/.foo.objs/byte/y.{cmi,cmo,cmt} (exit 2) + File "src/y.ml", line 1, characters 16-17: + 1 | module type F = X + ^ + Error: Unbound module type X + [1] + +This should be ignored if we are in vendored_dirs + + $ cat > dune << EOF + > (vendored_dirs src) + > (executable + > (name bar) + > (libraries foo)) + > EOF + $ cat > bar.ml + + $ dune build ./bar.exe + File "src/y.ml", line 1, characters 16-17: + 1 | module type F = X + ^ + Error: Unbound module type X + [1] diff --git a/test/blackbox-tests/test-cases/intf-only/excludes-by-modules-field.t b/test/blackbox-tests/test-cases/intf-only/excludes-by-modules-field.t deleted file mode 100644 index d0c117e0365..00000000000 --- a/test/blackbox-tests/test-cases/intf-only/excludes-by-modules-field.t +++ /dev/null @@ -1,27 +0,0 @@ -Specifying a module without implementation that isn't inside the (modules ..) -field - - $ cat >dune-project < (lang dune 3.7) - > EOF - $ cat >dune < (library - > (name foo) - > (wrapped false) - > (modules_without_implementation x) - > (modules y)) - > EOF - - $ touch x.mli - - $ cat >y.ml < module type F = X - > EOF - - $ dune build --display short - ocamlc .foo.objs/byte/y.{cmi,cmo,cmt} (exit 2) - File "y.ml", line 1, characters 16-17: - 1 | module type F = X - ^ - Error: Unbound module type X - [1] diff --git a/test/blackbox-tests/test-cases/private-modules/private-module-excluded-by-modules-field.t b/test/blackbox-tests/test-cases/private-modules/private-module-excluded-by-modules-field.t index 3dc7a533e18..de1433fd218 100644 --- a/test/blackbox-tests/test-cases/private-modules/private-module-excluded-by-modules-field.t +++ b/test/blackbox-tests/test-cases/private-modules/private-module-excluded-by-modules-field.t @@ -1,11 +1,12 @@ Demonstrate the behavior when a module is listed by private_modules by not by modules: - $ cat >dune-project < dune-project << EOF > (lang dune 3.7) > EOF - $ cat >dune < src/dune << EOF > (library > (name foo) > (wrapped false) @@ -13,18 +14,41 @@ modules: > (private_modules x)) > EOF - $ cat >x.ml < src/x.ml << EOF > let foo = () > EOF - $ cat >y.ml < src/y.ml << EOF > let () = X.foo () > EOF -X is silently ignored: +X is warned about: $ dune build - File "y.ml", line 1, characters 9-14: + File "src/dune", line 5, characters 18-19: + 5 | (private_modules x)) + ^ + Warning: These modules appear in the private_modules field: + - X + They must also appear in the modules field. + File "src/y.ml", line 1, characters 9-14: + 1 | let () = X.foo () + ^^^^^ + Error: Unbound module X + [1] + +This warning should be ignored if we are in vendored_dirs + + $ cat > dune << EOF + > (vendored_dirs src) + > (executable + > (name bar) + > (libraries foo)) + > EOF + $ cat > bar.ml + + $ dune build ./bar.exe + File "src/y.ml", line 1, characters 9-14: 1 | let () = X.foo () ^^^^^ Error: Unbound module X diff --git a/test/blackbox-tests/test-cases/virtual-libraries/virtual-modules-excluded-by-modules-field.t b/test/blackbox-tests/test-cases/virtual-libraries/virtual-modules-excluded-by-modules-field.t index c48f2b5926d..e13f1da399f 100644 --- a/test/blackbox-tests/test-cases/virtual-libraries/virtual-modules-excluded-by-modules-field.t +++ b/test/blackbox-tests/test-cases/virtual-libraries/virtual-modules-excluded-by-modules-field.t @@ -1,9 +1,11 @@ Specifying a virtual module that isn't inside the (modules ..) field: - $ cat >dune-project < dune-project << EOF > (lang dune 3.7) > EOF - $ cat >dune < src/dune << EOF > (library > (name foo) > (wrapped false) @@ -11,30 +13,58 @@ Specifying a virtual module that isn't inside the (modules ..) field: > (modules y)) > EOF - $ touch x.mli + $ touch src/x.mli - $ cat >y.ml < src/y.ml << EOF > module type F = X > EOF - $ mkdir impl - $ cat >impl/dune < src/impl/dune << EOF > (library > (name impl) > (implements foo)) > EOF - $ touch impl/x.ml + $ touch src/impl/x.ml + +X is warned about: $ dune build --display short - ocamldep impl/.impl.objs/x.impl.d - ocamlc .foo.objs/byte/y.{cmi,cmo,cmt} (exit 2) - File "y.ml", line 1, characters 16-17: + File "src/dune", line 4, characters 18-19: + 4 | (virtual_modules x) + ^ + Warning: These modules appear in the virtual_modules field: + - X + They must also appear in the modules field. + ocamldep src/impl/.impl.objs/x.impl.d + ocamlc src/.foo.objs/byte/y.{cmi,cmo,cmt} (exit 2) + File "src/y.ml", line 1, characters 16-17: 1 | module type F = X ^ Error: Unbound module type X - File "impl/dune", line 1, characters 0-40: + File "src/impl/dune", line 1, characters 0-40: 1 | (library 2 | (name impl) 3 | (implements foo)) - Error: No rule found for .foo.objs/y.impl.all-deps + Error: No rule found for src/.foo.objs/y.impl.all-deps + [1] + + +This should be ignored if we are in vendored_dirs + + $ cat > dune << EOF + > (vendored_dirs src) + > (executable + > (name bar) + > (libraries foo)) + > EOF + $ cat > bar.ml + + $ dune build ./bar.exe + Error: No implementation found for virtual library "foo" in + _build/default/src. + -> required by executable bar in dune:3 + -> required by _build/default/.bar.eobjs/byte/dune__exe__Bar.cmi + -> required by _build/default/.bar.eobjs/native/dune__exe__Bar.cmx + -> required by _build/default/bar.exe [1]