mirror
/
go
mirror of https://github.com/golang/go.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

internal/lsp/cache: report a critical error when go.work is invalid 

When a go.work file fails to validate, the workspace is left in an
invalid state: we will detect that the workspace is defined by the
go.work, but will not actually parse any active modules. This should be
a critical error.

Fix this by adding allowing the workspace to surface critical errors via
a new cache.workspace.criticalError method.

Additionally:
 - only build the workspace mod file in workspace.build if the mode is
   fileSystemWorkspace (in all other modes the modfile is already
   determined)
 - rename workspace.invalidate to workspace.Clone, to be consistent with
   other data structures
 - rename CriticalError.DiagList to CriticalError.Diagnostics
 - add several TODOs for observations while reading the code
 - create a new file for regtests related to broken workspaces
 - make the regtest sandbox panic when duplicate paths are present in
   the sandbox file set (an error I made while writing the test)

Updates golang/go#53933

Change-Id: If8625ab190129bc9c57e784314bc9cc92644c955
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417593
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
This commit is contained in:
Robert Findley 2022-07-25 09:57:53 -04:00
parent b3b5c13b29
commit f1bb5ca08f
14 changed files with 234 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
gopls/internal/regtest/diagnostics/diagnostics_test.go
View File

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

120
gopls/internal/regtest/workspace/broken_test.go Normal file
View File

@ -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)
}
*/
})
}

4
gopls/internal/regtest/workspace/workspace_test.go
View File

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

6
internal/lsp/cache/load.go vendored
View File

@ -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,
}
}
}

3
internal/lsp/cache/mod_tidy.go vendored
View File

@ -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}
})

15
internal/lsp/cache/session.go vendored
View File

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

35
internal/lsp/cache/snapshot.go vendored
View File

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

21
internal/lsp/cache/view.go vendored
View File

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

53
internal/lsp/cache/workspace.go vendored
View File

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

2
internal/lsp/cache/workspace_test.go vendored
View File

@ -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)
}

2
internal/lsp/diagnostics.go
View File

@ -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", " ")

3
internal/lsp/fake/sandbox.go
View File

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

2
internal/lsp/regtest/expectation.go
View File

@ -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),
}
}

8
internal/lsp/source/view.go
View File

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