Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generated .merlin doesn't understand ppx_compare #230

Closed
jvillard opened this issue Aug 23, 2017 · 8 comments
Closed

generated .merlin doesn't understand ppx_compare #230

jvillard opened this issue Aug 23, 2017 · 8 comments
Labels

Comments

@jvillard
Copy link
Contributor

The generated .merlin for this jbuild produces the PKG ppx_compare.runtime-lib directive, but merlin needs PKG ppx_compare instead.

In particular, this line will generate a merlin error (Uninterpreted extension 'compare.equal'.):

let equal_foo = [%compare.equal : foo]
@jvillard
Copy link
Contributor Author

Note also that the FLG line in the gist contains lots of duplication:

FLG -open InferModules -g -short-paths -safe-string -principal -strict-formats -strict-sequence -bin-annot -w +3+5+6+8+10+11+12+18+19+20+21+23+26+29+27+32+33+34+35+37+38+39+50+52+57-4-9-40-41-42-45-48 -open InferStdlib -open IStd -open InferGenerated -g -short-paths -safe-string -principal -strict-formats -strict-sequence -bin-annot -w +3+5+6+8+10+11+12+18+19+20+21+23+26+29+27+32+33+34+35+37+38+39+50+52+57-4-9-40-41-42-45-48 -open InferStdlib -open IStd -open InferGenerated -open InferModules -g -short-paths -safe-string -principal -strict-formats -strict-sequence -bin-annot -w +3+5+6+8+10+11+12+18+19+20+21+23+26+29+27+32+33+34+35+37+38+39+50+52+57-4-9-40-41-42-45-48 -open InferStdlib -open IStd -open InferGenerated

@rgrinberg
Copy link
Member

Note that jbuilder will generate an appropriate FLG -ppx rather than a PKG ppx_compare directive. This is a really strange issue though, I just tried using ppx_compare in a minimal example and jbuilder generated a correct .merlin for me.

Are you able to use the generated compare functions at all? I.e. does jbuilder actually preprocess things correctly?

@jvillard
Copy link
Contributor Author

Yes the build is fine, the only thing going awry is the .merlin (see also #229 on the same project).

I just noticed that if I remove the last stanza from my jbuild, which is the only one without (preprocess (pps (ppx_compare))), then the .merlin does contain a FLG -ppx line:

FLG -ppx '/home/jul/infer/infer/src/_build/default/.ppx/ppx_compare/ppx.exe --as-ppx --cookie '\''library-name="InferModules"'\'''

However merlin still gives errors about extension points.

@ghost
Copy link

ghost commented Aug 31, 2017

The main problem here is that the scope of a .merlin file is the whole directory. So when we have different build configurations in the same directory, jbuilder has to somehow merge them (function merge_two in src/merlin.ml). For library dependencies, it takes the union. For preprocessing specifications, it compares them and gives up if they are not equal.

However merlin still gives errors about extension points.

There were a lot of issues related to quoting, I believe it should work fine with the latest version of jbuilder and merlin.

@trefis
Copy link
Collaborator

trefis commented Sep 1, 2017

The main problem here is that the scope of a .merlin file is the whole directory

Actually since ocaml/merlin@cf93c20 the scope could be the file instead of the directory.

Although generating a ton of .foo.merlin in the source directory might be a bit annoying.
However if something like ocaml/merlin#712 ever lands (but I think merlin would have to look in <workspace-root>/_build/dir/ and not dir/_build it might actually be a decent option.

@diml , @let-def : any thoughts?

@ghost
Copy link

ghost commented Sep 3, 2017

Although generating a ton of .foo.merlin in the source directory might be a bit annoying.

Yh, that doesn't seem right to me.

However if something like ocaml/merlin#712 ever lands (but I think merlin would have to look in /_build/dir/ and not dir/_build it might actually be a decent option.

Seems good, however for jbuilder it should be <workspace-root>/_build/default/dir

@ghost
Copy link

ghost commented Sep 22, 2017

@trefis @let-def I suggest the following design which would shift the decision of how to organize the configuration to the user/build system: in .merlin files, allow:

GEN <command>

then to get the configuration for a file foo.ml, merlin would run <command> foo.ml which would be expected to output the merlin configuration for foo.ml.

@rgrinberg
Copy link
Member

I'm closing this as we're waiting for the obsoletion of .merlin files. It's highly unlikely that will work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants