-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: zero-config gopls workspaces #57979
Comments
Change https://go.dev/cl/495256 mentions this issue: |
To avoid inconsistent state where we load command-line-arguments packages for files that would be contained in an ad-hoc package, ensure that the view is loaded before doing file loads, when in ad-hoc mode. Along the way, introduce the concept of 'ViewType' discussed in our zero-config-gopls design (golang/go#57979). Furthermore, move certain data onto the immutable workspaceInformation type: - moduleMode depends only on ViewType - inGOPATH can be precomputed Updates golang/go#57979 Fixes golang/go#57209 Change-Id: If54cea65fbc72e6e704eccc6fe59d30ae5d01069 Reviewed-on: https://go-review.googlesource.com/c/tools/+/495256 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
Change https://go.dev/cl/496880 mentions this issue: |
When no go.work or go.mod file is found, gopls searches to see if there is exactly one module in a nested directory, in which case it narrows the workspace to this one module. This is a legacy workaround for polyglot repositories, and will be made obsolete by golang/go#57979. However, in the meantime this feature is still necessary, and is the last remaining place where we walk the workspace looking for modules. As reported in golang/go#56496, this search can be expensive in very large directories. Reduce the search limit 10x, from 1M->100K, and use the more efficient filepath.WalkDir. Fixes golang/go#56496 Change-Id: Ia46dd90ac2220b09debc68742dd882885c38eb42 Reviewed-on: https://go-review.googlesource.com/c/tools/+/496880 Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com>
Change https://go.dev/cl/526160 mentions this issue: |
Change https://go.dev/cl/526417 mentions this issue: |
Any change to the working dir must necessarily result in a new view (see minorOptionsChange). Therefore, it should be considered an immutable part of the view, and rename it to goCommandDir, which more closely matches its meaning (and there are various other things that could be considered "working" dirs). Also store the folder on the workspaceInformation, to move toward an immutable foundation that can be safely shared with the snapshot. Updates golang/go#57979 For golang/go#42814 Change-Id: I9a9e3aa18fa85bace827d9c8dd1607e851cfcfb8 Reviewed-on: https://go-review.googlesource.com/c/tools/+/526160 Auto-Submit: Robert Findley <rfindley@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
In order to move toward tracking options by Folder, not view, move them into the Server. This will also help us fix bugs related to configuration lifecycle events. For golang/go#57979 Updates golang/go#42814 Change-Id: Id281cad20697756138a7bdc67f718a7468a04d4a Reviewed-on: https://go-review.googlesource.com/c/tools/+/526417 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com>
Ok, remaining TODOs for this issue are tracked in #29202, #64887, and #64888. The fundamental goal of this issue is achieved: gopls is zero-config with respect to workspace layout. Following https://go.dev/cl/551897, it will also be zero-config with respect to GOOS/GOARCH combinations (at which point I think we can close #29202). Closing this as complete! |
This CL leverages the new zero-config implementation to add support for dynamic build tags (golang/go#29202). While this CL is large, the gist of the change is relatively simple: - in bestView, check that the view GOOS/GOARCH actually matches the file - in defineView, loop through supported ports to find one that matches the file, and apply the necessary GOOS= GOARCH= env overlay - detect that views must be re-selected whenever a build constraint changes Everything else in the CL is supporting / refactoring around this minor adjustment to view selection. Notably, the logic to check whether a file matches a port (using go/build) required some care, because the go/build API is cumbersome and not particularly efficient. We therefore check ports as little as possible, and trim the file content that is passed into build.Context.MatchFile. Earlier attempts at this change were simpler, because they simply matched all available ports all the time, but this had significant cost (around a millisecond overhead added to every operation, including change processing). However, the good news is that with the logic as it is, I believe it is safe to support all available ports, because we only loop through this list when checking views, an infrequent operation. For golang/go#57979 For golang/go#29202 Change-Id: Ib654e18038dda74164b57d51b2d5274f91a1306d Reviewed-on: https://go-review.googlesource.com/c/tools/+/551897 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Run-TryBot: Robert Findley <rfindley@google.com>
The validBuildConfiguration helper never had a well-defined meaning, and now just means that the view is an ad-hoc view. Delete it and check the view type directly. Also, revisit the log message formatting view information. For golang/go#57979 Change-Id: Ia09f697dd96c1930f1c97c74f08a81698ad17f30 Reviewed-on: https://go-review.googlesource.com/c/tools/+/553095 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
With zero-config gopls, we no longer need to scan for modules when defining the default view for a folder. If there is no go.mod or go.work file in a parent directory, just use an ad-hoc view until the first file is opened. Delete tests that were explicitly testing the view narrowing logic, and so no longer make sense. For golang/go#57979 Change-Id: Ib2ff96068b2e17d652f24d5ec05e1f2335a7f222 Reviewed-on: https://go-review.googlesource.com/c/tools/+/553096 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The critical error logic was hard to follow, and ill defined because it was constructed in two places: once during initialization, and another time in Snapshot.CriticalError. CriticalError is now rebranded as an InitializationError, and constructed only during snapshot initialization. It covers a load error, and an unparsable go.work or go.mod file. Critical errors are applied to orphaned files. For golang/go#57979 Change-Id: Ib3cdf602954202be0c87594c26dbbd0ff7e6458a Reviewed-on: https://go-review.googlesource.com/c/tools/+/553097 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
Change https://go.dev/cl/555197 mentions this issue: |
Add View Type, EnvOverlay, and Root to the view debug page. Also inline the View debug page into the Session debug page, to save a click. For golang/go#57979 Change-Id: Id6fbf86a55329078adcada049e34607ee918da11 Reviewed-on: https://go-review.googlesource.com/c/tools/+/555197 Auto-Submit: Robert Findley <rfindley@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
Change https://go.dev/cl/557500 mentions this issue: |
I think there is a problem with the automatic GOOS/GOARCH thing where it doesn't work if cgo is used. For example editing this file on any OS that isn't freebsd. |
@aarzilli you are right: setting GOOS and GOARCH will, by default, disable cgo. I haven't yet tested, but I wonder if we can make this work by setting CC_FOR_${GOOS}_${GOARCH}? |
I don't know how much that would help. I, for example, do not have CC_FOR_anything_other_than_linux_amd64. I think probably what needs to happen is that FakeImportC needs to be passed to go/types where appropriate (or maybe go/types needs to do it automatically when it can't do better). |
Indeed, I'll give that a try. Thanks. |
Change https://go.dev/cl/560467 mentions this issue: |
Change https://go.dev/cl/559636 mentions this issue: |
With zero-config gopls (golang/go#57979), this function is no longer used. Change-Id: Ie59a7d39c62eab340fb6e44ddd9b7a0b1cabd92e Reviewed-on: https://go-review.googlesource.com/c/tools/+/560467 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
When using the gopls daemon, or with the multi-View workspaces that will be increasingly common following golang/go#57979, there is a lot of redundant work performed scanning the module cache. This CL eliminates that redundancy, by moving module cache information into the cache.Cache shared by all Sessions and Views. There should be effectively no change in behavior for gopls resulting from this CL. In ModuleResolver.scan, we still require that module cache roots are scanned. However, we no longer invalidate this scan in ModuleResolver.ClearForNewScan: re-scanning the module cache is the responsibility of a new ScanModuleCache function, which is independently scheduled. To enable this separation of refresh logic, a new refreshTimer type is extracted to encapsulate the refresh logic. For golang/go#44863 Change-Id: I333d55fca009be7984a514ed4abdc9a9fcafc08a Reviewed-on: https://go-review.googlesource.com/c/tools/+/559636 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Was this already released? How can I start using it? |
Change https://go.dev/cl/566936 mentions this issue: |
Rewrite the now-obsolete documentation. Notably, remove the section on experimental workspace mode, as it is long unsupported. For golang/go#57979 Change-Id: I8ba77d626d0b24b0ab34a78103985a5a881def21 Reviewed-on: https://go-review.googlesource.com/c/tools/+/566936 Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Robert Findley <rfindley@google.com>
Zero-config gopls workspaces
This issue describes a change to gopls' internal data model that will allow it to "Do The Right Thing" when the user opens a Go file. By decoupling the relationship between builds and workspace folders, we can eliminate complexity related to configuring the workspace (hence "zero-config"), and lay the groundwork for later improvements such as better support for working on multiple sets of build tags simultaneously (#29202).
After this change, users can work on multiple modules inside of a workspace regardless of whether they are related by a go.work file or explicitly open as separate workspace folders.
Background
Right now, gopls determines a unique build (called a
View
) for each workspace folder. When a workspace folder is opened, gopls performs the following steps:workspace/configuration
request withscopeUri
set to the folder URI.a. Check
go env GOWORK
.b. Else, look for
go.mod
in a parent directory (recursively).c. Else, look for
go.mod
in a nested directory, if there is only one such nested directory. This was done to support polyglot workspaces where the Go project is in a nested directory, but is a source of both confusion and unpredictable startup time.d. Else, use the folder as root.
go/packages.Load
.Problems
There are several problems with this model:
"experimentalWorkspaceModule"
setting,"expandWorkspaceToModule"
setting, and"directoryFilters"
setting.New Model
We can address these problems by decoupling Views from workspace folders. The set of views will be dynamic, depending on both the set of open folders and the set of open files, and will be chosen to cover all open files.
Specifically, define new
View
andFolder
types approximately as follows:A
Session
consists of a set ofView
objects describing modules (go.mod
files), workspaces (go.work
files), GOPATH directories or ad-hoc packages that the user is working on. This set is determined by both the workspace folders specified by the editor and the set of open files.View types
workspace
View is defined by ago.work
file.source
is the path to thego.work
file.module
View is defined by a singlego.mod
file.source
is the path to thego.mod
file.GOPATH
View is defined by a folder inside aGOPATH
directory, withGO111MODULE=off
orGO111MODULE=auto
and nogo.mod
file.source
is the path to the directory.adhoc
View is defined by a folder outside ofGOPATH
, with no enclosinggo.mod
file. In this case, we consider files in the same directory to be part of a package, andsource
is the path to the directory.The set of Views
We define the set of Views to ensure that we have coverage for each open folder, and each open file.
go env GOWORK
is set, create aworkspace
View.go.mod
in a parent directory. If found, create amodule
View.GOPATH
, andGO111MODULE
is not explicitly set toon
, create aGOPATH
View. IfGO111MODULE=on
explicitly, fail.adhoc
View for the workspace folder. This may not be desirable for the user if they have modules contained in nested directories. In this case we could either prompt the user, or scan for modules in nested directories, creating Views for each (but notably if we do decide to scan the filesystem, we would create a View for each go.mod or go.work file encountered, rather than fail if there are more than one).Match to an existing View
go.mod
files.go.mod
file is found, search for existingworkspace
ormodule
type Views containing this module in theirmodules
set.GOPATH
type Views whosesource
directory contains the file.adhoc
type Views whosesource
is equal tofilepath.Dir(file)
.If no existing View matches the file, create a new one
module
type. Apply an explicitGOWORK=off
to the View configuration to ensure that we can load the module.Initializing views
Initialize views using the following logic. This essentially matches gopls’ current behavior.
workspace
Views, loadmodulepath/...
for each workspace module.module
Views, loadmodulepath/...
for the main module.GOPATH
Views, load./...
from the View dir.ad-hoc
Views, load./
from the View dir.Type-check packages (and report their compiler diagnostics) as follows:
workspace
Views, type-check any package whose module is a workspace module.module
Views, type-check any package whose module is the main module.GOPATH
Views, type-check any package contained indir
.adhoc
Views, type-check the ad-hoc package.Resolving requests to Views
When a file-oriented request is handled by gopls (a request prefixed with
textDocument/
, such astextDocument/definition
), gopls must usually resolve package metadata associated with the file.In most cases, gopls currently chooses an existing view that best applies to the file (
cache.bestViewForURI
), but this is already problematic, because it can lead to path-dependency and incomplete results (c.f. #57558). For example: when finding references from a package imported from multiple views, gopls currently only shows references in one view.Wherever possible, gopls should multiplex queries across all Views and merge their results. This would lead to consistent behavior of cross references. In a future where gopls has better build-tag support, this could also lead to multiple locations for jump-to-definition results.
In some cases (for example
hover
orsignatureHelp
), we must pick one view. In these cases we can apply some heuristic, but it should be of secondary significance (any hover or signatureHelp result is better than none).Updating Views
Based on the algorithms used to determine Views above, the following notifications may affect the set of Views:
didOpen
anddidClose
cause gopls to re-evaluate Views, ensuring that we have a View for each open file contained in a workspace folder.didChangeConfiguration
anddidChangeWorkspaceFolders
causes gopls to updateFolder
layout and configuration. Note that configuration may affect e.g. GOWORK values and therefore may lead to a new set of Views.didChange
ordidChangeWatchedFile
may cause gopls to re-evaluate Views if the change is to ago.mod
orgo.work
file (for example, ago.mod
file deleted or added, or ago.work
file changed in any way).Following these changes, gopls will re-run the algorithm above to determine a new set of Views. It will re-use existing Views that have not changed.
Whenever new Views are created, they are reinitialized as above.
Differences from the current model
The algorithms described above are not vastly divergent from gopls’ current behavior. The significant differences may be summarized as follows:
go.mod
files are added or removed, orgo.work
files changed, we reconfigure the set of Views. This simplifies the logic of handling metadata invalidation in each view.Downsides
While this change will make gopls “do the right thing” in more cases, there are a several notable downsides:
go.work
orgo.mod
in a parent directory of the workspace folder. This means thatworkspace/symbols
requests may return empty results, or results that depend on the set of open files. Users can mitigate this by using ago.work
file.a
andb
in their workspace, anda
depends on a version ofb
in the module cache, find reference on a symbol in theb
directory of the workspace will not include references ina
. Users can mitigate this by using ago.work
file. It would also be possible for us to implement looser heuristics in our references search.Future extension to build tags
By decoupling Views from workspace folders, it becomes possible for gopls to support working on multiple sets of build tags simultaneously. One can imagine that the algorithm above to compute views based on open files could be extended to GOOS and GOARCH: if an open file is not included in an existing view because of its GOOS or GOARCH build constraints, create a new view with updated environment.
The downsides above apply: potentially increased memory, and potentially confusing UX as the behavior of certain workspace-wide queries (such as references or workspace symbols) depends on the set of open files. We can work to mitigate these downsides, and in my opinion they do not outweigh the upsides, as these queries simply don't work in the current model.
Task List
Here's an approximate plan of attack for implementing this feature, which I'm aiming to complete by the end of the year. (migrated from #57979 (comment)).
This is inside baseball, but may be interesting to @adonovan and @hyangah.
Phase 1: making Views immutable:
Session.getWorkspaceInformation
logic to be unit-testable, and rename/refactor. This will be the basis of the new workspace algorithm.Folder
type, to reuse across new/multiple views.NewView
, which should no longer query workspace information, but rather should be provided immutable workspace and folder information.getWorkspaceInformation
. Whenever ago.work
file changes, create a new view (at least if the newgo.work
parses and is saved to disk).At this point, gopls should still behave identically, but Views will have immutable options and main modules. There may be a bit more churn when configuration or go.work files change, but such events should be very infrequent, and it seems reasonable to reload the workspace when they occur.
Phase 2: supporting multiple views per folder
bestViewForURI
logic to return nil if no view matches, and lift upsnapshot.contains
to theView
, since views are now immutable.Views
necessary to cover all open files (computeViews
). Compute the diff with the current set, and minimally (re)create views that are necessary.Phase 3: support for multiple GOOS/GOARCH combinations
computeViews
algorithm to consider GOOS and GOARCH combinations. I'm not sure exactly how this algorithm will work: presumably if the current GOOS/GOARCH combination doesn't match an open file, we'll pick another, but the algorithm to pick another is non-trivial.The text was updated successfully, but these errors were encountered: