[BSP] - Support for workspace/reload

[ckipp01]

Feature Request

A part of the BSP spec that doesn't seem to be implemented yet in the Mill BSP is support is the workspace/reload request. This is useful for editors like Metals so when the user edits their build file, we can detect it and send a reload request for the server.

[lefou]

I'm not sure, we can implement it in the current design, as the whole BSP server is implemented as an opaque mill command. Mill very well supports reloading (with it's --watch feature), in fact it will detect the need for it in most cases automatically, but only after the command execution finished. So, IMHO, a proper implementation of BSP workspace/reload might be to just finish the BSP/start target. But if we run mill with --watch, we might see unexpected output in between, also stopping the BSP/start target might be a bit harder. But without --watch I don't know if a BSP client is expected to handle a stop of the server that gracefully, e.g. just restart it. If not, we probably need something in between mill invokations without --watch and with --watch. Or we might come up with a dedicated start option / entry point in mill.

[ckipp01]

So, IMHO, a proper implementation of BSP workspace/reload might be to just finish the BSP/start target

I'm not extremely familiar with all the mill terms, so my apologies if I get some of the lingo wrong. But in this case, by finish the BSP/start target do you mean terminating the BSP connection?

But if we run mill with --watch, be might see unexpected output in between

Depending on what you mean by unexpected output here it's important to remember that all the communication during the connection must be able to be parsed as JSON-RPC, if not you run into all sorts of issues. This happened with sbt here, and it caused the server not work correctly with neither IntelliJ or Metals with certain versions. Again, it depends on what you mean by output here, but just make sure any weird output isn't piped into the actual BSP communication that isn't part of the spec.

But without --watch I don't know if a BSP client is expected to handle a stop of the server that gracefully, e.g. just restart it

Well the client will handle the lifecycle of the the connection, and end with a request to shutdown and then exit as outlined here. As for a restart, not really part of the BSP spec, but in the Metals case we have a build-restart command that we expose that a LSP client can trigger to shutdown and restart a BSP connection. However, I wouldn't use this as an alternative to workspace/reload as we only recommend to use it when you're having issues with the connection and want to restart it.

Mainly, I think it's helpful to think of it in terms of flow of how it will be used with a client. Again from a Metals perspective I'll outline what we'd like to ideally have happen.

1. BSP connection made with whatever is in .bsp/mill.json
2. User edits the build.sc
3. Metals recognizes that the build definition has changed and then triggers a workspace/reload
4. User just keeps on editing like normal

I think however Mill implements stage 3 it's just important (if possible) to not rely on any special logic in the client specific to Mill, but simply stay inline with the spec. That's probably easier said than done, but it makes it way easier for clients to deal with various BSP servers.

[lefou]

Thanks for your comment. As an outcome, I think we should not implement our BSP support as a T.command. To better control the lifecycle we might provide a dedicated entry-point (or cmdline option).

[srdo]

I think that's a good idea. That will also allow us to better control what goes on stdout, since as a Command, there will be some time where Mill might log to stdout before reaching the BSP code (e.g. Compiling build.sc)

[lefou]

I started a draft pull request: #1093

[lefou]

Forget about #1093. I already closed it.

I largely refactored Mill BSP support and it now supports a new mode of operation, which also support workspace/reload. It's all still in development, but I'd like to encourage the brave, to checkout and test it. See PR #1536. Any kind of feedback is greatly appreciated!

[lefou]

@ckipp01 How does Metals know, that an edition of build.sc should trigger a workspace/reload? I didn't find any reference in BSP that I could mark a target as "workspace source".

[ckipp01]

Hey, good question. There is a couple things behind the scenes happening that Metals uses to trigger this. Basically in LSP world when a user makes a change to any file and saves it that sends a textDocument/didSave notification to Metals:

https://github.com/scalameta/metals/blob/12c6268ffff8cbaa4ad77790ce8aadcef5d93fa5/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1161-L1177

After that if you follow it a bit you see that this will eventually get called and it will do a check to see if the file is a build related file:

https://github.com/scalameta/metals/blob/12c6268ffff8cbaa4ad77790ce8aadcef5d93fa5/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L2527-L2536

If it does recognize it as a build file, it then calls slowConnectToBuildServer which will check to see if the file actually changed or not and if so, then either restart the build server (with Bloop this means re-import to pick up the changes and then re-starting Bloop), but with Mill for example it'd fall into this here:

https://github.com/scalameta/metals/blob/12c6268ffff8cbaa4ad77790ce8aadcef5d93fa5/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L2035-L2036

Then this will actually trigger the reload:

https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L2538-L2592

[lefou]

@ckipp01 Thanks for the insights. After looking into BuildTools sources, the educated guessing whether a source change is related to the build system and if it will affect the workspace is a clear sign that it should be moved into BSP. That way the decision is moved where it belongs, into the build tool. E.g. looking into the logic of MavenBuildTool, it would always fail for all Polyglot Maven projects. For Mill. it will probably pick up to much files.

[lefou]

FTR, I opened build-server-protocol/build-server-protocol#235.

[ckipp01]

I think in part it's due to the first build server for BSP being Bloop, and there is really no way for Bloop to know what those files are, since in reality it knows nothing of your "build" files, but only the structure via the json it reads up. So it'd never be able to tell the client "hey, if any of these files change you should reload", hence why this has always been a request from client -> server.

With sbt now having BSP support, Mill as well, it might make sense to dive deeper into this to see if something like this could be done.