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 ocaml#7026 This will be made into an error in 3.9. Signed-off-by: Ali Caglayan <[email protected]>
Alizter · Apr 23, 2023 · 12bc931 · 12bc931
1 parent 165a6b5
commit 12bc931
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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,18 +162,24 @@ 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
     let line_list modules =
       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
@@ -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
@@ -19,6 +19,12 @@ field
   > EOF
 
   $ dune build --display short
+  File "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 .foo.objs/byte/y.{cmi,cmo,cmt} (exit 2)
   File "y.ml", line 1, characters 16-17:
   1 | module type F = 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
@@ -24,6 +24,12 @@ modules:
 X is silently ignored:
 
   $ dune build
+  File "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 "y.ml", line 1, characters 9-14:
   1 | let () = X.foo ()
                ^^^^^

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
@@ -26,6 +26,12 @@ Specifying a virtual module that isn't inside the (modules ..) field:
   $ touch impl/x.ml
 
   $ dune build --display short
+  File "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 impl/.impl.objs/x.impl.d
         ocamlc .foo.objs/byte/y.{cmi,cmo,cmt} (exit 2)
   File "y.ml", line 1, characters 16-17: