interop between dune and merlin since 2.8 does not work with ppx_expect

[jberdine]

Expected Behavior

With the project in #4478, building with dune and then querying using ocamlmerlin succeeds:

$ dune build
$ ocamlmerlin single errors -filename lib/sublib/bar.ml < lib/sublib/bar.ml
{"class":"return","value":[],"notifications":[],"timing":{"clock":39,"cpu":34,"query":2,"pp":0,"reader":4,"ppx":22,"typer":7,"error":0}}

Actual Behavior

With dune 2.7.1 and merlin 4.1, the actual behavior is as expected. But with later versions of dune, ocamlmerlin cannot find the source file:

$ dune build
$ ocamlmerlin single errors -filename lib/sublib/bar.ml < lib/sublib/bar.ml
{"class":"return","value":[{"start":{"line":0,"col":-1},"end":{"line":0,"col":-1},"type":"typer","sub":[],"valid":true,"message":"I/O error: bar.ml: No such file or\ndirectory"}],"notifications":[],"timing":{"clock":47,"cpu":29,"query":2,"pp":0,"reader":4,"ppx":21,"typer":2,"error":0}}

Reproduction

• PR with a reproducing test: Add dune-merlin-ppx_expect test #4478

Specifications

• Version of dune (output of dune --version): 2.8.5
• Version of ocaml (output of ocamlc --version): 4.11.1
• Operating system (distribution and version): macos 11.2.3

[jberdine]

Cc @voodoos who may know about this.

[voodoos]

Yes it is a known issue and in fact it is already fixed on master (and the 411 branch).
It will be part of the next release.

You can pin the following branch if you would like to try it beforehand: opam pin https://github.com/ocaml/merlin.git#411

[jberdine]

Thanks, I will retest. So far in my full environment (which uses 4.12), opam pin add dune --dev does not resolve this issue.

[voodoos]

It is expected because the issue in on Merlin's side, not Dune's.
opam pin https://github.com/ocaml/merlin.git should fix it for 4.12.
(for OCaml 4.11 use opam pin https://github.com/ocaml/merlin.git#411)

Please tell me if that is effectively the case when you find the time to try 🙂

[jberdine]

Ah, sorry for messing up the test.

I have just tried dune 2.8.0-395-g65404cf973 and ocamlmerlin v4.1-59-g0caac250d6 . The test case in the linked PR does now work as expected. There file not found error persists in my full environment, so there is something else going on that I need to figure out.

[voodoos]

I will also try to reproduce.

[voodoos]

When isolating you test from Dune's test tree and building it with dune 2.8.3 I do get the error using merlin 4.1-412:
{"class":"return","value":[{"start":{"line":0,"col":-1},"end":{"line":0,"col":-1},"type":"typer","sub":[],"valid":true,"message":"I/O error: bar.ml: No such file or\ndirectory"}],"notifications":[],"timing":{"clock":61,"cpu":33,"query":1,"pp":0,"reader":4,"ppx":25,"typer":4,"error":0}}

However after pinning opam pin https://github.com/ocaml/merlin.git I get the expected behaviour:
{"class":"return","value":[],"notifications":[],"timing":{"clock":60,"cpu":36,"query":1,"pp":0,"reader":4,"ppx":22,"typer":9,"error":0}}

I think the issue comes from the fact that you try to build your test from inside Dune's tree. Calling dune build from that folder does nothing. To have a working test you need a run.t file with instructions that will be played in a sandboxed environment when running the tests. (and it not really possible to test ocamlmerlin behavior in Dune tests because of the sandboxing)

(I believe Cram tests folders are treated similarly to data_only folders)

[voodoos]

I pushed a commit to your branch with a working test that can be run using dune build @dune-merlin-ppx_expect:
6a36538

It does work when Merlin master is installed, but not with current stable (so the CI is expected to fail).

[jberdine]

@voodoos I updated the test case in #4478 so that it hits the file not found error with:

❯ dune --version
2.8.0-395-g65404cf973

and

❯ ocamlmerlin -version
The Merlin toolkit version v4.1-59-g0caac250d6, for Ocaml 4.12.0

The main point seems that using a subdir stanza leads to the file not being found.

[voodoos]

Indeed, this is an issue. Dune should pass more information to Merlin to handle these cases.

If I am not mistaken this is probably not a new issue, that is, not due to the new "interop", as Merlin has always been considering the ppx workdir to be the directory in which the nearest .merlin (and dune file) resides.

There is a discussion on how to improve the current behaviour (which is known to be imperfect) in this Merlin issue: ocaml/merlin#1292.

Your test is a good example of broken behaviour, thank you for taking the time to putting it in place, we will try to improve that in the next iterations of Dune.

[jberdine]

Thanks for the confirmation. It seems that a number of things have to conspire to trigger this behavior.

I'm not sure whether or not to categorize this as a new issue. On one hand, with dune 2.7.1, this example works. On the other hand, that seems to be because the .merlin file is generated in the lib subdir even though the nearest enclosing dune file is in the directory above, which makes the paths correct perhaps just by lucky coincidence. I wonder if there is a "quick fix" change in similar spirit to ocaml/merlin#1284 that would set the workdir to where the .merlin file used to be generated, which with subdir stanzas is not necessarily where the nearest enclosing dune file is.

Maybe this is more relevant to the linked merlin issue, but it seems clearly desirable for dune to pass enough information to merlin that merlin can replicate the ppx.exe invocation that dune uses for the "real" build. It does make me wonder if it might be more robust over the longer term to use a higher-level abstraction of e.g. having dune pass the full command to merlin, rather than merlin having to reconstruct it. This would be somewhat analogous to compilation databases.

[voodoos]

Oh you are right, because dune generated the .merlin in the subdir it was working before and this is a regression with the new interop. We definitively need to improve that.

Meanwhile, I just tried a silly (but easy) workaround: adding an empty dune file to your subdir tricks Merlin into the correct behaviour. (with merlin master that is pending release).

[voodoos]

Reopening since there is still an issue to solve. We might open a new issue without all the debug history (or hide a few comments).

[rgrinberg]

@voodoos any updates on this? It's not clear to me what still needs to be done for this issue.

[voodoos]

Not sure any more, I will try to reproduce when I find the time. If I remind well there is still room for improvement in the way Merlin starts the ppxes (that might differ from what Dune does).