Open jchavarri opened 5 months ago
I'm wondering if you've considered implementing this via command line arguments, and respawning the ocamllsp when context changes:
set-context
command to extension--dune-context CONTEXT
option to ocamllsp
--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:
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"
}
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:dune-workspace
every time one wants to switch contextocaml-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 windowProposal
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 inselect-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'socaml-merlin
:ocaml-lsp-server
ocamllsp/getDuneContexts
to the lsp server. This request will resolve to callingocaml-merlin
behind the scenes (see below)didChangeConfiguration
for a newly addedduneContext
config. This handling will forward the change toocaml-merlin
(see below)ocaml-merlin
GetContexts
that returns a list of available contexts in the current workspace, so that they can be sent throughocamllsp/getDuneContexts
and shown in a list inside VSCode extension UISetContext
that allows to define the current context, which will be called from ocaml-lsp-serverdidChangeConfiguration
handlerDoes the above plan make sense? Thanks!