Run a language server for each workspace

[alex88]

This fixes #69 and possibly #58

It spawns a "default" language server for open files not belonging to a workspace and a language server for each workspace.
This way each language server uses the correct workspace root folder instead of the first open workspace.

Solution found in https://github.com/microsoft/vscode/wiki/Adopting-Multi-Root-Workspace-APIs and https://github.com/microsoft/vscode-extension-samples/tree/master/lsp-multi-server-sample

The other way would be to handle the different workspaces on the language server but that is probably more complicated and this might be a viable solution in the meantime.

If you are ok with this solution I'll do more manual testing and see if I can cleanup the code a little more

[axelson]

Thanks for looking into this! I think that the language server per workspace approach is correct for now. Yes please do some more manual testing and any code cleanup. Additionally it would be great if we could move away from the deprecated ${workspaceRoot} and change "elixirLS.projectDir" to be scoped to a resource (I think that's the only configuration setting that would need to change).

[alex88]

I'll work on it this weekend as during the week I'm full of work!

[alex88]

@axelson I've rebased on master and fixed the tests, since the default language server doesn't start until the user opens a file (before it was starting on extension activation) I had to open an in-memory elixir file in the test to assert that the language server is started

[jamesblack]

@alex88 I've compiled your branch to try and help do some testing as I am currently working with a workspace that includes one javascript app and one elixir app. Is the language server supposed to start for the elixir app without elixirLS.projectDir setup? Thanks for your hard work!

EDIT: It works if you create a proper workspace (awesome job @alex88) thank you so much for this effort. I originally expected it to work based on opening a folder. Similar to ESLint in JS land my bad.

[alex88]

@jamesblack sorry for the late reply. Thanks for trying it!
How was your folder setup and what you mean with "proper workspace"? When I have multiple projects I always open the two project roots. Were you opening a subfolder? Maybe we can see how ESLint does it (maybe goes up in the path until it finds a mix.exs) and improve also that scenario

[axelson]

Trying this out locally I get an error:

This is what I have in my developer tools:

[alex88]

@axelson that's weird, did you just run the extension from vscode debug panel without having the extension already installed?
Because I've tried to launch it and open two projects and I didn't get the error.

Also the only command the extension registers is extension.copyDebugInfo not spec

I've just tested it and it seems to be working fine

if you have time just ping me on slack @alex88 if you want to do some quick tests

[alex88]

I've also just rebased on master (to catch up with 0.4.0) and it still seems to be working fine

[jayjun]

@alex88 Same error here. I created a multi-root workspace by opening a new window then File > Add Folder to Workspace foo and bar individually.

Notice how both projects have their own .elixir_ls directories. At workspace root, I have a multi.code-workspace file with

{
  "folders": [
    {
      "path": "foo"
    },
    {
      "path": "bar"
    }
  ]
}

[alex88]

Now I was able to reproduce it, my bad, I'll fix it soon

[jayjun]

Multiple language servers cannot register the same command again because VS Code needs to disambiguate which language server to send the command to.

microsoft/vscode-languageserver-node#333 (comment) suggests registering a separate command per language server then letting the user pick. For a single command that should be sent to all language servers, register it once in the extension then dispatch requests ourselves.

@axelson This change needs coordination with ElixirLS.

[alex88]

microsoft/vscode-languageserver-node#333 (comment) suggests registering a separate command per language server then letting the user pick. For a single command that should be sent to all language servers, then register it once in the extension then dispatch requests ourselves.

I was just reading that and microsoft/vscode-extension-samples#45

Just saw that elixir-ls has "executeCommandProvider" => %{"commands" => ["spec"]} which is where the command is being registered. I also saw that it's code_lens.ex that's calling that command. It's probably better to send the command only to itself instead of sending it to all language servers (as most of the times only one will have that line of code available)

[alex88]

I did a quick try to implement what is suggested here microsoft/vscode-languageserver-node#333 (comment)

Basically I've tried to generate an unique ID when initializing the language server and using that ID when declaring the spec command and when calling it.

I'll attach here the affected code:

server.ex

  defstruct [
    :id,
    :build_ref,
    ...

  defp handle_request(initialize_req(_id, root_uri, client_capabilities), state) do
    ...
    id = :crypto.strong_rand_bytes(32) |> Base.url_encode64 |> binary_part(0, 32)

    ...

    state = %{state | client_capabilities: client_capabilities, id: id}

    ...

    {:ok, %{"capabilities" => server_capabilities(id)}, state}
  end
...
  defp handle_request(code_lens_req(_id, uri), state) do
    if dialyzer_enabled?(state) and state.settings["suggestSpecs"] != false do
      {:async, fn -> CodeLens.code_lens(state.id, uri, state.source_files[uri].text) end, state}
    else
      {:ok, nil, state}
    end
  end
...
  defp server_capabilities(id) do
    %{
      ...
      "executeCommandProvider" => %{"commands" => ["spec-#{id}"]},
      ...
      }
    }
  end

code_lens.ex:

  def code_lens(id, uri, text) do
    resp =
      for {_, line, {mod, fun, arity}, contract, is_macro} <- Server.suggest_contracts(uri),
          SourceFile.function_def_on_line?(text, line, fun),
          spec = ContractTranslator.translate_contract(fun, contract, is_macro) do
        %{
          "range" => range(line - 1, 0, line - 1, 0),
          "command" => %{
            "title" => "@spec #{spec}",
            "command" => "spec-#{id}",
            "arguments" => [
              %{
                "uri" => uri,
                "mod" => to_string(mod),
                "fun" => to_string(fun),
                "arity" => arity,
                "spec" => spec,
                "line" => line
              }
            ]
          }
        }
      end

    {:ok, resp}
  end

execute_command.ex

def execute("spec-" <> _, args, source_files) do

this way it's working properly.

However as I have no experience with elixir-ls (or language servers in general as you've seen), would this be a good solution? Any potential issues?

[jayjun]

I think appending an ID is the best solution too. Just unsure about the dependency on :crypto and a nit that maybe "spec:" <> id is more Elixir-esque.

Can you send a pull request to elixir-ls? Nothing should break from this change so it can be merged first.

[alex88]

I think appending an ID is the best solution too. Just unsure about the dependency on :crypto and a nit that maybe "spec:" <> id is more Elixir-esque.

Do you have a better suggestion on what to use instead? I was searching for UUIDs but those require an external dependency too.
I'll do the PR now

[axelson]

:crypto.strong_rand_bytes is what ecto uses to generate UUID's:
https://github.com/elixir-ecto/ecto/blob/v3.4.4/lib/ecto/uuid.ex#L163

So it seems likely that it will be okay for this use-case. But there is the possibility of degradation due to a too-small entropy pool, but I'm not familiar with it to really understand the trade-offs. But I think it is okay to move forward with :crypto for now, swapping it out later should be straightforward.

[alex88]

@axelson thank you for merging the change on elixir-ls, should I update the submodule here or is it better to hold it until a new elixir-ls release?

[axelson]

@alex88 please add it to this PR, since this PR explicitly requires the update.

[alex88]

Done, tested by opening both projects and testing that the spec autocomplete works properly

[axelson]

Thanks, this is working great now! ❤️

[alex88]

@axelson thank you!

[jayjun]

Discovered ElixirLS: Copy Debug Info command is subtly broken.

To reproduce, open a multi-root workspace but do not open any Elixir files. Then trigger the command and this error appears.

Command 'ElixirLS: Copy Debug Info' resulted in an error (command 'extension.copyDebugInfo' not found)

This happened because package.json registers the command but the actual callback is only registered when the extension starts. And the extension only starts after an Elixir file is opened.

Moreover, the callback reports the global Elixir version, not the version that the language server is using. Since our issue template asks for this info, this may lead to confusing bug reports.

[alex88]

@jayjun I'll try to see what's causing this issue, however my multi-root changes only changed when to trigger the language server start, not when the extension activate() function is called (if I'm not mistaken). However the issue can be reproduced also when opening a single folder.

[alex88]

The only way to get it working on vscode startup (without having to open an elixir file) would be with https://code.visualstudio.com/api/references/activation-events#Start-up

[alex88]

Just tested commit 2909ef7 before the merge of multi workspace and the issue was already there, maybe adding:

"workspaceContains:**/*.ex"
"workspaceContains:**/*.exs"

would work to activate the extension as soon as a folder with elixir files is open?

[axelson]

I think that makes sense and would make a good change so I've opened a PR for it (#107)

[jayjun]

@alex88 You’re right, my bad. It’s a preexisting issue. Thanks for investigating anyway.

On my second point, should debug info list Erlang/Elixir versions for each running language server?

[alex88]

@jayjun I think it shouldn't, because the erlang/elixir version of the language server depends on what version the language server is compiled with when the extension is packed (I remember seeing an issue about having the server use the same elixir version of the user project but I can't find it right now).
So if you have the extension version you know already the language server version, at that point the extra information is the user's elixir version.
However I might be wrong

[jayjun]

Yes, I am referring to the user’s Elixir version for each workspace. Each language server is launched from the workspace folder so asdf can run a different version depending on ${workspaceFolder}/.tool-versions. With #103, the debugger too.

[alex88]

Mhh yeah maybe in that case the best way would be to pick the current file workspace? If no files are open have the user pick one?