feature: warn if modules is missing any mentioned modules

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

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
 

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,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)

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

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]

test/blackbox-tests/test-cases/private-modules/private-module-excluded-by-modules-field.t

@@ -1,30 +1,54 @@
 Demonstrate the behavior when a module is listed by private_modules by not by
 modules:
 
-  $ cat >dune-project <<EOF
+  $ cat > dune-project << EOF
   > (lang dune 3.7)
   > EOF
 
-  $ cat >dune <<EOF
+  $ mkdir src
+  $ cat > src/dune << EOF
   > (library
   >  (name foo)
   >  (wrapped false)
   >  (modules y)
   >  (private_modules x))
   > EOF
 
-  $ cat >x.ml <<EOF
+  $ cat > src/x.ml << EOF
   > let foo = ()
   > EOF
 
-  $ cat >y.ml <<EOF
+  $ cat > 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