From aa0c72341e16c18511b9a64b1651267468f8c6e9 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Mon, 15 Mar 2021 18:16:25 -0400 Subject: [PATCH] internal/lsp/cache: get control of reloadOrphanedFiles According to its comment, reloadOrphanedFiles is intended to work around overlay bugs. But as of 1.16, there are no overlay bugs that we know of. So what is it still doing? Apparently, quite a bit, not much of it useful. Clean up as much as possible. - Files with no valid package declaration are ignored by the go command. There's no point trying to reload them; stop. - During metadata invalidation, we clear out all IDs for a file, even if only one of its IDs is invalidated, e.g. when a test package is removed. That leaves valid metadata for the non-test, so we don't refresh it in the workspace reload, and only catch it as an orphan. It seems to me we should only remove the invalidated ID. - If the client incorrectly sends us a didOpen for a non-Go file, we will attempt to load it as an orphaned file. Fix the regtest that did that. - TestEmptyGOPATHXTest_40825 set up an invalid GOPATH: you can't work in GOPATH/src. However, it exists to test code that no longer exists, so just delete it. After this change, almost none of the regression tests trigger orphaned file reloading. It's difficult/impractical to rule it out entirely because some of them only appear racily. Since I intend to remove the code path, I'm not too worried about more creeping in before I'm done. The only useful case is multiple ad-hoc packages. Because we only allow one "command-line-arguments" package in the workspace, if you switch between two the old one becomes orphaned. I hope to work on that soon. Change-Id: Ia355cf104280ce51f6189c6638e8da8f4aef2ace Reviewed-on: https://go-review.googlesource.com/c/tools/+/302089 Trust: Heschi Kreinick Trust: Rebecca Stambler Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley Reviewed-by: Rebecca Stambler --- .../regtest/diagnostics/diagnostics_test.go | 38 +++++-------- gopls/internal/regtest/watch/watch_test.go | 16 +++--- internal/lsp/cache/check.go | 2 +- internal/lsp/cache/snapshot.go | 53 +++++++++++++------ 4 files changed, 61 insertions(+), 48 deletions(-) diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index 692e2f7d47..3a4beee676 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -572,12 +572,8 @@ hi mom WithOptions(EditorConfig{ Env: map[string]string{"GO111MODULE": go111module}, }).Run(t, files, func(t *testing.T, env *Env) { - env.OpenFile("hello.txt") env.Await( - OnceMet( - env.DoneWithOpen(), - NoShowMessage(), - ), + NoOutstandingWork(), ) }) }) @@ -603,7 +599,14 @@ func main() { fmt.Println("") } ` - WithOptions(InGOPATH()).Run(t, collision, func(t *testing.T, env *Env) { + WithOptions( + InGOPATH(), + EditorConfig{ + Env: map[string]string{ + "GO111MODULE": "off", + }, + }, + ).Run(t, collision, func(t *testing.T, env *Env) { env.OpenFile("x/x.go") env.Await( env.DiagnosticAtRegexpWithMessage("x/x.go", `^`, "found packages main (main.go) and x (x.go)"), @@ -898,24 +901,6 @@ package foo_ }) } -// Reproduces golang/go#40825. -func TestEmptyGOPATHXTest_40825(t *testing.T) { - const files = ` --- x.go -- -package x --- x_test.go -- -` - - WithOptions(InGOPATH()).Run(t, files, func(t *testing.T, env *Env) { - env.OpenFile("x_test.go") - env.EditBuffer("x_test.go", fake.NewEdit(0, 0, 0, 0, "pack")) - env.Await( - env.DoneWithChange(), - NoShowMessage(), - ) - }) -} - func TestIgnoredFiles(t *testing.T) { const ws = ` -- go.mod -- @@ -1491,6 +1476,11 @@ package foo_ WithOptions( ProxyFiles(proxy), InGOPATH(), + EditorConfig{ + Env: map[string]string{ + "GO111MODULE": "off", + }, + }, ).Run(t, contents, func(t *testing.T, env *Env) { // Simulate typing character by character. env.OpenFile("foo/foo_test.go") diff --git a/gopls/internal/regtest/watch/watch_test.go b/gopls/internal/regtest/watch/watch_test.go index ae43beaf0c..9cc9d0a22b 100644 --- a/gopls/internal/regtest/watch/watch_test.go +++ b/gopls/internal/regtest/watch/watch_test.go @@ -617,6 +617,11 @@ func main() { ` WithOptions( InGOPATH(), + EditorConfig{ + Env: map[string]string{ + "GO111MODULE": "auto", + }, + }, Modes(Experimental), // module is in a subdirectory ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("foo/main.go") @@ -624,12 +629,6 @@ func main() { if err := env.Sandbox.RunGoCommand(env.Ctx, "foo", "mod", []string{"init", "mod.com"}); err != nil { t.Fatal(err) } - env.Await( - OnceMet( - env.DoneWithChangeWatchedFiles(), - env.DiagnosticAtRegexp("foo/main.go", `"blah"`), - ), - ) env.RegexpReplace("foo/main.go", `"blah"`, `"mod.com/blah"`) env.Await( EmptyDiagnostics("foo/main.go"), @@ -661,6 +660,11 @@ func main() { ` WithOptions( InGOPATH(), + EditorConfig{ + Env: map[string]string{ + "GO111MODULE": "auto", + }, + }, ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("foo/main.go") env.RemoveWorkspaceFile("foo/go.mod") diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 9df626be6c..2d2824ad72 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -764,7 +764,7 @@ func isValidImport(pkgPath, importPkgPath packagePath) bool { if i == -1 { return true } - if pkgPath == "command-line-arguments" { + if isCommandLineArguments(string(pkgPath)) { return true } return strings.HasPrefix(string(pkgPath), string(importPkgPath[:i])) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index b80d518629..53ba44975c 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1085,7 +1085,7 @@ func shouldShowAdHocPackagesWarning(snapshot source.Snapshot, pkgs []source.Pack func containsCommandLineArguments(pkgs []source.Package) bool { for _, pkg := range pkgs { - if strings.Contains(pkg.ID(), "command-line-arguments") { + if isCommandLineArguments(pkg.ID()) { return true } } @@ -1096,6 +1096,13 @@ 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 + // initialization error? Is it possible for the initialization error to be + // corrected without a successful reinitialization? + if s.initializedErr != nil { + return s.initializedErr + } + if ctx.Err() != nil { return &source.CriticalError{MainError: ctx.Err()} } @@ -1114,10 +1121,7 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalErr DiagList: diags, } } - // TODO(rstambler): Should we be more careful about returning the - // initialization error? Is it possible for the initialization error to be - // corrected without a successful reinitialization? - return s.initializedErr + return nil } func (s *snapshot) AwaitInitialized(ctx context.Context) { @@ -1173,11 +1177,27 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error { // that exist only in overlays. As a workaround, we search all of the files // available in the snapshot and reload their metadata individually using a // file= query if the metadata is unavailable. - scopes := s.orphanedFileScopes() + files := s.orphanedFiles() + + // Files without a valid package declaration can't be loaded. Don't try. + var scopes []interface{} + for _, file := range files { + pgf, err := s.ParseGo(ctx, file, source.ParseHeader) + if err != nil { + continue + } + if !pgf.File.Package.IsValid() { + continue + } + scopes = append(scopes, fileURI(file.URI())) + } + if len(scopes) == 0 { return nil } + // The regtests match this exact log message, keep them in sync. + event.Log(ctx, "reloadOrphanedFiles reloading", tag.Query.Of(scopes)) err := s.load(ctx, false, scopes...) // If we failed to load some files, i.e. they have no metadata, @@ -1203,11 +1223,11 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error { return nil } -func (s *snapshot) orphanedFileScopes() []interface{} { +func (s *snapshot) orphanedFiles() []source.VersionedFileHandle { s.mu.Lock() defer s.mu.Unlock() - scopeSet := make(map[span.URI]struct{}) + var files []source.VersionedFileHandle for uri, fh := range s.files { // Don't try to reload metadata for go.mod files. if fh.Kind() != source.Go { @@ -1228,14 +1248,10 @@ func (s *snapshot) orphanedFileScopes() []interface{} { continue } if s.getMetadataForURILocked(uri) == nil { - scopeSet[uri] = struct{}{} + files = append(files, fh) } } - var scopes []interface{} - for uri := range scopeSet { - scopes = append(scopes, fileURI(uri)) - } - return scopes + return files } func contains(views []*View, view *View) bool { @@ -1485,14 +1501,17 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC } // Copy the URI to package ID mappings, skipping only those URIs whose // metadata will be reloaded in future calls to load. -copyIDs: for k, ids := range s.ids { + var newIDs []packageID for _, id := range ids { if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok { - continue copyIDs + continue } + newIDs = append(newIDs, id) + } + if len(newIDs) != 0 { + result.ids[k] = newIDs } - result.ids[k] = ids } // Copy the set of initially loaded packages. for id, pkgPath := range s.workspacePackages {