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: dynamic configuration overwrites env in multi-workspace sessions #39592

Closed
findleyr opened this issue Jun 15, 2020 · 1 comment
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

Comments

@findleyr
Copy link
Contributor

There are several places in the gopls codebase that assume a source.Options may be cloned by passing it by value. Perhaps this was once correct, but isn't correct anymore, as for example UserOptions.Env is a slice.

One place where this shows up is in addView, which calls session.Options() and then passes that into session.NewView. By passing around the session.options.UserOptions.Env slice, each view can end up mutating the same underlying array when appending to the slice during fetchConfig.

Another place this manifests is in didChangeConfiguration. This fetchConfig call, which is supposed to just fetch the new view options, actually mutates the existing view options:
https://cs.opensource.google/go/tools/+/master:internal/lsp/workspace.go;l=49;drc=9778d966fa8150a8a0027e8169749bae53a2ee86
Later on, we check if there were any major changes, and fail to detect a UserOptions.Env change:
https://cs.opensource.google/go/tools/+/master:internal/lsp/cache/view.go;l=191;drc=9001f16f58f90e85dd6b3c2b727e3ba0d4f85838

We should explictly clone source.Options before mutating it. Note this would also make it safe to pass around the (quite large) options type by reference, and/or make it opaque.

cc @stamblerre @heschik

@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 Jun 15, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jun 15, 2020
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.5.0 Jun 15, 2020
@stamblerre stamblerre modified the milestones: gopls/v0.5.0, gopls/v1.0.0 Jul 22, 2020
@stamblerre stamblerre assigned stamblerre and unassigned stamblerre Sep 10, 2020
@gopherbot
Copy link

Change https://golang.org/cl/254558 mentions this issue: internal/lsp: pass options by reference instead of by value

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.5.1 Sep 16, 2020
@golang golang locked and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

No branches or pull requests

3 participants