From 39c2fd8bffdf79eca7f67b3fe37d3571d05625a4 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 4 Nov 2022 13:14:16 -0400 Subject: [PATCH] internal/lsp/cache: simplify importsState modfile hashing logic While checking for changes to go.mod files in the importsState, there is no reason for special handling based on workspace mode: we can simply hash all active modfiles. This moves us towards an improved API for the workspace: it should simply be responsible for tracking active modfiles. This also incidentally avoids the panic reported in golang/go#55837. Fixes golang/go#55837 Change-Id: I8cb345d1689be12382683186afe3f9addb19d467 Reviewed-on: https://go-review.googlesource.com/c/tools/+/447956 gopls-CI: kokoro Reviewed-by: Alan Donovan Run-TryBot: Robert Findley TryBot-Result: Gopher Robot --- gopls/internal/lsp/cache/imports.go | 29 ++++++---------------- gopls/internal/lsp/cache/load.go | 4 +-- gopls/internal/lsp/cache/snapshot.go | 8 +++--- gopls/internal/lsp/cache/view.go | 8 +++--- gopls/internal/lsp/cache/workspace.go | 5 +++- gopls/internal/lsp/cache/workspace_test.go | 2 +- 6 files changed, 22 insertions(+), 34 deletions(-) diff --git a/gopls/internal/lsp/cache/imports.go b/gopls/internal/lsp/cache/imports.go index a73f9beff3..2bda377746 100644 --- a/gopls/internal/lsp/cache/imports.go +++ b/gopls/internal/lsp/cache/imports.go @@ -13,11 +13,11 @@ import ( "sync" "time" + "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/keys" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/imports" - "golang.org/x/tools/gopls/internal/lsp/source" ) type importsState struct { @@ -37,33 +37,18 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot s.mu.Lock() defer s.mu.Unlock() - // Find the hash of the active mod file, if any. Using the unsaved content + // Find the hash of active mod files, if any. Using the unsaved content // is slightly wasteful, since we'll drop caches a little too often, but // the mod file shouldn't be changing while people are autocompleting. - var modFileHash source.Hash - // If we are using 'legacyWorkspace' mode, we can just read the modfile from - // the snapshot. Otherwise, we need to get the synthetic workspace mod file. // - // TODO(rfindley): we should be able to just always use the synthetic - // workspace module, or alternatively use the go.work file. - if snapshot.workspace.moduleSource == legacyWorkspace { - for m := range snapshot.workspace.getActiveModFiles() { // range to access the only element - modFH, err := snapshot.GetFile(ctx, m) - if err != nil { - return err - } - modFileHash = modFH.FileIdentity().Hash - } - } else { - modFile, err := snapshot.workspace.modFile(ctx, snapshot) + // TODO(rfindley): consider instead hashing on-disk modfiles here. + var modFileHash source.Hash + for m := range snapshot.workspace.ActiveModFiles() { + fh, err := snapshot.GetFile(ctx, m) if err != nil { return err } - modBytes, err := modFile.Format() - if err != nil { - return err - } - modFileHash = source.HashOf(modBytes) + modFileHash.XORWith(fh.FileIdentity().Hash) } // view.goEnv is immutable -- changes make a new view. Options can change. diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index 67b235a809..9cabffc0d0 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -352,10 +352,10 @@ https://github.com/golang/tools/blob/master/gopls/doc/workspace.md.` // If the user has one active go.mod file, they may still be editing files // in nested modules. Check the module of each open file and add warnings // that the nested module must be opened as a workspace folder. - if len(s.workspace.getActiveModFiles()) == 1 { + if len(s.workspace.ActiveModFiles()) == 1 { // Get the active root go.mod file to compare against. var rootModURI span.URI - for uri := range s.workspace.getActiveModFiles() { + for uri := range s.workspace.ActiveModFiles() { rootModURI = uri } nestedModules := map[string][]source.VersionedFileHandle{} diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index eed7dfc6ea..b05f401c52 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -250,7 +250,7 @@ func (s *snapshot) FileSet() *token.FileSet { func (s *snapshot) ModFiles() []span.URI { var uris []span.URI - for modURI := range s.workspace.getActiveModFiles() { + for modURI := range s.workspace.ActiveModFiles() { uris = append(uris, modURI) } return uris @@ -281,7 +281,7 @@ func (s *snapshot) ValidBuildConfiguration() bool { } // Check if the user is working within a module or if we have found // multiple modules in the workspace. - if len(s.workspace.getActiveModFiles()) > 0 { + if len(s.workspace.ActiveModFiles()) > 0 { return true } // The user may have a multiple directories in their GOPATH. @@ -308,7 +308,7 @@ func (s *snapshot) workspaceMode() workspaceMode { // If the view is not in a module and contains no modules, but still has a // valid workspace configuration, do not create the workspace module. // It could be using GOPATH or a different build system entirely. - if len(s.workspace.getActiveModFiles()) == 0 && validBuildConfiguration { + if len(s.workspace.ActiveModFiles()) == 0 && validBuildConfiguration { return mode } mode |= moduleMode @@ -480,7 +480,7 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat if mode == source.LoadWorkspace { switch s.workspace.moduleSource { case legacyWorkspace: - for m := range s.workspace.getActiveModFiles() { // range to access the only element + for m := range s.workspace.ActiveModFiles() { // range to access the only element modURI = m } case goWorkWorkspace: diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index dec2cb0808..a408ee7a03 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -581,13 +581,13 @@ func (v *View) Session() *Session { func (s *snapshot) IgnoredFile(uri span.URI) bool { filename := uri.Filename() var prefixes []string - if len(s.workspace.getActiveModFiles()) == 0 { + if len(s.workspace.ActiveModFiles()) == 0 { for _, entry := range filepath.SplitList(s.view.gopath) { prefixes = append(prefixes, filepath.Join(entry, "src")) } } else { prefixes = append(prefixes, s.view.gomodcache) - for m := range s.workspace.getActiveModFiles() { + for m := range s.workspace.ActiveModFiles() { prefixes = append(prefixes, dirURI(m).Filename()) } } @@ -679,8 +679,8 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) { }) } - if len(s.workspace.getActiveModFiles()) > 0 { - for modURI := range s.workspace.getActiveModFiles() { + if len(s.workspace.ActiveModFiles()) > 0 { + for modURI := range s.workspace.ActiveModFiles() { // Be careful not to add context cancellation errors as critical module // errors. fh, err := s.GetFile(ctx, modURI) diff --git a/gopls/internal/lsp/cache/workspace.go b/gopls/internal/lsp/cache/workspace.go index 2020718ea7..b280ef369a 100644 --- a/gopls/internal/lsp/cache/workspace.go +++ b/gopls/internal/lsp/cache/workspace.go @@ -73,6 +73,7 @@ type workspaceCommon struct { type workspace struct { workspaceCommon + // The source of modules in this workspace. moduleSource workspaceSource // activeModFiles holds the active go.mod files. @@ -192,11 +193,13 @@ func (ws *workspace) loadExplicitWorkspaceFile(ctx context.Context, fs source.Fi var noHardcodedWorkspace = errors.New("no hardcoded workspace") +// TODO(rfindley): eliminate getKnownModFiles. func (w *workspace) getKnownModFiles() map[span.URI]struct{} { return w.knownModFiles } -func (w *workspace) getActiveModFiles() map[span.URI]struct{} { +// ActiveModFiles returns the set of active mod files for the current workspace. +func (w *workspace) ActiveModFiles() map[span.URI]struct{} { return w.activeModFiles } diff --git a/gopls/internal/lsp/cache/workspace_test.go b/gopls/internal/lsp/cache/workspace_test.go index 37e8f2cc46..188869562c 100644 --- a/gopls/internal/lsp/cache/workspace_test.go +++ b/gopls/internal/lsp/cache/workspace_test.go @@ -386,7 +386,7 @@ func checkState(ctx context.Context, t *testing.T, fs source.FileSource, rel fak t.Errorf("module source = %v, want %v", got.moduleSource, want.source) } modules := make(map[span.URI]struct{}) - for k := range got.getActiveModFiles() { + for k := range got.ActiveModFiles() { modules[k] = struct{}{} } for _, modPath := range want.modules {