diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index ac5307e553..e1b55b5b13 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -543,7 +543,7 @@ func _() { // Expect a module/GOPATH error if there is an error in the file at startup. // Tests golang/go#37279. -func TestShowCriticalError_Issue37279(t *testing.T) { +func TestBrokenWorkspace_OutsideModule(t *testing.T) { const noModule = ` -- a.go -- package foo diff --git a/gopls/internal/regtest/workspace/broken_test.go b/gopls/internal/regtest/workspace/broken_test.go new file mode 100644 index 0000000000..a818fe40bc --- /dev/null +++ b/gopls/internal/regtest/workspace/broken_test.go @@ -0,0 +1,120 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "testing" + + "golang.org/x/tools/internal/lsp" + . "golang.org/x/tools/internal/lsp/regtest" + "golang.org/x/tools/internal/testenv" +) + +// This file holds various tests for UX with respect to broken workspaces. +// +// TODO: consolidate other tests here. + +// Test for golang/go#53933 +func TestBrokenWorkspace_DuplicateModules(t *testing.T) { + testenv.NeedsGo1Point(t, 18) + + // This proxy module content is replaced by the workspace, but is still + // required for module resolution to function in the Go command. + const proxy = ` +-- example.com/foo@v0.0.1/go.mod -- +module example.com/foo + +go 1.12 +-- example.com/foo@v1.2.3/foo.go -- +package foo +` + + const src = ` +-- go.work -- +go 1.18 + +use ( + ./package1 + ./package1/vendor/example.com/foo + ./package2 + ./package2/vendor/example.com/foo +) + +-- package1/go.mod -- +module mod.test + +go 1.18 + +require example.com/foo v0.0.1 +-- package1/main.go -- +package main + +import "example.com/foo" + +func main() { + _ = foo.CompleteMe +} +-- package1/vendor/example.com/foo/go.mod -- +module example.com/foo + +go 1.18 +-- package1/vendor/example.com/foo/foo.go -- +package foo + +const CompleteMe = 111 +-- package2/go.mod -- +module mod2.test + +go 1.18 + +require example.com/foo v0.0.1 +-- package2/main.go -- +package main + +import "example.com/foo" + +func main() { + _ = foo.CompleteMe +} +-- package2/vendor/example.com/foo/go.mod -- +module example.com/foo + +go 1.18 +-- package2/vendor/example.com/foo/foo.go -- +package foo + +const CompleteMe = 222 +` + + WithOptions( + ProxyFiles(proxy), + ).Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("package1/main.go") + env.Await( + OutstandingWork(lsp.WorkspaceLoadFailure, `found module "example.com/foo" multiple times in the workspace`), + ) + + /* TODO(golang/go#54069): once we allow network when reinitializing the + * workspace, we should be able to fix the error here. + + // Remove the redundant vendored copy of example.com. + env.WriteWorkspaceFile("go.work", `go 1.18 + use ( + ./package1 + ./package2 + ./package2/vendor/example.com/foo + ) + `) + env.Await(NoOutstandingWork()) + + // Check that definitions in package1 go to the copy vendored in package2. + location, _ := env.GoToDefinition("package1/main.go", env.RegexpSearch("package1/main.go", "CompleteMe")) + const wantLocation = "package2/vendor/example.com/foo/foo.go" + if !strings.HasSuffix(location, wantLocation) { + t.Errorf("got definition of CompleteMe at %q, want %q", location, wantLocation) + } + */ + }) +} diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index deb8a83695..bc576226e8 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/gopls/internal/regtest/workspace/workspace_test.go @@ -290,10 +290,6 @@ func Hello() {} module b.com go 1.12 --- b.com@v1.2.4/b/b.go -- -package b - -func Hello() {} ` const multiModule = ` -- go.mod -- diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 79789fe60d..9d84891911 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -310,8 +310,8 @@ You can work with multiple modules by opening each one as a workspace folder. Improvements to this workflow will be coming soon, and you can learn more here: https://github.com/golang/tools/blob/master/gopls/doc/workspace.md.` return &source.CriticalError{ - MainError: fmt.Errorf(msg), - DiagList: s.applyCriticalErrorToFiles(ctx, msg, openFiles), + MainError: fmt.Errorf(msg), + Diagnostics: s.applyCriticalErrorToFiles(ctx, msg, openFiles), } } @@ -349,7 +349,7 @@ You can learn more here: https://github.com/golang/tools/blob/master/gopls/doc/w MainError: fmt.Errorf(`You are working in a nested module. Please open it as a separate workspace folder. Learn more: https://github.com/golang/tools/blob/master/gopls/doc/workspace.md.`), - DiagList: srcDiags, + Diagnostics: srcDiags, } } } diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index 361f526ddf..704e1a63a4 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -61,7 +61,7 @@ func (s *snapshot) ModTidy(ctx context.Context, pm *source.ParsedModule) (*sourc if criticalErr := s.GetCriticalError(ctx); criticalErr != nil { return &source.TidiedModule{ - Diagnostics: criticalErr.DiagList, + Diagnostics: criticalErr.Diagnostics, }, nil } @@ -70,7 +70,6 @@ func (s *snapshot) ModTidy(ctx context.Context, pm *source.ParsedModule) (*sourc } handle := memoize.NewPromise("modTidy", func(ctx context.Context, arg interface{}) interface{} { - tidied, err := modTidyImpl(ctx, arg.(*snapshot), uri.Filename(), pm) return modTidyResult{tidied, err} }) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index a27efe334f..8869064828 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -180,21 +180,26 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, s.cache.options(options) } - // Set the module-specific information. - ws, err := s.getWorkspaceInformation(ctx, folder, options) + // Get immutable workspace configuration. + // + // TODO(rfindley): this info isn't actually immutable. For example, GOWORK + // could be changed, or a user's environment could be modified. + // We need a mechanism to invalidate it. + wsInfo, err := s.getWorkspaceInformation(ctx, folder, options) if err != nil { return nil, nil, func() {}, err } + root := folder if options.ExpandWorkspaceToModule { - root, err = findWorkspaceRoot(ctx, root, s, pathExcludedByFilterFunc(root.Filename(), ws.gomodcache, options), options.ExperimentalWorkspaceModule) + root, err = findWorkspaceRoot(ctx, root, s, pathExcludedByFilterFunc(root.Filename(), wsInfo.gomodcache, options), options.ExperimentalWorkspaceModule) if err != nil { return nil, nil, func() {}, err } } // Build the gopls workspace, collecting active modules in the view. - workspace, err := newWorkspace(ctx, root, s, pathExcludedByFilterFunc(root.Filename(), ws.gomodcache, options), ws.userGo111Module == off, options.ExperimentalWorkspaceModule) + workspace, err := newWorkspace(ctx, root, s, pathExcludedByFilterFunc(root.Filename(), wsInfo.gomodcache, options), wsInfo.userGo111Module == off, options.ExperimentalWorkspaceModule) if err != nil { return nil, nil, func() {}, err } @@ -217,7 +222,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, filesByURI: map[span.URI]*fileBase{}, filesByBase: map[string][]*fileBase{}, rootURI: root, - workspaceInformation: *ws, + workspaceInformation: *wsInfo, } v.importsState = &importsState{ ctx: backgroundCtx, diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index b183bc5f88..7c143fe81e 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1337,6 +1337,10 @@ func (s *snapshot) awaitLoaded(ctx context.Context) error { } func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError { + if wsErr := s.workspace.criticalError(ctx, s); wsErr != nil { + return wsErr + } + loadErr := s.awaitLoadedAllErrors(ctx) if loadErr != nil && errors.Is(loadErr.MainError, context.Canceled) { return nil @@ -1396,32 +1400,45 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalErr // Do not return results until the snapshot's view has been initialized. s.AwaitInitialized(ctx) - // TODO(rstambler): Should we be more careful about returning the + // TODO(rfindley): Should we be more careful about returning the // initialization error? Is it possible for the initialization error to be // corrected without a successful reinitialization? s.mu.Lock() initializedErr := s.initializedErr s.mu.Unlock() + if initializedErr != nil { return initializedErr } + // TODO(rfindley): revisit this handling. Calling reloadWorkspace with a + // cancelled context should have the same effect, so this preemptive handling + // should not be necessary. + // + // Also: GetCriticalError ignores context cancellation errors. Should we be + // returning nil here? if ctx.Err() != nil { return &source.CriticalError{MainError: ctx.Err()} } + // TODO(rfindley): reloading is not idempotent: if we try to reload or load + // orphaned files below and fail, we won't try again. For that reason, we + // could get different results from subsequent calls to this function, which + // may cause critical errors to be suppressed. + if err := s.reloadWorkspace(ctx); err != nil { diags := s.extractGoCommandErrors(ctx, err) return &source.CriticalError{ - MainError: err, - DiagList: diags, + MainError: err, + Diagnostics: diags, } } + if err := s.reloadOrphanedFiles(ctx); err != nil { diags := s.extractGoCommandErrors(ctx, err) return &source.CriticalError{ - MainError: err, - DiagList: diags, + MainError: err, + Diagnostics: diags, } } return nil @@ -1607,7 +1624,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC defer done() var vendorChanged bool - newWorkspace, workspaceChanged, workspaceReload := s.workspace.invalidate(ctx, changes, &unappliedChanges{ + newWorkspace, workspaceChanged, workspaceReload := s.workspace.Clone(ctx, changes, &unappliedChanges{ originalSnapshot: s, changes: changes, }) @@ -2235,7 +2252,7 @@ func (s *snapshot) BuildGoplsMod(ctx context.Context) (*modfile.File, error) { return buildWorkspaceModFile(ctx, allModules, s) } -// TODO(rfindley): move this to workspacemodule.go +// TODO(rfindley): move this to workspace.go func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{}, fs source.FileSource) (*modfile.File, error) { file := &modfile.File{} file.AddModuleStmt("gopls-workspace") @@ -2273,8 +2290,8 @@ func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{}, goVersion = parsed.Go.Version } path := parsed.Module.Mod.Path - if _, ok := paths[path]; ok { - return nil, fmt.Errorf("found module %q twice in the workspace", path) + if seen, ok := paths[path]; ok { + return nil, fmt.Errorf("found module %q multiple times in the workspace, at:\n\t%q\n\t%q", path, seen, modURI) } paths[path] = modURI // If the module's path includes a major version, we expect it to have diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index e33e340045..71d27ab909 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -112,6 +112,9 @@ type workspaceInformation struct { environmentVariables // userGo111Module is the user's value of GO111MODULE. + // + // TODO(rfindley): is there really value in memoizing this variable? It seems + // simpler to make this a function/method. userGo111Module go111module // The value of GO111MODULE we want to run with. @@ -703,18 +706,18 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) { event.Error(ctx, "initial workspace load failed", err) extractedDiags := s.extractGoCommandErrors(ctx, err) criticalErr = &source.CriticalError{ - MainError: err, - DiagList: append(modDiagnostics, extractedDiags...), + MainError: err, + Diagnostics: append(modDiagnostics, extractedDiags...), } case len(modDiagnostics) == 1: criticalErr = &source.CriticalError{ - MainError: fmt.Errorf(modDiagnostics[0].Message), - DiagList: modDiagnostics, + MainError: fmt.Errorf(modDiagnostics[0].Message), + Diagnostics: modDiagnostics, } case len(modDiagnostics) > 1: criticalErr = &source.CriticalError{ - MainError: fmt.Errorf("error loading module names"), - DiagList: modDiagnostics, + MainError: fmt.Errorf("error loading module names"), + Diagnostics: modDiagnostics, } } @@ -941,6 +944,12 @@ func (s *Session) getGoEnv(ctx context.Context, folder string, goversion int, go for k := range vars { args = append(args, k) } + // TODO(rfindley): GOWORK is not a property of the session. It may change + // when a workfile is added or removed. + // + // We need to distinguish between GOWORK values that are set by the GOWORK + // environment variable, and GOWORK values that are computed based on the + // location of a go.work file in the directory hierarchy. args = append(args, "GOWORK") inv := gocommand.Invocation{ diff --git a/internal/lsp/cache/workspace.go b/internal/lsp/cache/workspace.go index 669ce9290c..0091be9d1e 100644 --- a/internal/lsp/cache/workspace.go +++ b/internal/lsp/cache/workspace.go @@ -28,7 +28,7 @@ const ( legacyWorkspace = iota // non-module or single module mode goplsModWorkspace // modules provided by a gopls.mod file goWorkWorkspace // modules provided by a go.work file - fileSystemWorkspace // modules scanned from the filesystem + fileSystemWorkspace // modules found by walking the filesystem ) func (s workspaceSource) String() string { @@ -95,7 +95,12 @@ type workspace struct { // // If there is no active workspace file (a gopls.mod or go.work), newWorkspace // scans the filesystem to find modules. -func newWorkspace(ctx context.Context, root span.URI, fs source.FileSource, excludePath func(string) bool, go111moduleOff bool, useWsModule bool) (*workspace, error) { +// +// TODO(rfindley): newWorkspace should perhaps never fail, relying instead on +// the criticalError method to surface problems in the workspace. +// TODO(rfindley): this function should accept the GOWORK value, if specified +// by the user. +func newWorkspace(ctx context.Context, root span.URI, fs source.FileSource, excludePath func(string) bool, go111moduleOff, useWsModule bool) (*workspace, error) { ws := &workspace{ root: root, excludePath: excludePath, @@ -178,6 +183,28 @@ func (w *workspace) getActiveModFiles() map[span.URI]struct{} { return w.activeModFiles } +// criticalError returns a critical error related to the workspace setup. +func (w *workspace) criticalError(ctx context.Context, fs source.FileSource) (res *source.CriticalError) { + // For now, we narrowly report errors related to `go.work` files. + // + // TODO(rfindley): investigate whether other workspace validation errors + // can be consolidated here. + if w.moduleSource == goWorkWorkspace { + // We should have already built the modfile, but build here to be + // consistent about accessing w.mod after w.build. + // + // TODO(rfindley): build eagerly. Building lazily is a premature + // optimization that poses a significant burden on the code. + w.build(ctx, fs) + if w.buildErr != nil { + return &source.CriticalError{ + MainError: w.buildErr, + } + } + } + return nil +} + // modFile gets the workspace modfile associated with this workspace, // computing it if it doesn't exist. // @@ -207,9 +234,10 @@ func (w *workspace) build(ctx context.Context, fs source.FileSource) { // would not be obvious to the user how to recover. ctx = xcontext.Detach(ctx) - // If our module source is not gopls.mod, try to build the workspace module - // from modules. Fall back on the pre-existing mod file if parsing fails. - if w.moduleSource != goplsModWorkspace { + // If the module source is from the filesystem, try to build the workspace + // module from active modules discovered by scanning the filesystem. Fall + // back on the pre-existing mod file if parsing fails. + if w.moduleSource == fileSystemWorkspace { file, err := buildWorkspaceModFile(ctx, w.activeModFiles, fs) switch { case err == nil: @@ -222,6 +250,7 @@ func (w *workspace) build(ctx context.Context, fs source.FileSource) { w.buildErr = err } } + if w.mod != nil { w.wsDirs = map[span.URI]struct{}{ w.root: {}, @@ -235,18 +264,21 @@ func (w *workspace) build(ctx context.Context, fs source.FileSource) { w.wsDirs[span.URIFromPath(r.New.Path)] = struct{}{} } } + // Ensure that there is always at least the root dir. if len(w.wsDirs) == 0 { w.wsDirs = map[span.URI]struct{}{ w.root: {}, } } + sum, err := buildWorkspaceSumFile(ctx, w.activeModFiles, fs) if err == nil { w.sum = sum } else { event.Error(ctx, "building workspace sum file", err) } + w.built = true } @@ -263,7 +295,7 @@ func (w *workspace) dirs(ctx context.Context, fs source.FileSource) []span.URI { return dirs } -// invalidate returns a (possibly) new workspace after invalidating the changed +// Clone returns a (possibly) new workspace after invalidating the changed // files. If w is still valid in the presence of changedURIs, it returns itself // unmodified. // @@ -271,7 +303,10 @@ func (w *workspace) dirs(ctx context.Context, fs source.FileSource) []span.URI { // Some workspace changes may affect workspace contents without requiring a // reload of metadata (for example, unsaved changes to a go.mod or go.sum // file). -func (w *workspace) invalidate(ctx context.Context, changes map[span.URI]*fileChange, fs source.FileSource) (_ *workspace, changed, reload bool) { +// +// TODO(rfindley): it looks wrong that we return 'reload' here. The caller +// should determine whether to reload. +func (w *workspace) Clone(ctx context.Context, changes map[span.URI]*fileChange, fs source.FileSource) (_ *workspace, changed, reload bool) { // Prevent races to w.modFile or w.wsDirs below, if w has not yet been built. w.buildMu.Lock() defer w.buildMu.Unlock() @@ -501,6 +536,10 @@ func parseGoWork(ctx context.Context, root, uri span.URI, contents []byte, fs so modURI := span.URIFromPath(filepath.Join(dir.Path, "go.mod")) modFiles[modURI] = struct{}{} } + + // TODO(rfindley): we should either not build the workspace modfile here, or + // not fail so hard. A failure in building the workspace modfile should not + // invalidate the active module paths extracted above. modFile, err := buildWorkspaceModFile(ctx, modFiles, fs) if err != nil { return nil, nil, err diff --git a/internal/lsp/cache/workspace_test.go b/internal/lsp/cache/workspace_test.go index b809ad196a..f8651fd650 100644 --- a/internal/lsp/cache/workspace_test.go +++ b/internal/lsp/cache/workspace_test.go @@ -298,7 +298,7 @@ replace gopls.test => ../../gopls.test2`, false}, t.Fatal(err) } } - got, gotChanged, gotReload := w.invalidate(ctx, changes, fs) + got, gotChanged, gotReload := w.Clone(ctx, changes, fs) if gotChanged != test.wantChanged { t.Errorf("w.invalidate(): got changed %t, want %t", gotChanged, test.wantChanged) } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 59fe1716b9..460659f30d 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -414,7 +414,7 @@ func (s *Server) showCriticalErrorStatus(ctx context.Context, snapshot source.Sn var errMsg string if err != nil { event.Error(ctx, "errors loading workspace", err.MainError, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) - for _, d := range err.DiagList { + for _, d := range err.Diagnostics { s.storeDiagnostics(snapshot, d.URI, modSource, []*source.Diagnostic{d}) } errMsg = strings.ReplaceAll(err.MainError.Error(), "\n", " ") diff --git a/internal/lsp/fake/sandbox.go b/internal/lsp/fake/sandbox.go index 72b01217ae..7efbdb3ae5 100644 --- a/internal/lsp/fake/sandbox.go +++ b/internal/lsp/fake/sandbox.go @@ -159,6 +159,9 @@ func UnpackTxt(txt string) map[string][]byte { dataMap := make(map[string][]byte) archive := txtar.Parse([]byte(txt)) for _, f := range archive.Files { + if _, ok := dataMap[f.Name]; ok { + panic(fmt.Sprintf("found file %q twice", f.Name)) + } dataMap[f.Name] = f.Data } return dataMap diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go index 737f83da89..32538851ee 100644 --- a/internal/lsp/regtest/expectation.go +++ b/internal/lsp/regtest/expectation.go @@ -308,7 +308,7 @@ func OutstandingWork(title, msg string) SimpleExpectation { } return SimpleExpectation{ check: check, - description: fmt.Sprintf("outstanding work: %s", title), + description: fmt.Sprintf("outstanding work: %q containing %q", title, msg), } } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index c067fe5a4f..f0d22c7c41 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -648,11 +648,15 @@ type Package interface { ParseMode() ParseMode } +// A CriticalError is a workspace-wide error that generally prevents gopls from +// functioning correctly. In the presence of critical errors, other diagnostics +// in the workspace may not make sense. type CriticalError struct { // MainError is the primary error. Must be non-nil. MainError error - // DiagList contains any supplemental (structured) diagnostics. - DiagList []*Diagnostic + + // Diagnostics contains any supplemental (structured) diagnostics. + Diagnostics []*Diagnostic } // An Diagnostic corresponds to an LSP Diagnostic.