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: 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
Assignees
Labels
Milestone

Comments

@ainar-g
Copy link
Contributor

@ainar-g 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 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

@gopherbot gopherbot commented Sep 10, 2020

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

@ainar-g
Copy link
Contributor Author

@ainar-g 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

@stamblerre stamblerre commented Sep 11, 2020

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 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
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.
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 11, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 11, 2020

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
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.