Dependency error with "dune -p" and "infer true" for menhir

[grayswandyr]

Hello,
I have an issue that I think comes from Dune (well at least an error message in its source code). The code in question is at: https://github.com/grayswandyr/electrod/tree/release/0.2.0
It happens on 4.07.1 while it compiles well on 4.06.1 for instance, I had never encountered this essage with Jbuilder/Dune on previous major releases of OCaml (I haven't tested 4.07.0). The error message says:

Module Parser in directory _build/default/src depends on Libelectrod.
This doesn't make sense to me.

Libelectrod is the main module of the library and is the only module exposed 
outside of the library. Consequently, it should be the one depending 
on all the other modules in the library.

Alas, this message doesn't really make sens to me either ;-)
Any help appreciated!

[grayswandyr]

OK so I was wrong, it doesn't come from 4.07.1. Actually it happens when the infer option of menhir is true and dune is called with -p. If, for the code linked above, the dune file contains (infer false) for the first and third Menhir rules, then the build succeeds; otherwise it fails with -p and succeeds without it.
(I renamed this issue.)

[ghost]

I can't reproduce this issue. I'm using dune 1.4.0 and menhir 20181026. The following works for me:

$ git rev-parse HEAD
4f140068122bf00923161145f65944f500be1dc0
$ dune build -p electrod

[grayswandyr]

Ouch. This is what I get:

$ dune --version
1.4.0
$ make clean 
$ opam switch
#  switch   compiler                    description
   4.06.1   ocaml-base-compiler.4.06.1  4.06.1
→  4.07.1   ocaml-base-compiler.4.07.1  4.07.1
   default  ocaml-system.4.05.0         default
$ git rev-parse HEAD
4f140068122bf00923161145f65944f500be1dc0
$ make clean
$ opam update

<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><>
[default] no changes from https://opam.ocaml.org
$ opam upgrade -y
Already up-to-date.
Nothing to do.
$ dune build -p electrod 
Module Parser in directory _build/default/src depends on Libelectrod.
This doesn't make sense to me.

Libelectrod is the main module of the library and is the only module exposed 
outside of the library. Consequently, it should be the one depending 
on all the other modules in the library.
$ vi src/dune // adding infer false
$ git diff
diff --git a/src/dune b/src/dune
index b3a4d85..428d743 100644
--- a/src/dune
+++ b/src/dune
@@ -48,7 +48,7 @@
 
 ;;; Electrod parser
 
-(menhir (modules Parser))
+(menhir (infer false) (modules Parser))
 
 ;;; SMV trace parser
 
@@ -57,6 +57,7 @@
   (modules Smv_trace_tokens))
 
 (menhir 
+ (infer false)
  (merge_into Smv_trace_parser)
   (flags (--external-tokens Smv_trace_tokens))
   (modules Smv_trace_parser Smv_trace_tokens))
$ make clean
$ dune build -p electrod // OK
$

[ghost]

I can reproduce this with 4.06.1 but not with 4.07.1, so this seems indeed linked to the version of the compiler. Looking at the diff between Parser.ml generated with 4.06.1 and 4.07.1, the former produces references to Libelectrod__Gen_goal.block while the latter produces references to Libelectrod.Gen_goal.block. Ideally, it should produce a reference to Gen_goal.block. Not sure how to fix this issue :/

[grayswandyr]

So I guess it must be types inferred by Menhir piggybacking on the compiler, right? We should perhaps Cc François Pottier?
Meanwhile, I can always add infer false to create my OPAM packages to have them pass linting. IIUC, inference is only useful at development time anyway and only serves to provide the developer with better error messages. As of now, my parser works correctly so that's OK for the time being.

[ghost]

Basically menhir reads the output of ocamlc -i and in this case the output of ocamlc -i contains references to Libelectrod.Gen_goal and these are not allowed. What I don't understand is why the path is not shortened to Gen_goal by ocamlc -i because of -short-paths. This looks like a bug in the compiler.

[grayswandyr]

At any rate, I can't reproduce the part where I said that the bug only happens with the -p option.
OTOH I confirm that it works for me with 4.06.1 and not 4.07.1, so oddly enough the converse of your own testing.

[ghost]

Actually, I observe the same as you:

• the build succeeds with 4.06.1
• the build fails with 4.07.1

[ghost]

BTW, a workaround is to write the following at the beginning of Parser.mly:

module Libelectrod = struct end

[grayswandyr]

Good idea! However the problem is still here with the Smv_trace_parser and this one creates a functorized parser for with the workaround doesn't seem to work out of the box. I don't have time to investigate, so meanwhile I'll stick to my infer false solution. Thanks for your help.
Now I guess other people are likely to encounter the same problem but I don't know the compiler enough to look into this.

[ifazk]

I've run into the same issue, but with 4.07.0+flambda and dune 1.5.1. Running dune rules shows some weird dependencies for building the inferred mli (my module's name is Parse instead of Libelectrod).

((deps
  ((paths
    ((External /home/ikabir/.opam/4.07.0+flambda/bin/ocamlc.opt)
     ...
     (In_build_dir default/src/parse/.parse.objs/parse.cmi)
     (In_build_dir default/src/parse/.parse.objs/parse__Position.cmi)
     (In_build_dir default/src/parse/.parse.objs/parse__Types.cmi)
     (In_build_dir default/src/parse/parser__mock.ml.pp.mock)))
   (vars ())))
 (targets ((In_build_dir default/src/parse/parser__mock.mli.inferred)))
 (context default)
 (action
  (chdir
   _build/default
   (with-stdout-to
    src/parse/parser__mock.mli.inferred
    (chdir
     .
     (run
      /home/ikabir/.opam/4.07.0+flambda/bin/ocamlc.opt
      -w
      @a-4-29-40-41-42-44-45-48-58-59-60-40
      -strict-sequence
      -strict-formats
      -short-paths
      -keep-locs
      -g
      -I
      src/parse/.parse.objs
      -I
      ...
      -open
      Parse
      -i
      -impl
      src/parse/parser__mock.ml.pp.mock))))))

The two parts that seem incorrect to me are are:

• (In_build_dir default/src/parse/.parse.objs/parse.cmi) in the dependencies
• -open Parse in the action

[ghost]

This needs to be reported on the OCaml bug tracker, as this seems to be a bug in -short-paths

[ifazk]

@diml, are you going to report it? If not, can you give us a bit more information about how dune uses -short-paths and what is going wrong so that we can report it?

[ghost]

@ifazk if you could report it that would be cool. The problem is the following: in parser.mly, the code references a module X. X is an alias defined in lib__.ml as follow:

module X = Lib__X

and the Lib__ module is implicitly opened when compiling any file of the library. Menhir produces a .ml file from parser.mly that contains the user code mentioning X, for instance it might be referencing a type X.t. Menhir calls ocamlc -i -open Lib__ ... on this file and reads the output. It is expected that the output of ocamlc -i still references X.t. This is the case with OCaml < 4.07. However, with 4.07, it references Lib.X instead. This is an issue as such references to the Lib module are not allowed from inside the library.

[ifazk]

Sorry for the delayed follow up, but I have been trying to create a minimal example that show the differences between < 4.07 and >= 4.07. Here's what i have:

(* lib__m2.mli *)
type t

(* lib__m2.ml *)
type t = int

(* lib__.ml *)
module M2 = Lib__m2

(* parser.ml *)
type t = M2.t

Now when i run the following for both 4.06.1 and 4.07.0,

ocamlc -c lib__m2.mli lib__m2.ml lib__.ml
ocamlc -i -open Lib__ parser.ml

I get same the output

type t = Lib__.M2.t

Should I be doing something else for Lib__ to not be displayed in 4.06?

[ghost]

can you try with -short-paths?

[ifazk]

Some updates:

• I have been running into the issue for both 4.06.1 and 4.07.1 when using dune 1.6.2 and menhir 20181113.
• Builds work under --profile dev but not under --profile release. See https://github.com/ifazk/itis/tree/dune for an example project where dune build --profile dev @install works, but dune build --profile release @install does not (for 4.06.0, 4.06.1, 4.07.0, and 4.07.1 with latest packages).
• I wasn't able to trigger the -short-paths bug manually. See below for details.

Setup (same as before):

(* lib__m2.mli *)
type t

(* lib__m2.ml *)
type t = int

(* lib__.ml *)
module M2 = Lib__m2

(* parser.ml *)
type t = M2.t

I get the same results for both 4.06.1 and 4.07.1:

ocamlc -c lib__m2.mli lib__m2.ml lib__.ml
ocamlc -i -open Lib__ parser.ml

type t = Lib__.M2.t

ocamlc -c lib__m2.mli lib__m2.ml lib__.ml
ocamlc -i -short-paths -open Lib__ parser.ml

type t = M2.t

ocamlc -c -short-paths lib__m2.mli lib__m2.ml lib__.ml
ocamlc -i -short-paths -open Lib__ parser.ml

type t = M2.t

[ghost]

I've lost track of this conversation, the problem only appears when the infer mode is selected, right? Maybe we should just disable infer by default. /cc @fpottier

[grayswandyr]

Alas, the effect of this option is very useful and I seem to remember it was strongly encouraged to be set to true in the Menhir manual. Now Menhir was recently updated on this front with other related options so I guess indeed @fpottier could be of some help here.

[ifazk]

For me, the problem only appears when I try to build with --profile release, so I'm not convinced that this is necessarily a infer mode problem. Building with --profile dev works under current defaults.

[ghost]

@ifazk, the conclusion I draw from your results is that the issue is related to -short-paths. Note that -short-paths is used by default with --profile dev but not with --profile release.

[ghost]

Maybe we could always pass -short-paths when calling ocamlc -i. Could you try with the following patch:

diff --git a/src/module_compilation.ml b/src/module_compilation.ml
index 40903b72..fd6a449d 100644
--- a/src/module_compilation.ml
+++ b/src/module_compilation.ml
@@ -214,6 +214,7 @@ let ocamlc_i ?sandbox ?(flags=[]) ~dep_graphs cctx (m : Module.t) ~output =
           | Some (m : Module.t) ->
             As ["-open"; Module.Name.to_string (Module.name m)])
        ; As flags
+       ; A "-short-paths"
        ; A "-i"; Ml_kind.flag Impl; Dep src
        ]
      >>^ (fun act -> Action.with_stdout_to output act)

?

[ifazk]

The patch worked for my project!

[fpottier]

@diml's last suggestion sounds reasonable to me (although I don't really know what -short-paths is supposed to do). Ideally, infer mode should remain enabled by default.

[ghost]

@fpottier, when several names are available for a given type, -short-paths causes the compiler to choose the nicest one, which is often the shortest one. So for instance if the compiler has the choice between Lib__.M2.t and M2.t, whith -short-paths it will print M2.t while without -short-paths the choice depend on internal implementation details and is likely to not be the one the user would like to see. -short-paths should really be the default, though it used to be slow and in some cases it was making unexpected choices.

I'll create a PR with the suggested patch.