"inconsistent assumptions over interface" when compiling ReasonML code with an interface

[TheSpyder]

I'm attaching a simple replication case with 4 files cut down from my real project. It has implementations in both OCaml and Reason (I discovered this while converting my project from OCaml to Reason). The first file defines a type, the second uses that type to define a method and has an interface file, the third file calls that method.

When compiled as OCaml it's fine (jbuilder build ocaml/Test3.exe) but when compiling as Reason (jbuilder build reason/Test3.exe) it gives me an error about inconsistent assumptions:

File "reason/test3.re", line 1:
Error: The files reason/test2.cmi and reason/test3.cmi
       make inconsistent assumptions over interface Test2

inconsistent.zip

To work around the error, touch reason/test3.re and rebuild (perhaps a race condition?). I also tracked down that if the reason/test2.rei file is deleted a fresh build succeeds.

I'm using jbuilder 1.0+beta10 and reason 1.13.6, with compiler 4.02.3+buckle-master although I've also verified it on reason 2.0.0 and compiler 4.04.2.

[cristianoc]

As far as I can see, interface files .rei are currently ignored.
At least that seems to be the case on the first hello world example I have tried.

[jordwalke]

I wonder if jBuilder forgot to add the -intf-suffix flag for reason implementation files. This is the symptom you would see.

[TheSpyder]

It does use -intf-suffix but the command shown with the error I get indicates it's -intf-suffix .ml. However looking at the build folder I think the problem is the file naming scheme.

$ ls -1 _build/default/reason/
...
test2.cmi
test2.cmti
test2.cmx
test2.o
test2.re
test2.re.ml
test2.rei
test2.rei.mli
...

I guess if the intf suffix was .rei.ml it would find it, but that's pretty weird. It might be better if jbuilder refmt'd the files to test2.ml and test2.mli instead of keeping the .re in there.

[rgrinberg]

Looks you all identified the problem. Here's a simple patch that seems to fix the issue for me:

diff --git a/src/import.ml b/src/import.ml
index 31f1b9d..fb4c64e 100644
--- a/src/import.ml
+++ b/src/import.ml
@@ -363,6 +363,13 @@ module Filename = struct
   let extension fn =
     let i = extension_start fn in
     String.sub fn ~pos:i ~len:(String.length fn - i)
+
+  (* test.x.y.z -> .x.y.z *)
+  let rec greedy_extension fn =
+    match split_extension fn with
+    | _, "" -> ""
+    | (f, e) when Filename.extension f = "" -> e
+    | (f, e) -> greedy_extension f ^ "." ^ e
 end
 
 module Option = struct
diff --git a/src/module_compilation.ml b/src/module_compilation.ml
index b4c6229..8ade45a 100644
--- a/src/module_compilation.ml
+++ b/src/module_compilation.ml
@@ -23,7 +23,7 @@ let build_cm sctx ?sandbox ~dynlink ~flags ~cm_kind ~(dep_graph:Ocamldep.dep_gra
              cmi exists and reads it instead of re-creating it, which
              could create a race condition. *)
           ([ "-intf-suffix"
-           ; Filename.extension m.impl.name
+           ; Filename.greedy_extension m.impl.name
            ],
            [Module.cm_file m ~dir Cmi], [])
         | Cmi, None -> assert false

We'll wait for @diml 's input before proceeding.

[ghost]

The fix looks good to me

[ghost]

Looking at this again I don't understand how the patch fixes the issue. To check the existence of the mli the compiler does:

    let sourceintf =
      Filename.remove_extension sourcefile ^ !Config.interface_suffix in
    if Sys.file_exists sourceintf then begin

where remove_extension is non-greedy. So when we give foo.re.ml to the compiler there is indeed a problem as it will look for foo.re.mli which doesn't exist as jbuilder names this file foo.rei.mli.

It seems to me that we should instead change the naming scheme for intermediate files as follow:

• foo.re --> foo.re.ml
• foo.rei --> foo.re.mli

Which is more in line to what we do to handle preprocessors.

@TheSpyder dropping the .re entirely would be problematic as then the generated file foo.ml will conflict with foo.re. By naming it foo.something.ml, we are sure that it won't be considered as a source file for the library/executable being defined.

[TheSpyder]

I had the same reaction to the patch but figured I was just unfamiliar with the code and intf suffix 😅

[ghost]

Ok, I implemented the change and tested it on the archive

[jordwalke]

Thanks for the fix! When is the next release?

[ghost]

I'll do a beta11 on Friday

[jordwalke]

Thanks @diml. Much appreciated.