[dune] Merlin's PPX invocations does not match Dune's

[voodoos]

Context

Merlin associates with each ppx command line a workdir value which represents the directory in witch the ppx executable will be started by Merlin. This configuration value is not given to Merlin but inferred from the folder containing the configuration. That is, for .merlin based configuration, that folder was always the one containing the .merlin.

This was not a perfect solution as it does not reflect the true running folder of the ppxes when Dune builds a project, but it was a best effort solution which seems to work in most cases.

Issue

This best-effort broke when switching to the Dune-based configuration reader because Merlin considered the folder in which the external configuration reader is started to be the workdir. However, unlike dot-merlin-reader, the dune ocaml-merlin process starting folder might not be the one with the dune file.

The issue notably appears when using ppx_expect and was reported by @jeremiedimino. After investigating it turns out that ppx_expect should not attempt to compute the hash off the file when called by Merlin and Jérémie opened an issue.

Other ppxes that need to interact with files from the sources might be equally broken right now.

Food for thought

This issue is a good opportunity to make Merlin ppx behavior closer to Dune's in term of workdir and execution environment. However this need changes to both sides and might not happen immediately. This raises two questions:

1. How can we have Merlin replicate more precisely the environnement and conditions of executions of ppxes by Dune ?
2. Meanwhile, should we issue a quick fix to restore the previous, less-broken, behavior ?

For 1. Dune would need to forward more information to merlin, especially the workdir which appears to always be the root of the current context folder in _build. We also need Dune to provide the filename to use for locations.

For 2. the fix is ready but it is adding more noise to the codebase for use cases that may be illegitimate (for example ppx_expect should not do what it is doing when invoked in Merlin). However the patch does "solve" the current issue and could always be reverted when a cleaner approach is merged.

[voodoos]

cc @emillon @jeremiedimino

[ghost]

The changes for 1 seems simple on the Dune side, and it seems fine to put them in a 2.8.5 release.

[voodoos]

They are indeed simple but they require changes on both sides. That means we should add a new conflict between dune and merlin.

If we don't merlin will report some "unknown configuration directive" to the user if dune attempts to pass more info.

I guess we don't want to add such constraints on a patch release like Dune 2.8.5 ?

[ghost]

I guess we don't want to add such constraints on a patch release like Dune 2.8.5 ?

It depends. Is the protocol between Dune and merlin public/versioned at this point? Or do we still consider that it is in-development mode? If that's the later, adding such a constraint seems fine to me. The new information is not part of the public API, so we are not really adding a new feature.

BTW, both of the choices you proposed seems fine to me. I just wanted to point out that we could do 1 without too much ceremony, in case you don't want to add a hack in merlin.

[voodoos]

We finally decided to merge #1284 and restore the original behaviour.
We will work on a more robust solution in the future.

(As far as I'm aware there are no plans to make the merlin/dune protocol public yet)

[voodoos]

Dune's PR ocaml/dune#4478 has a test that illustrates the fact that, as it expected, the current behaviour does not work when the ppx is defined in a subdir stanza.

[voodoos]

Related issue: #1114

[rgrinberg]

Personally, I think it's time to address the versioning issue and for all. There's a couple of new motivators:

1. We are looking to increase the sophistication of interaction between dune and merlin for a much better user experience.
2. We have a real rpc protocol in dune with a principled versioning mechanism (thanks @cwong-ocaml). Let's just reuse that.

In practice, I think this would mean freezing the current implementation and redoing it on top of rpc.

@jeremiedimino that's indeed an issue with ppx_expect, but is there a mechanism for other ppx's to handle this correctly? I.e. can they detect they're running in merlin mode?

[ghost]

@jeremiedimino that's indeed an issue with ppx_expect, but is there a mechanism for other ppx's to handle this correctly? I.e. can they detect they're running in merlin mode?

They can look at tool_name and check if it's "merlin". This is what ppx_expect does now.