vscode-ocaml-platform

vscode-ocaml-platform copied to clipboard

We are currently experimenting with separate Melange and OCaml libraries and place them in different Dune contexts, in order to improve the developer experience (see https://github.com/melange-re/melange/discussions/694).

Under this multi-context setup, it would be useful as a user to select which Dune context to use in VSCode for code definitions, types and errors.

We are putting out this proposal to gather feedback from maintainers, and if accepted, make sure the implementation plan is ok.

Current status

It is currently possible to choose the context by editing dune-workspace files and adding (merlin) to the context we want to work on, but this has a few downsides:

• It requires to manually edit dune-workspace every time one wants to switch context
• Can lead to unwanted changes in this file when using git or other source versioning tools
• There is some bug where dune ocaml-merlin won't pick up the new context straight away, so it shows errors like :No config found for file foo.ml. Try calling 'dune build' until one restarts the whole VSCode window

Proposal

To improve the experience, the proposal is to add a new command select-context to this extension, which will show users a list of contexts to choose from, and record their choice in vscode settings (similar to the current experience with sandbox selection in select-sandbox).

Implementation

For this to happen, besides the new VSCode command select-context, we will have to add a few other pieces in ocaml-lsp-server and Dune's ocaml-merlin:

ocaml-lsp-server

• Add a new request ocamllsp/getDuneContexts to the lsp server. This request will resolve to calling ocaml-merlin behind the scenes (see below)
• Add a new handling logic in didChangeConfiguration for a newly added duneContext config. This handling will forward the change to ocaml-merlin (see below)

ocaml-merlin

• Add new command GetContexts that returns a list of available contexts in the current workspace, so that they can be sent through ocamllsp/getDuneContexts and shown in a list inside VSCode extension UI
• Add a new command SetContext that allows to define the current context, which will be called from ocaml-lsp-server didChangeConfiguration handler

---

Does the above plan make sense? Thanks!

I'm wondering if you've considered implementing this via command line arguments, and respawning the ocamllsp when context changes:

• add a set-context command to extension
• add --dune-context CONTEXT option to ocamllsp
• add --context CONTEXT option dune ocaml ocaml-merlin

that way, when one changes context in editor, a new ocamllsp process will be spawned with different command line arguments

this won't require patches to merlin as I understand

@andreypopp Thanks for the suggestion, I didn't consider that. Yes, I think that would avoid touching merlin because essentially we would be sidestepping the additions to the protocol by moving them to the command line flag.

Do you know if restarting the ocamllsp process often would have any downsides? (I understand the ocaml-merlin process is also killed/restarted once the lsp is up again as well).

Also, I wonder if the suggestion in https://github.com/ocaml/dune/pull/10324#issuecomment-2034599181 would be somehow related or affected by this decision. If we ended up handling context switching (and merlin config) through Dune RPC, then I assume no patches to merlin would be necessary either.

Do you know if restarting the ocamllsp process often would have any downsides? (I understand the ocaml-merlin process is also killed/restarted once the lsp is up again as well).

Maybe we shouldn't even kill/restart it on switch but keep the processes working? e.g. on start we start ocamllsp for the default context, once set-context is used, we start another instance.

In the future, we can even consider showing diagnostics from several ocamllsp processes at once (though need to think through it, not to overwhelm a user with multiple similar errors).

Also, I wonder if the suggestion in https://github.com/ocaml/dune/pull/10324#issuecomment-2034599181 would be somehow related or affected by this decision. If we ended up handling context switching (and merlin config) through Dune RPC, then I assume no patches to merlin would be necessary either.

No idea, but if we go the cli configuration way then I think dune ocaml ocaml-merlin changes will be simpler.

By the way, if we are going to allow melange context to have a different compiler version than default context, then we definitely should start a separate ocamllsp/merlin instance for different contexts. As I understand now merlin works only with a single OCaml version once it is installed into the switch.

e.g. on start we start ocamllsp for the default context, once set-context is used, we start another instance.

I understand ocaml-lsp is quite heavy on memory, not sure how spanning two instances would work out? I guess we should measure the impact after implementing this idea.

By the way, if we are going to allow melange context to have a different compiler version than default context, then we definitely should start a separate ocamllsp/merlin instance for different contexts.

Good point. I wasn't considering this case at all, just because Ahrefs use case is within a single opam switch. But considering Dune contexts can exist on different switches, that looks like the way to go.

Following the discussion in https://github.com/ocaml/dune/pull/10324 , adding a --context CONTEXT flag to dune ocaml-merlin has the advantage of not requiring changes to the protocol. This means in particular that no modification have to be made to the unrelated dot-merlin-reader binary. It should be a simpler and less invasive approach.

As I understand now merlin works only with a single OCaml version once it is installed into the switch.

That's correct.

After some discussions with @andreypopp, and with vscode-ocaml users at Ahrefs (@davesnx), we are evaluating alternative implementations to the solution proposed in #1449.

The main limitation of the context selection in #1449 is that it is global and only one context (LSP server) can be active at any given time. So in a project where two or more contexts exist, and libraries or executables can be defined on one or the other, global selection is problematic because there will always be files in the project that don't belong to the chosen context. These files will show broken errors when the selected context is not the right one, which is quite a bad experience.

To mitigate this, we could go with an alternate solution, where users can define multiple language server configurations and assign them to specific folders in the workspace. This could be implemented as an extension of the existing ocaml.server.args where, besides taking a list of strings, it can also take a list of objects, where each object has the shape {folder, args}. For example:

{
  "ocaml.server.args": [
    {
      "dir": "frontend",
      "args": ["--context", "melange"]
    },
    {
      "dir": "backend",
      "args": ["--context", "default"]
    }
  ]
}

Then, when opening a document, the extension would:

• get the path to the document
• iterate over this list and try to find a match
• if there's a match, and the server and client for that match haven't been started, start them
• the client will include settings to only deal with files under a given pattern based on the dir property, e.g.:

let client_options () =
  let documentSelector =
  LanguageClient.DocumentSelector.
    [| language ~pattern:"melange/**/*" "ocaml"
      ; language ~pattern:"melange/**/*" "ocaml.interface"
      ; language ~pattern:"melange/**/*" "ocaml.ocamllex"
      ; language ~pattern:"melange/**/*" "ocaml.menhir"
      ; language ~pattern:"melange/**/*" "reason"
    |] in ...

This way, we could also remove the need for a specific duneContexts command to get the contexts, as it'd be users who would have to manually keep this new configuration up to date when Dune contexts change.

Would this be a better approach and is ocaml.server.args the right place to put it? Are there any potential improvements or cases that are not being considered? Thanks!

Would this be a better approach and is ocaml.server.args the right place to put it? Are there any potential improvements or cases that are not being considered? Thanks!

Are there any other server arguments that would have to change for each directory?

If not, something like this could be simpler:

{
  "ocaml.server.contexts": {
    "frontend": "melange",
    "backend": "default"
  }
}

Thanks for the input @mnxn. After evaluating more carefully the folder layouts we have internally, I am thinking the solution I was suggesting (either with ocaml.server.args or ocaml.server.contexts) will be pretty cumbersome, because in complex projects there are all sorts of folder nesting, with some subfolders belonging to one context or another, at any level. Trying to cover all these cases using folder strings (or glob patterns) will be very error prone, and I am not sure about the performance consequences of keeping 10+ LSP clients & servers active.

Besides, the changes to the extension to support multiple LSP clients + servers seems a bit convoluted too.

Instead, I think for a first version we can let users choose context using one of these options:

a) Single workspace: switch context using the select-context command added in #1449. b) Per-context workspace: if switching contexts gets too annoying, there's the possibility to define a new VSCode workspace for a specific context, e.g. in a melange.code-workspace file, and then set the context configuration over there:

"settings": {
  "ocaml.dune.context": "melange"
}