Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: create a new snapshot on didChangeConfiguration #42814

Closed
stamblerre opened this issue Nov 24, 2020 · 8 comments
Closed

x/tools/gopls: create a new snapshot on didChangeConfiguration #42814

stamblerre opened this issue Nov 24, 2020 · 8 comments
Labels
gopls/metadata Issues related to metadata loading in gopls gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@stamblerre
Copy link
Contributor

It would probably simplify our logic to treat this as an invalidation event, especially since a change in environment leads to metadata invalidations.

@stamblerre stamblerre added this to the gopls/unplanned milestone Nov 24, 2020
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Nov 24, 2020
@stamblerre stamblerre added this to To Do in gopls on-deck Feb 28, 2021
@krobelus
Copy link

krobelus commented Aug 8, 2021

I'm not sure if this is the same issue: I expect that settings sent via didChangeConfiguration are applied immediately

For example, given this file

package main

func main() {
	println()

}
  1. I start goplswith no configuration
  2. I send {"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"formatting.gofumpt":true}}}
  3. I send {"jsonrpc":"2.0","method":"textDocument/formatting", ...}
  4. Nothing happens; I expect the empty line after pringln() to go away.

It works when I restart the language server, or when I use workspace/configuration instead.
I don't think a restart should be necessary, and not all clients (want to) implement workspace/configuration. For example it doesn't really make sense for the client I'm using - https://github.com/kak-lsp/kak-lsp - some implementation reasons prevent mult-workspace-in-single-editor-session for now.


Tangentially related: my main problem with workspace/configuration is that it needs section headers. To send "formatting.gofumpt", the user needs to specify {"gopls":"formatting.gofumpt"}. OTOH "formatting.gofumpt" already works when sent as initialization option. I'd like to use a single config object for both, so the user doesn't have to care about protocol details. In the config system of kak-lsp, there is one object for each language, so I'm having a hard time understanding the need for the section header.

@stamblerre
Copy link
Contributor Author

@krobelus: Your topic is actually a separate issue, but I can answer it briefly. For some reason, VS Code doesn't actually send the modified settings in its didChangeConfiguration request, so we rely on workspace/configuration to get the updated values. We also use workspace/configuration to get the settings at initialization.

You shouldn't need section headers for workspace/configuration. Here is an example request/response from VS Code:

[Trace - 17:08:47.027 PM] Received request 'workspace/configuration - (2)'.
Params: {"items":[{"scopeUri":"file:///Users/rstambler/code/src/golang.org/x/tools","section":"gopls"}]}


[Trace - 17:08:47.033 PM] Sending response 'workspace/configuration - (2)' in 5ms.
Result: [{"verboseOutput":true,"ui.diagnostic.analyses":{"fillreturns":true},"ui.semanticTokens":true,"experimentalUseInvalidMetadata":true,"ui.diagnostic.staticcheck":true,"ui.completion.experimentalPostfixCompletions":true,"ui.completion.usePlaceholders":true,"build.experimentalTemplateSupport":true,"build.experimentalWorkspaceModule":true}]

@krobelus
Copy link

Thanks that's good to know. It looks like gopls accepts the same options in both didChangeConfiguration and workspace/configuration which makes things simpler, but other servers might not.

@findleyr
Copy link
Contributor

This is a persistent source of bugs. We should fix in the next release.

@gopherbot
Copy link

Change https://go.dev/cl/524837 mentions this issue: gopls/internal: move Options and FileKind from View to Snapshot

gopherbot pushed a commit to golang/tools that referenced this issue Sep 1, 2023
In preparation for making snapshots truly idempotent, move certain
methods that depend on settings into the snapshot. Nothing should access
settings through the View.

For golang/go#42814

Change-Id: Ib3ced985dc89515f5a6e6049c7be316342706e54
Reviewed-on: https://go-review.googlesource.com/c/tools/+/524837
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/526160 mentions this issue: gopls/internal/lsp/cache: move workingDir to workspaceInformation

@gopherbot
Copy link

Change https://go.dev/cl/526417 mentions this issue: gopls/internal/lsp/cache: move Option management to the Server

@gopherbot
Copy link

Change https://go.dev/cl/526159 mentions this issue: gopls/internal/lsp: move options into the snapshot

gopherbot pushed a commit to golang/tools that referenced this issue Sep 7, 2023
Any change to the working dir must necessarily result in a new view (see
minorOptionsChange). Therefore, it should be considered an immutable
part of the view, and rename it to goCommandDir, which more closely
matches its meaning (and there are various other things that could be
considered "working" dirs).

Also store the folder on the workspaceInformation, to move toward an
immutable foundation that can be safely shared with the snapshot.

Updates golang/go#57979
For golang/go#42814

Change-Id: I9a9e3aa18fa85bace827d9c8dd1607e851cfcfb8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/526160
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 8, 2023
In order to move toward tracking options by Folder, not view, move them
into the Server. This will also help us fix bugs related to
configuration lifecycle events.

For golang/go#57979
Updates golang/go#42814

Change-Id: Id281cad20697756138a7bdc67f718a7468a04d4a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/526417
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/metadata Issues related to metadata loading in gopls gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
No open projects
Development

No branches or pull requests

4 participants