Open findleyr opened 11 months ago
I think I have a use, but I'm not sure where to comment, so here it goes.
What I'm noticing is that using neovim's lsp implementation I'm running into this issue: https://github.com/neovim/neovim/issues/23291 Which causes significant startup lag ~20s when opening go files.
I most recently noticed this issue after adding a go.work
file into a monorepo with an example structure:
monorepo/
- go.work
- go/
- go.mod
- go files...
- cache/
- js/
- lots/of/other/dirs
Content of go.work
:
go 1.21.3
use ./go
Combine this with the gopls issue with globbing seen here: https://github.com/golang/go/issues/41504
We get the following behaviour:
go.work
instructs gopls
to use monorepo/
as the root rather than monorepo/go/
gopls
requests to watch files monorepo/**/*.{go,mod,sum}
(lots and lots of files)I think this behaviour is related to this issue and the expandWorkspaceToModule
setting because I think some behaviour of this setting could (should?) use the contents of go.work
to limit the directories to glob.
In my case I would expect gopls
to read go.work
and see use ./go
and then set the root to ./go
, not the cwd of go.work
.
Alternative behaviour could be only loading the glob for any root (sub-path) that matches a use
statement in go.work
for the current file being opened (or attached for the lsp).
I'm all ears to hear about workarounds or solutions to my problem.
Thanks for raising this issue, and for the detailed explanation of the performance bottleneck you're experiencing. I was unaware of that neovim issue, and will keep it in mind.
I think I may not have explained clearly that the current default behavior will remain unchanged (which is "expandWorkspaceToModule": true
). Updated the description to clarify.
I looked into your use case, and don't think that it is affected by "expandWorkspaceToModule"
. If neovim is computing monorepo/
as the workspace folder, I think we will always ask to watch everything in it (e.g. it is considered a workspace dir). To work around your problem, you could put the go.work file in the go/
subdir, or in an entirely different dir altogether and set the GOWORK
environment variable. Longer term, if GOWORK
is set, we should just watch the go.work
file itself and the modules it activates.
I filed #63742 to follow up on the overly broad watch patterns.
Thank you for the clarification. I think the behaviour of expaindWorkspaceToModule
is good to have (i.e. gopls should try to use a heuristic function to find the correct root_path
for lsp config).
Since this is kind of a 2-part issue (nvim lsp watch files implementation + gopls overbroad watchfiles glob) I decided to try to change the root_path
myself and ended up with just a specific config for gopls
:
lspconfig.gopls.setup({
on_attach = [...],
capabilities = [...],
settings = [...],
-- override root_path for issue: https://github.com/golang/go/issues/63536
root_path = function(fname)
local root_files = {
'go/go.mod', -- monorepo override so root_path is ./monorepo/go/** not ./monorepo/**
'go.work',
'go.mod',
'.git',
}
-- return first parent dir that homes a found root_file
return lspconfig.util.root_pattern(unpack(root_files))(fname) or lspconfig.util.path.dirname(fname)
end,
})
And now the number of watchfiles is back down in the ~3k instead of in the ~15k.
Note: The neovim lsp watchfiles function being slow still exists, but the delay is ~20ms and not like ~20s.
Alternatively users can just disable the watchfiles function entirely following steps here: https://github.com/neovim/neovim/issues/23291#issuecomment-1560742827
I use this setting with vscode. My work uses some fairly large mono repos with Bazel. Most of the Go parts can use the standard Go tools, but not all. I like to open just the directory I am working on (e.g. a single library, or a single main program), since then all of VSCode's search and navigation features work well, without stuff I don't care about. To be clear, the directory looks approximately like:
monorepo
├── go.mod
├── stuff_i_dont_care_about
│ └── build_error.go
└── stuff_i_work_on
└── stuff.go
And I open the sub-directory within the Go module called stuff_i_work_on
by running code (my dir)
on the command line. The problem is I can't get gopls to ignore the hundreds of build errors in other directories I don't care about. I've tried:
"gopls": {
"build.directoryFilters": [
"-"
]
}
which as far as I understand it, should remove everything from gopls's build, but it does not seem to work. I have also tried variants that include "+stuff_i_work_on/**"
. If I open the Go module root, then I can use build.directoryFilters
to filter gopls correctly. So maybe this is a "bug report" for this configuration?
If I set "expandWorkspaceToModule": false
then gopls/vscode works as I expect and want when I open a sub-directory.
I have the same use case as @evanj. I use vs-code and my work uses a very large monorepo and bazel and I like to load only the service directories I care about (eg cd go-monorepo/src/.../.../service/ && code .
).
I haven't touched my config in a long time, but I distinctly remember having to add expandWorkspaceToModule: false
otherwise gopls would get bogged down, have a ton of errors, and sometimes crash/never resolve.
I will experiment with turning it off, since maybe the heuristics used etc have improved and report back, but I imagine it is still an issue.
Thanks very much for the detailed feedback!
I think it's probably incorrect to expand the workspace if GOPACKAGESDRIVER is set, so perhaps we can just fix the default behavior in that case. I'll investigate.
Coming back to this, I will note that in the case @evanj is reporting it sounds like there is no GOPACKAGESDRIVER involved. It just so happens that some things still work in the absence of a bazel driver. So my comment above was misguided.
After digging into this more, I've actually come to the conclusion that most of the way gopls behaves with "expandWorkspaceToModule": false
is correct, i.e. the current default behavior is wrong. Specifically, if the user opens a subdirectory of the module, we should (1) only report diagnostics for packages in that subdirectory, (2) only actively reload packages contained in that directory. If the user opens a specific directory, we should honor that choice rather than effectively translating the workspace to a parent directory. We should not override the user's choice of workspace.
It is still problematic to have a setting that affects gopls' core behavior so deeply (if only for the implications on test coverage), so we should still endeavor to remove this setting, but in removing it I think we should also change the default behavior. This may cause churn for users of e.g. vim or emacs clients that implicitly relied on gopls choosing the correct workspace. So perhaps we should keep this setting for one more release cycle, with a different default.
CC @adonovan @hyangah
Change https://go.dev/cl/550075 mentions this issue: gopls/internal/lsp/cache: reconcile view modes and workspace packages
In the above CL, I'm removing the deprecation warning, based on the feedback here. This needs more thought.
Bumping any decision to gopls@v0.16.0.
After digging into this more, I've actually come to the conclusion that most of the way gopls behaves with "expandWorkspaceToModule": false is correct, i.e. the current default behavior is wrong. Specifically, if the user opens a subdirectory of the module, we should (1) only report diagnostics for packages in that subdirectory, (2) only actively reload packages contained in that directory. If the user opens a specific directory, we should honor that choice rather than effectively translating the workspace to a parent directory. We should not override the user's choice of workspace.
@findleyr What will be the scope of functionalities supported when a user opens a directory narrower than a module boundary?
Where will gopls present the diagnostics and features when they are outside of the user's workspace boundary? go
allows to keep configurations and dependencies affecting the build from the current editor's workspace outside of it (e.g. module cache, go.mod, go.work, vendor, go env files). Even though I agree that we can be lazy about loading packages outside editor's current workspace, I think we still need a way to report diagnostics about the configuration.
Will features like renaming/refactoring, autocompletion, imports be still supported? For example, when a user is working on a core rpc library that's imported by other distant packages, what will be the users' setup like to continue to work on the core rpc library while not severely breaking dependencies by accident? Or, when a user is working on a small part of a huge module, will the user be able to discover and start to import a core package in the module that's outside of users' active workspace?
VS Code, and other traditional editors supports opening individual files without opening from a folder (e.g. by running code main.go
from terminal or by clicking file icons from window manager's UI). I thought the expandWorkspaceToModule
functionality was necessary to support this UX in vscode. If that's true, changing the default may cause churns to vscode users.
@hyangah I agree that it makes sense to spell out these capabilities explicitly.
The most critical implication of "workspace package" is the set of packages that are eagerly type-checked on every keystroke, for the purposes of computing diagnostics. If the user opens a narrower workspace folder with expandWorkspaceToModule=false, we won't include packages outside of this folder in the eager diagnostics, even if they are in the same module. This means that there is potentially a significant reduction in CPU while editing, but diagnostics for non-workspace packages could go stale. Consequently, we don't show diagnostics for packages outside of the workspace.
With regard to other operations:
VS Code, and other traditional editors supports opening individual files without opening from a folder (e.g. by running code main.go from terminal or by clicking file icons from window manager's UI). I thought the expandWorkspaceToModule functionality was necessary to support this UX in vscode. If that's true, changing the default may cause churns to vscode users.
Interesting: do you know which directory is sent as workspace folder in this case? At the very least gopls will work, though it might have an undesirable scope.
Fundamentally I think the argument for removing this setting is one of orthogonality. Users already have a way to specify the scope of their workspace: the workspace folder. gopls deciding on a different scope is an overlapping feature, and not obvious to the user.
But I'm no longer sure what is right, so let's delay any decision.
do you know which directory is sent as workspace folder in this case? At the very least gopls will work, though it might have an undesirable scope.
When I opened x/tools/cmd/godoc/main.go, I see
initialize
request is sent with "workspaceFolder": null
.[Info - 10:16:36 AM] 2023/12/18 10:16:36 go info for /Users/hakim/projects/tools/cmd/godoc
(go dir /Users/hakim/projects/tools)
(go version go version go1.21.5 darwin/amd64)
(valid build configuration = true)
(build flags: [])
(selected go env: [GO111MODULE=, GOCACHE=/Users/hakim/Library/Caches/go-build, GOFLAGS=, GOMODCACHE=/Users/hakim/go/pkg/mod, GOPATH=/Users/hakim/go, GOPRIVATE=, GOROOT=/Users/hakim/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.5.darwin-amd64, GOWORK=/Users/hakim/projects/tools/go.work])
But this doesn't work when a file from the other directory opens from the same window (vscode reuses the current window to open a new file). gopls tries to analyze the file reported with didOpen
, but insists on the first workspace root folder it picked and fails to load. (I thought zero config gopls might be helpful here)
[Info - 10:21:36 AM] 2023/12/18 10:21:36 254.922831ms for GOROOT= GOPATH=/Users/hakim/go GO111MODULE=auto GOPROXY=off PWD=/Users/hakim/projects/tools go list -mod=readonly -e -json=Name,ImportPath,Error,Dir,GoFiles,IgnoredGoFiles,IgnoredOtherFiles,CFiles,CgoFiles,CXXFiles,MFiles,HFiles,FFiles,SFiles,SwigFiles,SwigCXXFiles,SysoFiles,TestGoFiles,XTestGoFiles,CompiledGoFiles,Export,DepOnly,Imports,ImportMap,TestImports,XTestImports,ForTest,DepsErrors,Module,EmbedFiles -compiled=true -test=true -export=false -deps=true -find=false -pgo=off -- /Users/hakim/projects/vulns/internal/checker
@hyangah in the absence of a preference (no workspace folder) we can continue to expand the workspace.
(I thought zero config gopls might be helpful here)
It will be.
Per the rationale provided in #57514: remove this unnecessary setting, in favor of always expanding/contracting the workspace as necessary (the current behavior with
"expandWorkspaceToModule": true
). If you need this setting, please leave a comment explaining why.