Avoid registering more than 1 formatting provider at a time

[ramya-rao-a]

For details on the issue see microsoft/vscode#72315

We register 2 formatting providers when the language server is enabled, but the format feature from it is disabled.

"go.useLanguageServer": true,
"go.languageServerExperimentalFeatures": {
    "format": false
}

We should avoid doing so, and un-register the provider from the language server in such scenario

[ramya-rao-a]

@stamblerre For more context, please see microsoft/vscode#72315 and
microsoft/vscode#70314 (comment).

Can we have gopls skip the registering of the formatter if the user has chosen not to have the formatting feature from gopls?

[stamblerre]

So the solution would be to always register formatting dynamically? How would this work for editors that don't support dynamic registration?

[ramya-rao-a]

I am not sure if the editor needs to be aware of the dynamic registration.

@dbaeumer, @octref Can you shed some light here?

[dbaeumer]

For editors that don't support dynamic registration there is currently no way for them to unregister. All a server can do is to register things statically then.

In general I think that a server should un-register a formatter if it gets disabled.

[stamblerre]

So based on my understanding of microsoft/vscode#70314 (comment), we should check if the client supports dynamic registration, and if not statically register. Otherwise, we should dynamically register only. Is this correct?

[ramya-rao-a]

How would you identify whether the editor supports dynamic registration or not?

[dbaeumer]

@stamblerre yes, that is corrent.
@ramya-rao-a this is advertised in the client capabilities send to the server.

[vincepri]

/cc

[vincepri]

Is there an upstream issue to track this?

[stamblerre]

Filed golang/go#32989.

[stamblerre]

@ramya-rao-a: With the fix in https://go-review.googlesource.com/c/tools/+/185244, I have started to see the duplicate formatting provider error. Is there something that needs to be changed on the VSCode-Go end before we can submit this change?

[ramya-rao-a]

Are you seeing the duplicate formatting problem even if you have opted into using the format feature from the language server i.e format is true in go.languageServerExperimentalFeatures?

If so, then add a breakpoint at https://github.com/microsoft/vscode-go/blob/0.11.3/src/goMain.ts#L248 and see if vscode-go is registering its own formatter even when you have chosen to use the one from gopls

Also, I believe gopls needs to know when to register the formatting provide and when not to. I am guessing that is where initializationOptions comes into play?

[stamblerre]

The approach we tried for gopls was the one recommended by @dbaeumer - registering dynamically, unless dynamic configuration is not supported (see https://go-review.googlesource.com/c/tools/+/185244).

Based on the linked code, I would guess the VSCode-Go extension needs some additional logic to de-register the default formatting provider if the language server's formatting provider is dynamically registered. In this instance, it registers both providers because the language server sends back false for the DocumentFormattingProvider in the server capabilities.

[vincepri]

The other question I have is how can we handle the case when format is set to false instead. Can we let users to use goimports/goreturns if they wish to do so? Should gopls skip the dynamic registration in this case?

[ramya-rao-a]

@stamblerre The Go extension registers the provider because gopls returns true for the DocumentFormattingProvider in the server capabilities

Edit: The Go extension registers its provider as it sees format is set to false in go.languageServerExperimentalFeatures.

My settings:

"go.useLanguageServer": true,
"go.languageServerExperimentalFeatures": {
    "format": false
  }
"[go]": {
    "editor.insertSpaces": false,
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.organizeImports": false
    }

And I have updated to the latest gopls

[vincepri]

@ramya-rao-a Can you try to use this patch https://go-review.googlesource.com/c/tools/+/185244/5/internal/lsp/general.go to see if the behavior is what you'd expect?

[ramya-rao-a]

Apologies, the assessment in my earlier comment was wrong, I have crossed it out and updated the scenario where the vscode-go extension would register its formatter which is either when

• format is set to false in go.languageServerExperimentalFeatures.
• Or if the language server has not set registered a formatting provider.

Currently, the problem is not that the vscode-go extension is registering the duplicate formatter. The problem is that gopls is not reading go.languageServerExperimentalFeatures to determine whether or not to register the formatter.

The microsoft/vscode#70314 (comment) only talks about half the issue i.e how to unregister a provider

gopls still has to make the call that the user has chosen not to use its formatting feature and so should then either not register its formatter or un-register it

[stamblerre]

Can't VSCode-Go do the configuration handling on its end? Doesn't this handling happen here:

vscode-go/src/goMain.ts

Line 144 in 4b516f9

provideDocumentFormattingEdits: (document: vscode.TextDocument, options: FormattingOptions, token: vscode.CancellationToken, next: ProvideDocumentFormattingEditsSignature) => {

?

[stamblerre]

I've been taking a look at this from the VSCode end, and I'm a bit stuck on how this should behave. I thought that the extension could register a formatting provider if the server didn't return DocumentFormattingProvider in its capabilities, and then dispose of that provider if the server dynamically registered formatting.

I tried to implement this via the client.onRequest function, but it seems like that doesn't behave like middleware, and there's no way to actually accept the registration from the language server when using that function. There also doesn't seem to be any way to reject the dynamic formatting registration request when it's sent by the language server or any way to tell the language server not to register formatting.

It's possible I'm completely misunderstanding something, but @dbaeumer's comment (microsoft/vscode#70314 (comment)) shows the behavior of the language server, whereas what we need to do is dispose of a provider when a different provider gets registered.

[ramya-rao-a]

I thought that the extension could register a formatting provider if the server didn't return DocumentFormattingProvider in its capabilities, and then dispose of that provider if the server dynamically registered formatting.

How would gopls decide whether or not to register the formatter if it is not able to read the go.languageServerExperimentalFeatures setting?

[stamblerre]

I had hoped that there would be some way for the client to either accept or reject the registration from the language server, but it seems like that's not the case. It seems pretty counter-intuitive to me that the language server should have to be passed settings for which features to register, since to my mind, that's what server and client capabilities are for.

This basically means that, if we want to allow the features of the language server to be configured, we have to expose a setting for each of these features to be enabled or disabled. I'm starting to think that perhaps the language server should really be all-or-nothing--it will be extremely hard to debug if each user has a different configuration. The experience with the language server should be as simple as turning it on.

My proposal is that we remove the ability to configure these features entirely. I understand that there will be friction, as some users have different preferences. Ideally, this will cause people to file more feature requests, and then the language server can be improved to fit the users' needs. These configurations are just masking the problem.

/cc @ianthehat

[stamblerre]

I think that the ultimate resolution to this issue would be to remove the go.experimentalLanguageServerFeatures setting for formatting so that users of gopls would be required to use gopls's formatting. I believe the that the main concern with this would be that gopls doesn't provide the features of goreturns, which some users prefer as a format tool. A solution would be for gopls to support features offered by goreturns (perhaps as code action quick fixes).

@ramya-rao-a: Are there any other concerns here?

[ramya-rao-a]

I am ok with the approach of removing format from go.experimentalLanguageServerFeatures.
The only concern is that we should resolve the formatting issues that people have currently reported before doing that.

[stamblerre]

Absolutely. We can wait on it until such a time as there are no issues with gopls formatting.

[stamblerre]

The majority of the issues with gopls formatting seem to have been resolved, and gopls will have goreturns-type functionality in the next release (https://golang.org/cl/224960). Do you think we can remove this setting in the next version of the extension (meaning 0.15, not 0.14 since that will be coming out soon)?

If we're ready to that, I can add that to #3156.

[ramya-rao-a]

Sure, that works

[ramya-rao-a]

Closing this issue as with #3156 in the upcoming version of the extension, we drop the support for falling back to the extension's format feature when using the language server. So, there wouldnt be multiple formatting providers being registered at the same time