Migrate to ocamlmerlin-lsp #269

rusty-key · 2019-02-28T11:23:51Z

Original PR is here: #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?

andreypopp · 2019-03-01T08:56:54Z

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

andreypopp

Great! I've left some comments inline.

andreypopp · 2019-03-01T08:55:24Z

.vscode/settings.json

@@ -1,5 +1,5 @@
 {
-  "editor.formatOnSave": true,
+  // "editor.formatOnSave": true,


Unrelated change?

andreypopp · 2019-03-01T08:56:06Z

package.json

-            "enum": ["merlin", "bsb"]
+            "enum": [
+              "merlin",
+              "bsb"


We keep support for bsb diagnostics here?

andreypopp · 2019-03-01T08:58:06Z

src/formatters/reason.ts

+    {
+      provideDocumentFormattingEdits(_document: vscode.TextDocument): vscode.TextEdit[] {
+        const formatterPath = configuration.get<string | undefined>("path.refmt");
+        const formatter = formatterPath ? path.resolve(rootPath, formatterPath) : "/usr/local/bin/refmt";


Should this be just refmt so it is being resolved from PATH instead of hard coding to /usr/local/bin/refmt?

andreypopp · 2019-03-01T08:59:17Z

src/formatters/reason.ts

+          fs.writeFileSync(tempFileName, textEditor.document.getText(), "utf8");
+          const formattedText = execSync(`${formatter} ${tempFileName}`).toString();
+          const textRange = getFullTextRange(textEditor);
+          fs.unlinkSync(tempFileName);


If we use temporary file we'd want to remove it incase refmt produces an error. But we probably can just pipe input on stdin instead?

I tried it, but it doesn't work with large files

andreypopp · 2019-03-01T08:59:50Z

src/formatters/reason.ts

+        if (textEditor) {
+          const tempFileName = `/tmp/vscode-refmt-${uuidv4()}.re`;
+          fs.writeFileSync(tempFileName, textEditor.document.getText(), "utf8");
+          const formattedText = execSync(`${formatter} ${tempFileName}`).toString();


I think this lacks error handling — what happens if we try to reformat invalid code? This will throw?

andreypopp · 2019-03-01T09:00:33Z

src/formatters/ocaml.ts

@@ -0,0 +1,37 @@
+import { execSync } from "child_process";


Same comments I left for reason formatter are applicable to this too, I think.

andreypopp · 2019-03-01T09:01:54Z

src/client/index.ts

+  return serverOptions;
+}
+
+export async function launchMerlinLsp(context: vscode.ExtensionContext, useEsy: boolean): Promise<void> {


Let change useEsy arg to be options: {useEsy: bool} instead, this will make things more clear on call sites.

rusty-key · 2019-03-10T11:26:06Z

@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?

bryphe · 2019-03-10T21:52:23Z

src/formatters/ocaml.ts

+        const textEditor = vscode.window.activeTextEditor;
+
+        if (textEditor) {
+          const tempFileName = `/tmp/vscode-reasonml-${uuidv4()}.ml`;


This should be using os.tmpdir() instead of /tmp - otherwise it won't work correctly on Windows.

I didn't think about windows at all. Will try to setup vm and test it there this week

Thanks @rusty-key !

andreypopp · 2019-03-11T17:32:47Z

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

jchavarri · 2019-03-17T12:42:27Z

package.json

-        "reason.diagnostics.tools": {
-          "type": "array",
-          "items": {
-            "enum": ["merlin", "bsb"]


@rusty-key @andreypopp One of the main advantages of this bsb integration was that bsb has full project visibility. So if one file changes and that change breaks other files, bsb is able to show those errors in the Diagnostics panel, even if the file is not opened. One has just to click on them to open the broken file, which is useful for large refactors.

Is something like that possible with merlin? I always believed that merlin only has a "local view" of the world. 🤔

jchavarri · 2019-03-25T22:38:04Z

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

andreypopp · 2019-03-25T22:52:32Z

@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.

rusty-key · 2019-03-30T16:59:14Z

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.

jchavarri

Thanks for this @rusty-key !!

jchavarri · 2019-03-30T22:45:49Z

src/client/index.ts

+  if (options.useEsy) {
+    run = {
+      args: ["exec-command", "--include-current-env", merlinLsp],
+      command: process.platform === "win32" ? "esy.cmd" : "esy",


The way this is implemented has some unexpected behavior. If I'm understanding correctly, if user sets in settings.json:

{ "reason.path.ocamlmerlin-lsp": "/some/folder/ocamlmerlin-lsp" }

then the command will be

esy /some/folder/ocamlmerlin-lsp

but I would expect it to be just

/some/folder/ocamlmerlin-lsp

If the path is set manually, then it should be used "as is", imo.

jchavarri · 2019-03-30T22:48:44Z

package.json

-          "default": "./node_modules/bs-platform/lib/bsb.exe",
-          "description": "The path to the `bsb` binary."
-        },
-        "reason.path.ocamlfind": {


@jordwalke Heads up: this will remove the APIs for introspecting environments that were added in https://github.com/freebroccolo/ocaml-language-server/pull/68.

jchavarri · 2019-03-30T22:50:35Z

package.json

-          "type": "string",
-          "default": "ocamlfind",
-          "description": "The path to the `ocamlfind` binary."
-        },
        "reason.path.esy": {


@rusty-key I think this can be removed too, it was used for the env introspection (like ocamlfind, env etc)

jchavarri · 2019-03-30T22:54:45Z

src/client/index.ts

+        env: {
+          ...process.env,
+          MERLIN_LOG: "-",
+          OCAMLFIND_CONF: "/dev/null",


Does this have any implications? Why does merlin fail if this is not set? cc @andreypopp

afaiu, merlin relies on ocamlfind by default and when it is not there, it fails. This flag can be removed as soon as findless branch will be merged and released:
https://github.com/ocaml/merlin/tree/findless

jchavarri · 2019-04-01T11:28:09Z

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?

jchavarri · 2019-04-02T22:44:45Z

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

rusty-key · 2019-04-03T09:10:32Z

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

jchavarri · 2019-04-03T10:40:43Z

src/formatters/utils.ts

+
+  if (!formatter) {
+    vscode.window.showInformationMessage(
+      `${formatterPath} is not available. Please specify "vscode-reasonml.path.${formatterName}"`,


Please specify "vscode-reasonml.path.${formatterName}"

This message is not correct. The setting would be reason.path.refmt.

Unrelated, but I can't get refmt to work even when using the right config.

rusty-key · 2019-04-05T13:26:51Z

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:

If extension detects esy configuration, it asks you to specify all required binaries such as merlin-lsp and ocamlformat as devDependencies.
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?

add local opam switch support

wokalski · 2019-06-24T10:29:48Z

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!

rusty-key · 2019-06-24T10:39:19Z

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

rusty-key · 2019-07-21T11:46:06Z

@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

jfrolich · 2020-03-28T15:50:00Z

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.

wokalski · 2020-03-28T17:16:52Z

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

andreypopp and others added 6 commits January 31, 2019 18:24

WIP: support for ocamlmerlin-lsp

4637b8a

use merlin-lsp for all projects

1072823

remove ols-related code

aec1eac

add formatters

a4c1821

use temp files to better handle large files

d52fee6

fix formatting

57590da

andreypopp reviewed Mar 1, 2019

View reviewed changes

andreypopp mentioned this pull request Mar 1, 2019

WIP: support for ocamlmerlin-lsp #264

Closed

2 tasks

rusty-key added 5 commits March 10, 2019 12:31

revert unrelated change

690f2a4

tweak options format

c705687

handle possible errors

32fe6d3

remove debug

798219e

remove unsupported settings

8bd13e5

bryphe reviewed Mar 10, 2019

View reviewed changes

jchavarri reviewed Mar 17, 2019

View reviewed changes

rusty-key added 4 commits March 29, 2019 16:04

prebuild merlin-lsp/merlin-reason and use them for reason projects

7000de8

use platform’s tmp dir

8f44e4e

fix lsp binaries

ce09262

remove ocamlfind dependency

c6d6408

jchavarri reviewed Mar 30, 2019

View reviewed changes

jchavarri reviewed Apr 3, 2019

View reviewed changes

get rid of custom paths

bf91e25

rusty-key force-pushed the ocamlmerlin-lsp branch from 78d74be to bf91e25 Compare April 5, 2019 11:43

rusty-key added 2 commits April 5, 2019 15:24

add some checks

280112c

use esy for formatters

8e11395

fix code style

9ae3159

Khady force-pushed the ocamlmerlin-lsp branch from 50023a6 to 9ae3159 Compare April 8, 2019 01:39

Khady and others added 9 commits April 8, 2019 09:16

add local opam switch support

be95b57

Merge pull request #1 from Khady/opam-switch-support

9e7085d

add local opam switch support

add restart ls command

b06b7ec

fix refmt’s temp file extension

9c7682d

remove debug

6a75a9a

add esy.json generation for bs projects

7e1dc9d

add resolutions for 4.02

a6c0889

ignore packaged extensions

d5845d8

init merlin-lsp when it is installed

6c78709

naartjie mentioned this pull request May 26, 2019

Update README: install ocp-indent / refmt #278

Closed

rusty-key added 5 commits July 20, 2019 22:22

"simplify" extension setup

c95bbc2

pass ocamlmerlin-reason path

c7c77e4

describe 4.02 setup

520bdd4

update readme

2b66c0e

remove unused utils

be6cac2

fix version

c759a28

Migrate to ocamlmerlin-lsp #269

Are you sure you want to change the base?

Migrate to ocamlmerlin-lsp #269

Conversation

rusty-key commented Feb 28, 2019 • edited Loading

andreypopp commented Mar 1, 2019

andreypopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusty-key commented Mar 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreypopp commented Mar 11, 2019

Choose a reason for hiding this comment

jchavarri commented Mar 25, 2019

andreypopp commented Mar 25, 2019

rusty-key commented Mar 30, 2019

jchavarri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri commented Apr 1, 2019

jchavarri commented Apr 2, 2019

rusty-key commented Apr 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusty-key commented Apr 5, 2019 • edited Loading

wokalski commented Jun 24, 2019

rusty-key commented Jun 24, 2019

rusty-key commented Jul 21, 2019 • edited Loading

jfrolich commented Mar 28, 2020

wokalski commented Mar 28, 2020

rusty-key commented Feb 28, 2019 •

edited

Loading

rusty-key commented Apr 5, 2019 •

edited

Loading

rusty-key commented Jul 21, 2019 •

edited

Loading