merlin icon indicating copy to clipboard operation
merlin copied to clipboard

Cannot locate implementations when building with dune

Open mrmr1993 opened this issue 5 years ago • 5 comments

Currently, :MerlinLocate in vim seems to ignore g:merlin_locate_preference="ml" when the target module has an interface file (at least when building with dune). In particular, locating a value M.t in a module with an interface file goes to the declaration in the signature instead of the definition.

This has never been particularly useful to me, since the value's information in the interface file can normally be gleaned from :MerlinTypeOf and :MerlinDocument. Worse still, if the interface file includes a signature that provides that particular item, merlin will jump to the declaration in the signature file, so the command may not leave you in the proximity of the implementation either.

For example, with the following setup, locating Test.f will land in intf.ml. test.ml:

let f x = x

test.mli:

include Intf.S

intf.ml:

module type S = sig
  val f : int -> int
end

It seems that this is a consequence of the .cmti files generated from modules with interfaces: the locations for the signature items point to the interface file, not the implementation file, so that location is displayed.

It would be much more useful to modify the command (or add a new one? :MerlinLocateDef?) to identify the global module and locate the definition in its .ml file instead. Sadly, this seems to require merlin to parse and typecheck the file to find the definition, but should in theory be possible.

I would be willing to work towards this if it is viable.

Potential scope creep 1

Even nicer would be a stepping version of :MerlinLocate that remembers the current path that is being located. In the example above, this would find

  • test.mli, at the include line
  • intf.ml, at the val f declaration
  • test.ml, at the let f definition

This would fully reveal all of the references that might be relevant, which I would find especially useful when modifying existing code. It would still also be nice to have a straight-to-definition version as above, even if this extension is worth exploring.

This naturally also has an impact on and increased complexity from module inclusion. For example, replacing test.ml with

include Test_impl

(where Test_impl defines the value f) adds an additional location to this list, as well as potentially more for any interface files and their inclusions if there is a test_impl.mli.

Potential scope creep 2

In code with heavy use of functors, it can be awkward to find the definition of a function. For example, if the above Test_impl was implemented as

module F (T : sig val f : 'a -> 'a end) = struct
  include T
end
module T = struct
  let f x = x
end
include F (T)

then the naive implementation of scope creep 1's stepping would land in the functor argument's signature. It would be really awesome if we could track the module substitutions that we accumulate, so that stepping once more from that functor argument's signature resolved to the definition in Test_impl.T.f.

Potential scope creep 3

Beyond scope creep 2, it would be excellent if we further extended the notion of a substitution environment, so that subsequent lookups from within a functor would still resolve in the substitution. For example, if Test_impl was instead implemented as

module F (Logger : sig val log : string -> unit end) = struct
  let f x = Logger.log (int_to_string x); x
end
module Logger = struct
  let log _ = ()
end
include F (Logger)

the value Test.f could be step-located back toTest_impl.F(_).f, and then step-locating Logger.log from within its body would step first to F's Logger signature, and then to Test_impl.logger.log.

This final extension adds some UX complexity in addition to the implementation complexity: when should it be cleared, should it be clearable manually, etc. I don't have a good sense of what a full implementation of this feature would look like, or what it would entail.

mrmr1993 avatar Nov 25 '20 05:11 mrmr1993

Regarding the first part of your issue, I believe that you are incorrect: if instructed to do so, merlin will look in .cmt rather than .cmti files, if the .cmt file is present. Unless I made a mistake, I think I reproduced the example you describe:

$ cat test.sh
#!/bin/bash

set -e

echo "let f x = x" > test.ml
echo "include Intf.S" > test.mli
echo "module type S = sig val f : int -> int end" > intf.ml

ocamlc -c -bin-annot intf.ml
ocamlc -c -bin-annot test.mli
ocamlc -c -bin-annot test.ml

echo "let _ = Test.f" | \
  ocamlmerlin single locate -look-for ml -position 1:13 -filename main.ml | \
  jq .value

echo "rm test.cmt # to see what you could experience"
rm test.cmt

echo "let _ = Test.f" | \
  ocamlmerlin single locate -look-for ml -position 1:13 -filename main.ml | \
  jq .value

rm *.cm*
rm *.ml
rm *.mli

and everything does work as expected:

$ ./test.sh
{
  "file": "/tmp/test-merlin/test.ml",
  "pos": {
    "line": 1,
    "col": 4
  }
}
rm test.cmt # to see what you could experience
{
  "file": "/tmp/test-merlin/intf.ml",
  "pos": {
    "line": 1,
    "col": 20
  }
}

I guess it will be apparent what I think the problem is in your situation: some .cmt file must be missing, while the .cmti file is present. I've heard rumors of such situations recently. I believe it was due to dune only building cmti (but not cmt) in some situations, perhaps in native mode? perhaps for executables? I don't remember; it does seem like more and more people are getting annoyed by this though. Ping @voodoos, @rgrinberg .


As for the rest of your message, really this would have deserved its own, separate issue, but I'll answer here briefly:

  • "scope creep 1": yes, we've had this idea of saving this "jump trace" for some years now. The reason why it's not implemented (apart from the obvious "no one has had time to do it") is that while it's a fairly easy change to make it the backend, it's not obvious how to expose that information to the user, in their editor.
  • "scope creep 2": if we just exposed the trace, there would be no problem, we already handle functor applications and the substitutions involved:
    $ cat test-functor.sh
    #!/bin/bash
    
    set -e
    
    cat > test_impl.ml <<EOF
    module F (T : sig val f : 'a -> 'a end) = struct
      include T
    end
    module T = struct
      let f x = x
    end
    include F (T)
    EOF
    
    ocamlc -c -bin-annot test_impl.ml
    
    echo "let _ = Test_impl.f" | \
      ocamlmerlin single locate -look-for ml -position 1:18 -filename main.ml | \
      jq .value
    
    rm *.cm*
    rm *.ml
    rm *.mli
    
    $ ./test-functor.sh
    {
      "file": "/tmp/test-merlin/test_impl.ml",
      "pos": {
        "line": 5,
        "col": 6
      }
    }
    
  • "scope creep 3": sounds quite hard to get such a feature to work correctly.

trefis avatar Nov 25 '20 07:11 trefis

Thanks, it seems dune wasn't exposing its native object directory in .merlin for native builds, so merlin didn't know about the .cmt files. I've opened PR ocaml/dune#3974 with a quick fix.

For brief replies to your brief answers:

  • "scope creep 1": yes, we've had this idea of saving this "jump trace" for some years now. The reason why it's not implemented (apart from the obvious "no one has had time to do it") is that while it's a fairly easy change to make it the backend, it's not obvious how to expose that information to the user, in their editor.

It would be really nice to have this. Should I open a dedicated issue for it?

  • "scope creep 2": if we just exposed the trace, there would be no problem, we already handle functor applications and the substitutions involved:

This is exactly what I wanted, it was just concealed from me by dune's bad .merlin files!

  • scope creep 3": sounds quite hard to get such a feature to work correctly.

Agreed.

mrmr1993 avatar Nov 25 '20 08:11 mrmr1993

It would be really nice to have this. Should I open a dedicated issue for it?

Please do!

trefis avatar Nov 25 '20 08:11 trefis

Please do!

I've opened #1206 for this. I'm leaving this open in case there is value in tracking this dune issue here, and have updated the title to reflect that the issue is with dune builds.

mrmr1993 avatar Nov 27 '20 02:11 mrmr1993

I arrived at this issue to find out how to resolve the issue of :MerlinLocate not jumping to the .ml file but corresponding .mli file. But, as I infer from the issue comments, is this not an easy problem that cannot be resolved right away? Is there a workaround that works for the most simple cases?

TonalidadeHidrica avatar Jun 27 '21 05:06 TonalidadeHidrica