Metals: Towards no longer needing to set the `metals.client` property

[ckipp01]

This has sort of been an ongoing conversation offline and also via pull
requests. Metals used to be fully configured via server properties. However, some
of this started to shift to clientExperimentalCapabilities in
https://github.com/scalameta/metals/pull/1414, and then further shifted to be
more focused on initializationOptions in
https://github.com/scalameta/metals/pull/1626 and
https://github.com/scalameta/metals/pull/1684. This further aligns with the LSP
spec and also makes bootstrapping the server easier. The configuration of the
server now belongs to the client. With nvim-metals as a test, I was able to
fully configure a client without needing to set the metals.client property.
Moving forward, I think this is a good goal to strive for no matter the client.
In order to make this happen there is a few things I'd like to work on that I've
split up into 3 stages.

Stage 1 - Migrate existing properties to initializationOptions

Below are all of the properties that are not either part of the
initializationOptions or clientExperimentalCapabilities.

The following properties are ones that we would need the client to be able to
set in some way. I think they make sense to migrate over, and it won't be a
ton of work.

• globSyntax
• ~isExitOnShutdown~ this one was already in there and I missed it 😬
• renameFileThreshold
• icons
• signature-help.command (This is in compilers, so at first thought I thought this was part of the InitializationOptions.compilerOptions, but it's actually not. So I'll add this in there.
• completion.command (Same as above)
• override-def-format

These are ones that exist currently as properties and should remain only to be
set as properties since they aren't "normally" set and probably shouldn't be set
unless you _really_ want to do a deep dive into Metals or are an advanced user.
I don't think it makes sense to have the client set these.

• isVerbose
• isAutoServer
• remoteTimeout
• askToReconnect
• statistics
• allowMultilineStringFormatting
• pc.debug

The following two aren't used anywhere in the code base that I could see. I
propose that we remove them.

• isWarningsEnabled
• bloopEmbeddedVersion

~The final one which I'm unsure what to do with is below. I personally don't feel
like this should be set as a property or really set at all by the client. The
only place we are using this is to set the serverCapabilities. I don't think
it really makes sense for a client to affect what the server says it supports. I
don't know the full story behind this setting, but I think it goes a bit against
the grain of how the initialize process should work.~

• ~allowMultilineStringFormatting~

Stage 2 - Changes to metals-language-client, metals-vscode, and coc-metals to not set metals.client.

Just to ensure that this all works, the plan is to make the necessary changes in
metals-language-client, which should just be changing a function, and then
changing each client to set the necessary initializationOptions.

State 3 - Write a blog post about configuring a client

If the above two goes well, I plan on writing a more technical focused post for
the site that details how we used to configure Metals, the reason we migrated to
initializationOptions, and then show some examples of how to configure a
client. Part of this process will also be ensuring the documentation is up to
date for initializationOptions and properties.

Hopefully this outlines the plans well enough and gives a bit of insight into
the changes that will be coming. Feel free to chime in about anything that
doesn't make sense or mention something I may be missing.

• [x] Stage 0 (#1898)
• [x] Stage 1 (#1903) (#1914)
• [x] Stage 2 (https://github.com/scalameta/metals-languageclient/pull/149) (https://github.com/scalameta/coc-metals/pull/257) (https://github.com/scalameta/metals-vscode/pull/353)
• [x] Stage 3 (#1940)

EDIT: If I end up coming across new things or figuring stuff out I didn't realize before I'll be putting them here.

[ayoub-benali]

Please ping me when reaching stage 2 for https://github.com/scalameta/metals-sublime

[tgodzik]

allowMultilineStringFormatting is the issue we were discussing with Emacs and range formatting. This should not be moved to the client, not unless there is a consensus that everyone wants to use the default c style formatting. In that case we could have a client property hasDefaultRangeFormatting

As for bloopEmbeddedVersion and isWarningsEnabled I agree that we can remove them

[ckipp01]

allowMultilineStringFormatting is the issue we were discussing with Emacs and range formatting.

Ahhh, of course. I totally forgot about that. Yea, sounds good, I'll leave that one as a property.

[ckipp01]

So I started shifting around some documentation in #1903 but realized pretty quickly that I was either going to duplicate a lot of descriptions or really have to refactor the way we list properties etc. So before I go ahead an completely redo that section, I want to get a ✅ on what I'm planning on doing before doing it.

Currently if you look on this page we have a section that lists all the Metals server properties. The majority of these are all duplicated in InitializationOptions now. There is also a small section under the Language Server Protocol initialize section where you now see the InitializationOptions. I'd like to instead of having a Metals Server Properties section that lists what the setting is and how it can be configured. So basically this:

---

Metals server properties

...

-Dmetals.glob-syntax

Controls the glob syntax for registering file watchers on absolute directories.
Registration happens via client/registerCapability for the
workspace/didChangeWatchedFiles method, if
the editor client supports it.

Possible values:

• uri (default): URI-encoded file paths, with forward slash / for file
  separators regardless of the operating system. Includes file:// prefix.
• vscode: use regular Path.toString for the absolute directory parts (/ on
  macOS+Linux and \ on Windows) and forward slashes / for relative parts.
  For example, C:\Users\IEUser\workspace\project/*.{scala,sbt,properties}.
  This mode is used by the VS Code client.

Will become this:

Metals Configuration

...

Glob Syntax

Controls the glob syntax for registering file watchers on absolute directories.
Registration happens via client/registerCapability for the
workspace/didChangeWatchedFiles method, if
the editor client supports it.

How is this configured:

• globSyntax in InitializationOptions
• Dmetals.glob-syntax as a server property

Possible values:

• uri (default): URI-encoded file paths, with forward slash / for file
  separators regardless of the operating system. Includes file:// prefix.
• vscode: use regular Path.toString for the absolute directory parts (/ on
  macOS+Linux and \ on Windows) and forward slashes / for relative parts.
  For example, C:\Users\IEUser\workspace\project/*.{scala,sbt,properties}.
  This mode is used by the VS Code client.

---

Then I don't need to worry about duplication. Thoughts on this?

[ckipp01]

@ayoub-benali just pinging you to let you know that everything needed to fully configured via InitializationOptions is now merged in. 👍

[ayoub-benali]

if I understand https://github.com/scalameta/metals-languageclient/pull/149 correctly it means the vscode and vim client packages will be working only with recent metals snapshot releases because they drop the metals.client flag.

Meaning the users will get their editor package updated without knowing that they have to dump metals version to the latest snapshot, which could cause issues.

Or is the plan to wait for next major Metals release ?

[ckipp01]

if I understand scalameta/metals-languageclient#149 correctly it means the vscode and vim client packages will be working only with recent metals snapshot releases because they drop the metals.client flag.

Meaning the users will get their editor package updated without knowing that they have to dump metals version to the latest snapshot, which could cause issues.

Or is the plan to wait for next major Metals release ?

Well the changes for the plugins won't actually get merged in/versioned until we release the next stable version of Metals. So it won't cause people to use a snapshot. However, you do bring up a good point though because with those changes, if the editor is no longer setting the metals.client, and someone updates their extension after the next release, but doesn't update metals, it may not configure correctly with old versions of Metals. So we actually lose a bit of comparability there. How reasonable is it that we expect users to use the latest stable if they update their extension after the next release? If not, we'll still need to set the client in the case that someone is using an older version of metals. Thoughts?

EDIT: I changed this a bit and instead of just fully removing the possibility of setting the metals.client flag, I just made it optional. That way we can just leave it set as it doesn't harm anything since if the InitializationOptions equivalent it set, that takes precedence. That way with the new version of the extension both old versions of Metals and new versions will both work without problem.

[kpbochenek]

How is this configured:

    globSyntax in InitializationOptions
    Dmetals.glob-syntax as a server property

If we want users to use InitializationOptions as preferred way to configure this setting maybe we should not mention system properties at all here?

Yes metals.client for me should change to optional and in future when we are sure no clients rely on that we can remove that.
We can say it is moved to @Deprecated land :)

[ckipp01]

If we want users to use InitializationOptions as preferred way to configure this setting maybe we should not mention system properties at all here?

Personally I'm all for removing the server properties that are used for configuring and only leave the ones that aren't set somewhere else. Is that alright?

[ckipp01]

As of #1940 this should all be good to go! If anyone stumbling across this thinks anything is miss or something isn't clear, we can revisit. For now I'm going to go ahead and close this.