merlin icon indicating copy to clipboard operation
merlin copied to clipboard

Rewrite merlin-completion-at-point integration to be faster and better

Open catern opened this issue 1 year ago • 6 comments

Let me know how you'd like me to approach this PR. This is what we use internally at Jane Street, and it's been written to be relatively upstreamable. At the same time, the old version of merlin-cap was in need of a lot of improvement and modernization to how the Emacs completion API is supposed to be used, and essentially none of the original code survived. So it's kind of just a giant code drop, which I know is pretty hard to review.

The merlin-completion-at-point integration is completely rewritten. The version before was simple and slow; the new version is much faster and much more featureful:

  • Completions are requested for an entire module at once and filtered locally in Emacs rather than doing the filtering on the Merlin side.

  • Completions are cached based on the OCaml atom being completed, so they are re-used instead of re-requested when a new character is typed.

  • Those completions are cached for a given position inside an OCaml atom, so that if a user types Li TAB ma TAB to complete "List.map" and then decides they actually want the module "Labels", when they delete the "ist.map" part and hit TAB, they'll use the previously-requested completions.

  • We avoid updating Merlin with the new buffer contents as completion proceeds, so that Merlin doesn't need to re-parse and re-type-check, substantially improving performance in expensive files. (merlin-cap--omit-bounds)

  • Completion requests are handled asynchronously and reused, so that if completion is interrupted and then resumed, we're able to use the results of the previous completion request. This makes completion UIs which use while-no-input (like corfu-mode) much more performant.

  • Completion is wrapped in while-no-input when non-essential is set; this makes completion UIs which don't use while-no-input (like company-mode) much more responsive.

  • Completions are sorted more intelligently: if they're a constructor or variant or label, they're likely to be more relevant to the user, so they're sorted first.

  • Module names in completions are suffixed with a ., matching Emacs behavior for file name completion (where directories are suffixed with a /); this makes completion of module paths much more fluent, since there's no need to hit . after every module name.

  • We use completion boundaries, so the built-in Emacs partial-completion feature now works: if the user types "Li.ma TAB", it will complete to "List.map".

  • Likewise, partial-completion will expand * as a glob, so if the user types "Deferred.*.map TAB" they will be presented with every module in "Deferred." which contains the method "map"

There are also several tests now, testing the new functionality.

catern avatar May 06 '24 23:05 catern

Completions are requested for an entire module at once and filtered locally in Emacs rather than doing the filtering on the Merlin side.

Does this mean that ocaml-lsp would need to be modified to do this the same filtering ? (If the end user is using ocaml-lsp rather than Merlin + Merlin Emacs plugin )

sidkshatriya avatar May 07 '24 06:05 sidkshatriya

Thanks for doing the upstreaming @catern, it looks like it would be a major improvement for emacs users.

@sidkshatriya : it's very probable that the ocaml-lsp-server and the associated lsp clients already rely on such client-side filtering

voodoos avatar May 07 '24 08:05 voodoos

I am going to ping recent contributors to the emacs mode, someone might be interested in taking part in the review process: @bcc32, @erikmd, @monnier, @mattiase, @bbatsov, @Chris00

@catern I see that you have some tests in your changes. Would there be a way to have those test ran by some Dune's cram test ?

voodoos avatar May 07 '24 08:05 voodoos

I'll try it locally soon as possible!

xvw avatar Jun 11 '24 14:06 xvw

I'll take a closer look as well soon. There are quite a few changes, so reviewing them carefully will take some time.

bbatsov avatar Jun 11 '24 14:06 bbatsov

Friendly ping. @xvw @bbatsov have you been able to take a look? This PR looked quite promising. It would be great if it could be pushed forward.

nojb avatar Aug 29 '24 10:08 nojb

Overall the PR looks reasonable to me, but I haven't gone in much depth given the massive amount of changes. I'd suggest that the tests be moved to a dedicated file and ran automatically by the CI, as inline tests are always a mess.

bbatsov avatar Aug 29 '24 11:08 bbatsov

I was not able to run it (via load-file) and I suspect that my Doom-Emacs configuration should generate some trouble. For the extraction of the test @bbatsov, it seems that the code base on the emacs side does not have any test, do you have maybe some document about test infrastructure in Emacs? Thanks in advance!

xvw avatar Aug 29 '24 11:08 xvw

Not exactly documentation, but I do use a setup similar to https://github.com/bbatsov/projectile/blob/master/.github/workflows/test.yml in most of my Emacs projects. Works reasonably well for me.

bbatsov avatar Aug 29 '24 11:08 bbatsov

Thanks a lot for doing that upstreaming @catern !

voodoos avatar Sep 11 '24 14:09 voodoos