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

Jump to bottom

x/tools/gopls: didChangeConfiguration ignores the new configuration #41311

Closed
ainar-g opened this issue Sep 10, 2020 · 6 comments
Closed

x/tools/gopls: didChangeConfiguration ignores the new configuration #41311

ainar-g opened this issue Sep 10, 2020 · 6 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.5.0

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Sep 10, 2020

I was trying to enable staticcheck using gopls version v0.5.0-pre1 and natebosch/vim-lsc, but couldn't. The plugin configuration was:

let g:lsc_server_commands = {
\   'go': {
\     'name': 'gopls',
\     'command': 'gopls serve',
\     'log_level': -1,
\     'suppress_stderr': v:true,
\     'workspace_config': {
\       'staticcheck': v:true,
\     },
\   },
\ }

I also used -rpc.trace and made sure that gopls actually receives the configuration with the workspace/didChangeConfiguration method. I inspected the code, and it seems like parameter changed of the server implementation is currently ignored:

func (s *Server) didChangeConfiguration(ctx context.Context, changed interface{}) error {
	// go through all the views getting the config
	for _, view := range s.session.Views() {
		options := view.Options()
		if err := s.fetchConfig(ctx, view.Name(), view.Folder(), &options); err != nil {
			return err
		}
		view, err := view.SetOptions(ctx, options)
		if err != nil {
			return err
		}
		go func() {
			snapshot, release := view.Snapshot(ctx)
			defer release()
			s.diagnoseDetached(snapshot)
		}()
	}
	return nil
}

If I enable stderr logging and add a log.Printf("%#v", changed) there, I can see, that the parameters are there:

[lsc:Error] StdErr from gopls: 2020/09/10 12:19:36 &protocol.DidChangeConfigurationParams{Settings:map[string]interface {}{"staticcheck":true}}

Is this a bug? Is there any additional information I should add?
@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 Sep 10, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 10, 2020
@stamblerre stamblerre self-assigned this Sep 10, 2020
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.5.0 Sep 10, 2020
@gopherbot
Copy link

Change https://golang.org/cl/254038 mentions this issue: internal/lsp: handle staticcheck in didChangeConfiguration

@ainar-g
Copy link
Contributor Author

ainar-g commented Sep 11, 2020

@stamblerre Thanks for looking into this so quickly! I applied the patch in the CL (patch set 5), but that didn't solve the issue. options.Staticcheck remains false before and after s.fetchConfig as well as at the end of updateAnalyzers, so no analysis is run. Am I missing something? (And is it better to leave such comments on the CL itself?)

@stamblerre
Copy link
Contributor

Thanks for testing it out! It's fine/probably easier to leave comments here.
Do you see the second workspace/configuration method in the log when fetchConfig runs for the second time, and does it return "staticcheck": true? I only tested with VS Code, but it fixed the issue there.

@ainar-g
Copy link
Contributor Author

ainar-g commented Sep 11, 2020

@stamblerre Ah, I see what's wrong now. vim-lsc assumes, that the server just accepts the configuration that the client sent with didChangeConfiguration. It doesn't know or respond to configuration. I cobbled together a patch to vim-lsc that fixes that, and voilà, staticcheck works now. I'll send the patch to the maintainers of that client. Sorry for the noise!

ainar-g added a commit to ainar-g/vim-lsc that referenced this issue Sep 11, 2020
@ainar-g
Add workspace/configuration handling
6a4e489 
After receiving workspace/didChangeConfiguration some servers will
request the full updated configuration from the client using
workspace/configuration instead of reading the changed configuration
section clients have sent them.  So, support this capability and respond
with a workspace configuration if there is one.

See also: golang/go#41311.
@ainar-g ainar-g mentioned this issue Sep 11, 2020
Merged
@stamblerre
Copy link
Contributor

Perfect--thanks for doing that! And definitely not noise - there was a gopls bug here too.

@gopherbot
Copy link

Change https://golang.org/cl/254427 mentions this issue: [gopls-release-branch.0.5] internal/lsp: handle staticcheck in didChangeConfiguration

gopherbot pushed a commit to golang/tools that referenced this issue Sep 13, 2020
@stamblerre
[gopls-release-branch.0.5] internal/lsp: handle staticcheck in didCha…
796b347 
…ngeConfiguration

As we have modified the ways that we control which analyzers get
executed for a given case, we have lost the behavior of enabling and
disabling staticcheck smoothly. This CL splits out the staticcheck
analyzers from the main group so that the "staticcheck" setting can
override whether or not a given staticcheck analysis is enabled.

Fixes golang/go#41311

Change-Id: I9c1695afe4a8f89cd0ee50a79e83b2f42a2c20cb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254427
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@golang golang locked and limited conversation to collaborators Sep 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.5.0
Development

No branches or pull requests
3 participants
@ainar-g @stamblerre @gopherbot