fix(ocaml-index): modules of overlapping exes

[Alizter]

Rather than make a rule that handles both libraries and executables, we make a rule that handles a collection of modules so that libraries, executables and melange can setup the ocaml index rules accordingly.

In the executable case we are more careful with the modules we choose and instead opt for reachable modules.

• fixes @ocaml-index fails when multiple executables in a directory don't specify (modules) #13566
• changelog

[voodoos]

It's been a while since I had all the context for this, but the change looks reasonable.

I think printing the actual indexing rules in more tests would also us to check more easily that it is. It would also help prevent future regressions.

Now, I am wondering whether we should actually act similarly for libraries (and thus make the api safer), my guess is that we also disrespect actual dependencies there ? But maybe that's less of an issue than for executables ?

[voodoos]

These tests are not really telling of the actual success of the index building process.

I suggest adding to this test, and the other indexing tests, a print of the rules of the alias @ocaml-index.
This will allow us to more easily asses the impact of this fix.

As an example, I modified the test to have the Foo executable actually use the Shared module, and printed the rules.

   $ cat > foo.ml <<EOF
-  > let () = print_endline Lib_a.value_a
+  > let () = print_endline Shared.x
   > EOF

Here is the rules diff from this patch:

   $ dune rules @ocaml-index
   ((deps
     ((File
       (External
        $TESTCASE_ROOT/bin/ocaml-index))
      (File (In_build_dir _build/default/.bar.eobjs/byte/dune__exe__Bar.cmt))
      (File (In_build_dir _build/default/.bar.eobjs/byte/dune__exe__Bar.cmti))
-     (File (In_build_dir _build/default/.bar.eobjs/byte/dune__exe__Foo.cmt))
-     (File (In_build_dir _build/default/.bar.eobjs/byte/dune__exe__Shared.cmt))
      (File (In_build_dir _build/default/lib_b/.lib_b.objs/cctx.ocaml-index))))
    (targets
     ((files (_build/default/.bar.eobjs/cctx.ocaml-index)) (directories ())))
    (context default)
    (action
     (chdir
      _build/default
      (run
       $TESTCASE_ROOT/bin/ocaml-index
       aggregate
       -o
       .bar.eobjs/cctx.ocaml-index
-      .bar.eobjs/byte/dune__exe__Shared.cmt
-      .bar.eobjs/byte/dune__exe__Foo.cmt
-      .bar.eobjs/byte/dune__exe__Bar.cmt
-      .bar.eobjs/byte/dune__exe__Bar.cmti))))
+      .bar.eobjs/byte/dune__exe__Bar.cmti
+      .bar.eobjs/byte/dune__exe__Bar.cmt))))
   
   ((deps
     ((File
       (External
        $TESTCASE_ROOT/bin/ocaml-index))
-     (File (In_build_dir _build/default/.foo.eobjs/byte/dune__exe__Bar.cmt))
      (File (In_build_dir _build/default/.foo.eobjs/byte/dune__exe__Foo.cmt))
      (File (In_build_dir _build/default/.foo.eobjs/byte/dune__exe__Foo.cmti))
      (File (In_build_dir _build/default/.foo.eobjs/byte/dune__exe__Shared.cmt))
      (File (In_build_dir _build/default/lib_a/.lib_a.objs/cctx.ocaml-index))))
    (targets
     ((files (_build/default/.foo.eobjs/cctx.ocaml-index)) (directories ())))
    (context default)
    (action
     (chdir
      _build/default
      (run
       $TESTCASE_ROOT/bin/ocaml-index
       aggregate
       -o
       .foo.eobjs/cctx.ocaml-index
       .foo.eobjs/byte/dune__exe__Shared.cmt
-      .foo.eobjs/byte/dune__exe__Foo.cmt
       .foo.eobjs/byte/dune__exe__Foo.cmti
-      .foo.eobjs/byte/dune__exe__Bar.cmt))))
+      .foo.eobjs/byte/dune__exe__Foo.cmt))))

It does fit better the actual module dependencies.

[voodoos]

and aggregates them in a cctx.ocaml-index file covering the whole compilation context

That's not true anymore, depending on the value of modules_to_index we might be missing things.

[rgrinberg]

Tbh, I don't really get what this is fixing. In particular:

In the executable case we are more careful with the modules we choose and instead opt for reachable modules.

Why would we limit the index to reachable modules? In the original bug report, the issue was separate executable stanzas. While you're changing the behavior for multiple executable in the same stanza.

[rgrinberg]

Why do you need this check if you're only passing user written modules?

[voodoos]

I think it's because of the now separate treatment for executable which do not use fold_user_written.

It bothers me that there are such different treatments for executables and libraries though, the propose change make the API of the ocaml_index module much less safe to use.

[voodoos]

Why would we limit the index to reachable modules? In the original bug report, the issue was separate executable stanzas. While you're changing the behavior for multiple executable in the same stanza.

I think it does fix the mentioned issue ? But I really need to see the impact on the dune rules printing in more tests.

I think the core issue is that ocaml_index rules assumed that all modules attached to a stanza are actually built as part of that stanza, and thus share the same set of library dependencies. But it's wrong in cases like #13566 because unreachable (and un-buildable) modules for one of the two executables are still part of the ones iterated over by fold_user_written even if they are not actually used to build the executable. When ocaml_index tries to build the index for this executable, it misses the required library dependency only declared for the other executable.

The index is still global, and not only restricted to reachable modules: we build one index for each stanza.

[rgrinberg]

I think the core issue is that ocaml_index rules assumed that all modules attached to a stanza are actually built as part of that stanza, and thus share the same set of library dependencies

This assumption should be correct.

I think the core issue is that ocaml_index rules assumed that all modules attached to a stanza are actually built as part of that stanza, and thus share the same set of library dependencies. But it's wrong in cases like #13566 because unreachable (and un-buildable) modules for one of the two executables are still part of the ones iterated over by fold_user_written even if they are not actually used to build the executable

I think the issue is that dune is allowing a module to be a part of two stanzas. If these two stanzas share a module and yet they're trying to build it with different configurations, it's not surprising that ocaml-index is confused. I think merlin would be confused as well. Which libraries and preprocess modules would be applicable to this shared module?

In that example, the user should specify the modules field per stanza. This is what we expect for libraries already.

[rgrinberg]

Here is an example that demonstrates why this configuration is invalid:

  $ make_dune_project "3.22"

  $ cat >baz.ml <<EOF
  > let print () = print_endline "baz"
  > module M = Lwt
  > EOF

  $ cat >dune <<EOF
  > (executable (name foo) (libraries lwt))
  > (executable (name bar))
  > EOF

  $ cat >foo.ml <<EOF
  > Baz.print ();;
  > EOF

  $ cat >bar.ml <<EOF
  > Baz.print ();;
  > EOF

  $ dune exec ./foo.exe
  baz
  $ dune exec ./bar.exe
  File "baz.ml", line 2, characters 11-14:
  2 | module M = Lwt
                 ^^^
  Error (warning 49 [no-cmi-file]): no cmi file was found
    in path for module Lwt
  [1]

[Alizter]

@rgrinberg I've tried forbidding such overlapping stanzas in the past here #12451. Is that something we should look into again?

We have overlapping module checks for lib-lib, but not for lib-exec nor exec-exec. I can't tell if the latter cases are a feature or a bug.

We could introduce such checks in a newer dune lang and forbid merlin / ocaml-index in these cases for older dune lang versions. In the newer dune lang we would requires that people use the modules field.

Furthermore, we could introduce something smarter where we check the configuration is the same, but that is not clear how to do, especially in the lib-exec case. The semantics of enabled_if need to be thought through. In the exec-exec case we already have executables.

[rgrinberg]

I think we should we enforce that every single module belongs to a single enabled stanza. I don't think there's a need to be any smarter than that.

[Alizter]

@rgrinberg Is that including library / executable overlap?

[rgrinberg]

Yes