From db08650e562b48cccd44f8a0137715d998160215 Mon Sep 17 00:00:00 2001 From: Ali Caglayan Date: Sat, 22 Apr 2023 00:35:36 +0200 Subject: [PATCH] feature: warn if modules is missing any mentioned modules We warn the user if modules_without_implementation, private_modules or virtual_modules contains any modules not in the modules field. Fixes #7026 This will be made into an error in 3.9. Signed-off-by: Ali Caglayan --- CHANGES.md | 5 ++ src/dune_rules/ml_sources.ml | 22 +++++--- src/dune_rules/modules_field_evaluator.ml | 49 +++++++++++++---- src/dune_rules/modules_field_evaluator.mli | 1 + .../intf-only/excluded-by-modules-field.t | 39 +++++++++++--- ...private-module-excluded-by-modules-field.t | 36 ++++++++++--- ...irtual-modules-excluded-by-modules-field.t | 54 ++++++++++++++----- 7 files changed, 164 insertions(+), 42 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e46ec7cc98e3..01cfa4c2e00e 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -20,6 +20,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 2cbbbb33873a..69b49a2ff742 100644 --- a/src/dune_rules/ml_sources.ml +++ b/src/dune_rules/ml_sources.ml @@ -287,6 +287,7 @@ let virtual_modules ~lookup_vlib vlib = } let make_lib_modules ~dir ~libs ~lookup_vlib ~(lib : Library.t) ~modules + ~is_vendored ~include_subdirs: (loc_include_subdirs, (include_subdirs : Dune_file.Include_subdirs.t)) = let open Resolve.Memo.O in @@ -338,7 +339,7 @@ let make_lib_modules ~dir ~libs ~lookup_vlib ~(lib : Library.t) ~modules let { Buildable.loc = stanza_loc; modules = modules_settings; _ } = lib.buildable in - Modules_field_evaluator.eval ~modules ~stanza_loc ~kind + Modules_field_evaluator.eval ~modules ~stanza_loc ~kind ~is_vendored ~private_modules: (Option.value ~default:Ordered_set_lang.standard lib.private_modules) ~src_dir:dir modules_settings @@ -371,7 +372,7 @@ let make_lib_modules ~dir ~libs ~lookup_vlib ~(lib : Library.t) ~modules ~modules ~main_module_name ~wrapped ) let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules - ~include_subdirs = + ~include_subdirs ~is_vendored = let rev_filter_partition = let rec loop l (acc : Modules.groups) = match l with @@ -406,7 +407,7 @@ let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules let+ sources, modules = let lookup_vlib = lookup_vlib ~loc:lib.buildable.loc in make_lib_modules ~dir ~libs:(Scope.libs scope) ~lookup_vlib ~modules - ~lib ~include_subdirs + ~lib ~include_subdirs ~is_vendored >>= Resolve.read_memo in let obj_dir = Library.obj_dir lib ~dir in @@ -417,10 +418,9 @@ let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules let { Buildable.loc = stanza_loc; modules = modules_settings; _ } = exes.buildable in - Modules_field_evaluator.eval ~modules ~stanza_loc - ~kind:Modules_field_evaluator.Exe_or_normal_lib - ~private_modules:Ordered_set_lang.standard ~src_dir:dir - modules_settings + Modules_field_evaluator.eval ~modules ~stanza_loc ~src_dir:dir + ~kind:Modules_field_evaluator.Exe_or_normal_lib ~is_vendored + ~private_modules:Ordered_set_lang.standard modules_settings in let modules = let project = Scope.project scope in @@ -434,7 +434,7 @@ let modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules let obj_dir = Obj_dir.make_melange_emit ~dir ~name:mel.target in let sources, modules = Modules_field_evaluator.eval ~modules ~stanza_loc:mel.loc - ~kind:Modules_field_evaluator.Exe_or_normal_lib + ~kind:Modules_field_evaluator.Exe_or_normal_lib ~is_vendored ~private_modules:Ordered_set_lang.standard ~src_dir:dir mel.modules in let modules = @@ -449,6 +449,11 @@ let make dune_file ~dir ~scope ~lib_config ~loc ~lookup_vlib ~include_subdirs: (loc_include_subdirs, (include_subdirs : Dune_file.Include_subdirs.t)) ~dirs = + let* is_vendored = + match Path.Build.drop_build_context dir with + | Some src_dir -> Source_tree.is_vendored src_dir + | None -> Memo.return false + in let+ modules_of_stanzas = let modules = let dialects = Dune_project.dialects (Scope.project scope) in @@ -503,6 +508,7 @@ let make dune_file ~dir ~scope ~lib_config ~loc ~lookup_vlib in modules_of_stanzas dune_file ~dir ~scope ~lookup_vlib ~modules ~include_subdirs:(loc_include_subdirs, include_subdirs) + ~is_vendored in let modules = Modules.make modules_of_stanzas in let artifacts = diff --git a/src/dune_rules/modules_field_evaluator.ml b/src/dune_rules/modules_field_evaluator.ml index 4c4209f94812..0beab2078d74 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,21 @@ 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) incldues 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 is_vendored then () + else + 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 +292,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 +328,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 @@ -326,7 +355,7 @@ let eval ~modules:(all_modules : Module.Source.t Module_trie.t) ~stanza_loc Module_trie.set all_modules path module_ 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 (settings : Stanza_common.Modules_settings.t) = let eval0 = eval0 @@ -335,6 +364,6 @@ let eval ~modules:(all_modules : Module.Source.t Module_trie.t) ~stanza_loc 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 7165f2a98ba9..ae0a78477efe 100644 --- a/src/dune_rules/modules_field_evaluator.mli +++ b/src/dune_rules/modules_field_evaluator.mli @@ -25,5 +25,6 @@ val eval : -> private_modules:Ordered_set_lang.t -> kind:kind -> src_dir:Path.Build.t + -> is_vendored:bool -> Stanza_common.Modules_settings.t -> (Loc.t * Module.Source.t) Module_trie.t * Module.t Module_trie.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 index d0c117e0365c..90baabf89b82 100644 --- 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 @@ -1,10 +1,12 @@ Specifying a module without implementation 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) @@ -12,15 +14,40 @@ field > (modules y)) > EOF - $ touch x.mli + $ touch src/x.mli - $ cat >y.ml < src/y.ml << EOF > module type F = X > EOF +X is warned about: + $ dune build --display short - ocamlc .foo.objs/byte/y.{cmi,cmo,cmt} (exit 2) - File "y.ml", line 1, characters 16-17: + 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 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 3dc7a533e181..de1433fd2187 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 c48f2b5926d1..e13f1da399fa 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]