x/tools/gopls: remove the "expandWorkspaceToModule" setting

[findleyr]

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.

[hjkatz]

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: neovim/neovim#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: #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)
• nvim's lsp now tries to watch way too many directories and 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.

[findleyr]

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.

[findleyr]

I filed #63742 to follow up on the overly broad watch patterns.

[hjkatz]

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: neovim/neovim#23291 (comment)

[evanj]

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.

[geitir]

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.

[findleyr]

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.

[findleyr]

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

[gopherbot]

Change https://go.dev/cl/550075 mentions this issue: gopls/internal/lsp/cache: reconcile view modes and workspace packages

[findleyr]

In the above CL, I'm removing the deprecation warning, based on the feedback here. This needs more thought.

Bumping any decision to [email protected].

[hyangah]

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.

[findleyr]

@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:

• I think references should probably also be limited to workspace packages, but could be convinced otherwise. The rationale is that references is used for navigation, and we want navigation to be fast. Similar logic applies for implements.
• renaming and other refactoring should operate on the full module, because we endeavor to enforce the invariant that refactoring operations do not break the build.
• Autocompletion and goimports are already able to limit the scope of the search based on the package name, and don't type check unimported packages, so I think they can continue to have full-module scope.

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.

[hyangah]

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

• The initialize request is sent with "workspaceFolder": null.
• Gopls sends a configuration query with the scope from the open file's package directory.
• Gopls expands the workspace to x/tools directory.

[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/[email protected], 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

[findleyr]

@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.