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

Add LSP frontend #905

Merged
merged 6 commits into from
Jan 28, 2019
Merged

Add LSP frontend #905

merged 6 commits into from
Jan 28, 2019

Conversation

andreypopp
Copy link
Member

@andreypopp andreypopp commented Jan 22, 2019

This adds ocamlmerlin-lsp which is a part of a new merlin-lsp.opam package.

ocamlmerlin-lsp adds LSP frontend for merlin with the following initial set of features:

  • textDocument/definition — analogue of locate
  • textDocument/typeDefinition — no analogue in current merlin frontend, like locate but locates the type of the specified expression.
  • textDocument/completion — analogue of complete-prefix
  • textDocument/publishDiagnostics — analogue of errors query, pushed by server on changes to sources.
  • textDocument/documentSymbol — analogue of outline query.

Structure:

  • merlin-lsp.opam — new merlin-lsp package definition
  • src/lsp — LSP protocol library
  • src/frontend/lspocamlmerlin-lsp frontend
  • tests-lsp — tests for ocamlmerlin-lsp (far from comprehensive)

I'm using this with neovim/ALE now and have vscode plugin which I'm not submitting in this PR but will do after some cleanup. For now it's hosted at https://github.com/andreypopp/vscode-merlin.

This is required when we introduce merlin-lsp.opam package in the next commits.
This library implements LSP.

The initial implementation of protocol structures is based on @bryphe
work in bryphe/merlin-language-server[1] and then with additions from
facebook/flow[2].

This uses `ppx_deriving_yojson` for JSON encoding/decoding.

Implemented only subset of LSP for now, see src/lsp/protocol.ml.

[1]: https://github.com/bryphe/merlin-language-server
[2]: https://github.com/facebook/flow
This adds `ocamlmerlin-lsp` executable which implements lsp frontend for
merlin using `lsp` library introduced in the previous commit.

The implementation is simple - it translates LSP requests into merlin
new protocol query commands for the exception of
`textDocument/typeDefinition` which is implemented from scratch as
there's no analogue in existing protocol.

The executable is being installed as a part of a new `merlin-lsp.opam`
package. The reason is partly b/c `merlin-lsp.opam` requires more
3rd-party dependencies (`ppx_deriving_yojson` in particular) and partly
because the implementation is relatively untested.
See tests-lsp/README.md for more info.

They are not configured to run on CI yet.
@andreypopp andreypopp changed the title Add LSP frontend. Add LSP frontend Jan 22, 2019
@andreypopp
Copy link
Member Author

Ok, it's green now on 4.02: I moved Std.Result to Lsp.Utils.Result for now so it won't interfere with merlin.lsp support of 4.02.

By the way merlin-lsp.opam itself is marked with ocaml >= 4.04 but it would be trivial (I think) to fix to run under 4.03 and then 4.02.

@hcarty
Copy link
Member

hcarty commented Jan 23, 2019

I think the dune version constraints need to be bumped since the language version is bumped in dune-project.

@andreypopp
Copy link
Member Author

@hcarty good catch! Will update the constraint.

@andreypopp
Copy link
Member Author

For the record the implementation is based heavily off bryphe/melrin-language-server by @bryphe with protocol additions from facebook/flow.

I already mentioned that in commit messages but should have mentioned that here too.

@let-def let-def mentioned this pull request Jan 25, 2019
@let-def let-def merged commit 64757b8 into ocaml:master Jan 28, 2019
@avsm
Copy link
Member

avsm commented Jan 28, 2019

Great to see this feature! However, we need to be careful of not having any license violations. In particular, the LICENSE file in Merlin is not a reproduction of the LICENSE present in @bryphe and the Flow repository. The names of the contributors should be added to the root LICENSE file to ensure that the correct names are present, or reproduced in the individual files which were derived from the upstream repositories.

@avsm
Copy link
Member

avsm commented Jan 28, 2019

And of course @let-def is too fast for me; he has addressed this in #906 :-)

@rbjorklin
Copy link

This looks interesting! Would this warrant a new release of merlin any time soon?

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

Successfully merging this pull request may close these issues.

5 participants