vscode-reasonml

vscode-reasonml copied to clipboard

Original PR is here: https://github.com/reasonml-editor/vscode-reasonml/pull/264

Native LSP implementation was merged recently into merlin repository. This PR switches extension to use it instead of OLS. This goes with some features removed (temporarily, I hope): — find occurances; — symbol rename; — case splitting; — code lens.

Also, it adds support for ocamlformat/refmt, because it was handled by OLS.

What do you think? What else should be done here?

Could you write a summary of what's removed and what's added here?

@andreypopp, I edited the description to mention removed features and addressed your comments

I guess I also need to provide a guide on how to build ocamlmerlin-lsp?

from this list:

— find occurances; — symbol rename; — case splitting; — code lens.

only "case splitting" is still unsupported by merlin-lsp, others were implemented in ocamlmerlin-lsp

Another question: what's the best way to keep compatibility with ocamlmerlin and BuckleScript projects? ocamlmerlin is a crucial part of this extension to jump to definition etc, but the historical way to do so (using reason-cli) hasn't been updated since last August.

Ideally, this extension could be used to run both BuckleScript and native projects, but I wonder what's the best way to use merlin with BS projects, considering it doesn't play well with esy yet? cc @andreypopp

@jchavarri I've been managing native dependencies with esy in bs projects so far — esy.json for esy and package.json for yarn/bs. It worked fine.

I included precompiled (with ocaml-4.02.3+buckle) versions of merlin-lsp and merlin-reason. Now extension should work with Bucklescript projects on mac and linux out-of-the-box.

Next steps are: — do the same for windows (if it is possible) — provide setup instructions for non-bs projects — automate setup for non-bs projects.

One thing I'm noticing when testing this branch is that when opening .re files with comments like /* */, merlin seems to choke:

# 0.02 lsp - debug
send: {
  "jsonrpc": "2.0",
  "method": "textDocument/publishDiagnostics",
  "params": {
    "uri":
      "file:///Users/javi/Development/github/Foo.re",
    "diagnostics": [
      {
        "range": {
          "start": { "line": 0, "character": 0 },
          "end": { "line": 0, "character": 0 }
        },
        "severity": 1,
        "message": "invalidCharacter.orComment.orString",
        "relatedInformation": [],
        "relatedLocations": []
      }
    ]
  }
}

I guess it has to do with the version of the Reason parser that is included with ocamlmerlin-reason?

Hm. Can't repro with /* */ comments now. But // definitely make merlin unhappy.

That's probably because I included ocamlmerlin-reason from reason-cli. I'll fix that

So, I talked to @jchavarri and we agreed that the best workflow would be one with esy because it installs all required dependencies locally and prevents global switches mess.

But I still really want to provide a seamless experience for BS users who don't want to deal with esy/opam. So, I decided to leave two options:

1. If extension detects esy configuration, it asks you to specify all required binaries such as merlin-lsp and ocamlformat as devDependencies.
2. If extension detects bsconfig without esy.json, it tries to use precompiled binaries, but still allow you to override formatters.

If we can't detect either, we just stop and ask user to provide of these configurations, because we have no idea what to do.

Next, we can provide a creation or population of esy.json and probably run esy to install dependencies. Also, I wonder, if we can hide all of this into extension internals and detect compiler version based on project configuration.

@andreypopp @jchavarri @jordwalke what do you think?

Can we perhaps make the scope of this PR more limited and merge it? Like, if there's no ocamlmerlin-lsp in path, just add an error and say that it's not in the path? It would be a great improvement itself and any follow ups would be just icing on the cake!

@wokalski, makes sense to me. I'll prepare changes over the week

@wokalski, it was quite a long week :)

I dumbed down the extension. Now it requires ocamlmerlin-lsp and ocamlmerlin-reason in configuration and doesn't work otherwise. Also, instead of trying to configure the environment automatically, I just added instructions to README.

I'll return to esy/opam and easier setup after this PR is merged.

cc @andreypopp @jchavarri @jordwalke

Is this abandoned? People were recommending vscode-reasonml as reason-language-server is pretty unreliable at the moment. This PR looks like a great improvement.

@jfrolich take a look at ocamllabs/vscode-ocaml-platform.